-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
base: main
Are you sure you want to change the base?
Conversation
9d46926
to
9a0a0a4
Compare
There was a problem hiding this 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.
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. |
@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.) |
About the (default) behavior, a data point: 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. 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]>
9a0a0a4
to
276827d
Compare
@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, As for Windows, I don't know if |
Before:
After:
Signed-off-by: Ihar Hrachyshka [email protected]
Description
This will make
eza
resolve symlinks pointing at valid directories like aregular 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 (reportingan error but proceeding to handle the rest of the files.) See below.
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.