mbox series

[v3,0/6] Send help text from "git cmd -h" to stdout

Message ID 20250116012524.1557441-1-gitster@pobox.com (mailing list archive)
Headers show
Series Send help text from "git cmd -h" to stdout | expand

Message

Junio C Hamano Jan. 16, 2025, 1:25 a.m. UTC 
Jonas Konrad noticed[*] that "git branch -h" started showing the help
text to the standard error stream.  It turns out that we are fairly
inconsistent in our implementation of "git cmd -h".  The users of
parse-options API will get "If -h is the only option on the command
line, give the help text to the standard output" for free, but some
commands manually check for the condition and then call the
usage_with_options() function, which gives the identical help text
to the standard error stream.  And "git branch -h" Jonas noticed was
one of them.

Older commands written before parse-options API became dominant show
the help text by calling the usage() function, which is meant to be
used when they fail to parse their command line arguments, which has
the same problem.  An explicit request for help text "git cmd -h"
should be fulfilled by showing the help on the standard output.

This series teachs "git $cmd -h" to send its help text to the
standard output stream consistently for built-in commands.


[Reference]

https://lore.kernel.org/git/04cfaa3b-847f-4850-9dd6-c1cf9f72807f@uni-muenster.de/

Jeff King (1):
  t0012: optionally check that "-h" output goes to stdout

Junio C Hamano (5):
  parse-options: add show_usage_help_and_exit_if_asked()
  builtins: send usage_with_options() help text to standard output
  usage: add show_usage_and_exit_if_asked()
  builtin: send usage() help text to standard output
  builtin: send usage() help text to standard output

 builtin/am.c                |  3 +--
 builtin/branch.c            |  4 ++--
 builtin/check-ref-format.c  |  4 ++--
 builtin/checkout--worker.c  |  6 +++---
 builtin/checkout-index.c    |  6 +++---
 builtin/commit-tree.c       |  4 ++--
 builtin/commit.c            |  8 ++++----
 builtin/credential.c        |  3 ++-
 builtin/diff-files.c        |  3 +--
 builtin/diff-index.c        |  3 +--
 builtin/diff-tree.c         |  3 +--
 builtin/fast-import.c       |  3 +--
 builtin/fetch-pack.c        |  3 +++
 builtin/fsmonitor--daemon.c |  4 ++--
 builtin/gc.c                |  4 ++--
 builtin/get-tar-commit-id.c |  4 +++-
 builtin/index-pack.c        |  3 +--
 builtin/ls-files.c          |  4 ++--
 builtin/mailsplit.c         |  4 ++--
 builtin/merge-index.c       |  7 ++++++-
 builtin/merge-ours.c        |  3 +--
 builtin/merge-recursive.c   |  6 ++++++
 builtin/merge.c             |  4 ++--
 builtin/pack-redundant.c    |  3 +--
 builtin/rebase.c            |  6 +++---
 builtin/remote-ext.c        |  2 ++
 builtin/remote-fd.c         |  1 +
 builtin/rev-list.c          |  3 +--
 builtin/rev-parse.c         |  2 ++
 builtin/unpack-file.c       |  8 ++++++--
 builtin/unpack-objects.c    |  2 ++
 builtin/update-index.c      |  4 ++--
 builtin/upload-archive.c    |  6 +++---
 builtin/var.c               |  1 +
 git-compat-util.h           |  2 ++
 parse-options.c             | 10 ++++++++++
 parse-options.h             |  4 ++++
 t/helper/test-simple-ipc.c  |  4 ++--
 t/t0012-help.sh             |  3 ++-
 t/t7600-merge.sh            |  2 +-
 usage.c                     | 28 +++++++++++++++++++++++++---
 41 files changed, 123 insertions(+), 64 deletions(-)

Comments

Jeff King Jan. 16, 2025, 10:46 a.m. UTC | #1 
On Wed, Jan 15, 2025 at 05:25:17PM -0800, Junio C Hamano wrote:

> Jonas Konrad noticed[*] that "git branch -h" started showing the help
> text to the standard error stream.  It turns out that we are fairly
> inconsistent in our implementation of "git cmd -h".  The users of
> parse-options API will get "If -h is the only option on the command
> line, give the help text to the standard output" for free, but some
> commands manually check for the condition and then call the
> usage_with_options() function, which gives the identical help text
> to the standard error stream.  And "git branch -h" Jonas noticed was
> one of them.
> 
> Older commands written before parse-options API became dominant show
> the help text by calling the usage() function, which is meant to be
> used when they fail to parse their command line arguments, which has
> the same problem.  An explicit request for help text "git cmd -h"
> should be fulfilled by showing the help on the standard output.
> 
> This series teachs "git $cmd -h" to send its help text to the
> standard output stream consistently for built-in commands.

I had a small complaint in patch 4, but otherwise this looks good to me.

If we want to switch the exit code for this case from 129 to 0, I think
we could easily do so on top (it would need modifications in three
places, but now that you've untangled all of the individual builtins,
that would get all of them).

I guess there may be non-builtins that would need to be handled
individually, though. We don't have too many of them these days, but
they are not covered by t0012.

-Peff
Junio C Hamano Jan. 16, 2025, 5:28 p.m. UTC | #2 
Jeff King <peff@peff.net> writes:

> If we want to switch the exit code for this case from 129 to 0, I think
> we could easily do so on top (it would need modifications in three
> places, but now that you've untangled all of the individual builtins,
> that would get all of them).

Yes, I consider it pretty much an orthogonal issue to update the
exit status.

> I guess there may be non-builtins that would need to be handled
> individually, though. We don't have too many of them these days, but
> they are not covered by t0012.

Yes.  We can probably leave them as they are, as we have established
our expectation with this series (i.e. an explicit end user request
for help text should emit to the standard output stream), so any bug
report can be handled without needing any policy discussion.  "Hey,
I noticed 'git cmd -h' writes to stderr, not stdout"---"Thanks, cmd
need to be fixed".