[PATCH 0/4] permit -h/--help-all in more scenarios

[PATCH 0/4] permit -h/--help-all in more scenarios

[D. Ben Knoble]

This series depends on ua/t1517-short-help-tests with some fixes, which show up
in the first patch. Merge that branch to a new topic branch, then apply this
series.

This series enables --help-all outside of repository contexts, and
allows -h with other arguments (without breaking existing ls-remote/grep
usage).

It consists of preparatory steps (fixes for a dependency branch;
refactoring to make an internal helper's arguments clearer) followed by
the main commits.

D. Ben Knoble (4):
  t1517: fixup for ua/t1517-short-help-tests
  parse-options: name flags passed to usage_with_options_internal
  builtin: also setup gently for --help-all
  builtins: show help on "-h"/"--help-all" with more than 2 arguments
    left

 builtin/merge-recursive.c     |  3 ++-
 git.c                         |  2 +-
 parse-options.c               | 41 +++++++++++++++++++++++++----------
 t/t1517-outside-repo.sh       |  9 +++++---
 t/t5200-update-server-info.sh |  2 +-
 usage.c                       |  3 ++-
 6 files changed, 41 insertions(+), 19 deletions(-)


base-commit: e4ef0485fd78fcb05866ea78df35796b904e4a8e
prerequisite-patch-id: ffce2dd036e61c8d36485a17321f858e454db874
prerequisite-patch-id: 52539022c824997adfc1be0bed8de6b1851d2187
-- 
2.48.1

[PATCH 1/4] t1517: fixup for ua/t1517-short-help-tests

[D. Ben Knoble]

- drop spurious message during test
- fix known breakages that actually work
- fix new t5200 test

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---

I expect this and other fixes to get squashed into the upstream branch, but I'm
including it here so it's easy to create a clean build.

 t/t1517-outside-repo.sh       | 5 ++---
 t/t5200-update-server-info.sh | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index 9443c0284f..e235ecccde 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -115,8 +115,8 @@
 	difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \
 	http-backend | http-fetch | http-push | init-db | instaweb.sh | \
 	merge-octopus | merge-one-file | merge-resolve | mergetool | \
-	mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \
-	remote-http | remote-https | replay | request-pull | send-email | \
+	mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \
+	remote-http | remote-https | replay | send-email | \
 	sh-i18n--envsubst | shell | show | stage | submodule | svn | \
 	upload-archive--writer | upload-pack | web--browse | whatchanged)
 		expect_outcome=expect_failure ;;
@@ -125,7 +125,6 @@
 	esac
 	test_$expect_outcome "'git $cmd -h' outside a repository" '
 		test_expect_code 129 nongit git $cmd -h >usage &&
-		echo "Hello" &&
 		test_grep "[Uu]sage: git $cmd " usage
 	'
 done
diff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh
index a1f129db4e..a551e955b5 100755
--- a/t/t5200-update-server-info.sh
+++ b/t/t5200-update-server-info.sh
@@ -48,7 +48,7 @@
 
 test_expect_success 'update-server-info does not crash with -h' '
 	test_expect_code 129 git update-server-info -h >usage &&
-	test_grep "[Uu]sage: git update-server-info " usage &&
+	test_grep "[Uu]sage: git update-server-info " usage
 '
 
 test_done
-- 
2.48.1

Re: [PATCH 1/4] t1517: fixup for ua/t1517-short-help-tests

[Usman Akinyemi]

> - fix known breakages that actually work
> - fix new t5200 test
Thanks for fixing this, I do not have to send the update version.
>
> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> ---
>
> I expect this and other fixes to get squashed into the upstream branch, but I'm
> including it here so it's easy to create a clean build.
>

>         difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \
>         http-backend | http-fetch | http-push | init-db | instaweb.sh | \
>         merge-octopus | merge-one-file | merge-resolve | mergetool | \
> -       mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \
> -       remote-http | remote-https | replay | request-pull | send-email | \
> +       mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \
> +       remote-http | remote-https | replay | send-email | \
Thanks
>         sh-i18n--envsubst | shell | show | stage | submodule | svn | \
>         upload-archive--writer | upload-pack | web--browse | whatchanged)
>                 expect_outcome=expect_failure ;;
> @@ -125,7 +125,6 @@

>
>  test_expect_success 'update-server-info does not crash with -h' '
>         test_expect_code 129 git update-server-info -h >usage &&
> -       test_grep "[Uu]sage: git update-server-info " usage &&
> +       test_grep "[Uu]sage: git update-server-info " usage
>  '
Looks good to me.
Thanks.
>

Re: [PATCH 1/4] t1517: fixup for ua/t1517-short-help-tests

[Junio C Hamano]

Usman Akinyemi <usmanakinyemi202@gmail.com> writes:

>> I expect this and other fixes to get squashed into the upstream branch, but I'm
>> including it here so it's easy to create a clean build.
>>
>
>>         difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \
>>         http-backend | http-fetch | http-push | init-db | instaweb.sh | \
>>         merge-octopus | merge-one-file | merge-resolve | mergetool | \
>> -       mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \
>> -       remote-http | remote-https | replay | request-pull | send-email | \
>> +       mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \
>> +       remote-http | remote-https | replay | send-email | \
> Thanks
>>         sh-i18n--envsubst | shell | show | stage | submodule | svn | \
>>         upload-archive--writer | upload-pack | web--browse | whatchanged)
>>                 expect_outcome=expect_failure ;;
>> @@ -125,7 +125,6 @@
>
>>
>>  test_expect_success 'update-server-info does not crash with -h' '
>>         test_expect_code 129 git update-server-info -h >usage &&
>> -       test_grep "[Uu]sage: git update-server-info " usage &&
>> +       test_grep "[Uu]sage: git update-server-info " usage
>>  '
> Looks good to me.
> Thanks.

So, can I ignore this step from the series and expect the fixes to
be already in your updated series we will see in the future?

Thanks.

[PATCH 2/4] parse-options: name flags passed to usage_with_options_internal

[D. Ben Knoble]

When reading or editing calls to usage_with_options_internal, it is
difficult to tell what trailing "0, 0", "0, 1", "1, 0" arguments mean
(NB there is never a "1, 1" case).

Give the flags readable names to improve call-sites.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 parse-options.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 5224203ffe..c3222cc9bb 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -953,10 +953,21 @@ static void free_preprocessed_options(struct option *options)
 	free(options);
 }
 
+enum usage_style {
+	style_normal = 0,
+	style_full = 1,
+};
+
+enum usage_output {
+	to_out = 0,
+	to_err = 1,
+};
+
 static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,
 							 const char * const *,
 							 const struct option *,
-							 int, int);
+							 enum usage_style,
+							 enum usage_output);
 
 enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 					 const struct option *options,
@@ -1088,7 +1099,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 		}
 
 		if (internal_help && !strcmp(arg + 2, "help-all"))
-			return usage_with_options_internal(ctx, usagestr, options, 1, 0);
+			return usage_with_options_internal(ctx, usagestr, options, style_full, to_out);
 		if (internal_help && !strcmp(arg + 2, "help"))
 			goto show_usage;
 		switch (parse_long_opt(ctx, arg + 2, options)) {
@@ -1129,7 +1140,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 	return PARSE_OPT_DONE;
 
  show_usage:
-	return usage_with_options_internal(ctx, usagestr, options, 0, 0);
+	return usage_with_options_internal(ctx, usagestr, options, style_normal, to_out);
 }
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
@@ -1278,10 +1289,11 @@ static const struct option *find_option_by_long_name(const struct option *opts,
 static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 							 const char * const *usagestr,
 							 const struct option *opts,
-							 int full, int err)
+							 enum usage_style help_style,
+							 enum usage_output to_where)
 {
 	const struct option *all_opts = opts;
-	FILE *outfile = err ? stderr : stdout;
+	FILE *outfile = to_where == to_err ? stderr : stdout;
 	int need_newline;
 
 	const char *usage_prefix = _("usage: %s");
@@ -1327,7 +1339,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
-	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
+	if (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
 		fprintf(outfile, "cat <<\\EOF\n");
 
 	while (*usagestr) {
@@ -1373,7 +1385,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 				fprintf(outfile, "%s\n", _(opts->help));
 			continue;
 		}
-		if (!full && (opts->flags & PARSE_OPT_HIDDEN))
+		if (help_style != style_full && (opts->flags & PARSE_OPT_HIDDEN))
 			continue;
 
 		if (need_newline) {
@@ -1435,7 +1447,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 	}
 	fputc('\n', outfile);
 
-	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
+	if (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
 		fputs("EOF\n", outfile);
 
 	return PARSE_OPT_HELP;
@@ -1444,7 +1456,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 void NORETURN usage_with_options(const char * const *usagestr,
 			const struct option *opts)
 {
-	usage_with_options_internal(NULL, usagestr, opts, 0, 1);
+	usage_with_options_internal(NULL, usagestr, opts, style_normal, to_err);
 	exit(129);
 }
 
@@ -1453,7 +1465,7 @@ void show_usage_with_options_if_asked(int ac, const char **av,
 				      const struct option *opts)
 {
 	if (ac == 2 && !strcmp(av[1], "-h")) {
-		usage_with_options_internal(NULL, usagestr, opts, 0, 0);
+		usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
 		exit(129);
 	}
 }
-- 
2.48.1

Re: [PATCH 2/4] parse-options: name flags passed to usage_with_options_internal

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

> When reading or editing calls to usage_with_options_internal, it is
> difficult to tell what trailing "0, 0", "0, 1", "1, 0" arguments mean
> (NB there is never a "1, 1" case).
>
> Give the flags readable names to improve call-sites.

It is a good idea to explicitly say that this step introduces no
change in behaviour, and only changes the way how these 0/1 are
spelled.

> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> ---
>  parse-options.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 5224203ffe..c3222cc9bb 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -953,10 +953,21 @@ static void free_preprocessed_options(struct option *options)
>  	free(options);
>  }
>  
> +enum usage_style {
> +	style_normal = 0,
> +	style_full = 1,
> +};
> +
> +enum usage_output {
> +	to_out = 0,
> +	to_err = 1,
> +};

These are very much internal implementation detail, so I am not sure
if this churn is a good thing, though.

For example, it ought to be sufficient, for the purpose of improved
readability, to instead doing this

>  static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,
>  							 const char * const *,
>  							 const struct option *,
> -							 int, int);
> +							 enum usage_style,
> +							 enum usage_output);

just do

		int full_usage,
		int usage_to_stderr);

here.  Dropping the parameter names in the function prototype is
allowed, and we encourage to do so in our codebase but _only_ when
the meaning of each parameter is obvious from their type.  The first
3 parameters we see above are of distinct types and except for the
second one being the usage string given to the users, they should be
obvious.  But the last two unnamed integers are not obvious and they
should have been spelled out---otherwise a developer who is adding
a new callsite cannot work from the prototype alone and has to go to
the implementation to figure out what to pass.

Adding two enums for this is a bit overkill, but is OK here locally.


> @@ -1088,7 +1099,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
>  		}
>  
>  		if (internal_help && !strcmp(arg + 2, "help-all"))
> -			return usage_with_options_internal(ctx, usagestr, options, 1, 0);
> +			return usage_with_options_internal(ctx, usagestr, options, style_full, to_out);

But this is not an improvement as-is.  Wrap long lines or the result
is even harder to read.

> @@ -1278,10 +1289,11 @@ static const struct option *find_option_by_long_name(const struct option *opts,
>  static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,
>  							 const char * const *usagestr,
>  							 const struct option *opts,
> -							 int full, int err)
> +							 enum usage_style help_style,
> +							 enum usage_output to_where)
>  {
>  	const struct option *all_opts = opts;

> -	FILE *outfile = err ? stderr : stdout;
> +	FILE *outfile = to_where == to_err ? stderr : stdout;

This one ...


> @@ -1327,7 +1339,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
> -	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
> +	if (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
>  		fprintf(outfile, "cat <<\\EOF\n");

... and this one become markedly harder to read.  I think the
primary reason is because unlike the original, the parameter names
are not biased.  "If we are doing full usage, do this" is far easier
to grok than "If the "style" we are told to use is the "style_full",
then do this", but use of "enum" inherently is about not making the
variables and parameters of that enum type unbiased.

> @@ -1373,7 +1385,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
>  				fprintf(outfile, "%s\n", _(opts->help));
>  			continue;
>  		}
> -		if (!full && (opts->flags & PARSE_OPT_HIDDEN))
> +		if (help_style != style_full && (opts->flags & PARSE_OPT_HIDDEN))
>  			continue;

Ditto.

>  		if (need_newline) {
> @@ -1435,7 +1447,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
>  	}
>  	fputc('\n', outfile);
>  
> -	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
> +	if (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
>  		fputs("EOF\n", outfile);

Ditto.

One way to reduce this churn is to do

	int err = (to_where == to_stderr);
	int full = (help_style == style_full);

at the very beginning of the function.  Then you do not have to
change the body of the function harder to read at all.

Thanks.

Re: [PATCH 2/4] parse-options: name flags passed to usage_with_options_internal

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> For example, it ought to be sufficient, for the purpose of improved
> readability, to instead doing this
>
>>  static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,
>>  							 const char * const *,
>>  							 const struct option *,
>> -							 int, int);
>> +							 enum usage_style,
>> +							 enum usage_output);
>
> just do
>
> 		int full_usage,
> 		int usage_to_stderr);
>
> here.  Dropping the parameter names in the function prototype is
> allowed, and we encourage to do so in our codebase but _only_ when
> the meaning of each parameter is obvious from their type.  The first
> 3 parameters we see above are of distinct types and except for the
> second one being the usage string given to the users, they should be
> obvious.  But the last two unnamed integers are not obvious and they
> should have been spelled out---otherwise a developer who is adding
> a new callsite cannot work from the prototype alone and has to go to
> the implementation to figure out what to pass.
>
> Adding two enums for this is a bit overkill, but is OK here locally.


And with that lessor impact change, you could still add a smaller
change to help callers (you do not need any change to the callee,
which uses biased parameter names for these two), if you wanted to
(though as I said this is internal implementation detail of the
parse_options API, so you do not have to).  For example, you could
do

#define USAGE_FULL 1
#define USAGE_TO_STDERR 1

without anything else and do

>> @@ -1088,7 +1099,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
>>  		}
>>  
>>  		if (internal_help && !strcmp(arg + 2, "help-all"))
>> -			return usage_with_options_internal(ctx, usagestr, options, 1, 0);
>> +			return usage_with_options_internal(ctx, usagestr, options, style_full, to_out);

			return usage_with_options_internal(ctx,	usagestr, options,
							   USAGE_FULL, 0);

which may make it easier to follow.  The point is that you be more
verbose only when you do a non-standard thing.

And without enum, of course you do not need any change like below.

> One way to reduce this churn is to do
>
> 	int err = (to_where == to_stderr);
> 	int full = (help_style == style_full);
>
> at the very beginning of the function.  Then you do not have to
> change the body of the function harder to read at all.

Re: [PATCH 2/4] parse-options: name flags passed to usage_with_options_internal

[D. Ben Knoble]

On Mon, Jul 28, 2025 at 11:26 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
>
> > When reading or editing calls to usage_with_options_internal, it is
> > difficult to tell what trailing "0, 0", "0, 1", "1, 0" arguments mean
> > (NB there is never a "1, 1" case).
> >
> > Give the flags readable names to improve call-sites.
>
> It is a good idea to explicitly say that this step introduces no
> change in behaviour, and only changes the way how these 0/1 are
> spelled.

Woops; definitely meant for that to be clearer. Will touch up.

>
> > Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> > ---
> >  parse-options.c | 32 ++++++++++++++++++++++----------
> >  1 file changed, 22 insertions(+), 10 deletions(-)
> >
> > diff --git a/parse-options.c b/parse-options.c
> > index 5224203ffe..c3222cc9bb 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -953,10 +953,21 @@ static void free_preprocessed_options(struct option *options)
> >       free(options);
> >  }
> >
> > +enum usage_style {
> > +     style_normal = 0,
> > +     style_full = 1,
> > +};
> > +
> > +enum usage_output {
> > +     to_out = 0,
> > +     to_err = 1,
> > +};
>
> These are very much internal implementation detail, so I am not sure
> if this churn is a good thing, though.
>
> For example, it ought to be sufficient, for the purpose of improved
> readability, to instead doing this
>
> >  static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,
> >                                                        const char * const *,
> >                                                        const struct option *,
> > -                                                      int, int);
> > +                                                      enum usage_style,
> > +                                                      enum usage_output);
>
> just do
>
>                 int full_usage,
>                 int usage_to_stderr);
>
> here.  Dropping the parameter names in the function prototype is
> allowed, and we encourage to do so in our codebase but _only_ when
> the meaning of each parameter is obvious from their type.  The first
> 3 parameters we see above are of distinct types and except for the
> second one being the usage string given to the users, they should be
> obvious.  But the last two unnamed integers are not obvious and they
> should have been spelled out---otherwise a developer who is adding
> a new callsite cannot work from the prototype alone and has to go to
> the implementation to figure out what to pass.

Yeah, but that relies on folks reading the prototype, no? I wanted it
to be easier to read at the call sites (_without_ special tooling,
preferably).

I'll snip the rest, though, due to your downthread suggestion to use
#define's instead, which I think gives the result I want without extra
churn in other places.


> > @@ -1088,7 +1099,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
> >               }
> >
> >               if (internal_help && !strcmp(arg + 2, "help-all"))
> > -                     return usage_with_options_internal(ctx, usagestr, options, 1, 0);
> > +                     return usage_with_options_internal(ctx, usagestr, options, style_full, to_out);
>
> But this is not an improvement as-is.  Wrap long lines or the result
> is even harder to read.

Ah, indeed: I wasn't sure where to draw the line there.

[PATCH 3/4] builtin: also setup gently for --help-all

[D. Ben Knoble]

Git experts often check the help summary of a command to make sure they
spell options right when suggesting advice to colleagues. Further, they
might check hidden options when responding to queries about deprecated
options like git-rebase(1)'s "preserve merges" option. But some commands
don't support "--help-all" outside of a git directory. Running (for
example)

    git rebase --help-all

outside a directory fails in "setup_git_directory", erroring with the
localized form of

    fatal: not a git repository (or any of the parent directories): .git

Like 99caeed05d (Let 'git <command> -h' show usage without a git dir,
2009-11-09), we want to show the "--help-all" output even without a git
dir. Make "--help-all" where we expect "-h" to mean
"setup_git_directory_gently", and interpose early in the natural place
("show_usage_with_options_if_asked").

Do the same for usage callers with show_usage_if_asked.

The exception is merge-recursive, whose help block doesn't use newer
APIs.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---

Notes:
    On interposition points:
    
    I originally considered leaving out the changes to
    show_usage_with_options_if_asked and relying on the parse-options API to
    do the right thing. Unfortunately, most commands can't make it all the
    way to parse-options when setting up gently, and trying to parse options
    without a repo creates myriad dependency problems (like: we might read
    config after parsing CLI options, so we have to make sure the parsed
    options overrride config).
    
    Some usage.c callers, like check-ref-format, probably deserve to be
    ported to parse-options at this point.
    
    I opted not to do anything too invasive with merge-recursive (like a
    prepatory "migrate to newer usage APIs") since I think it's going the
    way of the dodo?

 builtin/merge-recursive.c |  3 ++-
 git.c                     |  2 +-
 parse-options.c           | 11 ++++++++---
 t/t1517-outside-repo.sh   |  4 ++++
 usage.c                   |  3 ++-
 5 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 03b5100cfa..17aa4db37a 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -38,7 +38,8 @@ int cmd_merge_recursive(int argc,
 	if (argv[0] && ends_with(argv[0], "-subtree"))
 		o.subtree_shift = "";
 
-	if (argc == 2 && !strcmp(argv[1], "-h")) {
+	if (argc == 2 && (!strcmp(argv[1], "-h") ||
+			  !strcmp(argv[1], "--help-all"))) {
 		struct strbuf msg = STRBUF_INIT;
 		strbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);
 		show_usage_if_asked(argc, argv, msg.buf);
diff --git a/git.c b/git.c
index 07a5fe39fb..40d3df1b76 100644
--- a/git.c
+++ b/git.c
@@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	const char *prefix;
 	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
 
-	help = argc == 2 && !strcmp(argv[1], "-h");
+	help = argc == 2 && (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help-all"));
 	if (help && (run_setup & RUN_SETUP))
 		/* demote to GENTLY to allow 'git cmd -h' outside repo */
 		run_setup = RUN_SETUP_GENTLY;
diff --git a/parse-options.c b/parse-options.c
index c3222cc9bb..e3ed42f709 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1464,9 +1464,14 @@ void show_usage_with_options_if_asked(int ac, const char **av,
 				      const char * const *usagestr,
 				      const struct option *opts)
 {
-	if (ac == 2 && !strcmp(av[1], "-h")) {
-		usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
-		exit(129);
+	if (ac == 2) {
+		if (!strcmp(av[1], "-h")) {
+			usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
+			exit(129);
+		} else if (!strcmp(av[1], "--help-all")) {
+			usage_with_options_internal(NULL, usagestr, opts, style_full, to_out);
+			exit(129);
+		}
 	}
 }
 
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index e235ecccde..b26a03d8a0 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -127,6 +127,10 @@
 		test_expect_code 129 nongit git $cmd -h >usage &&
 		test_grep "[Uu]sage: git $cmd " usage
 	'
+	test_$expect_outcome "'git $cmd --help-all' outside a repository" '
+		test_expect_code 129 nongit git $cmd --help-all >usage &&
+		test_grep "[Uu]sage: git $cmd " usage
+	'
 done
 
 test_expect_success 'prune does not crash with -h' '
diff --git a/usage.c b/usage.c
index 81913236a4..4c245ba0cb 100644
--- a/usage.c
+++ b/usage.c
@@ -192,7 +192,8 @@ static void show_usage_if_asked_helper(const char *err, ...)
 
 void show_usage_if_asked(int ac, const char **av, const char *err)
 {
-	if (ac == 2 && !strcmp(av[1], "-h"))
+	if (ac == 2 && (!strcmp(av[1], "-h") ||
+			!strcmp(av[1], "--help-all")))
 		show_usage_if_asked_helper(err);
 }
 
-- 
2.48.1

Re: [PATCH 3/4] builtin: also setup gently for --help-all

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

>     I originally considered leaving out the changes to
>     show_usage_with_options_if_asked and relying on the parse-options API to
>     do the right thing. Unfortunately, most commands can't make it all the
>     way to parse-options when setting up gently, and trying to parse options
>     without a repo creates myriad dependency problems (like: we might read
>     config after parsing CLI options, so we have to make sure the parsed
>     options overrride config).
>     
>     Some usage.c callers, like check-ref-format, probably deserve to be
>     ported to parse-options at this point.

It is unclear what you mean by all of the above, but hopefully it
would become clear as we read the code changes.

>     I opted not to do anything too invasive with merge-recursive (like a
>     prepatory "migrate to newer usage APIs") since I think it's going the
>     way of the dodo?

I think this is fine.

> diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
> index 03b5100cfa..17aa4db37a 100644
> --- a/builtin/merge-recursive.c
> +++ b/builtin/merge-recursive.c
> @@ -38,7 +38,8 @@ int cmd_merge_recursive(int argc,
>  	if (argv[0] && ends_with(argv[0], "-subtree"))
>  		o.subtree_shift = "";
>  
> -	if (argc == 2 && !strcmp(argv[1], "-h")) {
> +	if (argc == 2 && (!strcmp(argv[1], "-h") ||
> +			  !strcmp(argv[1], "--help-all"))) {
>  		struct strbuf msg = STRBUF_INIT;
>  		strbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);
>  		show_usage_if_asked(argc, argv, msg.buf);

;-)

> diff --git a/git.c b/git.c
> index 07a5fe39fb..40d3df1b76 100644
> --- a/git.c
> +++ b/git.c
> @@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
>  	const char *prefix;
>  	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
>  
> -	help = argc == 2 && !strcmp(argv[1], "-h");
> +	help = argc == 2 && (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help-all"));
>  	if (help && (run_setup & RUN_SETUP))
>  		/* demote to GENTLY to allow 'git cmd -h' outside repo */
>  		run_setup = RUN_SETUP_GENTLY;

OK.  That's obvious and straight-forward.  Behave as closely as the
case when "-h" is given, and let the lower-layer deal with the
differences between "-h" and "--help-all".

> diff --git a/parse-options.c b/parse-options.c
> index c3222cc9bb..e3ed42f709 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -1464,9 +1464,14 @@ void show_usage_with_options_if_asked(int ac, const char **av,
>  				      const char * const *usagestr,
>  				      const struct option *opts)
>  {
> -	if (ac == 2 && !strcmp(av[1], "-h")) {
> -		usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
> -		exit(129);
> +	if (ac == 2) {
> +		if (!strcmp(av[1], "-h")) {
> +			usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
> +			exit(129);
> +		} else if (!strcmp(av[1], "--help-all")) {
> +			usage_with_options_internal(NULL, usagestr, opts, style_full, to_out);
> +			exit(129);
> +		}
>  	}
>  }

Again, that is obvious and straight-forward.

> diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
> index e235ecccde..b26a03d8a0 100755
> --- a/t/t1517-outside-repo.sh
> +++ b/t/t1517-outside-repo.sh
> @@ -127,6 +127,10 @@
>  		test_expect_code 129 nongit git $cmd -h >usage &&
>  		test_grep "[Uu]sage: git $cmd " usage
>  	'
> +	test_$expect_outcome "'git $cmd --help-all' outside a repository" '
> +		test_expect_code 129 nongit git $cmd --help-all >usage &&
> +		test_grep "[Uu]sage: git $cmd " usage
> +	'
>  done
>  
>  test_expect_success 'prune does not crash with -h' '
> diff --git a/usage.c b/usage.c
> index 81913236a4..4c245ba0cb 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -192,7 +192,8 @@ static void show_usage_if_asked_helper(const char *err, ...)
>  
>  void show_usage_if_asked(int ac, const char **av, const char *err)
>  {
> -	if (ac == 2 && !strcmp(av[1], "-h"))
> +	if (ac == 2 && (!strcmp(av[1], "-h") ||
> +			!strcmp(av[1], "--help-all")))
>  		show_usage_if_asked_helper(err);
>  }

This looks good.

I don't understand your "I originally considered leaving out ..." at
all.  We are special casing a lone "-h" here already because we know
this is where we should stop without exercising unnecessary code
because we may not even have repo!=NULL and that is the reason why
this function exists in the first place.  It is obvious to me that
we need the same special casing for "--help-all".

In other words, I think all the above changes are good.

Thanks.

Re: [PATCH 3/4] builtin: also setup gently for --help-all

[D. Ben Knoble]

On Mon, Jul 28, 2025 at 11:33 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
>
> >     I originally considered leaving out the changes to
> >     show_usage_with_options_if_asked and relying on the parse-options API to
> >     do the right thing. Unfortunately, most commands can't make it all the
> >     way to parse-options when setting up gently, and trying to parse options
> >     without a repo creates myriad dependency problems (like: we might read
> >     config after parsing CLI options, so we have to make sure the parsed
> >     options overrride config).
> >
> >     Some usage.c callers, like check-ref-format, probably deserve to be
> >     ported to parse-options at this point.
>
> It is unclear what you mean by all of the above, but hopefully it
> would become clear as we read the code changes.
>
> [snip]
>
> I don't understand your "I originally considered leaving out ..." at
> all.  We are special casing a lone "-h" here already because we know
> this is where we should stop without exercising unnecessary code
> because we may not even have repo!=NULL and that is the reason why
> this function exists in the first place.  It is obvious to me that
> we need the same special casing for "--help-all".
>
> In other words, I think all the above changes are good.
>
> Thanks.

I'm glad it was obvious to you! I spent a lot of time trying to get
commands to call parse_options early, before touching any repository
variables, since that was the only place where I knew "--help-all" was
handled. This turned out to be a terrible mess, so I looked around for
a different path forward (the one here now), and that turned out to
work much better.

In that old world, parse-options doesn't change and the builtins need
a lot of surgery (and probably still aren't completely right)—I can
see how that note is confusing without that context, though, and I'll
drop it in future rounds (esp. now we've discussed this version).

[PATCH 4/4] builtins: show help on "-h"/"--help-all" with more than 2 arguments left

[D. Ben Knoble]

When asking for short help on a previous command, the user may use their
shell history to recall a command like

    git rebase new-base

Then inserting "-h" after "rebase" doesn't yield the help; make it so.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---

Notes:
    I reviewed the following results
    
        git grep '[^[:alpha:]-]-h' 'Documentation/**adoc' ':(exclude)Documentation/RelNotes'
    
    The 2 commands that accept "-h" to mean other things are git-grep(1) and
    git-ls-remote(1).
    
    - "git grep -h 123" gives results sans filenames, while a lone "-h"
      gives help. Behavior is unchanged from 2.48.1 (my local install).
    - "git ls-remote -h" gives help; "git ls-remote -h origin" lists heads.
      Behavior is unchanged from 2.48.1.
    
    But I'd welcome a more thorough check to make sure I didn't overlook
    things. This is probably the most controversial patch, and if it's
    dropped, that would be alright (though it is helpful for me to reduce
    typing).

 builtin/merge-recursive.c | 2 +-
 git.c                     | 2 +-
 parse-options.c           | 2 +-
 usage.c                   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 17aa4db37a..c433d26bdf 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -38,7 +38,7 @@ int cmd_merge_recursive(int argc,
 	if (argv[0] && ends_with(argv[0], "-subtree"))
 		o.subtree_shift = "";
 
-	if (argc == 2 && (!strcmp(argv[1], "-h") ||
+	if (argc >= 2 && (!strcmp(argv[1], "-h") ||
 			  !strcmp(argv[1], "--help-all"))) {
 		struct strbuf msg = STRBUF_INIT;
 		strbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);
diff --git a/git.c b/git.c
index 40d3df1b76..7fe4e15e2d 100644
--- a/git.c
+++ b/git.c
@@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	const char *prefix;
 	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
 
-	help = argc == 2 && (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help-all"));
+	help = argc >= 2 && (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help-all"));
 	if (help && (run_setup & RUN_SETUP))
 		/* demote to GENTLY to allow 'git cmd -h' outside repo */
 		run_setup = RUN_SETUP_GENTLY;
diff --git a/parse-options.c b/parse-options.c
index e3ed42f709..b9a31c261c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1464,7 +1464,7 @@ void show_usage_with_options_if_asked(int ac, const char **av,
 				      const char * const *usagestr,
 				      const struct option *opts)
 {
-	if (ac == 2) {
+	if (ac >= 2) {
 		if (!strcmp(av[1], "-h")) {
 			usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
 			exit(129);
diff --git a/usage.c b/usage.c
index 4c245ba0cb..244e8d37ed 100644
--- a/usage.c
+++ b/usage.c
@@ -192,7 +192,7 @@ static void show_usage_if_asked_helper(const char *err, ...)
 
 void show_usage_if_asked(int ac, const char **av, const char *err)
 {
-	if (ac == 2 && (!strcmp(av[1], "-h") ||
+	if (ac >= 2 && (!strcmp(av[1], "-h") ||
 			!strcmp(av[1], "--help-all")))
 		show_usage_if_asked_helper(err);
 }
-- 
2.48.1

Re: [PATCH 4/4] builtins: show help on "-h"/"--help-all" with more than 2 arguments left

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

> When asking for short help on a previous command, the user may use their
> shell history to recall a command like
>
>     git rebase new-base
>
> Then inserting "-h" after "rebase" doesn't yield the help; make it so.

I doubt this is a good idea for at least two reasons.

 * As "git help cli" says, we should be discouraging, not
   encouraging peope to say "git rebase new-base -h".

 * "git rebase -h new-base" that shows help is probably a bug (think
   what should happen with s/rebase/grep/) in the first place.

If anything, we probably should fix the "-h" codepath to

 - react and do the short-help only when "-h" is the only command
   line option; with argument, it should probably barf, saying "-h
   does not take an argument".

 - if "-h" resulted in reported an alias, it should stop there.
   E.g. "git -c alias.x=ls-files x -h" would currently invoke "git
   ls-files -h" after reporting that 'x' is aliased to 'ls-files'.
   If the alias is to one of our commands, it is not too risky, but
   otherwise we should not assume it is safe to append "-h" to the
   underlying command and run it.  Imagine

   $ git -c alias.x='!echo rm -rf .' x -h

   and worse yet, if your alias did not have "echo" in it ;-)???

The only end-user expectation we can safely assume is when they
say,
 
   $ git frotz -h

is that they would get a help on 'frotz' without doing any harm.  
If frotz is an alias to some external command, for which we have no
idea what it would do when we run it with "-h" appended to the
command line, the user would be in a lot of pain if the aliased
operation is destructive.

Re: [PATCH 4/4] builtins: show help on "-h"/"--help-all" with more than 2 arguments left

[D. Ben Knoble]

On Sat, Jul 26, 2025 at 8:28 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
>
> > When asking for short help on a previous command, the user may use their
> > shell history to recall a command like
> >
> >     git rebase new-base
> >
> > Then inserting "-h" after "rebase" doesn't yield the help; make it so.
>
> I doubt this is a good idea for at least two reasons.

As I said in the cover letter, I think this is the most controversial
and can be dropped. However…

>  * As "git help cli" says, we should be discouraging, not
>    encouraging peope to say "git rebase new-base -h".

…that's not what I'm encouraging here. Instead, it's more like below:

>  * "git rebase -h new-base" that shows help is probably a bug (think
>    what should happen with s/rebase/grep/) in the first place.

And at least according to my tests, "git grep -h new-base" still greps
rather than shows help. Compare
- "git grep -h squash" (greps squash)
- "git rebase -h @{u}" (shows help)

> If anything, we probably should fix the "-h" codepath to
>
>  - react and do the short-help only when "-h" is the only command
>    line option; with argument, it should probably barf, saying "-h
>    does not take an argument".

I think we have the first half already ("argc == 2" in the preimage).
I'm not interested in writing the second half right now, personally,
if we end up dropping this patch, so someone else could take that up.

>  - if "-h" resulted in reported an alias, it should stop there.
>    E.g. "git -c alias.x=ls-files x -h" would currently invoke "git
>    ls-files -h" after reporting that 'x' is aliased to 'ls-files'.
>    If the alias is to one of our commands, it is not too risky, but
>    otherwise we should not assume it is safe to append "-h" to the
>    underlying command and run it.  Imagine
>
>    $ git -c alias.x='!echo rm -rf .' x -h
>
>    and worse yet, if your alias did not have "echo" in it ;-)???
>
> The only end-user expectation we can safely assume is when they
> say,
>
>    $ git frotz -h
>
> is that they would get a help on 'frotz' without doing any harm.
> If frotz is an alias to some external command, for which we have no
> idea what it would do when we run it with "-h" appended to the
> command line, the user would be in a lot of pain if the aliased
> operation is destructive.
>

I think this is being discussed on another series, and I'll leave it there :)

Re: [PATCH 4/4] builtins: show help on "-h"/"--help-all" with more than 2 arguments left

[Jeff King]

On Wed, Jul 30, 2025 at 05:55:32PM -0400, D. Ben Knoble wrote:

> >  * "git rebase -h new-base" that shows help is probably a bug (think
> >    what should happen with s/rebase/grep/) in the first place.
> 
> And at least according to my tests, "git grep -h new-base" still greps
> rather than shows help. Compare
> - "git grep -h squash" (greps squash)
> - "git rebase -h @{u}" (shows help)

I was somewhat surprised that grep would still work, looking at the
diff. The reason is that it does not call any of the touched functions,
but instead relies on this line in parse-options to trigger help:

  $ git grep -A2 'lone -h'
  parse-options.c:                /* lone -h asks for help */
  parse-options.c-                if (internal_help && ctx->total == 1 && !strcmp(arg + 1, "h"))
  parse-options.c-                        goto show_usage;

rather than any of the if_asked functions you touched. So I think there
may be two problems:

  1. You didn't touch this spot in the parse-options code. Would you
     need to for it to be consistent with the non-parse-options callers
     that use the if_asked functions?

  2. We can only get here if we make it past the help check in
     run_builtin(), that you do modify in your patch. That works for
     git-grep because it does not use RUN_SETUP, and calls
     parse_options() before checking whether we are in a repository.

     So in run_builtin() we do set "help" to 1, but it does nothing
     without the RUN_SETUP flag. But imagine a hypothetical git-foo that
     takes a "-h" option and does require a repository. It would set the
     RUN_SETUP flag, and then:

       git foo -h bar

     would show the help before we even get into cmd_foo() to parse the
     options.

BTW, I applied your patch 4 manually to dig into this. I wasn't able to
apply the whole series. It doesn't go on top of the current 'master',
and applying with "am -3" mentions "sha1 information is lacking or
useless". Did you build this on some other unpublished series?

-Peff

Re: [PATCH 4/4] builtins: show help on "-h"/"--help-all" with more than 2 arguments left

[D. Ben Knoble]

On Sat, Aug 2, 2025 at 5:23 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jul 30, 2025 at 05:55:32PM -0400, D. Ben Knoble wrote:
>
> > >  * "git rebase -h new-base" that shows help is probably a bug (think
> > >    what should happen with s/rebase/grep/) in the first place.
> >
> > And at least according to my tests, "git grep -h new-base" still greps
> > rather than shows help. Compare
> > - "git grep -h squash" (greps squash)
> > - "git rebase -h @{u}" (shows help)
>
> I was somewhat surprised that grep would still work, looking at the
> diff. The reason is that it does not call any of the touched functions,
> but instead relies on this line in parse-options to trigger help:
>
>   $ git grep -A2 'lone -h'
>   parse-options.c:                /* lone -h asks for help */
>   parse-options.c-                if (internal_help && ctx->total == 1 && !strcmp(arg + 1, "h"))
>   parse-options.c-                        goto show_usage;
>
> rather than any of the if_asked functions you touched. So I think there
> may be two problems:
>
>   1. You didn't touch this spot in the parse-options code. Would you
>      need to for it to be consistent with the non-parse-options callers
>      that use the if_asked functions?
>
>   2. We can only get here if we make it past the help check in
>      run_builtin(), that you do modify in your patch. That works for
>      git-grep because it does not use RUN_SETUP, and calls
>      parse_options() before checking whether we are in a repository.
>
>      So in run_builtin() we do set "help" to 1, but it does nothing
>      without the RUN_SETUP flag. But imagine a hypothetical git-foo that
>      takes a "-h" option and does require a repository. It would set the
>      RUN_SETUP flag, and then:
>
>        git foo -h bar
>
>      would show the help before we even get into cmd_foo() to parse the
>      options.

I think I need to consider both questions in parallel: as you point
out, this patch probably doesn't work for a hypothetical command that
both needs a repository and has a "-h" option. (I note that ls-remote
also is RUN_SETUP_GENTLY, like grep). Since no such command exists
today, we /could/ take some version of this patch and refine later if
a command needs both RUN_SETUP and a "-h" option. Or we could reject
this patch (assuming there's no workaround for now). Given Junio's
concern, I'm inclined to just drop the patch from the series…

…which moots question 1, I think. OTOH, if we keep the patch, it does
seem like we might want the parse-options API to be consistent.
Fortunately, I don't think this area needs adjusted for 3/4 based on
the tests.

>
> BTW, I applied your patch 4 manually to dig into this. I wasn't able to
> apply the whole series. It doesn't go on top of the current 'master',
> and applying with "am -3" mentions "sha1 information is lacking or
> useless". Did you build this on some other unpublished series?
>
> -Peff

The base is published and mentioned in the cover letter [1]; if I can
make that more explicit in any way going forward, please let me know!

[1]: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/

Re: [PATCH 4/4] builtins: show help on "-h"/"--help-all" with more than 2 arguments left

[Jeff King]

On Sat, Aug 02, 2025 at 12:10:17PM -0400, D. Ben Knoble wrote:

> >   1. You didn't touch this spot in the parse-options code. Would you
> >      need to for it to be consistent with the non-parse-options callers
> >      that use the if_asked functions?
> >
> >   2. We can only get here if we make it past the help check in
> >      run_builtin(), that you do modify in your patch. That works for
> >      git-grep because it does not use RUN_SETUP, and calls
> >      parse_options() before checking whether we are in a repository.
> >
> >      So in run_builtin() we do set "help" to 1, but it does nothing
> >      without the RUN_SETUP flag. But imagine a hypothetical git-foo that
> >      takes a "-h" option and does require a repository. It would set the
> >      RUN_SETUP flag, and then:
> >
> >        git foo -h bar
> >
> >      would show the help before we even get into cmd_foo() to parse the
> >      options.
> 
> I think I need to consider both questions in parallel: as you point
> out, this patch probably doesn't work for a hypothetical command that
> both needs a repository and has a "-h" option. (I note that ls-remote
> also is RUN_SETUP_GENTLY, like grep). Since no such command exists
> today, we /could/ take some version of this patch and refine later if
> a command needs both RUN_SETUP and a "-h" option. Or we could reject
> this patch (assuming there's no workaround for now). Given Junio's
> concern, I'm inclined to just drop the patch from the series…
> 
> …which moots question 1, I think. OTOH, if we keep the patch, it does
> seem like we might want the parse-options API to be consistent.
> Fortunately, I don't think this area needs adjusted for 3/4 based on
> the tests.

I think I mostly share Junio's concern. The issue is that we want to
detect the "user is asking for help" situation without having access to
the option-parsing information for the actual sub-command. And so our
strategy has been to make the rule for triggering "asking for help" to
be fairly conservative.

If we loosened it now, even though it happens to work for all current
commands, we'd later potentially have to re-tighten (which is awkward)
or start carrying extra signals back to git.c (e.g., a HAVE_H_OPTION
flag).

> > BTW, I applied your patch 4 manually to dig into this. I wasn't able to
> > apply the whole series. It doesn't go on top of the current 'master',
> > and applying with "am -3" mentions "sha1 information is lacking or
> > useless". Did you build this on some other unpublished series?
> 
> The base is published and mentioned in the cover letter [1]; if I can
> make that more explicit in any way going forward, please let me know!
> 
> [1]: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/

Ah, hmm. I was trying on top of ua/t1517-short-help-tests, which still
fails. But it works if I merge that branch to 'master'. I guess that's
what you meant by "Merge that branch to a new topic branch".

-Peff

Re: [PATCH 4/4] builtins: show help on "-h"/"--help-all" with more than 2 arguments left

[D. Ben Knoble]

On Sat, Aug 2, 2025 at 12:28 PM Jeff King <peff@peff.net> wrote:
>
> On Sat, Aug 02, 2025 at 12:10:17PM -0400, D. Ben Knoble wrote:
>
> > >   1. You didn't touch this spot in the parse-options code. Would you
> > >      need to for it to be consistent with the non-parse-options callers
> > >      that use the if_asked functions?
> > >
> > >   2. We can only get here if we make it past the help check in
> > >      run_builtin(), that you do modify in your patch. That works for
> > >      git-grep because it does not use RUN_SETUP, and calls
> > >      parse_options() before checking whether we are in a repository.
> > >
> > >      So in run_builtin() we do set "help" to 1, but it does nothing
> > >      without the RUN_SETUP flag. But imagine a hypothetical git-foo that
> > >      takes a "-h" option and does require a repository. It would set the
> > >      RUN_SETUP flag, and then:
> > >
> > >        git foo -h bar
> > >
> > >      would show the help before we even get into cmd_foo() to parse the
> > >      options.
> >
> > I think I need to consider both questions in parallel: as you point
> > out, this patch probably doesn't work for a hypothetical command that
> > both needs a repository and has a "-h" option. (I note that ls-remote
> > also is RUN_SETUP_GENTLY, like grep). Since no such command exists
> > today, we /could/ take some version of this patch and refine later if
> > a command needs both RUN_SETUP and a "-h" option. Or we could reject
> > this patch (assuming there's no workaround for now). Given Junio's
> > concern, I'm inclined to just drop the patch from the series…
> >
> > …which moots question 1, I think. OTOH, if we keep the patch, it does
> > seem like we might want the parse-options API to be consistent.
> > Fortunately, I don't think this area needs adjusted for 3/4 based on
> > the tests.
>
> I think I mostly share Junio's concern. The issue is that we want to
> detect the "user is asking for help" situation without having access to
> the option-parsing information for the actual sub-command. And so our
> strategy has been to make the rule for triggering "asking for help" to
> be fairly conservative.
>
> If we loosened it now, even though it happens to work for all current
> commands, we'd later potentially have to re-tighten (which is awkward)
> or start carrying extra signals back to git.c (e.g., a HAVE_H_OPTION
> flag).

Sensible. Will drop.

>
> > > BTW, I applied your patch 4 manually to dig into this. I wasn't able to
> > > apply the whole series. It doesn't go on top of the current 'master',
> > > and applying with "am -3" mentions "sha1 information is lacking or
> > > useless". Did you build this on some other unpublished series?
> >
> > The base is published and mentioned in the cover letter [1]; if I can
> > make that more explicit in any way going forward, please let me know!
> >
> > [1]: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/
>
> Ah, hmm. I was trying on top of ua/t1517-short-help-tests, which still
> fails. But it works if I merge that branch to 'master'. I guess that's
> what you meant by "Merge that branch to a new topic branch".

Ah, yep, that could be clearer (and it didn't occur to me that basing
on a merge would cause application issues relative to basing directly
on the branch). If it helps, the graph I have is pasted below (but
beware GMail whitespace munging?)

    git log --graph --boundary ^origin/master @

* 3099d83cdf (HEAD -> help-all-tweaks) builtins: show help on
"-h"/"--help-all" with more than 2 arguments left
* 352fe87c80 builtin: also setup gently for --help-all
* 56665594a8 parse-options: name flags passed to usage_with_options_internal
* 852a4547af t1517: fixup for ua/t1517-short-help-tests
*   14c7e9dddd Merge remote-tracking branch
'broken-out/ua/t1517-short-help-tests' into help-all-tweaks-tests
|\
| * 344c7067e6 (broken-out/ua/t1517-short-help-tests) t5200: move
`update-server-info -h` test from t1517
| * e552926bc4 t/t1517: automate `git subcmd -h` tests outside a repository
| o 16bd9f20a4 (tag: v2.50.0) Git 2.50
o | e4ef0485fd (origin/master, origin/HEAD, broken-out/master,
broken-out/HEAD) The fourteenth batch
 /

I've also (now) made the series available on GitHub at

    https://github.com/benknoble/git/tree/help-all-tweaks

>
> -Peff

[PATCH v2 0/3] permit -h/--help-all in more scenarios

[D. Ben Knoble]

Changes from v1:
- tweak refactor commit message to indicate no behavior is changed;
- use #define'd constants instead of an enum for internal flags;
- drop controversial [4/4]

QQ: Is there a better way to add --cc's to format-patch than grabbing the right
folks off threads from lore.kernel.org/git? (I could also grab them out of my
mail client.) I assume this is where using tools like b4 starts to really shine?
To be clear: I mean CC's _in addition to_ the use of

    sendemail.ccCmd = perl contrib/contacts/git-contacts

to keep folks that participated in prior rounds of discussion abreast of the
next version.

--------
Original
--------

This series depends on ua/t1517-short-help-tests with some fixes, which
show up in the first patch. Merge that branch to a new topic branch:

    git switch -c topic v2.50.0 # or origin/master
    git merge gitster/ua/t1517-short-help-tests

then apply this series.

This series enables --help-all outside of repository contexts, and
allows -h with other arguments (without breaking existing ls-remote/grep
usage).

It consists of preparatory steps (fixes for a dependency branch;
refactoring to make an internal helper's arguments clearer) followed by
the main commits.

v1: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/
Published-as: https://github.com/benknoble/git/tree/help-all-tweaks

D. Ben Knoble (3):
  t1517: fixup for ua/t1517-short-help-tests
  parse-options: refactor flags for usage_with_options_internal
  builtin: also setup gently for --help-all

 builtin/merge-recursive.c     |  3 ++-
 git.c                         |  2 +-
 parse-options.c               | 30 +++++++++++++++++++++++-------
 t/t1517-outside-repo.sh       | 11 +++++++----
 t/t5200-update-server-info.sh |  2 +-
 usage.c                       |  3 ++-
 6 files changed, 36 insertions(+), 15 deletions(-)

Diff-intervalle contre v1 :
1:  852a4547af ! 1:  7a3e0a601d t1517: fixup for ua/t1517-short-help-tests
    @@ Commit message
     
         - drop spurious message during test
         - fix known breakages that actually work
    +    - fix instaweb marker for known failure
         - fix new t5200 test
     
     
    @@ Notes
     
      ## t/t1517-outside-repo.sh ##
     @@
    + 	case "$cmd" in
    + 	archimport | cvsexportcommit | cvsimport | cvsserver | daemon | \
      	difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \
    - 	http-backend | http-fetch | http-push | init-db | instaweb.sh | \
    +-	http-backend | http-fetch | http-push | init-db | instaweb.sh | \
    ++	http-backend | http-fetch | http-push | init-db | \
      	merge-octopus | merge-one-file | merge-resolve | mergetool | \
     -	mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \
     -	remote-http | remote-https | replay | request-pull | send-email | \
2:  56665594a8 ! 2:  db74b1eff7 parse-options: name flags passed to usage_with_options_internal
    @@ Metadata
     Author: D. Ben Knoble <ben.knoble+github@gmail.com>
     
      ## Commit message ##
    -    parse-options: name flags passed to usage_with_options_internal
    +    parse-options: refactor flags for usage_with_options_internal
     
         When reading or editing calls to usage_with_options_internal, it is
         difficult to tell what trailing "0, 0", "0, 1", "1, 0" arguments mean
         (NB there is never a "1, 1" case).
     
    -    Give the flags readable names to improve call-sites.
    +    Give the flags readable names to improve call-sites without changing any
    +    behavior.
     
      ## parse-options.c ##
     @@ parse-options.c: static void free_preprocessed_options(struct option *options)
      	free(options);
      }
      
    -+enum usage_style {
    -+	style_normal = 0,
    -+	style_full = 1,
    -+};
    -+
    -+enum usage_output {
    -+	to_out = 0,
    -+	to_err = 1,
    -+};
    ++#define USAGE_NORMAL 0
    ++#define USAGE_FULL 1
    ++#define USAGE_TO_STDOUT 0
    ++#define USAGE_TO_STDERR 1
     +
      static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,
      							 const char * const *,
      							 const struct option *,
     -							 int, int);
    -+							 enum usage_style,
    -+							 enum usage_output);
    ++							 int full_usage,
    ++							 int usage_to_stderr);
      
      enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
      					 const struct option *options,
    @@ parse-options.c: enum parse_opt_result parse_options_step(struct parse_opt_ctx_t
      
      		if (internal_help && !strcmp(arg + 2, "help-all"))
     -			return usage_with_options_internal(ctx, usagestr, options, 1, 0);
    -+			return usage_with_options_internal(ctx, usagestr, options, style_full, to_out);
    ++			return usage_with_options_internal(ctx, usagestr, options,
    ++							   USAGE_FULL, USAGE_TO_STDOUT);
      		if (internal_help && !strcmp(arg + 2, "help"))
      			goto show_usage;
      		switch (parse_long_opt(ctx, arg + 2, options)) {
    @@ parse-options.c: enum parse_opt_result parse_options_step(struct parse_opt_ctx_t
      
       show_usage:
     -	return usage_with_options_internal(ctx, usagestr, options, 0, 0);
    -+	return usage_with_options_internal(ctx, usagestr, options, style_normal, to_out);
    ++	return usage_with_options_internal(ctx, usagestr, options,
    ++					   USAGE_NORMAL, USAGE_TO_STDOUT);
      }
      
      int parse_options_end(struct parse_opt_ctx_t *ctx)
    -@@ parse-options.c: static const struct option *find_option_by_long_name(const struct option *opts,
    - static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,
    - 							 const char * const *usagestr,
    - 							 const struct option *opts,
    --							 int full, int err)
    -+							 enum usage_style help_style,
    -+							 enum usage_output to_where)
    - {
    - 	const struct option *all_opts = opts;
    --	FILE *outfile = err ? stderr : stdout;
    -+	FILE *outfile = to_where == to_err ? stderr : stdout;
    - 	int need_newline;
    - 
    - 	const char *usage_prefix = _("usage: %s");
    -@@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
    - 	if (!usagestr)
    - 		return PARSE_OPT_HELP;
    - 
    --	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
    -+	if (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
    - 		fprintf(outfile, "cat <<\\EOF\n");
    - 
    - 	while (*usagestr) {
    -@@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
    - 				fprintf(outfile, "%s\n", _(opts->help));
    - 			continue;
    - 		}
    --		if (!full && (opts->flags & PARSE_OPT_HIDDEN))
    -+		if (help_style != style_full && (opts->flags & PARSE_OPT_HIDDEN))
    - 			continue;
    - 
    - 		if (need_newline) {
    -@@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
    - 	}
    - 	fputc('\n', outfile);
    - 
    --	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
    -+	if (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
    - 		fputs("EOF\n", outfile);
    - 
    - 	return PARSE_OPT_HELP;
     @@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
      void NORETURN usage_with_options(const char * const *usagestr,
      			const struct option *opts)
      {
     -	usage_with_options_internal(NULL, usagestr, opts, 0, 1);
    -+	usage_with_options_internal(NULL, usagestr, opts, style_normal, to_err);
    ++	usage_with_options_internal(NULL, usagestr, opts,
    ++				    USAGE_NORMAL, USAGE_TO_STDERR);
      	exit(129);
      }
      
    @@ parse-options.c: void show_usage_with_options_if_asked(int ac, const char **av,
      {
      	if (ac == 2 && !strcmp(av[1], "-h")) {
     -		usage_with_options_internal(NULL, usagestr, opts, 0, 0);
    -+		usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
    ++		usage_with_options_internal(NULL, usagestr, opts,
    ++					    USAGE_NORMAL, USAGE_TO_STDOUT);
      		exit(129);
      	}
      }
3:  352fe87c80 ! 3:  cb4113f77d builtin: also setup gently for --help-all
    @@ Commit message
         The exception is merge-recursive, whose help block doesn't use newer
         APIs.
     
    +    Best-viewed-with: --ignore-space-change
    +
     
      ## Notes ##
         Some usage.c callers, like check-ref-format, probably deserve to be
    @@ parse-options.c: void show_usage_with_options_if_asked(int ac, const char **av,
      				      const struct option *opts)
      {
     -	if (ac == 2 && !strcmp(av[1], "-h")) {
    --		usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
    +-		usage_with_options_internal(NULL, usagestr, opts,
    +-					    USAGE_NORMAL, USAGE_TO_STDOUT);
     -		exit(129);
     +	if (ac == 2) {
     +		if (!strcmp(av[1], "-h")) {
    -+			usage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);
    ++			usage_with_options_internal(NULL, usagestr, opts,
    ++						    USAGE_NORMAL, USAGE_TO_STDOUT);
     +			exit(129);
     +		} else if (!strcmp(av[1], "--help-all")) {
    -+			usage_with_options_internal(NULL, usagestr, opts, style_full, to_out);
    ++			usage_with_options_internal(NULL, usagestr, opts,
    ++						    USAGE_FULL, USAGE_TO_STDOUT);
     +			exit(129);
     +		}
      	}
4:  3099d83cdf < -:  ---------- builtins: show help on "-h"/"--help-all" with more than 2 arguments left

base-commit: e4ef0485fd78fcb05866ea78df35796b904e4a8e
prerequisite-patch-id: ffce2dd036e61c8d36485a17321f858e454db874
prerequisite-patch-id: 52539022c824997adfc1be0bed8de6b1851d2187
-- 
2.48.1

[PATCH v3 0/3] permit -h/--help-all in more scenarios

[D. Ben Knoble]

Changes from v2:
- Rebase on unpublished ua/t1517… v5 (https://lore.kernel.org/git/20250803020744.1037392-1-usmanakinyemi202@gmail.com/)
  which is rebased on master
- Changed fixup to account for instaweb Perl dependency

Changes from v1:
- tweak refactor commit message to indicate no behavior is changed;
- use #define'd constants instead of an enum for internal flags;
- drop controversial [4/4]

QQ: Is there a better way to add --cc's to format-patch than grabbing
the right folks off threads from lore.kernel.org/git? (I could also grab
them out of my mail client.) I assume this is where using tools like b4
starts to really shine? To be clear: I mean CC's _in addition to_ the
use of

    sendemail.ccCmd = perl contrib/contacts/git-contacts

to keep folks that participated in prior rounds of discussion abreast of the
next version.

--------
Original
--------

This series depends on ua/t1517-short-help-tests with some fixes, which
show up in the first patch. Start a new topic from the (unpublished)
v5 of that branch:

    git switch -c topic origin/master
    git am ua/t1517-short-help-tests-v5.mbox

Then apply this series.

This series enables --help-all outside of repository contexts, and
allows -h with other arguments (without breaking existing ls-remote/grep
usage).

It consists of preparatory steps (fixes for a dependency branch;
refactoring to make an internal helper's arguments clearer) followed by
the main commits.

v1: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/
v2: https://lore.kernel.org/git/20250803012613.54086-1-ben.knoble+github@gmail.com/
Published-as: https://github.com/benknoble/git/tree/help-all-tweaks

D. Ben Knoble (3):
  t1517: fixup for ua/t1517-short-help-tests
  parse-options: refactor flags for usage_with_options_internal
  builtin: also setup gently for --help-all

 builtin/merge-recursive.c |  3 ++-
 git.c                     |  2 +-
 parse-options.c           | 30 +++++++++++++++++++++++-------
 t/t1517-outside-repo.sh   | 12 +++++++++++-
 usage.c                   |  3 ++-
 5 files changed, 39 insertions(+), 11 deletions(-)

Diff-intervalle contre v2 :
1:  e552926bc4 < -:  ---------- t/t1517: automate `git subcmd -h` tests outside a repository
2:  344c7067e6 < -:  ---------- t5200: move `update-server-info -h` test from t1517
3:  7a3e0a601d < -:  ---------- t1517: fixup for ua/t1517-short-help-tests
-:  ---------- > 1:  855931592f t1517: fixup for ua/t1517-short-help-tests
4:  db74b1eff7 = 2:  2392039374 parse-options: refactor flags for usage_with_options_internal
5:  cb4113f77d ! 3:  4dfc046e6d builtin: also setup gently for --help-all
    @@ t/t1517-outside-repo.sh
      		test_expect_code 129 nongit git $cmd -h >usage &&
      		test_grep "[Uu]sage: git $cmd " usage
      	'
    -+	test_$expect_outcome "'git $cmd --help-all' outside a repository" '
    ++	test_$expect_outcome $prereq "'git $cmd --help-all' outside a repository" '
     +		test_expect_code 129 nongit git $cmd --help-all >usage &&
     +		test_grep "[Uu]sage: git $cmd " usage
     +	'
      done
      
    - test_expect_success 'prune does not crash with -h' '
    + test_done
     
      ## usage.c ##
     @@ usage.c: static void show_usage_if_asked_helper(const char *err, ...)

base-commit: 866e6a391f466baeeb98bc585845ea638322c04b
prerequisite-patch-id: eb2c55de73b2b733aa0c8c94469a45d9bc361d81
prerequisite-patch-id: 12ddad29e334853cf61c797a2c08302893f4b909
prerequisite-patch-id: 250bae2541030fcdfc5b35ede44c23e1138c7a3c
-- 
2.48.1

Re: [PATCH v3 0/3] permit -h/--help-all in more scenarios

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

> This series enables --help-all outside of repository contexts,

I've been familiar with "git commit -h" (and commands other than
"commit") outside a repository, but did not even know that "git
commit --help-all" didn't work outside.  For commands that use
parse-options, these come from the same source of informatino, so it
does not make any sense for one to work and the other to refuse to
work.  Good.

> and
> allows -h with other arguments (without breaking existing ls-remote/grep
> usage).

I somehow thought we already talked you out of this.

Do you mean something like "git add -h foo" and "git add -h -N foo"
would say "'git add foo' would add the current contents in foo to
the index" and "'git add -N foo' would make the index aware of the
path foo without actually adding its contents (yet)"?  I do not think
it makes much sense to behave exactly the same as "git add -h" when
the user says "git add -h foo" or "git add -h -N foo", as if we
didn't even see the extra things on the command line.

Thanks.

Re: [PATCH v3 0/3] permit -h/--help-all in more scenarios

[D. Ben Knoble]

On Mon, Aug 4, 2025 at 12:53 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
>
> > This series enables --help-all outside of repository contexts,
>
> I've been familiar with "git commit -h" (and commands other than
> "commit") outside a repository, but did not even know that "git
> commit --help-all" didn't work outside.  For commands that use
> parse-options, these come from the same source of informatino, so it
> does not make any sense for one to work and the other to refuse to
> work.  Good.
>
> > and
> > allows -h with other arguments (without breaking existing ls-remote/grep
> > usage).
>
> I somehow thought we already talked you out of this.

You did! Forgot to drop that segment from the cover letter.

> Do you mean something like "git add -h foo" and "git add -h -N foo"
> would say "'git add foo' would add the current contents in foo to
> the index" and "'git add -N foo' would make the index aware of the
> path foo without actually adding its contents (yet)"?  I do not think
> it makes much sense to behave exactly the same as "git add -h" when
> the user says "git add -h foo" or "git add -h -N foo", as if we
> didn't even see the extra things on the command line.

So this is moot. Good catch, thanks.

[PATCH v3 1/3] t1517: fixup for ua/t1517-short-help-tests

[D. Ben Knoble]

- fix instaweb test prereqs

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 t/t1517-outside-repo.sh | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index 8a417af47a..3dc602872a 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -123,7 +123,13 @@
 	*)
 		expect_outcome=expect_success ;;
 	esac
-	test_$expect_outcome "'git $cmd -h' outside a repository" '
+	case "$cmd" in
+	instaweb)
+		prereq=PERL ;;
+	*)
+		prereq= ;;
+	esac
+	test_$expect_outcome $prereq "'git $cmd -h' outside a repository" '
 		test_expect_code 129 nongit git $cmd -h >usage &&
 		test_grep "[Uu]sage: git $cmd " usage
 	'
-- 
2.48.1

Re: [PATCH v3 1/3] t1517: fixup for ua/t1517-short-help-tests

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble+github@gmail.com> writes:

> - fix instaweb test prereqs
>
> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> ---
>  t/t1517-outside-repo.sh | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
> index 8a417af47a..3dc602872a 100755
> --- a/t/t1517-outside-repo.sh
> +++ b/t/t1517-outside-repo.sh
> @@ -123,7 +123,13 @@
>  	*)
>  		expect_outcome=expect_success ;;
>  	esac
> -	test_$expect_outcome "'git $cmd -h' outside a repository" '
> +	case "$cmd" in
> +	instaweb)
> +		prereq=PERL ;;
> +	*)
> +		prereq= ;;
> +	esac
> +	test_$expect_outcome $prereq "'git $cmd -h' outside a repository" '
>  		test_expect_code 129 nongit git $cmd -h >usage &&
>  		test_grep "[Uu]sage: git $cmd " usage
>  	'

Ideally this would want to be squashed (or moved) into the base
topic.  Can you two coordinate among yourselves?

Thanks.

Re: [PATCH v3 1/3] t1517: fixup for ua/t1517-short-help-tests

[D. Ben Knoble]

On Sun, Aug 3, 2025 at 12:41 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "D. Ben Knoble" <ben.knoble+github@gmail.com> writes:
>
> > - fix instaweb test prereqs
> >
> > Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> > ---
> >  t/t1517-outside-repo.sh | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
> > index 8a417af47a..3dc602872a 100755
> > --- a/t/t1517-outside-repo.sh
> > +++ b/t/t1517-outside-repo.sh
> > @@ -123,7 +123,13 @@
> >       *)
> >               expect_outcome=expect_success ;;
> >       esac
> > -     test_$expect_outcome "'git $cmd -h' outside a repository" '
> > +     case "$cmd" in
> > +     instaweb)
> > +             prereq=PERL ;;
> > +     *)
> > +             prereq= ;;
> > +     esac
> > +     test_$expect_outcome $prereq "'git $cmd -h' outside a repository" '
> >               test_expect_code 129 nongit git $cmd -h >usage &&
> >               test_grep "[Uu]sage: git $cmd " usage
> >       '
>
> Ideally this would want to be squashed (or moved) into the base
> topic.  Can you two coordinate among yourselves?

Agreed, and happy to—just not sure what that looks like here. Advice welcome :)

[PATCH v3 2/3] parse-options: refactor flags for usage_with_options_internal

[D. Ben Knoble]

When reading or editing calls to usage_with_options_internal, it is
difficult to tell what trailing "0, 0", "0, 1", "1, 0" arguments mean
(NB there is never a "1, 1" case).

Give the flags readable names to improve call-sites without changing any
behavior.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---
 parse-options.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 5224203ffe..169d76fb65 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -953,10 +953,16 @@ static void free_preprocessed_options(struct option *options)
 	free(options);
 }
 
+#define USAGE_NORMAL 0
+#define USAGE_FULL 1
+#define USAGE_TO_STDOUT 0
+#define USAGE_TO_STDERR 1
+
 static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,
 							 const char * const *,
 							 const struct option *,
-							 int, int);
+							 int full_usage,
+							 int usage_to_stderr);
 
 enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 					 const struct option *options,
@@ -1088,7 +1094,8 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 		}
 
 		if (internal_help && !strcmp(arg + 2, "help-all"))
-			return usage_with_options_internal(ctx, usagestr, options, 1, 0);
+			return usage_with_options_internal(ctx, usagestr, options,
+							   USAGE_FULL, USAGE_TO_STDOUT);
 		if (internal_help && !strcmp(arg + 2, "help"))
 			goto show_usage;
 		switch (parse_long_opt(ctx, arg + 2, options)) {
@@ -1129,7 +1136,8 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 	return PARSE_OPT_DONE;
 
  show_usage:
-	return usage_with_options_internal(ctx, usagestr, options, 0, 0);
+	return usage_with_options_internal(ctx, usagestr, options,
+					   USAGE_NORMAL, USAGE_TO_STDOUT);
 }
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
@@ -1444,7 +1452,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 void NORETURN usage_with_options(const char * const *usagestr,
 			const struct option *opts)
 {
-	usage_with_options_internal(NULL, usagestr, opts, 0, 1);
+	usage_with_options_internal(NULL, usagestr, opts,
+				    USAGE_NORMAL, USAGE_TO_STDERR);
 	exit(129);
 }
 
@@ -1453,7 +1462,8 @@ void show_usage_with_options_if_asked(int ac, const char **av,
 				      const struct option *opts)
 {
 	if (ac == 2 && !strcmp(av[1], "-h")) {
-		usage_with_options_internal(NULL, usagestr, opts, 0, 0);
+		usage_with_options_internal(NULL, usagestr, opts,
+					    USAGE_NORMAL, USAGE_TO_STDOUT);
 		exit(129);
 	}
 }
-- 
2.48.1

[PATCH v3 3/3] builtin: also setup gently for --help-all

[D. Ben Knoble]

Git experts often check the help summary of a command to make sure they
spell options right when suggesting advice to colleagues. Further, they
might check hidden options when responding to queries about deprecated
options like git-rebase(1)'s "preserve merges" option. But some commands
don't support "--help-all" outside of a git directory. Running (for
example)

    git rebase --help-all

outside a directory fails in "setup_git_directory", erroring with the
localized form of

    fatal: not a git repository (or any of the parent directories): .git

Like 99caeed05d (Let 'git <command> -h' show usage without a git dir,
2009-11-09), we want to show the "--help-all" output even without a git
dir. Make "--help-all" where we expect "-h" to mean
"setup_git_directory_gently", and interpose early in the natural place
("show_usage_with_options_if_asked").

Do the same for usage callers with show_usage_if_asked.

The exception is merge-recursive, whose help block doesn't use newer
APIs.

Best-viewed-with: --ignore-space-change
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---

Notes:
    Some usage.c callers, like check-ref-format, probably deserve to be
    ported to parse-options at this point.

 builtin/merge-recursive.c |  3 ++-
 git.c                     |  2 +-
 parse-options.c           | 14 ++++++++++----
 t/t1517-outside-repo.sh   |  4 ++++
 usage.c                   |  3 ++-
 5 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 03b5100cfa..17aa4db37a 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -38,7 +38,8 @@ int cmd_merge_recursive(int argc,
 	if (argv[0] && ends_with(argv[0], "-subtree"))
 		o.subtree_shift = "";
 
-	if (argc == 2 && !strcmp(argv[1], "-h")) {
+	if (argc == 2 && (!strcmp(argv[1], "-h") ||
+			  !strcmp(argv[1], "--help-all"))) {
 		struct strbuf msg = STRBUF_INIT;
 		strbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);
 		show_usage_if_asked(argc, argv, msg.buf);
diff --git a/git.c b/git.c
index 07a5fe39fb..40d3df1b76 100644
--- a/git.c
+++ b/git.c
@@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	const char *prefix;
 	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
 
-	help = argc == 2 && !strcmp(argv[1], "-h");
+	help = argc == 2 && (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help-all"));
 	if (help && (run_setup & RUN_SETUP))
 		/* demote to GENTLY to allow 'git cmd -h' outside repo */
 		run_setup = RUN_SETUP_GENTLY;
diff --git a/parse-options.c b/parse-options.c
index 169d76fb65..d9f960b7b5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1461,10 +1461,16 @@ void show_usage_with_options_if_asked(int ac, const char **av,
 				      const char * const *usagestr,
 				      const struct option *opts)
 {
-	if (ac == 2 && !strcmp(av[1], "-h")) {
-		usage_with_options_internal(NULL, usagestr, opts,
-					    USAGE_NORMAL, USAGE_TO_STDOUT);
-		exit(129);
+	if (ac == 2) {
+		if (!strcmp(av[1], "-h")) {
+			usage_with_options_internal(NULL, usagestr, opts,
+						    USAGE_NORMAL, USAGE_TO_STDOUT);
+			exit(129);
+		} else if (!strcmp(av[1], "--help-all")) {
+			usage_with_options_internal(NULL, usagestr, opts,
+						    USAGE_FULL, USAGE_TO_STDOUT);
+			exit(129);
+		}
 	}
 }
 
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index 3dc602872a..e34321dd44 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -133,6 +133,10 @@
 		test_expect_code 129 nongit git $cmd -h >usage &&
 		test_grep "[Uu]sage: git $cmd " usage
 	'
+	test_$expect_outcome $prereq "'git $cmd --help-all' outside a repository" '
+		test_expect_code 129 nongit git $cmd --help-all >usage &&
+		test_grep "[Uu]sage: git $cmd " usage
+	'
 done
 
 test_done
diff --git a/usage.c b/usage.c
index 81913236a4..4c245ba0cb 100644
--- a/usage.c
+++ b/usage.c
@@ -192,7 +192,8 @@ static void show_usage_if_asked_helper(const char *err, ...)
 
 void show_usage_if_asked(int ac, const char **av, const char *err)
 {
-	if (ac == 2 && !strcmp(av[1], "-h"))
+	if (ac == 2 && (!strcmp(av[1], "-h") ||
+			!strcmp(av[1], "--help-all")))
 		show_usage_if_asked_helper(err);
 }
 
-- 
2.48.1

[PATCH v2 1/3] t1517: fixup for ua/t1517-short-help-tests

[D. Ben Knoble]

- drop spurious message during test
- fix known breakages that actually work
- fix instaweb marker for known failure: it is not a failure, but should not
  have .sh suffix
- fix new t5200 test

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---

Notes:
    I expect this and other fixes to get squashed into the upstream branch,
    but I'm including it here so it's easy to create a clean build.

 t/t1517-outside-repo.sh       | 7 +++----
 t/t5200-update-server-info.sh | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index 9443c0284f..deb72da66e 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -113,10 +113,10 @@
 	case "$cmd" in
 	archimport | cvsexportcommit | cvsimport | cvsserver | daemon | \
 	difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \
-	http-backend | http-fetch | http-push | init-db | instaweb.sh | \
+	http-backend | http-fetch | http-push | init-db | \
 	merge-octopus | merge-one-file | merge-resolve | mergetool | \
-	mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \
-	remote-http | remote-https | replay | request-pull | send-email | \
+	mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \
+	remote-http | remote-https | replay | send-email | \
 	sh-i18n--envsubst | shell | show | stage | submodule | svn | \
 	upload-archive--writer | upload-pack | web--browse | whatchanged)
 		expect_outcome=expect_failure ;;
@@ -125,7 +125,6 @@
 	esac
 	test_$expect_outcome "'git $cmd -h' outside a repository" '
 		test_expect_code 129 nongit git $cmd -h >usage &&
-		echo "Hello" &&
 		test_grep "[Uu]sage: git $cmd " usage
 	'
 done
diff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh
index a1f129db4e..a551e955b5 100755
--- a/t/t5200-update-server-info.sh
+++ b/t/t5200-update-server-info.sh
@@ -48,7 +48,7 @@
 
 test_expect_success 'update-server-info does not crash with -h' '
 	test_expect_code 129 git update-server-info -h >usage &&
-	test_grep "[Uu]sage: git update-server-info " usage &&
+	test_grep "[Uu]sage: git update-server-info " usage
 '
 
 test_done
-- 
2.48.1

[PATCH v2 2/3] parse-options: refactor flags for usage_with_options_internal

[D. Ben Knoble]

When reading or editing calls to usage_with_options_internal, it is
difficult to tell what trailing "0, 0", "0, 1", "1, 0" arguments mean
(NB there is never a "1, 1" case).

Give the flags readable names to improve call-sites without changing any
behavior.

Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---

I went with a slightly more verbose approach here: Junio argued that the "0"
case is common so deserves to be short, but it's still hard IMO to tell what "0"
represents when reading only the function calls (say, in a patch with minimal
context).

Unfortunately, the deep rightward indent is also a bit challenging on my eyes :/

 parse-options.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 5224203ffe..169d76fb65 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -953,10 +953,16 @@ static void free_preprocessed_options(struct option *options)
 	free(options);
 }
 
+#define USAGE_NORMAL 0
+#define USAGE_FULL 1
+#define USAGE_TO_STDOUT 0
+#define USAGE_TO_STDERR 1
+
 static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,
 							 const char * const *,
 							 const struct option *,
-							 int, int);
+							 int full_usage,
+							 int usage_to_stderr);
 
 enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 					 const struct option *options,
@@ -1088,7 +1094,8 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 		}
 
 		if (internal_help && !strcmp(arg + 2, "help-all"))
-			return usage_with_options_internal(ctx, usagestr, options, 1, 0);
+			return usage_with_options_internal(ctx, usagestr, options,
+							   USAGE_FULL, USAGE_TO_STDOUT);
 		if (internal_help && !strcmp(arg + 2, "help"))
 			goto show_usage;
 		switch (parse_long_opt(ctx, arg + 2, options)) {
@@ -1129,7 +1136,8 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 	return PARSE_OPT_DONE;
 
  show_usage:
-	return usage_with_options_internal(ctx, usagestr, options, 0, 0);
+	return usage_with_options_internal(ctx, usagestr, options,
+					   USAGE_NORMAL, USAGE_TO_STDOUT);
 }
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
@@ -1444,7 +1452,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 void NORETURN usage_with_options(const char * const *usagestr,
 			const struct option *opts)
 {
-	usage_with_options_internal(NULL, usagestr, opts, 0, 1);
+	usage_with_options_internal(NULL, usagestr, opts,
+				    USAGE_NORMAL, USAGE_TO_STDERR);
 	exit(129);
 }
 
@@ -1453,7 +1462,8 @@ void show_usage_with_options_if_asked(int ac, const char **av,
 				      const struct option *opts)
 {
 	if (ac == 2 && !strcmp(av[1], "-h")) {
-		usage_with_options_internal(NULL, usagestr, opts, 0, 0);
+		usage_with_options_internal(NULL, usagestr, opts,
+					    USAGE_NORMAL, USAGE_TO_STDOUT);
 		exit(129);
 	}
 }
-- 
2.48.1

[PATCH v2 3/3] builtin: also setup gently for --help-all

[D. Ben Knoble]

Git experts often check the help summary of a command to make sure they
spell options right when suggesting advice to colleagues. Further, they
might check hidden options when responding to queries about deprecated
options like git-rebase(1)'s "preserve merges" option. But some commands
don't support "--help-all" outside of a git directory. Running (for
example)

    git rebase --help-all

outside a directory fails in "setup_git_directory", erroring with the
localized form of

    fatal: not a git repository (or any of the parent directories): .git

Like 99caeed05d (Let 'git <command> -h' show usage without a git dir,
2009-11-09), we want to show the "--help-all" output even without a git
dir. Make "--help-all" where we expect "-h" to mean
"setup_git_directory_gently", and interpose early in the natural place
("show_usage_with_options_if_asked").

Do the same for usage callers with show_usage_if_asked.

The exception is merge-recursive, whose help block doesn't use newer
APIs.

Best-viewed-with: --ignore-space-change
Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
---

Notes:
    Some usage.c callers, like check-ref-format, probably deserve to be
    ported to parse-options at this point.

 builtin/merge-recursive.c |  3 ++-
 git.c                     |  2 +-
 parse-options.c           | 14 ++++++++++----
 t/t1517-outside-repo.sh   |  4 ++++
 usage.c                   |  3 ++-
 5 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 03b5100cfa..17aa4db37a 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -38,7 +38,8 @@ int cmd_merge_recursive(int argc,
 	if (argv[0] && ends_with(argv[0], "-subtree"))
 		o.subtree_shift = "";
 
-	if (argc == 2 && !strcmp(argv[1], "-h")) {
+	if (argc == 2 && (!strcmp(argv[1], "-h") ||
+			  !strcmp(argv[1], "--help-all"))) {
 		struct strbuf msg = STRBUF_INIT;
 		strbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);
 		show_usage_if_asked(argc, argv, msg.buf);
diff --git a/git.c b/git.c
index 07a5fe39fb..40d3df1b76 100644
--- a/git.c
+++ b/git.c
@@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct
 	const char *prefix;
 	int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
 
-	help = argc == 2 && !strcmp(argv[1], "-h");
+	help = argc == 2 && (!strcmp(argv[1], "-h") || !strcmp(argv[1], "--help-all"));
 	if (help && (run_setup & RUN_SETUP))
 		/* demote to GENTLY to allow 'git cmd -h' outside repo */
 		run_setup = RUN_SETUP_GENTLY;
diff --git a/parse-options.c b/parse-options.c
index 169d76fb65..d9f960b7b5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1461,10 +1461,16 @@ void show_usage_with_options_if_asked(int ac, const char **av,
 				      const char * const *usagestr,
 				      const struct option *opts)
 {
-	if (ac == 2 && !strcmp(av[1], "-h")) {
-		usage_with_options_internal(NULL, usagestr, opts,
-					    USAGE_NORMAL, USAGE_TO_STDOUT);
-		exit(129);
+	if (ac == 2) {
+		if (!strcmp(av[1], "-h")) {
+			usage_with_options_internal(NULL, usagestr, opts,
+						    USAGE_NORMAL, USAGE_TO_STDOUT);
+			exit(129);
+		} else if (!strcmp(av[1], "--help-all")) {
+			usage_with_options_internal(NULL, usagestr, opts,
+						    USAGE_FULL, USAGE_TO_STDOUT);
+			exit(129);
+		}
 	}
 }
 
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index deb72da66e..a780c54133 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -127,6 +127,10 @@
 		test_expect_code 129 nongit git $cmd -h >usage &&
 		test_grep "[Uu]sage: git $cmd " usage
 	'
+	test_$expect_outcome "'git $cmd --help-all' outside a repository" '
+		test_expect_code 129 nongit git $cmd --help-all >usage &&
+		test_grep "[Uu]sage: git $cmd " usage
+	'
 done
 
 test_expect_success 'prune does not crash with -h' '
diff --git a/usage.c b/usage.c
index 81913236a4..4c245ba0cb 100644
--- a/usage.c
+++ b/usage.c
@@ -192,7 +192,8 @@ static void show_usage_if_asked_helper(const char *err, ...)
 
 void show_usage_if_asked(int ac, const char **av, const char *err)
 {
-	if (ac == 2 && !strcmp(av[1], "-h"))
+	if (ac == 2 && (!strcmp(av[1], "-h") ||
+			!strcmp(av[1], "--help-all")))
 		show_usage_if_asked_helper(err);
 }
 
-- 
2.48.1