[Bug] setup_git_env called without repository

[Bug] setup_git_env called without repository

[Zero King]

Hi,

After upgrading to Git 2.13.0, I'm seeing the following error message
when running `git am -h`.

    $ git am -h
    fatal: BUG: setup_git_env called without repository

And with Git built from the next branch:

    $ git am -h
    BUG: environment.c:172: setup_git_env called without repository
    Abort trap: 6

-- 
Best regards,
Zero King

Re: [Bug] setup_git_env called without repository

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 1:45 PM, Zero King <l2dy@macports.org> wrote:
> After upgrading to Git 2.13.0, I'm seeing the following error message
> when running `git am -h`.
>
>    $ git am -h
>    fatal: BUG: setup_git_env called without repository
>
> And with Git built from the next branch:
>
>    $ git am -h
>    BUG: environment.c:172: setup_git_env called without repository
>    Abort trap: 6

Jeff, bisects to your b1ef400eec ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20).

Re: [Bug] setup_git_env called without repository

[Jeff King]

On Mon, May 29, 2017 at 03:01:11PM +0200, Ævar Arnfjörð Bjarmason wrote:

> On Mon, May 29, 2017 at 1:45 PM, Zero King <l2dy@macports.org> wrote:
> > After upgrading to Git 2.13.0, I'm seeing the following error message
> > when running `git am -h`.
> >
> >    $ git am -h
> >    fatal: BUG: setup_git_env called without repository
> >
> > And with Git built from the next branch:
> >
> >    $ git am -h
> >    BUG: environment.c:172: setup_git_env called without repository
> >    Abort trap: 6
> 
> Jeff, bisects to your b1ef400eec ("setup_git_env: avoid blind
> fall-back to ".git"", 2016-10-20).

Well, yes. But that commit is just making it easier to notice existing
violations of the setup_git_env() rules. The interesting thing is where
the violation comes from. :)

In this case, the "am" builtin uses git_pathdup() and git_config() to
set up some defaults before calling parse_options(), which is where we
handle "-h". Normally that's fine, because it uses the RUN_SETUP flag to
tell the git wrapper to barf if we're not in a repository.

But when the wrapper sees that there is a single command-line argument
and that it's "-h", it skips all of the setup and runs the builtin's
cmd_am() function anyway, under the assumption that the builtin won't do
anything meaningful before noticing the "-h" and dying. This goes back
to Jonathan's 99caeed05 (Let 'git <command> -h' show usage without a git
dir, 2009-11-09).

I have mixed feelings on that commit. It's unquestionably more friendly
to make "git foo -h" work outside of a repository, rather than printing
"Not a git repository". But it does break the assumptions of the cmd_*
functions.

In this case it's fairly harmless. We'd fill in bogus values for the
am_state (a bogus git-dir, but also we'd quietly ignore any repo-level
config when we _are_ in a repo), but those aren't actually used before
we hit the "-h" handling. Conceivably a cmd_* function could show
defaults as part of "-h" output, but I don't know of any cases where
that is true.

I'd be much more worried about cases where the cmd function doesn't
handle "-h" at all, and just proceeds with a broken setup. That said,
it's been this way for almost 8 years. And I see that several commands
already have workarounds for dying early in this case (try grepping for
'"-h"'). So probably we should just follow suit, like:

diff --git a/builtin/am.c b/builtin/am.c
index 0f63dcab1..5ee146bfb 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2311,6 +2311,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(usage, options);
+
 	git_config(git_am_config, NULL);
 
 	am_state_init(&state);

I've used the same incantation there as in other commands; possibly
this should be abstracted to a simple:

  check_help_option(argc, argv, usage, options);

that each builtin could call.

What would be _really_ nice is if the usage/options pointers for each
builtin were made available to git.c, and then it could just call
usage_with_options() itself when it sees "-h". I think that would end up
with more boilerplate than it saves, though, as we'd have to add the
pointer to the definition of each builtin struct in git.c. Possibly it
could be helped with some macro trickery and naming conventions, but I
don't know if it's worth it.

The other thing to add is a test to make sure this doesn't pop up again
in another builtin.  We should be able to do:

  for i in $builtins
  do
	test_expect_success "$i -h works" "
		test_expect_code 129 $i -h
	"
  done

We'd need a list of builtins; probably a "git help --list-builtins"
would be helpful there. I ran something similar by hand and "git am" is
the only command that has this problem (for now, anyway). Interestingly,
"git credential" doesn't parse the "-h" at all, and so it just hangs
waiting for input (and would misbehave by ignoring repo config in this
case!). It should be fixed, too.

I'll try to put together patches in the next day or so. Comments welcome
in the meantime.

-Peff

[PATCH 0/8] consistent "-h" handling in builtins

[Jeff King]

On Mon, May 29, 2017 at 11:32:50AM -0400, Jeff King wrote:

> I'll try to put together patches in the next day or so. Comments welcome
> in the meantime.

So here they are. For those just joining us, the immediate problem is
that "git am -h" is broken (whether you're in a repo or not). That's
fixed by the first patch, which can go to "maint" separately (although
the rest are pretty unadventurous).

The rest of it cleans up "-h" handling for a variety of builtin
commands, and then adds a test to make sure they all behave sanely
(which unsurprisingly is how I found all the problems fixed by the
earlier patches).

  [1/8]: am: handle "-h" argument earlier
  [2/8]: credential: handle invalid arguments earlier
  [3/8]: upload-archive: handle "-h" option early
  [4/8]: remote-{ext,fd}: print usage message on invalid arguments
  [5/8]: submodule--helper: show usage for "-h"
  [6/8]: version: convert to parse-options
  [7/8]: git: add hidden --list-builtins option
  [8/8]: t0012: test "-h" with builtins

 builtin/am.c                |  3 +++
 builtin/credential.c        |  4 ++--
 builtin/remote-ext.c        |  5 ++++-
 builtin/remote-fd.c         |  5 ++++-
 builtin/submodule--helper.c |  5 ++---
 builtin/upload-archive.c    |  5 ++++-
 git.c                       | 12 ++++++++++++
 help.c                      | 25 ++++++++++++++++++++-----
 t/t0012-help.sh             | 12 ++++++++++++
 9 files changed, 63 insertions(+), 13 deletions(-)

-Peff

[PATCH 1/8] am: handle "-h" argument earlier

[Jeff King]

If the user provides "-h" on the command line, then our
parse_options() invocation will show a usage message and
quit. But if "-h" is the only argument, the git wrapper
behaves specially: it ignores our RUN_SETUP flag and calls
cmd_am() without having done repository setup at all.  This
is due to 99caeed05 (Let 'git <command> -h' show usage
without a git dir, 2009-11-09).

Before cmd_am() calls parse_options(), though, it runs a few
other setup functions. One of these is am_state_init(),
which uses git_pathdup() to set up the default rebase-apply
path. But calling git_pathdup() when we haven't done
repository setup will fall back to using ".git". That's
mostly harmless (since we won't use the value anyway), but
is forbidden since b1ef400eec ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20), and we now BUG().

We can't easily move that setup to after the parse_options()
call; the point is to set up defaults that are overwritten
by the option parsing. Instead, we'll detect the "-h" case
early and show the usage then. This matches the behavior of
other builtins which have a similar setup-ordering issue
(e.g., git-branch).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/am.c b/builtin/am.c
index 0f63dcab1..5ee146bfb 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2311,6 +2311,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(usage, options);
+
 	git_config(git_am_config, NULL);
 
 	am_state_init(&state);
-- 
2.13.0.613.g11c956365

[PATCH 2/8] credential: handle invalid arguments earlier

[Jeff King]

The git-credential command only takes one argument: the
operation to perform. If we don't have one, we complain
immediately. But if we have one that we don't recognize, we
don't notice until after we've read the credential from
stdin. This is likely to confuse a user invoking "git
credential -h", as the program will hang waiting for their
input before showing anything.

Let's detect this case early. Likewise, we never noticed
when there are extra arguments beyond the one we're
expecting. Let's catch this with the same conditional.

Note that we don't need to handle "--help" similarly,
because the git wrapper does this before even calling
cmd_credential().

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/credential.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/credential.c b/builtin/credential.c
index 0412fa00f..879acfbcd 100644
--- a/builtin/credential.c
+++ b/builtin/credential.c
@@ -10,9 +10,9 @@ int cmd_credential(int argc, const char **argv, const char *prefix)
 	const char *op;
 	struct credential c = CREDENTIAL_INIT;
 
-	op = argv[1];
-	if (!op)
+	if (argc != 2 || !strcmp(argv[1], "-h"))
 		usage(usage_msg);
+	op = argv[1];
 
 	if (credential_read(&c, stdin) < 0)
 		die("unable to read credential from stdin");
-- 
2.13.0.613.g11c956365

[PATCH 3/8] upload-archive: handle "-h" option early

[Jeff King]

Normally upload-archive forks off upload-archive--writer to
do the real work, and relays any errors back over the
sideband channel. This is a good thing when the command is
properly invoked remotely via ssh or git-daemon. But it's
confusing to curious users who try "git upload-archive -h".

Let's catch this invocation early and give a real usage
message, rather than spewing "-h does not appear to be a git
repository" amidst packet-lines. The chance of a false
positive due to a real client asking for the repo "-h" is
quite small.

Likewise, we'll catch "-h" in upload-archive--writer. People
shouldn't be invoking it manually, but it doesn't hurt to
give a sane message if they do.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/upload-archive.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index cde06977b..84532ae9a 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -22,7 +22,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
 	struct argv_array sent_argv = ARGV_ARRAY_INIT;
 	const char *arg_cmd = "argument ";
 
-	if (argc != 2)
+	if (argc != 2 || !strcmp(argv[1], "-h"))
 		usage(upload_archive_usage);
 
 	if (!enter_repo(argv[1], 0))
@@ -76,6 +76,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 {
 	struct child_process writer = { argv };
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage(upload_archive_usage);
+
 	/*
 	 * Set up sideband subprocess.
 	 *
-- 
2.13.0.613.g11c956365

[PATCH 4/8] remote-{ext,fd}: print usage message on invalid arguments

[Jeff King]

We just say "Expected two arguments" when we get a different
number of arguments, but we can be slightly friendlier.
People shouldn't generally be running remote helpers
themselves, but curious users might say "git remote-ext -h".

Signed-off-by: Jeff King <peff@peff.net>
---
According to remote-curl.c, we should actually handle the
1-argument case, too. I didn't dig into that because it's
orthogonal to this series, and it's not clear that anybody
cares.

 builtin/remote-ext.c | 5 ++++-
 builtin/remote-fd.c  | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 11b48bfb4..bfb21ba7d 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -3,6 +3,9 @@
 #include "run-command.h"
 #include "pkt-line.h"
 
+static const char usage_msg[] =
+	"git remote-ext <remote> <url>";
+
 /*
  * URL syntax:
  *	'command [arg1 [arg2 [...]]]'	Invoke command with given arguments.
@@ -193,7 +196,7 @@ static int command_loop(const char *child)
 int cmd_remote_ext(int argc, const char **argv, const char *prefix)
 {
 	if (argc != 3)
-		die("Expected two arguments");
+		usage(usage_msg);
 
 	return command_loop(argv[2]);
 }
diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c
index 08d7121b6..91dfe07e0 100644
--- a/builtin/remote-fd.c
+++ b/builtin/remote-fd.c
@@ -1,6 +1,9 @@
 #include "builtin.h"
 #include "transport.h"
 
+static const char usage_msg[] =
+	"git remote-fd <remote> <url>";
+
 /*
  * URL syntax:
  *	'fd::<inoutfd>[/<anything>]'		Read/write socket pair
@@ -57,7 +60,7 @@ int cmd_remote_fd(int argc, const char **argv, const char *prefix)
 	char *end;
 
 	if (argc != 3)
-		die("Expected two arguments");
+		usage(usage_msg);
 
 	input_fd = (int)strtoul(argv[2], &end, 10);
 
-- 
2.13.0.613.g11c956365

[PATCH 5/8] submodule--helper: show usage for "-h"

[Jeff King]

Normal users shouldn't ever call submodule--helper, but it
doesn't hurt to give them a normal usage message if they try
"-h".

Signed-off-by: Jeff King <peff@peff.net>
---
The usage message isn't that helpful _either_, and I admit
my ulterior motive is just to make the test at the end of
this series pass. :)

I was tempted to actually dump the names from the commands
array to stdout, but this command really is an internal
helper. It's probably not worth spending time on such
niceties.

 builtin/submodule--helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 566a5b6a6..a78b003c2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1222,9 +1222,8 @@ static struct cmd_struct commands[] = {
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 {
 	int i;
-	if (argc < 2)
-		die(_("submodule--helper subcommand must be "
-		      "called with a subcommand"));
+	if (argc < 2 || !strcmp(argv[1], "-h"))
+		usage("git submodule--helper <command>");
 
 	for (i = 0; i < ARRAY_SIZE(commands); i++) {
 		if (!strcmp(argv[1], commands[i].cmd)) {
-- 
2.13.0.613.g11c956365

[PATCH 6/8] version: convert to parse-options

[Jeff King]

The "git version" command didn't traditionally accept any
options, and in fact ignores any you give it. When we added
simple option parsing for "--build-options" in 6b9c38e14, we
didn't improve this; we just loop over the arguments and
pick out the one we recognize.

Instead, let's move to a real parsing loop, complain about
nonsense options, and recognize conventions like "-h".

Signed-off-by: Jeff King <peff@peff.net>
---
I assume nobody was running "git version --foobar" and expecting it to
work. I guess we could also complain about "git version foobar" (no
dashes), but this patch doesn't. Mainly I wanted the auto-generated
options list.

 help.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/help.c b/help.c
index a07f01e6f..41017e88e 100644
--- a/help.c
+++ b/help.c
@@ -8,6 +8,7 @@
 #include "column.h"
 #include "version.h"
 #include "refs.h"
+#include "parse-options.h"
 
 void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
@@ -424,16 +425,30 @@ const char *help_unknown_cmd(const char *cmd)
 
 int cmd_version(int argc, const char **argv, const char *prefix)
 {
+	int build_options = 0;
+	const char * const usage[] = {
+		N_("git version [<options>]"),
+		NULL
+	};
+	struct option options[] = {
+		OPT_BOOL(0, "build-options", &build_options,
+			 "also print build options"),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
 	/*
 	 * The format of this string should be kept stable for compatibility
 	 * with external projects that rely on the output of "git version".
+	 *
+	 * Always show the version, even if other options are given.
 	 */
 	printf("git version %s\n", git_version_string);
-	while (*++argv) {
-		if (!strcmp(*argv, "--build-options")) {
-			printf("sizeof-long: %d\n", (int)sizeof(long));
-			/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
-		}
+
+	if (build_options) {
+		printf("sizeof-long: %d\n", (int)sizeof(long));
+		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
 	}
 	return 0;
 }
-- 
2.13.0.613.g11c956365

[PATCH 7/8] git: add hidden --list-builtins option

[Jeff King]

It can be useful in the test suite to be able to iterate
over the list of builtins. We could do this with some
Makefile magic. But since the authoritative list is in the
commands array inside git.c, and since this could also be
handy for debugging, let's add a hidden command-line option
to dump that list.

Signed-off-by: Jeff King <peff@peff.net>
---
The forward declaration here is necessary because of the function
ordering. We could push handle_options() much lower in the file, but I
didn't think it was worth the churn.

 git.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/git.c b/git.c
index 8ff44f081..1b8b7f51a 100644
--- a/git.c
+++ b/git.c
@@ -26,6 +26,8 @@ static const char *env_names[] = {
 static char *orig_env[4];
 static int save_restore_env_balance;
 
+static void list_builtins(void);
+
 static void save_env_before_alias(void)
 {
 	int i;
@@ -232,6 +234,9 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			}
 			(*argv)++;
 			(*argc)--;
+		} else if (!strcmp(cmd, "--list-builtins")) {
+			list_builtins();
+			exit(0);
 		} else {
 			fprintf(stderr, "Unknown option: %s\n", cmd);
 			usage(git_usage_string);
@@ -529,6 +534,13 @@ int is_builtin(const char *s)
 	return !!get_builtin(s);
 }
 
+static void list_builtins(void)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(commands); i++)
+		printf("%s\n", commands[i].cmd);
+}
+
 #ifdef STRIP_EXTENSION
 static void strip_extension(const char **argv)
 {
-- 
2.13.0.613.g11c956365

[PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

Since commit 99caeed05 (Let 'git <command> -h' show usage
without a git dir, 2009-11-09), the git wrapper handles "-h"
specially, skipping any repository setup but still calling
the builtin's cmd_foo() function. This means that every
cmd_foo() must be ready to handle this case, but we don't
have any systematic tests. This led to "git am -h" being
broken for some time without anybody noticing.

This patch just tests that "git foo -h" works for every
builtin, where we see a 129 exit code (the normal code for
our usage() helper), and that the word "usage" appears in
the output.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t0012-help.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 8faba2e8b..487b92a5d 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -49,4 +49,16 @@ test_expect_success "--help does not work for guides" "
 	test_i18ncmp expect actual
 "
 
+test_expect_success 'generate builtin list' '
+	git --list-builtins >builtins
+'
+
+while read builtin
+do
+	test_expect_success "$builtin can handle -h" '
+		test_expect_code 129 git $builtin -h >output 2>&1 &&
+		test_i18ngrep usage output
+	'
+done <builtins
+
 test_done
-- 
2.13.0.613.g11c956365

Re: [PATCH 1/8] am: handle "-h" argument earlier

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We can't easily move that setup to after the parse_options()
> call; the point is to set up defaults that are overwritten
> by the option parsing. Instead, we'll detect the "-h" case
> early and show the usage then. This matches the behavior of
> other builtins which have a similar setup-ordering issue
> (e.g., git-branch).

Thanks.  And this structure of the series is very much appreciated.


> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/am.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 0f63dcab1..5ee146bfb 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2311,6 +2311,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>  
> +	if (argc == 2 && !strcmp(argv[1], "-h"))
> +		usage_with_options(usage, options);
> +
>  	git_config(git_am_config, NULL);
>  
>  	am_state_init(&state);

Re: [PATCH 0/8] consistent "-h" handling in builtins

[Junio C Hamano]

The series was pretty straight-forward.  Thanks for working on this.

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This patch just tests that "git foo -h" works for every
> builtin, where we see a 129 exit code (the normal code for
> our usage() helper), and that the word "usage" appears in
> the output.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t0012-help.sh | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index 8faba2e8b..487b92a5d 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -49,4 +49,16 @@ test_expect_success "--help does not work for guides" "
>  	test_i18ncmp expect actual
>  "
>  
> +test_expect_success 'generate builtin list' '
> +	git --list-builtins >builtins
> +'
> +
> +while read builtin
> +do
> +	test_expect_success "$builtin can handle -h" '
> +		test_expect_code 129 git $builtin -h >output 2>&1 &&
> +		test_i18ngrep usage output
> +	'
> +done <builtins
> +

These still seem to need further tweaks?

    diff-files
    diff-index
    diff-tree
    rev-list

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

On Tue, May 30, 2017 at 03:03:18PM +0900, Junio C Hamano wrote:

> > +test_expect_success 'generate builtin list' '
> > +	git --list-builtins >builtins
> > +'
> > +
> > +while read builtin
> > +do
> > +	test_expect_success "$builtin can handle -h" '
> > +		test_expect_code 129 git $builtin -h >output 2>&1 &&
> > +		test_i18ngrep usage output
> > +	'
> > +done <builtins
> > +
> 
> These still seem to need further tweaks?
> 
>     diff-files
>     diff-index
>     diff-tree
>     rev-list

How so? They pass the test for me, and the output for a manual run looks
sane.

-Peff

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, May 30, 2017 at 03:03:18PM +0900, Junio C Hamano wrote:
>
>> > +test_expect_success 'generate builtin list' '
>> > +	git --list-builtins >builtins
>> > +'
>> > +
>> > +while read builtin
>> > +do
>> > +	test_expect_success "$builtin can handle -h" '
>> > +		test_expect_code 129 git $builtin -h >output 2>&1 &&
>> > +		test_i18ngrep usage output
>> > +	'
>> > +done <builtins
>> > +
>> 
>> These still seem to need further tweaks?
>> 
>>     diff-files
>>     diff-index
>>     diff-tree
>>     rev-list
>
> How so? They pass the test for me, and the output for a manual run looks
> sane.

Am I missing some patches (I have these 8) outside the series,
perhaps?

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

On Tue, May 30, 2017 at 03:08:25PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, May 30, 2017 at 03:03:18PM +0900, Junio C Hamano wrote:
> >
> >> > +test_expect_success 'generate builtin list' '
> >> > +	git --list-builtins >builtins
> >> > +'
> >> > +
> >> > +while read builtin
> >> > +do
> >> > +	test_expect_success "$builtin can handle -h" '
> >> > +		test_expect_code 129 git $builtin -h >output 2>&1 &&
> >> > +		test_i18ngrep usage output
> >> > +	'
> >> > +done <builtins
> >> > +
> >> 
> >> These still seem to need further tweaks?
> >> 
> >>     diff-files
> >>     diff-index
> >>     diff-tree
> >>     rev-list
> >
> > How so? They pass the test for me, and the output for a manual run looks
> > sane.
> 
> Am I missing some patches (I have these 8) outside the series,
> perhaps?

Nope, I have those patches directly on your e83352ef23, and it passes. I
wonder if there's something funny between our environments. What does
the failure look like for you?

-Peff

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Nope, I have those patches directly on your e83352ef23, and it passes. I
> wonder if there's something funny between our environments. What does
> the failure look like for you?

Travis seems to be seeing the same failure.  Curiously, the topic by
itself passes for me; iow, pu fails, pu^2 doesn't fail.

    git.git/pu$ ./git rev-list -h
    BUG: environment.c:173: setup_git_env called without repository
    Aborted (core dumped)

Hmph...

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

On Tue, May 30, 2017 at 10:23:54PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Nope, I have those patches directly on your e83352ef23, and it passes. I
> > wonder if there's something funny between our environments. What does
> > the failure look like for you?
> 
> Travis seems to be seeing the same failure.  Curiously, the topic by
> itself passes for me; iow, pu fails, pu^2 doesn't fail.
> 
>     git.git/pu$ ./git rev-list -h
>     BUG: environment.c:173: setup_git_env called without repository
>     Aborted (core dumped)
> 
> Hmph...

Ah, OK, I can reproduce when merged with pu. Bisecting it was tricky.
To see the problem, you need both my new test _and_ b1ef400ee
(setup_git_env: avoid blind fall-back to ".git", 2016-10-20). The latter
is only in v2.13, so topics forked from v2.12 need that commit applied.

Anyway, the problem is sk/dash-is-previous, specifically fc5684b47
(revision.c: args starting with "-" might be a revision, 2017-02-25). It
looks like the revision parser used to just bail on "-h", because
revision.c would say "I don't recognize this" and then cmd_rev_list()
would similarly say "I don't recognize this" and call usage(). But now
we actually try to read it as a ref, which obviously requires being
inside a repository.

Normally that's OK, but because of the "-h doesn't set up the repo"
thing from 99caeed05, we may not have setup the repo, and so looking up
refs is forbidden. The fix is probably to have revision.c explicitly
recognize "-h" and bail on it as an unknown option (it can't handle
the flag itself because only the caller knows the full usage()).

I do wonder, though, if there's any other downside to trying to look up
other options as revisions (at least it wastes time doing nonsense
revision lookups on options known only to cmd_rev_list()).  I'm not sure
why that commit passes everything starting with a dash as a possible
revision, rather than just "-".

I.e.:

diff --git a/revision.c b/revision.c
index 5470c33ac..1e26c3951 100644
--- a/revision.c
+++ b/revision.c
@@ -2233,7 +2233,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 			}
 			if (opts < 0)
 				exit(128);
-			maybe_opt = 1;
+			if (arg[1]) {
+				/* arg is an unknown option */
+				argv[left++] = arg;
+				continue;
+			} else {
+				/* special token "-" */
+				maybe_opt = 1;
+			}
 		}
 
 

I don't see anything in the commit message, but I didn't dig in the
mailing list. 


-Peff

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

[+cc Siddharth, so quoting copiously]

On Tue, May 30, 2017 at 11:27:56AM -0400, Jeff King wrote:

> > Travis seems to be seeing the same failure.  Curiously, the topic by
> > itself passes for me; iow, pu fails, pu^2 doesn't fail.
> > 
> >     git.git/pu$ ./git rev-list -h
> >     BUG: environment.c:173: setup_git_env called without repository
> >     Aborted (core dumped)
> > 
> > Hmph...
> 
> Ah, OK, I can reproduce when merged with pu. Bisecting it was tricky.
> To see the problem, you need both my new test _and_ b1ef400ee
> (setup_git_env: avoid blind fall-back to ".git", 2016-10-20). The latter
> is only in v2.13, so topics forked from v2.12 need that commit applied.
> 
> Anyway, the problem is sk/dash-is-previous, specifically fc5684b47
> (revision.c: args starting with "-" might be a revision, 2017-02-25). It
> looks like the revision parser used to just bail on "-h", because
> revision.c would say "I don't recognize this" and then cmd_rev_list()
> would similarly say "I don't recognize this" and call usage(). But now
> we actually try to read it as a ref, which obviously requires being
> inside a repository.
> 
> Normally that's OK, but because of the "-h doesn't set up the repo"
> thing from 99caeed05, we may not have setup the repo, and so looking up
> refs is forbidden. The fix is probably to have revision.c explicitly
> recognize "-h" and bail on it as an unknown option (it can't handle
> the flag itself because only the caller knows the full usage()).
> 
> I do wonder, though, if there's any other downside to trying to look up
> other options as revisions (at least it wastes time doing nonsense
> revision lookups on options known only to cmd_rev_list()).  I'm not sure
> why that commit passes everything starting with a dash as a possible
> revision, rather than just "-".
> 
> I.e.:
> 
> diff --git a/revision.c b/revision.c
> index 5470c33ac..1e26c3951 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2233,7 +2233,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  			}
>  			if (opts < 0)
>  				exit(128);
> -			maybe_opt = 1;
> +			if (arg[1]) {
> +				/* arg is an unknown option */
> +				argv[left++] = arg;
> +				continue;
> +			} else {
> +				/* special token "-" */
> +				maybe_opt = 1;
> +			}
>  		}
>  
>  
> 
> I don't see anything in the commit message, but I didn't dig in the
> mailing list.

I think this line of reasoning comes from

  http://public-inbox.org/git/20170206181026.GA4010@ubuntu-512mb-blr1-01.localdomain/

And the idea is that ranges like "-.." should work. TBH, I'm not sure
how I feel about that, for exactly the reason that came up here: it
makes it hard to syntactically differentiate the "-" shorthand from
actual options. We do have @{-1} already for this purpose. I don't mind
"-" as a shortcut for things like "git checkout -" or "git show -", but
it feels like most of the benefit is lost when you're combining it with
other operators.

-Peff

[PATCH 6.5?/8] version: move --build-options to a test helper

[Ævar Arnfjörð Bjarmason]

Move the undocumented --build-options argument to a test helper. It's
purely used for testing git itself, so it belongs in a test helper
instead of something that's part of the public plumbing.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Tue, May 30, 2017 at 7:17 AM, Jeff King <peff@peff.net> wrote:
> The "git version" command didn't traditionally accept any
> options, and in fact ignores any you give it. When we added
> simple option parsing for "--build-options" in 6b9c38e14, we
> didn't improve this; we just loop over the arguments and
> pick out the one we recognize.
>
> Instead, let's move to a real parsing loop, complain about
> nonsense options, and recognize conventions like "-h".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I assume nobody was running "git version --foobar" and expecting it to
> work. I guess we could also complain about "git version foobar" (no
> dashes), but this patch doesn't. Mainly I wanted the auto-generated
> options list.

Looks good to me. I started hacking this up the other day, but then
thought "wait a minute, isn't this just a test helper?" and wrote this
which I've rebased on top of your change.

I may be missing something here but isn't this a much straightforward
way to accomplish this, or is this used by some external program
outside of git.git that's going to rely on --build-options output?

 Makefile                      | 1 +
 help.c                        | 7 -------
 t/helper/.gitignore           | 1 +
 t/helper/test-long-is-64bit.c | 6 ++++++
 t/test-lib.sh                 | 9 +--------
 5 files changed, 9 insertions(+), 15 deletions(-)
 create mode 100644 t/helper/test-long-is-64bit.c

diff --git a/Makefile b/Makefile
index 2ed6db728a..aa908ae75a 100644
--- a/Makefile
+++ b/Makefile
@@ -623,6 +623,7 @@ TEST_PROGRAMS_NEED_X += test-hashmap
 TEST_PROGRAMS_NEED_X += test-index-version
 TEST_PROGRAMS_NEED_X += test-lazy-init-name-hash
 TEST_PROGRAMS_NEED_X += test-line-buffer
+TEST_PROGRAMS_NEED_X += test-long-is-64bit
 TEST_PROGRAMS_NEED_X += test-match-trees
 TEST_PROGRAMS_NEED_X += test-mergesort
 TEST_PROGRAMS_NEED_X += test-mktemp
diff --git a/help.c b/help.c
index f637fc8006..0a7628a922 100644
--- a/help.c
+++ b/help.c
@@ -384,14 +384,11 @@ const char *help_unknown_cmd(const char *cmd)
 
 int cmd_version(int argc, const char **argv, const char *prefix)
 {
-	int build_options = 0;
 	const char * const usage[] = {
 		N_("git version [<options>]"),
 		NULL
 	};
 	struct option options[] = {
-		OPT_BOOL(0, "build-options", &build_options,
-			 "also print build options"),
 		OPT_END()
 	};
 
@@ -405,10 +402,6 @@ int cmd_version(int argc, const char **argv, const char *prefix)
 	 */
 	printf("git version %s\n", git_version_string);
 
-	if (build_options) {
-		printf("sizeof-long: %d\n", (int)sizeof(long));
-		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
-	}
 	return 0;
 }
 
diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index 721650256e..739c4c745c 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -13,6 +13,7 @@
 /test-index-version
 /test-lazy-init-name-hash
 /test-line-buffer
+/test-long-is-64bit
 /test-match-trees
 /test-mergesort
 /test-mktemp
diff --git a/t/helper/test-long-is-64bit.c b/t/helper/test-long-is-64bit.c
new file mode 100644
index 0000000000..45fc120432
--- /dev/null
+++ b/t/helper/test-long-is-64bit.c
@@ -0,0 +1,6 @@
+#include "git-compat-util.h"
+
+int cmd_main(int argc, const char **argv)
+{
+	return (8 <= (int)sizeof(long)) ? 0 : 1;
+}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ec2571f018..bf649fbc03 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1165,14 +1165,7 @@ run_with_limited_cmdline () {
 
 test_lazy_prereq CMDLINE_LIMIT 'run_with_limited_cmdline true'
 
-build_option () {
-	git version --build-options |
-	sed -ne "s/^$1: //p"
-}
-
-test_lazy_prereq LONG_IS_64BIT '
-	test 8 -le "$(build_option sizeof-long)"
-'
+test_lazy_prereq LONG_IS_64BIT 'test-long-is-64bit'
 
 test_lazy_prereq TIME_IS_64BIT 'test-date is64bit'
 test_lazy_prereq TIME_T_IS_64BIT 'test-date time_t-is64bit'
-- 
2.13.0.303.g4ebf302169

Re: [PATCH 6.5?/8] version: move --build-options to a test helper

[Jeff King]

On Tue, May 30, 2017 at 08:45:53PM +0000, Ævar Arnfjörð Bjarmason wrote:

> On Tue, May 30, 2017 at 7:17 AM, Jeff King <peff@peff.net> wrote:
> > The "git version" command didn't traditionally accept any
> > options, and in fact ignores any you give it. When we added
> > simple option parsing for "--build-options" in 6b9c38e14, we
> > didn't improve this; we just loop over the arguments and
> > pick out the one we recognize.
> >
> > Instead, let's move to a real parsing loop, complain about
> > nonsense options, and recognize conventions like "-h".
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > I assume nobody was running "git version --foobar" and expecting it to
> > work. I guess we could also complain about "git version foobar" (no
> > dashes), but this patch doesn't. Mainly I wanted the auto-generated
> > options list.
> 
> Looks good to me. I started hacking this up the other day, but then
> thought "wait a minute, isn't this just a test helper?" and wrote this
> which I've rebased on top of your change.
> 
> I may be missing something here but isn't this a much straightforward
> way to accomplish this, or is this used by some external program
> outside of git.git that's going to rely on --build-options output?

My intent in putting it into the actual git binary was that it could
also be useful for collecting build-time knobs from users (who may be
using a binary package). Like:

  http://public-inbox.org/git/20160712035719.GA30281@sigill.intra.peff.net/

We haven't filled in that NEEDSWORK yet, but I'd rather see us go in
that direction than remove the option entirely.

-Peff

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> And the idea is that ranges like "-.." should work. TBH, I'm not sure
> how I feel about that, for exactly the reason that came up here: it
> makes it hard to syntactically differentiate the "-" shorthand from
> actual options. We do have @{-1} already for this purpose. I don't mind
> "-" as a shortcut for things like "git checkout -" or "git show -", but
> it feels like most of the benefit is lost when you're combining it with
> other operators.

All correct and that is why I haven't seriously considered merging
the topic further down (yet).  Things like -.. makes readers of the
commands go "Huh?", and "git tbdiff ..- -.." does not even work ;-)

Re: [PATCH 6.5?/8] version: move --build-options to a test helper

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2116 bytes --]

Hi,

On Tue, 30 May 2017, Jeff King wrote:

> On Tue, May 30, 2017 at 08:45:53PM +0000, Ævar Arnfjörð Bjarmason wrote:
> 
> > On Tue, May 30, 2017 at 7:17 AM, Jeff King <peff@peff.net> wrote:
> > > The "git version" command didn't traditionally accept any
> > > options, and in fact ignores any you give it. When we added
> > > simple option parsing for "--build-options" in 6b9c38e14, we
> > > didn't improve this; we just loop over the arguments and
> > > pick out the one we recognize.
> > >
> > > Instead, let's move to a real parsing loop, complain about
> > > nonsense options, and recognize conventions like "-h".
> > >
> > > Signed-off-by: Jeff King <peff@peff.net>
> > > ---
> > > I assume nobody was running "git version --foobar" and expecting it to
> > > work. I guess we could also complain about "git version foobar" (no
> > > dashes), but this patch doesn't. Mainly I wanted the auto-generated
> > > options list.
> > 
> > Looks good to me. I started hacking this up the other day, but then
> > thought "wait a minute, isn't this just a test helper?" and wrote this
> > which I've rebased on top of your change.
> > 
> > I may be missing something here but isn't this a much straightforward
> > way to accomplish this, or is this used by some external program
> > outside of git.git that's going to rely on --build-options output?
> 
> My intent in putting it into the actual git binary was that it could
> also be useful for collecting build-time knobs from users (who may be
> using a binary package). Like:
> 
>   http://public-inbox.org/git/20160712035719.GA30281@sigill.intra.peff.net/
> 
> We haven't filled in that NEEDSWORK yet, but I'd rather see us go in
> that direction than remove the option entirely.

FWIW it also helped Git for Windows.

The two additional bits we added to the build options (the commit from
which Git was built and the architecture) did not hurt one bit, either.

In other words, it would make my life a lot harder if --build-options were
moved to a test helper that does not ship with the end product.

Ciao,
Dscho

Re: [PATCH 6.5?/8] version: move --build-options to a test helper

[Jeff King]

On Wed, May 31, 2017 at 05:27:21PM +0200, Johannes Schindelin wrote:

> > My intent in putting it into the actual git binary was that it could
> > also be useful for collecting build-time knobs from users (who may be
> > using a binary package). Like:
> > 
> >   http://public-inbox.org/git/20160712035719.GA30281@sigill.intra.peff.net/
> > 
> > We haven't filled in that NEEDSWORK yet, but I'd rather see us go in
> > that direction than remove the option entirely.
> 
> FWIW it also helped Git for Windows.
> 
> The two additional bits we added to the build options (the commit from
> which Git was built and the architecture) did not hurt one bit, either.
> 
> In other words, it would make my life a lot harder if --build-options were
> moved to a test helper that does not ship with the end product.

Cool, I'm glad it has helped already. If you have further bits added to
the output, is it worth sending that patch upstream?

-Peff

Re: [PATCH 6.5?/8] version: move --build-options to a test helper

[Johannes Schindelin]

Hi Peff,

On Wed, 31 May 2017, Jeff King wrote:

> On Wed, May 31, 2017 at 05:27:21PM +0200, Johannes Schindelin wrote:
> 
> > > My intent in putting it into the actual git binary was that it could
> > > also be useful for collecting build-time knobs from users (who may be
> > > using a binary package). Like:
> > > 
> > >   http://public-inbox.org/git/20160712035719.GA30281@sigill.intra.peff.net/
> > > 
> > > We haven't filled in that NEEDSWORK yet, but I'd rather see us go in
> > > that direction than remove the option entirely.
> > 
> > FWIW it also helped Git for Windows.
> > 
> > The two additional bits we added to the build options (the commit from
> > which Git was built and the architecture) did not hurt one bit, either.
> > 
> > In other words, it would make my life a lot harder if --build-options were
> > moved to a test helper that does not ship with the end product.
> 
> Cool, I'm glad it has helped already. If you have further bits added to
> the output, is it worth sending that patch upstream?

Yes, of course.

The day only has 24h though and I am still stuck with other things I try
to contribute that seem to be requiring a lot more effort (mostly trying
to make my case that there are certain coding paradigms I find too sloppy
to put my name on) from my side to get accepted than I'd like.

So yeah, as soon as the queue drains a bit more, I have tons more patches
ready to go upstream.

Ciao,
Dscho

Re: [PATCH 6.5?/8] version: move --build-options to a test helper

[Ævar Arnfjörð Bjarmason]

On Wed, May 31, 2017 at 5:46 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Peff,
>
> On Wed, 31 May 2017, Jeff King wrote:
>
>> On Wed, May 31, 2017 at 05:27:21PM +0200, Johannes Schindelin wrote:
>>
>> > > My intent in putting it into the actual git binary was that it could
>> > > also be useful for collecting build-time knobs from users (who may be
>> > > using a binary package). Like:
>> > >
>> > >   http://public-inbox.org/git/20160712035719.GA30281@sigill.intra.peff.net/
>> > >
>> > > We haven't filled in that NEEDSWORK yet, but I'd rather see us go in
>> > > that direction than remove the option entirely.
>> >
>> > FWIW it also helped Git for Windows.
>> >
>> > The two additional bits we added to the build options (the commit from
>> > which Git was built and the architecture) did not hurt one bit, either.
>> >
>> > In other words, it would make my life a lot harder if --build-options were
>> > moved to a test helper that does not ship with the end product.
>>
>> Cool, I'm glad it has helped already. If you have further bits added to
>> the output, is it worth sending that patch upstream?
>
> Yes, of course.
>
> The day only has 24h though and I am still stuck with other things I try
> to contribute that seem to be requiring a lot more effort (mostly trying
> to make my case that there are certain coding paradigms I find too sloppy
> to put my name on) from my side to get accepted than I'd like.
>
> So yeah, as soon as the queue drains a bit more, I have tons more patches
> ready to go upstream.

Thanks both. It makes sense to discard this patch.

I wasn't sure whether anyone really cared about this, and thought a
patch was a better starting point of discussion.

Re: [PATCH 6.5?/8] version: move --build-options to a test helper

[Jeff King]

On Wed, May 31, 2017 at 07:51:20PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Thanks both. It makes sense to discard this patch.
> 
> I wasn't sure whether anyone really cared about this, and thought a
> patch was a better starting point of discussion.

I will never complain about somebody starting a discussion with a patch
as long as they don't mind if it gets shot down. :)

Thanks for raising the point. I do think it was worth thinking about.

-Peff

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Anyway, the problem is sk/dash-is-previous, specifically fc5684b47
> (revision.c: args starting with "-" might be a revision, 2017-02-25). It
> looks like the revision parser used to just bail on "-h", because
> revision.c would say "I don't recognize this" and then cmd_rev_list()
> would similarly say "I don't recognize this" and call usage(). But now
> we actually try to read it as a ref, which obviously requires being
> inside a repository.

Heh, I found another ;-)  

95e98cd9 ("revision.c: use refs_for_each*() instead of
for_each_*_submodule()", 2017-04-19), which is in the middle of
Duy's nd/prune-in-worktree series, does this:

#0  die (err=0x6128f8 "BUG: setup_git_env called without repository") at usage.c:114
#1  0x00000000004f9467 in setup_git_env () at environment.c:172
#2  0x00000000004f966c in get_git_dir () at environment.c:214
#3  0x000000000055113b in get_main_ref_store () at refs.c:1544
#4  0x0000000000570ee0 in handle_revision_pseudo_opt (submodule=0x0, 
    revs=0x7fffffffd6a0, argc=1, argv=0x7fffffffe180, flags=0x7fffffffc59c)
    at revision.c:2110
#5  0x00000000005716f5 in setup_revisions (argc=2, argv=0x7fffffffe178, 
    revs=0x7fffffffd6a0, opt=0x0) at revision.c:2254
#6  0x000000000043074a in cmd_diff_files (argc=2, argv=0x7fffffffe178, prefix=0x0)
    at builtin/diff-files.c:29
#7  0x0000000000405907 in run_builtin (p=0x87ba00 <commands+672>, argc=2, 
    argv=0x7fffffffe178) at git.c:376
#8  0x0000000000405bb5 in handle_builtin (argc=2, argv=0x7fffffffe178) at git.c:584
#9  0x0000000000405e04 in cmd_main (argc=2, argv=0x7fffffffe178) at git.c:683
#10 0x00000000004a3364 in main (argc=2, argv=0x7fffffffe178) at common-main.c:43

when jk/consistent-h is merged into it and then "git diff-files -h"
is run.

I guess anything that calls setup_revisions() from the "git cmd -h"
bypass need to be prepared with that

  check_help_option(argc, argv, usage, options);

thing.  Which is a bit sad, but I tend to agree with you that
restructuring to make usage[] of everybody available to git.c
is probably too noisy for the benefit it would give us.

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

On Thu, Jun 01, 2017 at 01:17:55PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Anyway, the problem is sk/dash-is-previous, specifically fc5684b47
> > (revision.c: args starting with "-" might be a revision, 2017-02-25). It
> > looks like the revision parser used to just bail on "-h", because
> > revision.c would say "I don't recognize this" and then cmd_rev_list()
> > would similarly say "I don't recognize this" and call usage(). But now
> > we actually try to read it as a ref, which obviously requires being
> > inside a repository.
> 
> Heh, I found another ;-)  
> 
> 95e98cd9 ("revision.c: use refs_for_each*() instead of
> for_each_*_submodule()", 2017-04-19), which is in the middle of
> Duy's nd/prune-in-worktree series, does this:

Hrm, yeah. The problem is that handle_revision_pseudo_opt() initializes
the ref store at the top of the function, even if we don't see any
arguments that require us to use it (and obviously in the "-h" case, we
don't).

That's an implementation detail that we could fix, but I do think in
general that we should probably just declare it forbidden to call
setup_revisions() when the repo hasn't been discovered.

> I guess anything that calls setup_revisions() from the "git cmd -h"
> bypass need to be prepared with that
> 
>   check_help_option(argc, argv, usage, options);
> 
> thing.  Which is a bit sad, but I tend to agree with you that
> restructuring to make usage[] of everybody available to git.c
> is probably too noisy for the benefit it would give us.

The other options are:

  - reverting the "-h" magic in git.c. It really is the source of most
    of this confusion, I think, because functions which assume RUN_SETUP
    are having that assumption broken. But at the same time I do think
    it makes "-h" a lot friendlier, and I'd prefer to keep it.

  - reverting the BUG() in setup_git_env(); this has been flushing out a
    lot of bugs, and I think is worth keeping

I did look at writing something like check_help_option(). One of the
annoyances is that we have two different usage formats: one that's a
straight string for usage(), and one that's an array-of-strings for
parse_options(). We could probably unify those.

It doesn't actually save that much code, though. The real value is that
it abstracts the "did git.c decide to skip RUN_SETUP?" logic.

-Peff

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Junio C Hamano]

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

> Heh, I found another ;-)  
>
> 95e98cd9 ("revision.c: use refs_for_each*() instead of
> for_each_*_submodule()", 2017-04-19), which is in the middle of
> Duy's nd/prune-in-worktree series, does this:
> ...
> when jk/consistent-h is merged into it and then "git diff-files -h"
> is run.
>
> I guess anything that calls setup_revisions() from the "git cmd -h"
> bypass need to be prepared with that
>
>   check_help_option(argc, argv, usage, options);
>
> thing.  Which is a bit sad, but I tend to agree with you that
> restructuring to make usage[] of everybody available to git.c
> is probably too noisy for the benefit it would give us.

For now, I will mix this in when queuing the whole thing in 'pu', as
I hate to push out something that does not even work for me to the
general public.

-- >8 --
Subject: [PATCH] diff- and log- family: handle "git cmd -h" early

"git $builtin -h" bypasses the usual repository setup and calls the
cmd_$builtin() function, expecting it to show the help text.

Unfortunately the commands in the log- and the diff- family want to
call into the revisions machinery, which by definition needs to have
a repository already discovered, before they can parse the options.

Handle the "git $builtin -h" special case very early in these
commands to work around potential issues.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/diff-files.c | 3 +++
 builtin/diff-index.c | 3 +++
 builtin/diff-tree.c  | 3 +++
 builtin/rev-list.c   | 3 +++
 4 files changed, 12 insertions(+)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 15c61fd8d1..6be1df684a 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -20,6 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	int result;
 	unsigned options = 0;
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage(diff_files_usage);
+
 	init_revisions(&rev, prefix);
 	gitmodules_config();
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 1af373d002..02dd35ba45 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -17,6 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	int i;
 	int result;
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage(diff_cache_usage);
+
 	init_revisions(&rev, prefix);
 	gitmodules_config();
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 5ea1c14317..f633b10b08 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -105,6 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	struct setup_revision_opt s_r_opt;
 	int read_stdin = 0;
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage(diff_tree_usage);
+
 	init_revisions(opt, prefix);
 	gitmodules_config();
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 718c6059c9..b250c515b1 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -277,6 +277,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	int use_bitmap_index = 0;
 	const char *show_progress = NULL;
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage(rev_list_usage);
+
 	git_config(git_default_config, NULL);
 	init_revisions(&revs, prefix);
 	revs.abbrev = DEFAULT_ABBREV;
-- 
2.13.0-513-g1c0955652f

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Junio C Hamano]

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

> For now, I will mix this in when queuing the whole thing in 'pu', as
> I hate to push out something that does not even work for me to the
> general public.
> 
> -- >8 --
> Subject: [PATCH] diff- and log- family: handle "git cmd -h" early
> ...

And then the check_help_option() thing may look like this.  

I am not proud of the way it "unifies" the two styles of usage
strings, obviously.

One benefit this patch has is that it makes it easier to highlight
what it does *not* touch.

    $ git grep -A2 -E -e 'a(rg)?c [!=]= 2 .*strcmp.*-h'

shows there are somewhat curious construct

	if (argc != 2 || !strcmp(argv[1], "-h"))
		usage(...);

left in the code.  Upon closer inspection, they all happen to be
doing the right thing for their current set of options and
arguments, but they are somewhat ugly.


 builtin/am.c               |  3 +--
 builtin/branch.c           |  3 +--
 builtin/check-ref-format.c |  3 +--
 builtin/checkout-index.c   |  7 ++++---
 builtin/commit.c           |  6 ++----
 builtin/diff-files.c       |  3 +--
 builtin/diff-index.c       |  3 +--
 builtin/diff-tree.c        |  3 +--
 builtin/gc.c               |  3 +--
 builtin/index-pack.c       |  3 +--
 builtin/ls-files.c         |  3 +--
 builtin/merge-ours.c       |  3 +--
 builtin/merge.c            |  3 +--
 builtin/pack-redundant.c   |  3 +--
 builtin/rev-list.c         |  3 +--
 builtin/update-index.c     |  3 +--
 builtin/upload-archive.c   |  3 +--
 fast-import.c              |  3 +--
 git-compat-util.h          |  3 +++
 usage.c                    | 11 +++++++++++
 20 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 8881d73615..12b7298907 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2307,8 +2307,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(usage, options);
+	check_help_option(argc, argv, usage, options);
 
 	git_config(git_am_config, NULL);
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 83fcda43dc..8c4465f0e4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -599,8 +599,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 	filter.kind = FILTER_REFS_BRANCHES;
 	filter.abbrev = -1;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_branch_usage, options);
+	check_help_option(argc, argv, builtin_branch_usage, options);
 
 	git_config(git_branch_config, NULL);
 
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac499450f..aab5776dd5 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -55,8 +55,7 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 	int flags = 0;
 	const char *refname;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_check_ref_format_usage);
+	check_help_option(argc, argv, builtin_check_ref_format_usage, NULL);
 
 	if (argc == 3 && !strcmp(argv[1], "--branch"))
 		return check_ref_format_branch(argv[2]);
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 07631d0c9c..8dd28ae8ba 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -179,9 +179,10 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_checkout_index_usage,
-				   builtin_checkout_index_options);
+	check_help_option(argc, argv,
+			  builtin_checkout_index_usage,
+			  builtin_checkout_index_options);
+
 	git_config(git_default_config, NULL);
 	prefix_length = prefix ? strlen(prefix) : 0;
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 66c9ac587b..05c2f61e33 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1376,8 +1376,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_status_usage, builtin_status_options);
+	check_help_option(argc, argv, builtin_status_usage, builtin_status_options);
 
 	status_init_config(&s, git_status_config);
 	argc = parse_options(argc, argv, prefix,
@@ -1661,8 +1660,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_commit_usage, builtin_commit_options);
+	check_help_option(argc, argv, builtin_commit_usage, builtin_commit_options);
 
 	status_init_config(&s, git_commit_config);
 	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index c97069a714..ff52edb46c 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -20,8 +20,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	int result;
 	unsigned options = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_files_usage);
+	check_help_option(argc, argv, diff_files_usage, NULL);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index d59bf6cf5f..518482850e 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -17,8 +17,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 	int i;
 	int result;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_cache_usage);
+	check_help_option(argc, argv, diff_cache_usage, NULL);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(&rev, prefix);
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index a4e7398b4b..aa12c02203 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -105,8 +105,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	struct setup_revision_opt s_r_opt;
 	int read_stdin = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_tree_usage);
+	check_help_option(argc, argv, diff_tree_usage, NULL);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	init_revisions(opt, prefix);
diff --git a/builtin/gc.c b/builtin/gc.c
index f484eda43c..b1a6163347 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -363,8 +363,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_gc_usage, builtin_gc_options);
+	check_help_option(argc, argv, builtin_gc_usage, builtin_gc_options);
 
 	argv_array_pushl(&pack_refs_cmd, "pack-refs", "--all", "--prune", NULL);
 	argv_array_pushl(&reflog, "reflog", "expire", "--all", NULL);
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 04b9dcaf0f..2be24276d6 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1640,8 +1640,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	unsigned foreign_nr = 1;	/* zero is a "good" value, assume bad */
 	int report_end_of_input = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(index_pack_usage);
+	check_help_option(argc, argv, index_pack_usage, NULL);
 
 	check_replace_refs = 0;
 	fsck_options.walk = mark_link;
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b376afc312..6d5334aae5 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -587,8 +587,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(ls_files_usage, builtin_ls_files_options);
+	check_help_option(argc, argv, ls_files_usage, builtin_ls_files_options);
 
 	memset(&dir, 0, sizeof(dir));
 	prefix = cmd_prefix;
diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
index 684411694f..52be2fa2f4 100644
--- a/builtin/merge-ours.c
+++ b/builtin/merge-ours.c
@@ -20,8 +20,7 @@ static const char *diff_index_args[] = {
 
 int cmd_merge_ours(int argc, const char **argv, const char *prefix)
 {
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_merge_ours_usage);
+	check_help_option(argc, argv, builtin_merge_ours_usage, NULL);
 
 	/*
 	 * We need to exit with 2 if the index does not match our HEAD tree,
diff --git a/builtin/merge.c b/builtin/merge.c
index eab03a026d..446eb0f3fb 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1108,8 +1108,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	void *branch_to_free;
 	int orig_argc = argc;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_merge_usage, builtin_merge_options);
+	check_help_option(argc, argv, builtin_merge_usage, builtin_merge_options);
 
 	/*
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index cb1df1c761..80603b9b47 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -601,8 +601,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
 	unsigned char *sha1;
 	char buf[42]; /* 40 byte sha1 + \n + \0 */
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(pack_redundant_usage);
+	check_help_option(argc, argv, pack_redundant_usage, NULL);
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index b250c515b1..ce6acf18c7 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -277,8 +277,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	int use_bitmap_index = 0;
 	const char *show_progress = NULL;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(rev_list_usage);
+	check_help_option(argc, argv, rev_list_usage, NULL);
 
 	git_config(git_default_config, NULL);
 	init_revisions(&revs, prefix);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 32fd977b43..e6df968056 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1009,8 +1009,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(update_index_usage, options);
+	check_help_option(argc, argv, update_index_usage, options);
 
 	git_config(git_default_config, NULL);
 
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 84532ae9a9..0e097969db 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -76,8 +76,7 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 {
 	struct child_process writer = { argv };
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(upload_archive_usage);
+	check_help_option(argc, argv, upload_archive_usage, NULL);
 
 	/*
 	 * Set up sideband subprocess.
diff --git a/fast-import.c b/fast-import.c
index 9a22fc92c0..fd7a4fb472 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3447,8 +3447,7 @@ int cmd_main(int argc, const char **argv)
 {
 	unsigned int i;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(fast_import_usage);
+	check_help_option(argc, argv, fast_import_usage, NULL);
 
 	setup_git_directory();
 	reset_pack_idx_option(&pack_idx_opts);
diff --git a/git-compat-util.h b/git-compat-util.h
index 22b756ed51..c30b6b9a72 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -418,6 +418,9 @@ extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+struct option;
+extern void check_help_option(int, const char **, const void *, struct option *);
+
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
 #include "compat/apple-common-crypto.h"
diff --git a/usage.c b/usage.c
index 2f87ca69a8..007d732094 100644
--- a/usage.c
+++ b/usage.c
@@ -5,6 +5,7 @@
  */
 #include "git-compat-util.h"
 #include "cache.h"
+#include "parse-options.h"
 
 void vreportf(const char *prefix, const char *err, va_list params)
 {
@@ -225,3 +226,13 @@ NORETURN void BUG(const char *fmt, ...)
 	va_end(ap);
 }
 #endif
+
+void check_help_option(int argc, const char **argv, const void *help, struct option *opt)
+{
+	if (argc == 2 && !strcmp(argv[1], "-h")) {
+		if (opt)
+			usage_with_options((const char * const *)help, opt);
+		else
+			usage((const char *)help);
+	}
+}
-- 
2.13.0-513-g1c0955652f

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

On Thu, Jun 01, 2017 at 02:42:13PM +0900, Junio C Hamano wrote:

> For now, I will mix this in when queuing the whole thing in 'pu', as
> I hate to push out something that does not even work for me to the
> general public.
> 
> -- >8 --
> Subject: [PATCH] diff- and log- family: handle "git cmd -h" early

Yeah, I think this is a good step in the interim.

-Peff

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

On Thu, Jun 01, 2017 at 02:54:08PM +0900, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > For now, I will mix this in when queuing the whole thing in 'pu', as
> > I hate to push out something that does not even work for me to the
> > general public.
> > 
> > -- >8 --
> > Subject: [PATCH] diff- and log- family: handle "git cmd -h" early
> > ...
> 
> And then the check_help_option() thing may look like this.  
> 
> I am not proud of the way it "unifies" the two styles of usage
> strings, obviously.

Heh, I came up with a similar hack. It is pretty nasty.

If we don't mind a one-time pain, I think we can just convert the
existing usage() to something more like usage_with_options(). The patch
below does that (and teaches the latter to handle a NULL options field).

The diff is ugly, and the conversion is mostly mechanical. But I think
some sites can be improved. For example, look at the change in bundle.c,
which now hands a set of strings rather than formatting the whole "or:"
chain manually.

I think we could take this even further and break some of the really
long entries (e.g., index-pack) into:

  static const char *index_pack_usage[] = {
	"git index-pack <pack-file>",
	"git index-pack --stdin",
	"",
	"-v",
	"-o <index-file>",
	etc...
	NULL
  };

With bonus points for actually describing each option (though at that
point it may be as much work to just convert the thing to parse-options,
which would also be fine with me).

That's tedious work which would need attention paid to each individual
command. So I'd probably do a single mechanical patch like this one
(that keeps the output identical), and then let people fix each one up
on top.

> One benefit this patch has is that it makes it easier to highlight
> what it does *not* touch.
> 
>     $ git grep -A2 -E -e 'a(rg)?c [!=]= 2 .*strcmp.*-h'
> 
> shows there are somewhat curious construct
> 
> 	if (argc != 2 || !strcmp(argv[1], "-h"))
> 		usage(...);
> 
> left in the code.  Upon closer inspection, they all happen to be
> doing the right thing for their current set of options and
> arguments, but they are somewhat ugly.

Yeah, I noticed those too while looking for prior art in my "-h" series.
I think they're all sane. And in fact some of them are necessary (and I
think I even added one like what you quoted above) because the arguments
are not otherwise parsed.

So for something that calls parse_options() later, just:

  if (argc == 2 && !strcmp(argv[1], "-h"))
	usage(...);

is sufficient to catch the single funny case, and parse_options()
catches the rest. But for some commands, this is all the parsing they
do. We could split that in two, like:

  /* early "-h" check */
  if (argc == 2 && !strcmp(argv[1], "-h"))
	usage(...);
  /* now check for argument sanity */
  if (argc != 2)
	usage(...);

That would make sense if we wanted to treat "-h" different from a normal
"huh?" response, but we don't currently. I don't know that it's worth
worrying about too much either way.

Anyway, here's the usage[] patch. I based it on the jk/consistent-h
branch, since I added a few new calls there. I won't be surprised if I
missed a NULL terminator somewhere, but it at least passes the big "-h"
loop in t0012. :)

---
diff --git a/builtin.h b/builtin.h
index 9e4a89816..c31fac64f 100644
--- a/builtin.h
+++ b/builtin.h
@@ -8,7 +8,7 @@
 
 #define DEFAULT_MERGE_LOG_LEN 20
 
-extern const char git_usage_string[];
+extern const char *git_usage_string[];
 extern const char git_more_info_string[];
 
 #define PRUNE_PACKED_DRY_RUN 01
diff --git a/builtin/blame.c b/builtin/blame.c
index 1043e5376..58cc1e8e8 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -30,10 +30,8 @@
 #include "dir.h"
 #include "progress.h"
 
-static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
-
 static const char *blame_opt_usage[] = {
-	blame_usage,
+	N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>"),
 	"",
 	N_("<rev-opts> are documented in git-rev-list(1)"),
 	NULL
@@ -2865,7 +2863,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		if (parse_range_arg(range_list.items[range_i].string,
 				    nth_line_cb, &sb, lno, anchor,
 				    &bottom, &top, sb.path))
-			usage(blame_usage);
+			usage(blame_opt_usage);
 		if (lno < top || ((lno || bottom) && lno < bottom))
 			die(Q_("file %s has only %lu line",
 			       "file %s has only %lu lines",
diff --git a/builtin/bundle.c b/builtin/bundle.c
index d0de59b94..2284a2413 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -9,11 +9,13 @@
  * bundle supporting "fetch", "pull", and "ls-remote".
  */
 
-static const char builtin_bundle_usage[] =
-  "git bundle create <file> <git-rev-list args>\n"
-  "   or: git bundle verify <file>\n"
-  "   or: git bundle list-heads <file> [<refname>...]\n"
-  "   or: git bundle unbundle <file> [<refname>...]";
+static const char *builtin_bundle_usage[] = {
+  "git bundle create <file> <git-rev-list args>",
+  "git bundle verify <file>",
+  "git bundle list-heads <file> [<refname>...]",
+  "git bundle unbundle <file> [<refname>...]",
+  NULL
+};
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
 {
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index eac499450..2d32db294 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -7,9 +7,11 @@
 #include "builtin.h"
 #include "strbuf.h"
 
-static const char builtin_check_ref_format_usage[] =
-"git check-ref-format [--normalize] [<options>] <refname>\n"
-"   or: git check-ref-format --branch <branchname-shorthand>";
+static const char *builtin_check_ref_format_usage[] = {
+	"git check-ref-format [--normalize] [<options>] <refname>",
+	"git check-ref-format --branch <branchname-shorthand>",
+	NULL
+};
 
 /*
  * Return a copy of refname but with leading slashes removed and runs
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index f39c2b273..1116a8590 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -10,7 +10,10 @@
 #include "utf8.h"
 #include "gpg-interface.h"
 
-static const char commit_tree_usage[] = "git commit-tree [(-p <sha1>)...] [-S[<keyid>]] [-m <message>] [-F <file>] <sha1>";
+static const char *commit_tree_usage[] = {
+	"git commit-tree [(-p <sha1>)...] [-S[<keyid>]] [-m <message>] [-F <file>] <sha1>",
+	NULL
+};
 
 static const char *sign_commit;
 
diff --git a/builtin/credential.c b/builtin/credential.c
index 879acfbcd..02e2d546f 100644
--- a/builtin/credential.c
+++ b/builtin/credential.c
@@ -2,8 +2,10 @@
 #include "credential.h"
 #include "builtin.h"
 
-static const char usage_msg[] =
-	"git credential [fill|approve|reject]";
+static const char *usage_msg[] = {
+	"git credential [fill|approve|reject]",
+	NULL
+};
 
 int cmd_credential(int argc, const char **argv, const char *prefix)
 {
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 15c61fd8d..a501196c2 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -10,9 +10,11 @@
 #include "builtin.h"
 #include "submodule.h"
 
-static const char diff_files_usage[] =
+static const char *diff_files_usage[] = {
 "git diff-files [-q] [-0 | -1 | -2 | -3 | -c | --cc] [<common-diff-options>] [<path>...]"
-COMMON_DIFF_OPTIONS_HELP;
+COMMON_DIFF_OPTIONS_HELP,
+NULL
+};
 
 int cmd_diff_files(int argc, const char **argv, const char *prefix)
 {
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 1af373d00..c633b0154 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -5,10 +5,12 @@
 #include "builtin.h"
 #include "submodule.h"
 
-static const char diff_cache_usage[] =
+static const char *diff_cache_usage[] = {
 "git diff-index [-m] [--cached] "
 "[<common-diff-options>] <tree-ish> [<path>...]"
-COMMON_DIFF_OPTIONS_HELP;
+COMMON_DIFF_OPTIONS_HELP,
+NULL
+};
 
 int cmd_diff_index(int argc, const char **argv, const char *prefix)
 {
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 5ea1c1431..925064e8c 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -79,12 +79,14 @@ static int diff_tree_stdin(char *line)
 	return -1;
 }
 
-static const char diff_tree_usage[] =
+static const char *diff_tree_usage[] = {
 "git diff-tree [--stdin] [-m] [-c] [--cc] [-s] [-v] [--pretty] [-t] [-r] [--root] "
 "[<common-diff-options>] <tree-ish> [<tree-ish>] [<path>...]\n"
 "  -r            diff recursively\n"
 "  --root        include the initial commit as diff against /dev/null\n"
-COMMON_DIFF_OPTIONS_HELP;
+COMMON_DIFF_OPTIONS_HELP,
+NULL
+};
 
 static void diff_tree_tweak_rev(struct rev_info *rev, struct setup_revision_opt *opt)
 {
diff --git a/builtin/diff.c b/builtin/diff.c
index 8c03ddaf5..2ab7a2b44 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -26,8 +26,10 @@ struct blobinfo {
 	unsigned mode;
 };
 
-static const char builtin_diff_usage[] =
-"git diff [<options>] [<commit> [<commit>]] [--] [<path>...]";
+static const char *builtin_diff_usage[] = {
+"git diff [<options>] [<commit> [<commit>]] [--] [<path>...]",
+NULL
+};
 
 static void stuff_change(struct diff_options *opt,
 			 unsigned old_mode, unsigned new_mode,
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 366b9d13f..aad1e470b 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -5,10 +5,12 @@
 #include "connect.h"
 #include "sha1-array.h"
 
-static const char fetch_pack_usage[] =
+static const char *fetch_pack_usage[] = {
 "git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
 "[--include-tag] [--upload-pack=<git-upload-pack>] [--depth=<n>] "
-"[--no-progress] [--diag-url] [-v] [<host>:]<directory> [<refs>...]";
+"[--no-progress] [--diag-url] [-v] [<host>:]<directory> [<refs>...]",
+NULL
+};
 
 static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
 			     const char *name)
diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
index e21c5416c..33ad8ff1f 100644
--- a/builtin/get-tar-commit-id.c
+++ b/builtin/get-tar-commit-id.c
@@ -7,8 +7,10 @@
 #include "builtin.h"
 #include "quote.h"
 
-static const char builtin_get_tar_commit_id_usage[] =
-"git get-tar-commit-id";
+static const char *builtin_get_tar_commit_id_usage[] = {
+"git get-tar-commit-id",
+NULL
+};
 
 /* ustar header + extended global header content */
 #define RECORDSIZE	(512)
diff --git a/builtin/help.c b/builtin/help.c
index 49f7a07f8..b8c65bcfd 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -459,7 +459,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 
 	if (show_all) {
 		git_config(git_help_config, NULL);
-		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
+		printf(_("usage: %s%s"), _(git_usage_string[0]), "\n\n");
 		load_command_list("git-", &main_cmds, &other_cmds);
 		list_commands(colopts, &main_cmds, &other_cmds);
 	}
@@ -476,7 +476,7 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!argv[0]) {
-		printf(_("usage: %s%s"), _(git_usage_string), "\n\n");
+		printf(_("usage: %s%s"), _(git_usage_string[0]), "\n\n");
 		list_common_cmds_help();
 		printf("\n%s\n", _(git_more_info_string));
 		return 0;
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 04b9dcaf0..26477cd84 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -12,8 +12,10 @@
 #include "streaming.h"
 #include "thread-utils.h"
 
-static const char index_pack_usage[] =
-"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
+static const char *index_pack_usage[] = {
+	"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--verify] [--strict] (<pack-file> | --stdin [--fix-thin] [<pack-file>])",
+	NULL
+};
 
 struct object_entry {
 	struct pack_idx_entry idx;
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 8a6acb0ec..5fef656e9 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -524,7 +524,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			die_errno(_("cannot chdir to %s"), argv[0]);
 		}
 	} else if (0 < argc) {
-		usage(init_db_usage[0]);
+		usage(init_db_usage);
 	}
 	if (is_bare_repository_cfg == 1) {
 		char *cwd = xgetcwd();
diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index cfb667a59..21d3cc615 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -8,8 +8,10 @@
 #include "strbuf.h"
 #include "mailinfo.h"
 
-static const char mailinfo_usage[] =
-	"git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] <msg> <patch> < mail >info";
+static const char *mailinfo_usage[] = {
+	"git mailinfo [-k | -b] [-m | --message-id] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] <msg> <patch> < mail >info",
+	NULL
+};
 
 int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 {
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 664400b81..0e504c104 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -9,8 +9,10 @@
 #include "string-list.h"
 #include "strbuf.h"
 
-static const char git_mailsplit_usage[] =
-"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] -o<directory> [(<mbox>|<Maildir>)...]";
+static const char *git_mailsplit_usage[] = {
+"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] -o<directory> [(<mbox>|<Maildir>)...]",
+NULL
+};
 
 static int is_from_line(const char *line, int len)
 {
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index c99443b09..203b0e5ed 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -65,6 +65,11 @@ static void merge_all(void)
 	}
 }
 
+static const char *usage_msg[] = {
+	"git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])",
+	NULL
+};
+
 int cmd_merge_index(int argc, const char **argv, const char *prefix)
 {
 	int i, force_file = 0;
@@ -75,7 +80,7 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
 	signal(SIGCHLD, SIG_DFL);
 
 	if (argc < 3)
-		usage("git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])");
+		usage(usage_msg);
 
 	read_cache();
 
diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
index 684411694..40c520f46 100644
--- a/builtin/merge-ours.c
+++ b/builtin/merge-ours.c
@@ -10,8 +10,10 @@
 #include "git-compat-util.h"
 #include "builtin.h"
 
-static const char builtin_merge_ours_usage[] =
-	"git merge-ours <base>... -- HEAD <remote>...";
+static const char *builtin_merge_ours_usage[] = {
+	"git merge-ours <base>... -- HEAD <remote>...",
+	NULL
+};
 
 static const char *diff_index_args[] = {
 	"diff-index", "--quiet", "--cached", "HEAD", "--", NULL
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index bad6735c7..3ef220ef9 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -5,7 +5,10 @@
 #include "exec_cmd.h"
 #include "merge-blobs.h"
 
-static const char merge_tree_usage[] = "git merge-tree <base-tree> <branch1> <branch2>";
+static const char *merge_tree_usage[] = {
+	"git merge-tree <base-tree> <branch1> <branch2>",
+	NULL
+};
 
 struct merge_list {
 	struct merge_list *next;
diff --git a/builtin/mktag.c b/builtin/mktag.c
index 031b750f0..a57b1d8f3 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -148,13 +148,18 @@ static int verify_tag(char *buffer, unsigned long size)
 	return 0;
 }
 
+static const char *usage_msg[] = {
+	"git mktag",
+	NULL
+};
+
 int cmd_mktag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	unsigned char result_sha1[20];
 
 	if (argc != 1)
-		usage("git mktag");
+		usage(usage_msg);
 
 	if (strbuf_read(&buf, 0, 4096) < 0) {
 		die_errno("could not read from stdin");
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index cb1df1c76..d65fc9de8 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -10,8 +10,10 @@
 
 #define BLKSIZE 512
 
-static const char pack_redundant_usage[] =
-"git pack-redundant [--verbose] [--alt-odb] (--all | <filename.pack>...)";
+static const char *pack_redundant_usage[] = {
+"git pack-redundant [--verbose] [--alt-odb] (--all | <filename.pack>...)",
+NULL
+};
 
 static int load_all_packs, verbose, alt_odb;
 
diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 81552e02e..2627ca391 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -162,7 +162,10 @@ static void generate_id_list(int stable)
 	strbuf_release(&line_buf);
 }
 
-static const char patch_id_usage[] = "git patch-id [--stable | --unstable]";
+static const char *patch_id_usage[] = {
+	"git patch-id [--stable | --unstable]",
+	NULL
+};
 
 static int git_patch_id_config(const char *var, const char *value, void *cb)
 {
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 920c16dac..a79e4b7d4 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -9,12 +9,18 @@
 #include "reachable.h"
 
 /* NEEDSWORK: switch to using parse_options */
-static const char reflog_expire_usage[] =
-"git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>...";
-static const char reflog_delete_usage[] =
-"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...";
-static const char reflog_exists_usage[] =
-"git reflog exists <ref>";
+static const char *reflog_expire_usage[] = {
+	"git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>...",
+	NULL
+};
+static const char *reflog_delete_usage[] = {
+	"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...",
+	NULL
+};
+static const char *reflog_exists_usage[] = {
+	"git reflog exists <ref>",
+	NULL
+};
 
 static timestamp_t default_reflog_expire;
 static timestamp_t default_reflog_expire_unreachable;
@@ -722,8 +728,10 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)
  * main "reflog"
  */
 
-static const char reflog_usage[] =
-"git reflog [ show | expire | delete | exists ]";
+static const char *reflog_usage[] = {
+"git reflog [ show | expire | delete | exists ]",
+NULL
+};
 
 int cmd_reflog(int argc, const char **argv, const char *prefix)
 {
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index bfb21ba7d..5f48a7393 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -3,8 +3,10 @@
 #include "run-command.h"
 #include "pkt-line.h"
 
-static const char usage_msg[] =
-	"git remote-ext <remote> <url>";
+static const char *usage_msg[] = {
+	"git remote-ext <remote> <url>",
+	NULL
+};
 
 /*
  * URL syntax:
diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c
index 91dfe07e0..28a7a99f8 100644
--- a/builtin/remote-fd.c
+++ b/builtin/remote-fd.c
@@ -1,8 +1,10 @@
 #include "builtin.h"
 #include "transport.h"
 
-static const char usage_msg[] =
-	"git remote-fd <remote> <url>";
+static const char *usage_msg[] = {
+	"git remote-fd <remote> <url>",
+	NULL
+};
 
 /*
  * URL syntax:
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 718c6059c..f06afd6a5 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -11,7 +11,7 @@
 #include "bisect.h"
 #include "progress.h"
 
-static const char rev_list_usage[] =
+static const char *rev_list_usage[] = {
 "git rev-list [OPTION] <commit-id>... [ -- paths... ]\n"
 "  limiting output:\n"
 "    --max-count=<n>\n"
@@ -47,8 +47,9 @@ static const char rev_list_usage[] =
 "  special purpose:\n"
 "    --bisect\n"
 "    --bisect-vars\n"
-"    --bisect-all"
-;
+"    --bisect-all",
+NULL
+};
 
 static struct progress *progress;
 static unsigned progress_counter;
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index efdc14473..7b377cfbf 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -530,12 +530,14 @@ static void die_no_single_rev(int quiet)
 		die("Needed a single revision");
 }
 
-static const char builtin_rev_parse_usage[] =
+static const char *builtin_rev_parse_usage[] = {
 N_("git rev-parse --parseopt [<options>] -- [<args>...]\n"
    "   or: git rev-parse --sq-quote [<arg>...]\n"
    "   or: git rev-parse [<options>] [<arg>...]\n"
    "\n"
-   "Run \"git rev-parse --parseopt -h\" for more information on the first usage.");
+   "Run \"git rev-parse --parseopt -h\" for more information on the first usage."),
+NULL
+};
 
 /*
  * Parse "opt" or "opt=<value>", setting value respectively to either
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1b4d2b346..945a1ca8d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -469,10 +469,14 @@ static int module_init(int argc, const char **argv, const char *prefix)
 
 static int module_name(int argc, const char **argv, const char *prefix)
 {
+	static const char *usage_msg[] = {
+		N_("git submodule--helper name <path>"),
+		NULL
+	};
 	const struct submodule *sub;
 
 	if (argc != 2)
-		usage(_("git submodule--helper name <path>"));
+		usage(usage_msg);
 
 	gitmodules_config();
 	sub = submodule_from_path(null_sha1, argv[1]);
@@ -1220,9 +1224,13 @@ static struct cmd_struct commands[] = {
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 {
+	static const char *usage_msg[] = {
+		"git submodule--helper <command>",
+		NULL
+	};
 	int i;
 	if (argc < 2 || !strcmp(argv[1], "-h"))
-		usage("git submodule--helper <command>");
+		usage(usage_msg);
 
 	for (i = 0; i < ARRAY_SIZE(commands); i++) {
 		if (!strcmp(argv[1], commands[i].cmd)) {
diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 6fc6bcdf7..d875802a3 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -20,12 +20,17 @@ static char *create_temp_file(unsigned char *sha1)
 	return path;
 }
 
+static const char *usage_msg[] = {
+	"git unpack-file <sha1>",
+	NULL
+};
+
 int cmd_unpack_file(int argc, const char **argv, const char *prefix)
 {
 	unsigned char sha1[20];
 
 	if (argc != 2 || !strcmp(argv[1], "-h"))
-		usage("git unpack-file <sha1>");
+		usage(usage_msg);
 	if (get_sha1(argv[1], sha1))
 		die("Not a valid object name %s", argv[1]);
 
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 8bc999776..7fcea5374 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -13,7 +13,10 @@
 #include "fsck.h"
 
 static int dry_run, quiet, recover, has_errors, strict;
-static const char unpack_usage[] = "git unpack-objects [-n] [-q] [-r] [--strict]";
+static const char *unpack_usage[] = {
+	"git unpack-objects [-n] [-q] [-r] [--strict]",
+	NULL
+};
 
 /* We always read in 4kB chunks. */
 static unsigned char buffer[4096];
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 84532ae9a..c73b116e8 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -9,8 +9,10 @@
 #include "run-command.h"
 #include "argv-array.h"
 
-static const char upload_archive_usage[] =
-	"git upload-archive <repo>";
+static const char *upload_archive_usage[] = {
+	"git upload-archive <repo>",
+	NULL
+};
 
 static const char deadchild[] =
 "git upload-archive: archiver died with error";
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53a2..7dc0d202c 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -5,7 +5,10 @@
  */
 #include "builtin.h"
 
-static const char var_usage[] = "git var (-l | <variable>)";
+static const char *var_usage[] = {
+	"git var (-l | <variable>)",
+	NULL
+};
 
 static const char *editor(int flag)
 {
diff --git a/daemon.c b/daemon.c
index ac7181a48..6f9030706 100644
--- a/daemon.c
+++ b/daemon.c
@@ -13,7 +13,7 @@ static int verbose;
 static int reuseaddr;
 static int informative_errors;
 
-static const char daemon_usage[] =
+static const char *daemon_usage[] = {
 "git daemon [--verbose] [--syslog] [--export-all]\n"
 "           [--timeout=<n>] [--init-timeout=<n>] [--max-connections=<n>]\n"
 "           [--strict-paths] [--base-path=<path>] [--base-path-relaxed]\n"
@@ -24,7 +24,9 @@ static const char daemon_usage[] =
 "           [--access-hook=<path>]\n"
 "           [--inetd | [--listen=<host_or_ipaddr>] [--port=<n>]\n"
 "                      [--detach] [--user=<user> [--group=<group>]]\n"
-"           [<directory>...]";
+"           [<directory>...]",
+NULL
+};
 
 /* List of acceptable pathname prefixes */
 static const char **ok_paths;
diff --git a/fast-import.c b/fast-import.c
index e69d21968..1abbae35f 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3408,8 +3408,10 @@ static void git_pack_config(void)
 	git_config(git_default_config, NULL);
 }
 
-static const char fast_import_usage[] =
-"git fast-import [--date-format=<f>] [--max-pack-size=<n>] [--big-file-threshold=<n>] [--depth=<n>] [--active-branches=<n>] [--export-marks=<marks.file>]";
+static const char *fast_import_usage[] = {
+"git fast-import [--date-format=<f>] [--max-pack-size=<n>] [--big-file-threshold=<n>] [--depth=<n>] [--active-branches=<n>] [--export-marks=<marks.file>]",
+NULL
+};
 
 static void parse_argv(void)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
index 4b7dcf21a..b109e4686 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -409,7 +409,7 @@ struct strbuf;
 
 /* General helper functions */
 extern void vreportf(const char *prefix, const char *err, va_list params);
-extern NORETURN void usage(const char *err);
+extern NORETURN void usage(const char * const *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
diff --git a/git.c b/git.c
index 1b8b7f51a..3496f8a23 100644
--- a/git.c
+++ b/git.c
@@ -3,12 +3,14 @@
 #include "help.h"
 #include "run-command.h"
 
-const char git_usage_string[] =
+const char *git_usage_string[] = {
 	"git [--version] [--help] [-C <path>] [-c name=value]\n"
 	"           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
 	"           [-p | --paginate | --no-pager] [--no-replace-objects] [--bare]\n"
 	"           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
-	"           <command> [<args>]";
+	"           <command> [<args>]",
+	NULL
+};
 
 const char git_more_info_string[] =
 	N_("'git help -a' and 'git help -g' list available subcommands and some\n"
@@ -694,7 +696,7 @@ int cmd_main(int argc, const char **argv)
 	} else {
 		/* The user didn't specify a command; give them help */
 		commit_pager_choice();
-		printf("usage: %s\n\n", git_usage_string);
+		printf("usage: %s\n\n", git_usage_string[0]);
 		list_common_cmds_help();
 		printf("\n%s\n", _(git_more_info_string));
 		exit(1);
diff --git a/http-fetch.c b/http-fetch.c
index 3b556d661..fb9da1b98 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -3,8 +3,11 @@
 #include "http.h"
 #include "walker.h"
 
-static const char http_fetch_usage[] = "git http-fetch "
-"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin] commit-id url";
+static const char *http_fetch_usage[] = {
+"git http-fetch "
+"[-c] [-t] [-a] [-v] [--recover] [-w ref] [--stdin] commit-id url",
+NULL
+};
 
 int cmd_main(int argc, const char **argv)
 {
diff --git a/http-push.c b/http-push.c
index 67c4d4b47..bd312358f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -18,8 +18,10 @@
 #include <expat.h>
 #endif
 
-static const char http_push_usage[] =
-"git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]\n";
+static const char *http_push_usage[] = {
+"git http-push [--all] [--dry-run] [--force] [--verbose] <remote> [<head>...]",
+NULL
+};
 
 #ifndef XML_STATUS_OK
 enum XML_Status {
diff --git a/parse-options.c b/parse-options.c
index a23a1e67f..b0cc2a410 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -600,6 +600,9 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		usagestr++;
 	}
 
+	if (!opts)
+		return PARSE_OPT_HELP;
+
 	if (opts->type != OPTION_GROUP)
 		fputc('\n', outfile);
 
diff --git a/remote-testsvn.c b/remote-testsvn.c
index f87bf851b..f5bb153dc 100644
--- a/remote-testsvn.c
+++ b/remote-testsvn.c
@@ -284,6 +284,11 @@ static int do_command(struct strbuf *line)
 	return 0;
 }
 
+static const char *testsvn_usage[] = {
+	"git-remote-svn <remote-name> [<url>]",
+	NULL
+};
+
 int cmd_main(int argc, const char **argv)
 {
 	struct strbuf buf = STRBUF_INIT, url_sb = STRBUF_INIT,
@@ -294,7 +299,7 @@ int cmd_main(int argc, const char **argv)
 
 	setup_git_directory();
 	if (argc < 2 || argc > 3) {
-		usage("git-remote-svn <remote-name> [<url>]");
+		usage(testsvn_usage);
 		return 1;
 	}
 
diff --git a/show-index.c b/show-index.c
index 1ead41e21..e0356d7fa 100644
--- a/show-index.c
+++ b/show-index.c
@@ -1,8 +1,10 @@
 #include "cache.h"
 #include "pack.h"
 
-static const char show_index_usage[] =
-"git show-index";
+static const char *show_index_usage[] = {
+"git show-index",
+NULL
+};
 
 int cmd_main(int argc, const char **argv)
 {
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index f414a3ac6..fadc50f00 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -1,12 +1,14 @@
 #include "cache.h"
 
-static const char *usage_msg = "\n"
-"  test-date relative [time_t]...\n"
-"  test-date show:<format> [time_t]...\n"
-"  test-date parse [date]...\n"
-"  test-date approxidate [date]...\n"
-"  test-date is64bit\n"
-"  test-date time_t-is64bit\n";
+static const char *usage_msg[] = {
+"test-date relative [time_t]...",
+"test-date show:<format> [time_t]...",
+"test-date parse [date]...",
+"test-date approxidate [date]...",
+"test-date is64bit",
+"test-date time_t-is64bit",
+NULL
+};
 
 static void show_relative_dates(const char **argv, struct timeval *now)
 {
diff --git a/t/helper/test-line-buffer.c b/t/helper/test-line-buffer.c
index 81575fe2a..288c0ab21 100644
--- a/t/helper/test-line-buffer.c
+++ b/t/helper/test-line-buffer.c
@@ -63,7 +63,7 @@ int cmd_main(int argc, const char **argv)
 	else if (argc == 2)
 		filename = argv[1];
 	else
-		usage("test-line-buffer [file | &fd] < script");
+		usagef("test-line-buffer [file | &fd] < script");
 
 	if (buffer_init(&stdin_buf, NULL))
 		die_errno("open error");
diff --git a/t/helper/test-mktemp.c b/t/helper/test-mktemp.c
index 89d9b2f7b..22f56e7df 100644
--- a/t/helper/test-mktemp.c
+++ b/t/helper/test-mktemp.c
@@ -3,10 +3,15 @@
  */
 #include "git-compat-util.h"
 
+static const char *usage_msg[] = {
+	"Expected 1 parameter defining the temporary file template",
+	NULL
+};
+
 int cmd_main(int argc, const char **argv)
 {
 	if (argc != 2)
-		usage("Expected 1 parameter defining the temporary file template");
+		usage(usage_msg);
 
 	xmkstemp(xstrdup(argv[1]));
 
diff --git a/t/helper/test-regex.c b/t/helper/test-regex.c
index b5ea8a97c..a36917047 100644
--- a/t/helper/test-regex.c
+++ b/t/helper/test-regex.c
@@ -47,7 +47,7 @@ int cmd_main(int argc, const char **argv)
 	if (argc == 2 && !strcmp(argv[1], "--bug"))
 		return test_regex_bug();
 	else if (argc < 3)
-		usage("test-regex --bug\n"
+		usagef("test-regex --bug\n"
 		      "test-regex <pattern> <string> [<options>]");
 
 	argv++;
diff --git a/t/helper/test-svn-fe.c b/t/helper/test-svn-fe.c
index 7667c0803..a1eb4e378 100644
--- a/t/helper/test-svn-fe.c
+++ b/t/helper/test-svn-fe.c
@@ -8,8 +8,10 @@
 #include "vcs-svn/sliding_window.h"
 #include "vcs-svn/line_buffer.h"
 
-static const char test_svnfe_usage[] =
-	"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)";
+static const char *test_svnfe_usage[] = {
+	"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)",
+	NULL
+};
 
 static int apply_delta(int argc, const char **argv)
 {
diff --git a/usage.c b/usage.c
index 2f87ca69a..ae81a6cc8 100644
--- a/usage.c
+++ b/usage.c
@@ -5,6 +5,7 @@
  */
 #include "git-compat-util.h"
 #include "cache.h"
+#include "parse-options.h"
 
 void vreportf(const char *prefix, const char *err, va_list params)
 {
@@ -94,9 +95,9 @@ void NORETURN usagef(const char *err, ...)
 	va_end(params);
 }
 
-void NORETURN usage(const char *err)
+void NORETURN usage(const char * const *err)
 {
-	usagef("%s", err);
+	usage_with_options(err, NULL);
 }
 
 void NORETURN die(const char *err, ...)

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If we don't mind a one-time pain, I think we can just convert the
> existing usage() to something more like usage_with_options(). The patch
> below does that (and teaches the latter to handle a NULL options field).
>
> The diff is ugly, and the conversion is mostly mechanical. But I think
> some sites can be improved. For example, look at the change in bundle.c,
> which now hands a set of strings rather than formatting the whole "or:"
> chain manually.
> ...
> With bonus points for actually describing each option (though at that
> point it may be as much work to just convert the thing to parse-options,
> which would also be fine with me).
>
> That's tedious work which would need attention paid to each individual
> command. So I'd probably do a single mechanical patch like this one
> (that keeps the output identical), and then let people fix each one up
> on top.

Yup.  Unlike my other hack, this does look more useful to me.  It
was kind of surprising that the change to parse-options is just a
single liner, but it's just "no input resulting in no output" ;-)

> diff --git a/git.c b/git.c
> index 1b8b7f51a..3496f8a23 100644
> --- a/git.c
> +++ b/git.c
> @@ -3,12 +3,14 @@
>  #include "help.h"
>  #include "run-command.h"
>  
> -const char git_usage_string[] =
> +const char *git_usage_string[] = {
>  	"git [--version] [--help] [-C <path>] [-c name=value]\n"
>  	"           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
>  	"           [-p | --paginate | --no-pager] [--no-replace-objects] [--bare]\n"
>  	"           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
> -	"           <command> [<args>]";
> +	"           <command> [<args>]",
> +	NULL
> +};
>  
>  const char git_more_info_string[] =
>  	N_("'git help -a' and 'git help -g' list available subcommands and some\n"
> @@ -694,7 +696,7 @@ int cmd_main(int argc, const char **argv)
>  	} else {
>  		/* The user didn't specify a command; give them help */
>  		commit_pager_choice();
> -		printf("usage: %s\n\n", git_usage_string);
> +		printf("usage: %s\n\n", git_usage_string[0]);
>  		list_common_cmds_help();
>  		printf("\n%s\n", _(git_more_info_string));
>  		exit(1);


> diff --git a/parse-options.c b/parse-options.c
> index a23a1e67f..b0cc2a410 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -600,6 +600,9 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
>  		usagestr++;
>  	}
>  
> +	if (!opts)
> +		return PARSE_OPT_HELP;
> +
>  	if (opts->type != OPTION_GROUP)
>  		fputc('\n', outfile);

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jonathan Nieder]

Hi,

Jeff King wrote:

> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -49,4 +49,16 @@ test_expect_success "--help does not work for guides" "
>  	test_i18ncmp expect actual
>  "
> 
> +test_expect_success 'generate builtin list' '
> +	git --list-builtins >builtins
> +'
> +
> +while read builtin
> +do
> +	test_expect_success "$builtin can handle -h" '
> +		test_expect_code 129 git $builtin -h >output 2>&1 &&
> +		test_i18ngrep usage output
> +	'
> +done <builtins

I love this.  A few thoughts:

- I think the "generate builtin list" test should be marked as setup
  so people know it can't be skipped.  I'll send a followup to do that.

- By the same token, if "generate builtin list" fails then these
  commands afterward would fail in an unpleasant way.  Fortunately
  that's straightforward to fix, too.

- This checks both stdout and stderr to verify that the usage appears
  somewhere.  I would have expected commands to be consistent ---
  should they always send usage to stdout, since the user requested it,
  or to stderr, since that's what we have done historically?

  So I understand why this test is agnostic about that but I suspect
  it's pointing to some inconsistency that could be fixed independently.

- Do we plan to translate the "usage:" prefix some day?  I could go
  both ways --- on one hand, having a predictable error:/usage:/fatal:
  prefix may make life easier for e.g. GUI authors trying to show
  different kinds of error messages in different ways, but on the other
  hand, always using English for the prefix may be unfriendly to non
  English speakers.

  In the past I had assumed it would never be translated but now I'm
  not so sure.  What do you think?

- Should we have something like this for non-builtins, too?

In any case,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

On Tue, Jun 13, 2017 at 04:08:03PM -0700, Jonathan Nieder wrote:

> > +test_expect_success 'generate builtin list' '
> > +	git --list-builtins >builtins
> > +'
> > +
> > +while read builtin
> > +do
> > +	test_expect_success "$builtin can handle -h" '
> > +		test_expect_code 129 git $builtin -h >output 2>&1 &&
> > +		test_i18ngrep usage output
> > +	'
> > +done <builtins
> 
> I love this.  A few thoughts:
> 
> - I think the "generate builtin list" test should be marked as setup
>   so people know it can't be skipped.  I'll send a followup to do that.
> 
> - By the same token, if "generate builtin list" fails then these
>   commands afterward would fail in an unpleasant way.  Fortunately
>   that's straightforward to fix, too.

Be my guest if you want, but I don't think either of those is actually
worth spending any time on. It's a nice convenience when tests are
independent, but ultimately if any single test fails, the results of all
tests after it must be suspect. There are too many hidden dependencies
to treat it any other way.

So I have no problem with skipping tests while debugging for a quicker
turnaround, with the knowledge that what you're seeing _might_ be
slightly invalidated. But I don't think it's worth dumping developer
time into trying to make each block independent. That's what individual
scripts are for.

(I also think it's particularly worthless for this script; the whole
thing runs in ~500ms on my machine. It takes longer to type
GIT_TEST_SKIP).

> - This checks both stdout and stderr to verify that the usage appears
>   somewhere.  I would have expected commands to be consistent ---
>   should they always send usage to stdout, since the user requested it,
>   or to stderr, since that's what we have done historically?
> 
>   So I understand why this test is agnostic about that but I suspect
>   it's pointing to some inconsistency that could be fixed independently.

The difference is between parse-option's usage_with_options() and
usage(). The former sends "-h" to stdout and errors to stderr, which I
think is sensible. The latter only knows about stderr.

I think it would be reasonable to have follow the parse-options strategy
consistently. It will definitely take some tweaking, though. There are
lots of commands that respect "-h" only by hitting some default
unhandled case. So you'll not only have to have a stdout analog for
usage(), but you'll have to teach each command when to use each.

> - Do we plan to translate the "usage:" prefix some day?  I could go
>   both ways --- on one hand, having a predictable error:/usage:/fatal:
>   prefix may make life easier for e.g. GUI authors trying to show
>   different kinds of error messages in different ways, but on the other
>   hand, always using English for the prefix may be unfriendly to non
>   English speakers.
> 
>   In the past I had assumed it would never be translated but now I'm
>   not so sure.  What do you think?

I don't have an opinion either way. I know we don't translate it now,
but it has been discussed. I figured it couldn't hurt to use i18ngrep to
future-proof it.

> - Should we have something like this for non-builtins, too?

That would be nice, though you'll probably need some cooperation from
the Makefile to get the list. I was specifically worried about catching
the particular setup_git_directory() interaction with builtins here, so
it seemed like a good stopping point.

But as a general rule, making sure that everything responds sensibly to
"-h" seems like a good thing.

-Peff