Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: recursively walk symlinks pointing at dirs #1110

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

booxter
Copy link

@booxter booxter commented Sep 1, 2024

Before:

/tmp/test-recursive-dir-symlinks
├── dir
│  └── test.txt
└── symlinkdir -> /tmp/test-recursive-dir-symlinks/dir

After:

/tmp/test-recursive-dir-symlinks
├── dir
│  └── test.txt
└── symlinkdir -> /tmp/test-recursive-dir-symlinks/dir
   └── test.txt

Signed-off-by: Ihar Hrachyshka [email protected]

Description

This will make eza resolve symlinks pointing at valid directories like a
regular directories, meaning: if requested, unravel them recursively. This is
handy in cases where an inspected directory contains multiple layers of symlink
indirection (hi nix!)

I've confirmed that loops (situations where symlinks from two directories
cross-point to each other) are handled gracefully by eza already (reporting
an error but proceeding to handle the rest of the files.) See below.

...
│  ├── links -> /tmp/test/test/test
│  │  └── link4 -> /tmp/test/test2
│  │     └── link3 -> /tmp/test/test/test
│  │        └── link4 -> /tmp/test/test2
│  │           └── link3 -> /tmp/test/test/test
│  │              └── link4 -> /tmp/test/test2
│  │                 └── link3 -> /tmp/test/test/test
│  │                    └── link4 -> /tmp/test/test2
│  │                       └── link3 -> /tmp/test/test/test
│  │                          └── link4 -> /tmp/test/test2
│  │                             └── link3 -> /tmp/test/test/test
│  │                                └── link4 -> /tmp/test/test2
│  │                                   └── link3 -> /tmp/test/test/test
│  │                                      └── link4 -> /tmp/test/test2
│  │                                         └── link3 -> /tmp/test/test/test
│  │                                            └── link4 -> /tmp/test/test2
│  │                                               └── <Too many levels of symbolic links (os error 62)>
│  └── test
│     └── link4 -> /tmp/test/test2
│        └── link3 -> /tmp/test/test/test
...
How Has This Been Tested?

Local test env: nix-darwin. Used included nix targets to validate the patch.

Confirmed that the test suite passed. (Only failures specific to nix-darwin are
observed.)

Checked on a a number of synthetic and real nested directories with several
layers of symlink indirection.

Checked that loops are properly handled by the tool.


This patch changes behavior of the tool. I am not sure this change justifies a
new CLI flag though. The first version of the patch enables it unconditionally.

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is cool, and looks like it works without issues, thanks!

This patch changes behavior of the tool. I am not sure this change justifies a
new CLI flag though. The first version of the patch enables it unconditionally.

I do think I'm gonna spend a brief amount of time just making sure how (a probably unrepresentative) sample of people feel about making this the new default... We may want to have a flag for disabling it, but that doesn't have to be this PR.

I do like the idea of this being the default behavior. I'll merge after making sure other people seem to agree.

@daviessm
Copy link
Contributor

daviessm commented Sep 1, 2024

I do like the idea of this being the default behavior. I'll merge after making sure other people seem to agree.

I'm not sure about this. Is it only for tree mode? I can definitely see it being used behind a flag/alias but I wouldn't want to have to disable it each time.

@booxter
Copy link
Author

booxter commented Sep 1, 2024

@daviessm non-tree version works the same way. The included test changes confirm this. (Side note: I am wondering if perhaps the suite should have another set of tests for tree mode? I haven't seen any.)

@booxter
Copy link
Author

booxter commented Sep 1, 2024

About the (default) behavior, a data point: tree does what eza is doing right now (not walking down the symlink rabbit hole).

It shouldn't be hard to make it an opt-in controlled by a flag (that people can then put in their aliases - I would.) I don't see a reason to disable this proposed behavior myself, but I may miss how other people use the tool.

@cafkafk
Copy link
Member

cafkafk commented Sep 2, 2024

It shouldn't be hard to make it an opt-in controlled by a flag (that people can then put in their aliases - I would.) I don't see a reason to disable this proposed behavior myself, but I may miss how other people use the tool.

I've asked a few people, and most seem to be fine with it being default if there is cycle detection, which is probably gonna be pretty compute expensive or require using a lot of platform specific code, e.g. man fts functions on Linux/BSD/MacOS (which also isn't in musl without external libraries), and some pretty obscure windows ones.

I think the best thing would be to start with just the scope of this PR, behind a flag, and then if people want to start tackling cycle detection (and manage to do so in a way that doesn't grind eza to a halt), we can make it the default behavior.

LMK what everyone things about this

Before:

```
/tmp/test-recursive-dir-symlinks
├── dir
│  └── test.txt
└── symlinkdir -> /tmp/test-recursive-dir-symlinks/dir
```

After:

```
/tmp/test-recursive-dir-symlinks
├── dir
│  └── test.txt
└── symlinkdir -> /tmp/test-recursive-dir-symlinks/dir
   └── test.txt
```

Signed-off-by: Ihar Hrachyshka <[email protected]>
@cafkafk cafkafk self-requested a review September 2, 2024 07:29
@booxter
Copy link
Author

booxter commented Sep 2, 2024

@cafkafk I can add an option. Before I do, I'd like to ask what the reason for force-pushing to the branch is. I don't see any visible changes.


About symbolic link loop detection. On POSIX systems, opendir already returns ELOOP on a loop detection. That's the error 62 from the snippet in the description. So I don't think Unix would need anything special.

As for Windows, I don't know if FindFirstFile that, apparently, std::fs::read_dir uses implements a similar behavior. We can try and see (I don't have access to win32 machine at the moment though.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

3 participants