Skip to content

Fix ls: panicking on dangling symlink with --color=auto -l #6346

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
May 11, 2024
Merged

Fix ls: panicking on dangling symlink with --color=auto -l #6346

merged 6 commits into from
May 11, 2024

Conversation

AnirbanHalder654322
Copy link
Contributor

fixes #6227

The get_metadata() function seemed to try to deference to the non existent file. We change it to directly get the metadata of the symlink itself incase the deference fails.

BenWiederhake
BenWiederhake requested changes May 4, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for tackling this!

  • I'm not yet entirely convinced that the second invocation is actually needed
  • If it is needed, what do we do if it fails?
  • And of course, please write tests so that we can't regress if we change this in the future :D
  • EDIT: Oh, and please fix the typo in the commit message :)

@github-actions GitHub Actions
Copy link

github-actions bot commented May 4, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@cakebaker cakebaker changed the title Fix ls: paniciking on dangling symlink with --color=auto -l Fix ls: panicking on dangling symlink with --color=auto -l May 5, 2024
@AnirbanHalder654322 AnirbanHalder654322 force-pushed the fix_ls_panic_on_dangling_symlink branch from 8408f41 to c8fa0bb Compare May 7, 2024 07:43
@AnirbanHalder654322
Copy link
Contributor Author

Changes since last push :

  • Changed the logic to use
        let md_res = get_metadata_with_deref_opt(target.p_buf.as_path(), path.must_dereference);
        let md = md_res.or(target.p_buf.symlink_metadata());

        apply_style_based_on_metadata(path, md.as_ref(),ok(), ls_colors, style_manager, &name)
  • Added test

@github-actions GitHub Actions
Copy link

github-actions bot commented May 7, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@github-actions GitHub Actions
Copy link

github-actions bot commented May 7, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

BenWiederhake
BenWiederhake requested changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Fixes the problem, and adds a test. Yay!

It looks to me like the test can be simplified by a lot, what did I miss?

@AnirbanHalder654322 AnirbanHalder654322 force-pushed the fix_ls_panic_on_dangling_symlink branch from a9fe7ab to 46a9f8c Compare May 9, 2024 12:11
@AnirbanHalder654322 AnirbanHalder654322 force-pushed the fix_ls_panic_on_dangling_symlink branch from 46a9f8c to a0c01e0 Compare May 9, 2024 12:18
@AnirbanHalder654322
Copy link
Contributor Author

Changes since last push:

  • Using at.symlink_file for symlink creation
  • Switched to regex matching for extracting colors from stdout
  • Squashed the commit into an older comment formatting commit to clean up history .

BenWiederhake
BenWiederhake reviewed May 9, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Just one last nitpick, then it can be merged :)

@AnirbanHalder654322
Copy link
Contributor Author

I was wondering if i should clean up the commit history a bit more ? Maybe only leave two commits one for the main change and another for the test ?

BenWiederhake
BenWiederhake reviewed May 9, 2024
View reviewed changes
@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

BenWiederhake
BenWiederhake approved these changes May 9, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

LGTM 👍

BenWiederhake
BenWiederhake approved these changes May 11, 2024
View reviewed changes
Copy link
Collaborator

@BenWiederhake BenWiederhake left a comment

Choose a reason for hiding this comment

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

Still looks good ^^

@BenWiederhake BenWiederhake merged commit 1a5639b into uutils:main May 11, 2024
@AnirbanHalder654322 AnirbanHalder654322 deleted the fix_ls_panic_on_dangling_symlink branch May 11, 2024 19:55
AnirbanHalder654322 added a commit to AnirbanHalder654322/coreutils that referenced this pull request May 19, 2024
@AnirbanHalder654322
Fix ls: panicking on dangling symlink with --color=auto -l (uutils#6346)
f90e106 
* Fixed unwrap being called on dereferenced dangling symlink

* Added test

* Switched to regex matching in test

* Remove unnecessary mkdir call

* Modified documentation of the test and added assertion of the colors

* Fixed a typo
WaleedKhamees pushed a commit to WaleedKhamees/coreutils that referenced this pull request May 20, 2024
@AnirbanHalder654322 @WaleedKhamees
Fix ls: panicking on dangling symlink with --color=auto -l (uutils#6346)
1d56d7d 
* Fixed unwrap being called on dereferenced dangling symlink

* Added test

* Switched to regex matching in test

* Remove unnecessary mkdir call

* Modified documentation of the test and added assertion of the colors

* Fixed a typo
@BrewTestBot BrewTestBot mentioned this pull request Jun 23, 2024
Merged
@moonfruit moonfruit mentioned this pull request Jun 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenWiederhake BenWiederhake BenWiederhake approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: ls: panics on dangling symlink with --color=auto -l
2 participants
@AnirbanHalder654322 @BenWiederhake