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

Resolve symlinks on directory listing #479

Merged
merged 10 commits into from Apr 18, 2021

Conversation

aliemjay
Copy link
Contributor

This has the benefit of showing the size and modification date of the pointed-to file. Symlink to directories now respects '--dirs-first' option and broken symlinks don't show up in directory listing.

The only downside AFAIK is more syscalls when listing directories.

@svenstaro Any input on this? should it be configurable? can we abandon the symlink symbol?

@svenstaro
Copy link
Owner

Could you rebase this?

This has the benefit of showing the size and modification date of the
pointed-to file. Symlink to directories now respects '--dirs-first'
option and broken symlinks don't show in directory listing.
@aliemjay
Copy link
Contributor Author

@svenstaro done.

@svenstaro
Copy link
Owner

I quite like this as it is. I like keeping the symlink symbol around. Of course, I'd appreciate a test for this if you want to make one but I'd say for this change it's optional. Once you're happy, remove the [RFC] prefix and I'll merge it. :)

This should facilitate testing because this symbol will no longer a part
of the entry text shown in html.
@aliemjay aliemjay changed the title [RFC] resolve symlinks on directory listing Resolve symlinks on directory listing Mar 29, 2021
@aliemjay
Copy link
Contributor Author

RFC tag removed!

CSS styles may still need some cleanup though: symlink class is no longer used, symlink-symbol border color seems off to me.

@svenstaro
Copy link
Owner

Alright, feel free to remove the disused class, no reason to keep it around unless we maybe want to make use of it still? Perhaps we can underline the symlinks in the symlink color?

tests/fixtures/mod.rs Outdated Show resolved Hide resolved
tests/fixtures/mod.rs Outdated Show resolved Hide resolved
@svenstaro
Copy link
Owner

Cool stuff with the fixtures! Now perhaps just make one test check whether the symlinks are actually rendered as they should be (while other files and dirs are still rendered as they should be.

@aliemjay
Copy link
Contributor Author

Cool stuff with the fixtures! Now perhaps just make one test check whether the symlinks are actually rendered as they should be (while other files and dirs are still rendered as they should be.

I believe that the current tests would still fail with the old behaviour because symlink filenames do always have a trailing /.
But I think that a single standalone test is better and that symlink files and directories should be made local to such test. Fixing that soon..

Replace some of the testing files and directories with symbolic links.
They should behave exactly the same.
For non-symlink files and directories, there is no need to call
`std::fs::metadata()` as the metadata are already obtained via
`entry.metadata()`
@aliemjay
Copy link
Contributor Author

I don't really have a strong opinion on symlink symbol style, but it looks ok to me without a special style.

@svenstaro
Copy link
Owner

@aliemjay could you resolve the conflict? I think this was in pretty good shape already the last time I looked at it.

@svenstaro
Copy link
Owner

Looking at this again, I'm thinking: Wouldn't it be kind of cool to render the pointed-to relative location of the symlink behind the arrow?

@aliemjay
Copy link
Contributor Author

I have no strong opinion on this, but a symlink may be absolute or may point outside the serve_path and that would show potentially unwanted path names.

@svenstaro
Copy link
Owner

svenstaro commented Apr 18, 2021

Perhaps that's something we could show actually in case it's pointing to somewhere that would be off-limits? So we'll end up with these possibilities:

  1. symlink -> relative path inside serve_path
  2. symlink -> absolute path inside serve_path
  3. symlink -> pointed to path outside of serve_path
  4. symlink -> pointed to path is invalid

I think it would just be nice to see where a symlink points in case it's valid. :)

@aliemjay
Copy link
Contributor Author

Well, as I said,I have no strong opinion but I see that symlinks should be totally abstracted away from the user and should be used by the admin only to organize the file tree.

@svenstaro
Copy link
Owner

That's a fair point! How about a follow-up PR with a new flag to show symlink destination? :)

@aliemjay
Copy link
Contributor Author

That would be interesting. I shall give it a try!

@svenstaro svenstaro merged commit a135c5d into svenstaro:master Apr 18, 2021
@svenstaro
Copy link
Owner

Merging this one as is! Follow ups: #498 and #499.

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

Successfully merging this pull request may close these issues.

None yet

2 participants