Skip to content

Make true return false less frequently#3014

Merged
sylvestre merged 6 commits intouutils:mainfrom
197g:main
Feb 2, 2022
Merged

Make true return false less frequently#3014
sylvestre merged 6 commits intouutils:mainfrom
197g:main

Conversation

@197g
Copy link
Contributor

@197g 197g commented Feb 1, 2022

This acknowledges a quirk in GNU true and makes true and false behave consistent to it.

Now treats recognized command line options and ignores unrecognized
command line options instead of returning a special exit status for
them.

true may return a non-true exit status (in particular EXIT_FAIL)
if writing the diagnostics of a GNU specific option fails. For example,
true --version > /dev/full would fail and have exit status 1. See commits
for more details.

This now passes tests/misc/false-status.sh.

Closes: #3007, #2996, #2989

Now treats recognized command line options and ignores unrecognized
command line options instead of returning a special exit status for
them.

There is one point of interest, which is related to an implementation
detail in GNU `true`. It may return a non-true exit status (in
particular EXIT_FAIL) if writing the diagnostics of a GNU specific
option fails. For example `true --version > /dev/full` would fail and
have exit status 1.

	This behavior was acknowledged in gnu in commit
	<9a6a486e6503520fd2581f2d3356b7149f1b225d>. No further
	justification provided for keeping this quirk.

POSIX knows no such options, and requires an exit status of 0 in all
cases. We replicate GNU here which is a consistency improvement over the
prior implementation. Adds documentation to clarify the intended
behavior more properly.
Copy link
Member

@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.

First of all, that title is amazing 😄 And the docs look great too! I think this makes a lot of sense. I wanted to suggest using AppSettings::IgnoreErrors at first, but it turns out that suppresses the help and version messages too, so the solution you found is the best we can do for now.

Could you add some tests for this behaviour?

@197g
Copy link
Contributor Author

197g commented Feb 1, 2022

While adding more exhaustive tests, I noticed that -h and -V are auto-generated by clap as well but these options do not work in GNU and are ignored. In the spirit of mimicking GNU even more closely I've modified App to hide these shorthands and tests verify that the output is indeed empty.

@tertsdiepraam
Copy link
Member

With that change, would it make sense to do it the other way around? I.e. use AppSettings::DisableHelpFlag and then create an arg that calls App::print_help if it is present? That way also only --help shows up in the documentation. The current solution is also fine, but feels more hacky.

197g added 3 commits February 1, 2022 13:02
This avoids hacking around the short options of these command line
arguments that have been introduced by clap. Additionally, we test and
correctly handle the combination of both version and help. The GNU
binary will ignore both arguments in this case while clap would perform
the first one. A test for this edge case was added.
@197g
Copy link
Contributor Author

197g commented Feb 1, 2022

Using dedicated options looks cleaner. At least the similarities, as well as differences, between true and false look more obvious to me now, with less nesting. In the whole process I found yet another edge case when combining both flags (true --version --help) which is treated like invalid command line options in GNU. A custom handler for these options was seemingly necessary in any case, help is now exclusive to trigger the empty error path in combination.

@tertsdiepraam
Copy link
Member

Who thought true and false could be so complicated :)

Nice work!

Copy link
Member

@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.

Awesome! Only one nit about the formatting of the about

Comment on lines 11 to 17
static ABOUT: &str = "
Returns false, an unsuccessful exit status.

Immediately returns with the exit status `1`. When invoked with one of the recognized options it
will try to write the help or version text. Any IO error during this operation is diagnosed, yet
the program will also return `1`.
";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static ABOUT: &str = "
Returns false, an unsuccessful exit status.
Immediately returns with the exit status `1`. When invoked with one of the recognized options it
will try to write the help or version text. Any IO error during this operation is diagnosed, yet
the program will also return `1`.
";
static ABOUT: &str = "\
Returns false, an unsuccessful exit status.
Immediately returns with the exit status `1`. When invoked with one of the recognized options it
will try to write the help or version text. Any IO error during this operation is diagnosed, yet
the program will also return `1`.
";

Comment on lines 11 to 17
static ABOUT: &str = "
Returns true, a successful exit status.

Immediately returns with the exit status `0`, except when invoked with one of the recognized
options. In those cases it will try to write the help or version text. Any IO error during this
operation causes the program to return `1` instead.
";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static ABOUT: &str = "
Returns true, a successful exit status.
Immediately returns with the exit status `0`, except when invoked with one of the recognized
options. In those cases it will try to write the help or version text. Any IO error during this
operation causes the program to return `1` instead.
";
static ABOUT: &str = "\
Returns true, a successful exit status.
Immediately returns with the exit status `0`, except when invoked with one of the recognized
options. In those cases it will try to write the help or version text. Any IO error during this
operation causes the program to return `1` instead.
";

@197g
Copy link
Contributor Author

197g commented Feb 1, 2022

Only one nit about the formatting of the about.

Good to know! Formatting was borrowed from base32, the first utility with all the options I was looking for as a reference. Might want to promote this comment to an issue for base32, base64, basenc as they share similar formatting in their About text.

@tertsdiepraam
Copy link
Member

Might want to promote this comment to an issue for base32, base64, basenc as they share similar formatting in their About text.

Oh I see. That's a good idea!

Copy link
Member

@rivy rivy left a comment

Choose a reason for hiding this comment

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

Looks good!
I like the revised logic.
👍🏻 for merge.

@sylvestre sylvestre merged commit b411d91 into uutils:main Feb 2, 2022
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.

true help page could be improved false help page could be improved

4 participants