tail: fix GNU test 'misc/tail' #4347
Conversation
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
bbara
force-pushed
the
tail-gnu
branch
2 times, most recently
from
b482c18
to
61f5f8a
Compare
|
GNU testsuite comparison: |
This is exciting! |
|
Not sure why Also, I am running the cargo tests with |
It might be just an intermittently failing test. If it keeps happening we can look into it, but I think it's unrelated to this change.
Yes, you need |
|
Fine thx. I fixed the unit tests and now I'll wait for the code coverage report. I guess I have to add some parse unit tests after that. |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
bbara
force-pushed
the
tail-gnu
branch
5 times, most recently
from
ef8087f
to
05cd035
Compare
|
GNU testsuite comparison: |
bbara
force-pushed
the
tail-gnu
branch
4 times, most recently
from
e79504d
to
b592dc8
Compare
bbara
force-pushed
the
tail-gnu
branch
2 times, most recently
from
d404ba8
to
b5b6d20
Compare
|
Is there a specific reason why the arguments are maintained as an iterator and not something better index- and rewind-able (e.g. something |
|
It's probably because |
|
I guess I am finished. What do you think @tertsdiepraam? :) |
There was a problem hiding this comment.
I found another cursed edge case:
tail -f -fShould try to read the file called -f. Now technically, your implementation does this correctly. However, that is due to another bug, because the current implementation doesn't allow -f to override itself. For example,
tail -f -f -fshould be allowed. If we fix that by adding .overrides_with(option::FOLLOW) to clap. Then -f -f is not parsed as obsolete syntax in your implementation.
This shows that the obsolete syntax has priority over parsing in clap, despite what the documentation states. So I think we should return to trying to parse the obsolete syntax before clap.
|
GNU testsuite comparison: |
bbara
force-pushed
the
tail-gnu
branch
2 times, most recently
from
5278a5e
to
fc1abd3
Compare
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
src/uu/tail/src/args.rs
Outdated
| // there are cases of ambiguity between clap's parsing and obsolete syntax parsing. | ||
| // e.g. "tail -c 5", which is a valid flag (handled by clap), | ||
| // but could also be interpreted as valid obsolete syntax (tail -c on file '5'). | ||
| // in this case, the clap result should be returned immediately. | ||
| // clap fails for unknown flags, but valid obsolete arguments (e.g. tail -l; tail -10c). | ||
| // clap succeeds for obsolete arguments starting with '+', but misinterprets them as | ||
| // input files (e.g. tail +f). | ||
| // in these cases, the argv[1] should be checked further. |
There was a problem hiding this comment.
Nice, I like that you explained more cases than I did, but let's bring back some of the formatting I had in my version and fix some of the punctuation. I tried to combine our approaches (again, feel free to improve further):
// At this point, there are a few possible cases:
//
// 1. clap has succeeded and the arguments would be invalid for the obsolete syntax.
// 2. The case of `tail -c 5` is ambiguous. clap parses this as `tail -c5`,
// but it could also be interpreted as valid obsolete syntax (tail -c on file '5').
// GNU chooses to interpret this as `tail -c5`, like clap.
// 3. `tail -f foo` is also ambiguous, but has the same effect in both cases. We can safely
// use the clap result here.
// 4. clap succeeded for obsolete arguments starting with '+', but misinterprets them as
// input files (e.g. tail +f).
// 5. clap failed because of unknown flags, but possibly valid obsolete arguments
// (e.g. tail -l; tail -10c).
//
// In cases 4 & 5, we want to try parsing the obsolete arguments, which corresponds to
// checking whether clap succeeded or the first argument starts with '+'.There was a problem hiding this comment.
Thanks for helping out with the format!
|
GNU testsuite comparison: |
Hi!
This PR targets compatibility to
gnu/tests/misc/tail.pl.It's not fully achieved (yet) as some error messages don't fit 1:1 (e.g. a couple of additional
Try 'tail --help' for more information.). Most of the code is to work around clap, which interprets a couple of arguments as input files.I imported most of the perl tests to the code base.
Please feel free to respond if I should handle something differently (add tests/comments, split commit, ...) or if this is not needed at all 😄
Best regards
bb