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

Merged
merged 5 commits into from
Sep 29, 2024

Conversation

booxter
Copy link
Contributor

@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 ihar.hrachyshka@gmail.com

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.

cafkafk
cafkafk previously approved these changes Sep 1, 2024
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
Contributor 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
Contributor 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

@cafkafk cafkafk self-requested a review September 2, 2024 07:29
@booxter
Copy link
Contributor 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.)

@booxter
Copy link
Contributor Author

booxter commented Sep 23, 2024

@cafkafk I've added a flag to control the new behavior. Let me know if it's good enough now.

MartinFillon
MartinFillon previously approved these changes Sep 24, 2024
Copy link
Contributor

@MartinFillon MartinFillon left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the pr and idea, this will be really useful for some edge cases as you mentionned

@cafkafk
Copy link
Member

cafkafk commented Sep 28, 2024

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.

I think this was just me doing a git pull origin main --rebase on the branch, so no changes to PR content :p

@cafkafk
Copy link
Member

cafkafk commented Sep 28, 2024

I've resolved the conflict with the annoying tests here, I'll review the code and potentially regenerate

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.

Okay, I've looked through the code now, and done reasonable testing for performance regressions and behavior. It's solid and ready to merge. I did spot that this is lacking completions however.

All this needs before merge is adding the new option to the completions files.

Thanks for making this, it's very useful!

@booxter
Copy link
Contributor Author

booxter commented Sep 28, 2024

@cafkafk I've updated completion files. I haven't tested this though (not sure how to do it with nix env, nor I know how nushell and fish even work). I also haven't touched bash completion since it looks like it extracts long options from --help. Let me know if this is ok.

booxter and others added 5 commits September 29, 2024 05:14
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 <ihar.hrachyshka@gmail.com>
The option enables drilling down into symbolic links that point to
directories.
Yes, we shouldn't have to do this.

Signed-off-by: Christina Sørensen <christina@cafkafk.com>
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.

Okay, thanks, then it's done! I'll merge now, thanks for the contribution! This is very useful for someone with a lot of symlink farms... like me :D


not sure how to do it with nix env

One possible way is to nix profile install . in the root of the repository, and then opening a new terminal, that should work in most cases.

@cafkafk cafkafk merged commit 1ab432d into eza-community:main Sep 29, 2024
20 checks passed
@booxter booxter deleted the recursive-symlinks-to-dirs branch September 30, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants