Re: Git branch outputs usage message on stderr
On Wed, Jan 15, 2025 at 05:55:19PM +0100, Kristoffer Haugsbakk wrote:
Show 12 quoted lines
> On Wed, Jan 15, 2025, at 16:28, Junio C Hamano wrote:
> > Somebody may want to go over "git help --all" and for each of them
> > try "git $cmd -h >/dev/null" to find those that give the help output
> > to their standard error stream.
>
> #!/bin/sh
>
> for cmd in $(git --list-cmds=builtins); do
> git $cmd -h >/dev/null
> done 2>&1 | grep '^usage: ' \
> | perl -pe 's/^usage:\s*(\(EXPERIMENTAL!\)\s*)?//; s/^(git\s+[a-zA-Z0-9-]+).*/\1/'
> [...]
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 1d273d91c2..469cb12eb2 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -255,7 +255,8 @@ do
(
GIT_CEILING_DIRECTORIES=$(pwd) &&
export GIT_CEILING_DIRECTORIES &&
- test_expect_code 129 git -C sub $builtin -h >output 2>&1
+ test_expect_code 129 git -C sub $builtin -h >output 2>err &&
+ test_must_be_empty err
) &&
test_grep usage output
'
which produces a similar list. In the case of git-branch, it is due to
1dacfbcf13 (branch -h: show usage even in an invalid repository,
2010-10-22). Instead of letting parse-options handle it (which then goes
to stdout), we call usage_with_options(), which is usually for
complaining about a broken option, not showing "-h".
The reason there is that some of the pre-parse_options() setup accesses
the ref store (causing a BUG() if you run "branch -h" outside of a
repository). In this case, I think it can simply be reordered like:
diff --git a/builtin/branch.c b/builtin/branch.c
index 6e7b0cfddb..4617e32fff 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -784,31 +784,28 @@ int cmd_branch(int argc,
filter.kind = FILTER_REFS_BRANCHES;
filter.abbrev = -1;
- if (argc == 2 && !strcmp(argv[1], "-h"))
- usage_with_options(builtin_branch_usage, options);
-
/*
* Try to set sort keys from config. If config does not set any,
* fall back on default (refname) sorting.
*/
git_config(git_branch_config, &sorting_options);
if (!sorting_options.nr)
string_list_append(&sorting_options, "refname");
track = git_branch_track;
+ argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
+ 0);
+
head = refs_resolve_refdup(get_main_ref_store(the_repository), "HEAD",
0, &head_oid, NULL);
if (!head)
die(_("failed to resolve HEAD as a valid ref"));
if (!strcmp(head, "HEAD"))
filter.detached = 1;
else if (!skip_prefix(head, "refs/heads/", &head))
die(_("HEAD not found below refs/heads!"));
- argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
- 0);
-
if (!delete && !rename && !copy && !edit_description && !new_upstream &&
!show_current && !unset_upstream && argc == 0)
list = 1;
Knowing that is safe means confirming manually that setup code is not
needed by parse_options(). E.g., if it were setting defaults the user
could overwrite with an option. In this case neither "head" nor
"filter.detached" is touched by any of the options.
But there are a ton of commands, as you saw, and handling each one would
be a pain. So it would probably be easier to just introduce a variant of
usage_with_options() that writes to stdout (the underlying _internal
function already does so, we'd just need a one-liner wrapper). And then
use that everywhere. Possibly it could even do the argc/argv check, too,
since every call site is going to be doing that itself.
-Peff
Jeff King <peff@peff.net>