Skip to content

ls: Improve error handling and other improvements#2809

Merged
sylvestre merged 23 commits intouutils:masterfrom
kimono-koans:ls_error_handling_2
Jan 5, 2022
Merged

ls: Improve error handling and other improvements#2809
sylvestre merged 23 commits intouutils:masterfrom
kimono-koans:ls_error_handling_2

Conversation

@kimono-koans
Copy link
Contributor

Referenced as an issue as well: #2785

  • print error in the correct order by flushing the stdout buffer before printing an error
  • print correct GNU error codes
  • correct formatting for config.inode, and for dangling links
  • correct padding for Format::Long
  • remove colors after the -> link symbol as this doesn't match GNU
  • correct the major, minor #s for char devices, and correct padding
  • improve speed for all metadata intensive ops by not allocating metadata unless in a Sort mode
  • new tests, have struggled with how to deal with stderr, stdout ordering in a test though
  • tried to implement UIoError, but am still having issues matching the formatting of GNU

Copy link
Collaborator

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Alright, good stuff! You'll notice that I left a lot of comments, which is mostly because you included a lot more changes than you originally set out to do. I admire your enthusiasm, but please keep this PR focused on the orignal task: improving the error handling. That makes it much easier to review. The other changes require a bit more discussion and I think it's best to do that in issues and other PRs.

As a general remark: please follow clippy suggestions instead of putting an #[allow(...)]. Performance optimizations are also tricky. First, they should (almost) always come with measurements to show the impact, so we can evaluate whether the change is worth merging. We generally do this with hyperfine, see the BENCHMARKING.md file in the ls folder. Second, they should really be done in a separate PR, so that we can clearly see where the performance change is coming from.

@kimono-koans
Copy link
Contributor Author

Appreciate your review and your good suggestions! I know I gave you too much experimentation!

I've excised my over-exubrance, and am working on small fixes elsewhere. I left 2 of your comments open. I hope you will take a look again, when I submit those changes.

Re: UIoError, I am still have problems with formatting. Perhaps you saw that I left my implementation in comments throughout the list function. If you run, you will pop an error in one test, and it seems to have to do with quoting style on the path? I just fixed an error message ordering issue and perhaps you will take a look when I resubmit.

Thanks!

@tertsdiepraam
Copy link
Collaborator

Thanks!

If you run, you will pop an error in one test, and it seems to have to do with quoting style on the path?

I found the two comments, but I can't reproduce this error. Do you maybe have a branch I can checkout where that error pops up?

@kimono-koans
Copy link
Contributor Author

kimono-koans commented Dec 29, 2021

Thanks!

If you run, you will pop an error in one test, and it seems to have to do with quoting style on the path?

I found the two comments, but I can't reproduce this error. Do you maybe have a branch I can checkout where that error pops up?

It's probably because I've been running the MSRV (caused clippy to not work correctly as well). I'll try again on stable and let you know if I have any issues.

@kimono-koans
Copy link
Contributor Author

Thanks!

If you run, you will pop an error in one test, and it seems to have to do with quoting style on the path?

I found the two comments, but I can't reproduce this error. Do you maybe have a branch I can checkout where that error pops up?

Unfortunately I get the 'Entity not found' error again, which doesn't match GNU semantics. This happens whether I create a UIoError::new or FromIo::map_err_context.

See my branch: https://github.com/kimono-koans/coreutils/tree/ls_uioerror_native

---- test_ls::test_ls_dangling_symlinks stdout ----
current_directory_resolved:
mkdir: /var/folders/8v/3rsh5g4n4xz5kmbw3zk3x_9c0000gn/T/.tmpXsugfD/temp_dir
symlink: /var/folders/8v/3rsh5g4n4xz5kmbw3zk3x_9c0000gn/T/.tmpXsugfD/does_not_exist,/var/folders/8v/3rsh5g4n4xz5kmbw3zk3x_9c0000gn/T/.tmpXsugfD/temp_dir/dangle
run: /Users/rswinford/Programming/coreutils/target/debug/coreutils ls -L temp_dir/dangle
run: /Users/rswinford/Programming/coreutils/target/debug/coreutils ls -H temp_dir/dangle
run: /Users/rswinford/Programming/coreutils/target/debug/coreutils ls temp_dir/dangle
run: /Users/rswinford/Programming/coreutils/target/debug/coreutils ls -Li temp_dir
thread 'test_ls::test_ls_dangling_symlinks' panicked at ''ls: dangle: Entity not found
ls: dangle: Entity not found
' does not contain 'cannot access'', tests/common/util.rs:396:9

---- test_ls::test_ls_files_dirs stdout ----
current_directory_resolved:
mkdir: /var/folders/8v/3rsh5g4n4xz5kmbw3zk3x_9c0000gn/T/.tmpOu6lCJ/a
mkdir: /var/folders/8v/3rsh5g4n4xz5kmbw3zk3x_9c0000gn/T/.tmpOu6lCJ/a/b
mkdir: /var/folders/8v/3rsh5g4n4xz5kmbw3zk3x_9c0000gn/T/.tmpOu6lCJ/a/b/c
mkdir: /var/folders/8v/3rsh5g4n4xz5kmbw3zk3x_9c0000gn/T/.tmpOu6lCJ/z
touch: /var/folders/8v/3rsh5g4n4xz5kmbw3zk3x_9c0000gn/T/.tmpOu6lCJ/a/a
touch: /var/folders/8v/3rsh5g4n4xz5kmbw3zk3x_9c0000gn/T/.tmpOu6lCJ/a/b/b
run: /Users/rswinford/Programming/coreutils/target/debug/coreutils ls a
run: /Users/rswinford/Programming/coreutils/target/debug/coreutils ls a/a
run: /Users/rswinford/Programming/coreutils/target/debug/coreutils ls a z
run: /Users/rswinford/Programming/coreutils/target/debug/coreutils ls doesntexist
thread 'test_ls::test_ls_files_dirs' panicked at ''ls: doesntexist: Entity not found
' does not contain ''doesntexist': No such file or directory'', tests/common/util.rs:396:9


failures:
    test_ls::test_ls_dangling_symlinks
    test_ls::test_ls_files_dirs

Copy link
Collaborator

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Great job on the inodes! I think you found a great solution. It just needsI'm still seeing some things I requested changes on, could you fix those?

Re: error handling: I think the we should stick to your custom implementation for now and fix UIoError separately and port ls to use UIoError later.

Copy link
Collaborator

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

I think this is ready to get merged! Great work and I look forward to seeing your PRs with the changes we had to cut from this one!

@sylvestre sylvestre merged commit 421330d into uutils:master Jan 5, 2022
@sylvestre
Copy link
Contributor

bravo:
PASS +5 / FAIL -4 / ERROR +0 / SKIP -1

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.

3 participants