fix(stat): Collection of fixes for stat(1)

[Alonely0]

This PR fixes #9071, #9072, as well as a few bugs found along the way.

[codspeed-hq]

CodSpeed Performance Report

Merging #9078 will not alter performance

_{Comparing Alonely0:stat_fix (613e9be) with main (f60072d)}

Summary

✅ 130 untouched
⏩ 30 skipped¹

Footnotes

1. 30 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[Alonely0]

I mean, that's not my fault lol.

[github-actions]

GNU testsuite comparison:

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

[rlee287]

I think it'd be better to add tests for the device number and the percent escape issues as well.

For the percent escape issue, I propose the following addition to test_stat.rs:

#[test]
fn test_double_percent() {
    let ts = TestScenario::new(util_name!());
    let result = ts.ucmd().args(&["-c", "%%m", "/"]).succeeds();
    let output = result.stdout_str().trim();
    assert!(!output.is_empty(), "Escaped percent should be present");
    assert_eq!(output, "%m");
}

For the device number format issue, I'd add %Hd %Ld %r %R %Hr %Lr to one of NORMAL_FORMAT_STR, DEV_FORMAT_STR, or FS_FORMAT_STR in test_stat.rs, but I'm not familiar enough with the purpose of each of those to suggest which one(s) they should be added to.

[Alonely0]

Yeah I agree, I'll add a few tests :). Ngl, I was trying to cut that corner cuz tomorrow I got an exam, but it should be quick enough.

[Alonely0]

Testing the value of these flags (%Hd %Ld %r %R %Hr %Lr) is actually correct is quite difficult without bringing in the machine's stat as a reference, because their output is not trivial. Rn, in CI it's probably either GNU or busybox, but knowing Ubuntu's switched to uutils we might as well be testing against an older version of uutils in the future. This is why I'm choosing against including that test, and only one about percent escaping. Maintainers, lmk if you would like me to include it nevertheless (it's already written lol).

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)

[Alonely0]

Those CI fails are still not my fault :) Also, I'll probably be opening a few PRs with perf improvements, passes of clippy nightly, and I'll probably refactor stat's format parser so it's less spaghetti-y; iirc stat is not the only coreutil that does use percent escaping (ls, date, du and printf come to my mind) so I feel strongly like adding a general impl to uu_core or common and refactoring these other coreutils. If you approve, I'll have it done by the end of next week (I'm done w/ my exams on Wednesday :) )

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/cksum/b2sum is no longer failing!

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/cksum/b2sum is no longer failing!

[sylvestre]

a bunch of jobs are still failing

[github-actions]

GNU testsuite comparison:

GNU test failed: tests/cp/cp-mv-enotsup-xattr. tests/cp/cp-mv-enotsup-xattr is passing on 'main'. Maybe you have to rebase?
Note: The gnu test tests/csplit/csplit-io-err was skipped on 'main' but is now failing.

[Alonely0]

a bunch of jobs are still failing

Correct me if I'm wrong, but it doesn't seem like any of the CI failures are related to the PR. Previously it was a bunch of cancelled jobs and a couple of network failures on rustup, and now it seems to be just a few unrelated failures? It's weird, because this does not touch uucore or anything that's shared across utils.

[Alonely0]

@sylvestre You menace, CI is gonna be busy for hours LOL... (4h and still counting...) /jk

[sylvestre]

@sylvestre You menace, CI is gonna be busy for hours LOL... (4h and still counting...) /jk

Yeah, I rebased almost all pr

[sylvestre]

See https://app.codecov.io/gh/uutils/coreutils/pull/9078?dropdown=coverage&src=pr&el=h1 for the tests improvements

[sylvestre]

please update mknod.rs also

[Alonely0]

please update mknod.rs also

There is also a makedev libc function, so it seems we can just use that.

[Alonely0]

I'll get to the FreeBSD shenanigans in a minute, after I'm done writing the extra tests.

[github-actions]

GNU testsuite comparison:

Congrats! The gnu test tests/tty/tty-eof is no longer failing!

[github-actions]

GNU testsuite comparison:

GNU test failed: tests/shuf/shuf-reservoir. tests/shuf/shuf-reservoir is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/sort/sort-stale-thread-mem. tests/sort/sort-stale-thread-mem is passing on 'main'. Maybe you have to rebase?

[Alonely0]

Should we add those two failing GNU tests (shuf-reservoir and sort-stale-thread-mem) to the intermittent list? It's not the first time I've seen them spuriously fail here, and it's not like we have touched them on this PR. The sort one smells a lot like a data race or stable sorting shenanigans, and shuf uses rng so it's a whole can of worms.

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

[Alonely0]

@sylvestre do you think this is ready to be merged or would any like any other changes? Lmk if I can help in any way.

[sylvestre]

would it be possible to improve the code coverage for R, H and L ?
see :

[Alonely0]

@sylvestre I mean, it's done as an integration test in test_stat.rs. If you mean adding a unit test to the module that essentially does the same, it's trivially done.

[sylvestre]

seems that the coverage isn't picking this then ? :)

[Alonely0]

seems that the coverage isn't picking this then ? :)

@sylvestre yeah, it does make sense because 613e9be tests the options as a raw string, tho I don't think there's really a better way to do this with the current stat codebase. It turns out that to do it as a unit test we have to construct a whole Stater instance and unit-test do_stat as a whole; I seriously doubt coverage would pick it up either, because this would be essentially equivalent to what I'm doing in test_stat. Personally, my disposition is to just disregard coverage's false negative, because otherwise we will probably have to refactor do_stat kinda heavily, convoluting the codebase only to compensate for an external tools' bugs... Lmk what are your thoughts on this.

[sylvestre]

Ok

[sylvestre]

Thanks!

[Alonely0]

Thanks!

To you! I'm sorry I had unnecessarily made the PR harder to review on accident... Anyway, thanks for merging this!

[omnivagant]

this seem to have solved a long standing issue I've had with uutils stat and Xen, that I've never managed to debug and figure out but that forced me to split it into a uutils-coreutils-stat subpackage in alpinelinux - looks like I can get rid of that separation now, thanks!