ls: Fix display of bad file descriptor errors

[kimono-koans]

Presently, 'ls' does not handle EBADF errors correctly. This PR correctly displays and formats bad file descriptor errors. Tests are provided which confirm the errors are formatted correctly, no directory heading is printed unless a bad file descriptor "directory" is entered, and errors are only printed on a directory entry (and never printed on a metadata request), which matches the GNU implementation behavior.

[kimono-koans]

Do the maintainers have a suggestion re: the failing tr test? The comment seems to indicate that this test is broken.

[tertsdiepraam]

I ran into the same thing locally just now on another PR. Seems to be happening randomly with a low probability. I'll open a PR to implement a possible fix.

Edit: here is the fix: #2896.

[tertsdiepraam]

This is some nice work you've done here, but I'm think we have to improve it a bit still before we can merge this.

As I understand it, we should be using the DirEntry metadata where possible right? This means we can make a method PathData::from(DirEntry, &Config), which immediately sets the file type and metadata cells, without the need for set_md or the get-parent-then-read-dir-to-find-self-and-get-metadata thing going on in md now. This conversion is going to be very cheap, so we also won't need to use DirEntry::file_type for performance reasons. Does that make sense?

[tertsdiepraam]

Sorry for waiting so long to respond, I've been quite busy :)

I want to start off by saying that this is a very good PR:

• It solves a real issue.
• I like that set_md is gone and PathData just handles it's state internally.
• The logic is now relatively easy to follow.

What I'm worried about is not really the code here, but the compounding complexity which each of the edge cases we handle. Instead of adding if statements everywhere code to work around wrong assumptions in the original code, we should focus on fixing those assumptions. ls is already quite a complex codebase with a long of branches as it is :)

As I understand it, the wrong assumption was that we take the metadata from the Path instead of DirEntry. The PathData struct is completely based on this assumption: every bit of data it retrieves from the OS is based on the PathBuf that it holds. Hence, fixing this issue needs that big block of code in the md function (which is good code by itself, but it's annoying that it's necessary).

So, the solution that I'm proposing is a bigger rewrite of PathData to be based entirely around DirEntry, instead of PathData. I.e.:

struct PathData {
    md: OnceCell<Option<Metadata>>,
    ft: OnceCell<Option<FileType>>,
    display_name: OsString,
    dir_entry: DirEntry, // instead of p_buf: PathBuf
    must_dereference: bool,
    security_context: String,
}

So, for example, the md function would then directly call DirEntry::metadata(), which would give the same behaviour as this PR.

I hope that clears it up a bit.

Now, there is a problem with this approach: how do we add . and .. if -a is passed, because we don't have DirEntry structs for those. So possibly PathData needs to support both a DirEntry source and a PathBuf source. I'm not sure, let me know if you have any ideas about that.

We have two options:

• Merge this and I'll open an issue for this change and further discussion.
• Or fix it in this PR.

Both are fine with me, let me know what you prefer!

PS: If you want this merged, could you clean up the Git history a bit?

[kimono-koans]

Appreciate your suggestions and, yes, maybe it was getting more complex than it needed to be.

I do, however, think there are a few problems with what you suggest. First, DirEntries are only available for directories. An invocation of ls may only happen on one or many files. Second, you cannot make a symlink_metadata call on a DirEntry. Third, we will need that Path information in other places.

I do think storing the DirEntries in the PathData struct is a good idea though, if the cost of allocating is not too high.

I've restructured a bit, so maybe it makes more sense? Thanks.

[tertsdiepraam]

First, DirEntries are only available for directories. An invocation of ls may only happen on one or many files. Second, you cannot make a symlink_metadata call on a DirEntry.

Good points! I think you're right and we need both. I really like the changes you've made!

if the cost of allocating is not too high

I don't think so. Each of the PathData allocations should be one single allocation anyway and I don't think we have to worry too much about memory.

Just some final cleanup and then this is ready to be merged!

[tertsdiepraam]

Excellent! Thank you! There's one outdated comment left and the test seems to be failing, otherwise it's ready.