[PATCH 1/3] run_builtin(): save "-h" detection result for later use

[PATCH 1/3] run_builtin(): save "-h" detection result for later use

[Nguyễn Thái Ngọc Duy]

When run_builtin() sees "-h" as the first argument, it assumes:

 - this is the call for help usage
 - the real git command will only print help usage then exit

So it skips all setup in this case.  Unfortunately, some commands do
other things before calling parse_options(), which is often where the
help usage is printed.  Some of those things may try to access the
repository, which has yet to be set up.

Make real commands aware of this fast path so that they can handle it
properly (i.e., print help usage then exit immediately) if they need
to do more initialization than git_config().

git_config() is a bit of a special case: it is perfectly legitimate to
use it without access to a valid repository. In the future, we should
make git_config() aware of the repository search status so that it can
skip reading $GIT_DIR/config if no repository is found.
---
 cache.h |    1 +
 git.c   |    8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 33decd9..43763c1 100644
--- a/cache.h
+++ b/cache.h
@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
 /* git.c */
 struct startup_info {
 	int have_repository;
+	int help; /* print help and exit, except git_config(), repo must not be touched */
 };
 extern struct startup_info *startup_info;
 
diff --git a/git.c b/git.c
index 50a1401..5962cdd 100644
--- a/git.c
+++ b/git.c
@@ -246,13 +246,13 @@ struct cmd_struct {
 
 static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 {
-	int status, help;
+	int status;
 	struct stat st;
 	const char *prefix;
 
 	prefix = NULL;
-	help = argc == 2 && !strcmp(argv[1], "-h");
-	if (!help) {
+	startup_info->help = argc == 2 && !strcmp(argv[1], "-h");
+	if (!startup_info->help) {
 		if (p->option & RUN_SETUP)
 			prefix = setup_git_directory();
 		if (p->option & RUN_SETUP_GENTLY) {
@@ -267,7 +267,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	}
 	commit_pager_choice();
 
-	if (!help && p->option & NEED_WORK_TREE)
+	if (!startup_info->help && p->option & NEED_WORK_TREE)
 		setup_work_tree();
 
 	trace_argv_printf(argv, "trace: built-in: git");
-- 
1.7.0.2.445.gcbdb3

[PATCH 2/3] builtins: utilize startup_info->help where possible

[Nguyễn Thái Ngọc Duy]

It helps reduce false alarms while I'm looking for "git foo -h" code
path that accesses repository. Anyway it looks like a good thing to
do.
---
 builtin/check-ref-format.c |    2 +-
 builtin/grep.c             |    2 +-
 builtin/index-pack.c       |    2 +-
 builtin/log.c              |    6 +-----
 builtin/merge-ours.c       |    2 +-
 builtin/pack-redundant.c   |    2 +-
 builtin/show-ref.c         |    2 +-
 7 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index ae3f281..c6511e3 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -58,7 +58,7 @@ static int check_ref_format_print(const char *arg)
 
 int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 {
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage(builtin_check_ref_format_usage);
 
 	if (argc == 3 && !strcmp(argv[1], "--branch"))
diff --git a/builtin/grep.c b/builtin/grep.c
index da32f3d..ec39909 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -934,7 +934,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	 * 'git grep -h', unlike 'git grep -h <pattern>', is a request
 	 * to show usage information and exit.
 	 */
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage_with_options(grep_usage, options);
 
 	memset(&opt, 0, sizeof(opt));
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index e243d9d..649ad18 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -881,7 +881,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	struct pack_idx_entry **idx_objects;
 	unsigned char pack_sha1[20];
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage(index_pack_usage);
 
 	read_replace_refs = 0;
diff --git a/builtin/log.c b/builtin/log.c
index 22d1290..a7ba9ed 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -69,11 +69,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	if (default_date_mode)
 		rev->date_mode = parse_date_format(default_date_mode);
 
-	/*
-	 * Check for -h before setup_revisions(), or "git log -h" will
-	 * fail when run without a git directory.
-	 */
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
index 6844116..8e0777b 100644
--- a/builtin/merge-ours.c
+++ b/builtin/merge-ours.c
@@ -20,7 +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"))
+	if (startup_info->help)
 		usage(builtin_merge_ours_usage);
 
 	/*
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 41e1615..3f090b2 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -601,7 +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"))
+	if (startup_info->help)
 		usage(pack_redundant_usage);
 
 	for (i = 1; i < argc; i++) {
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index be9b512..7083fa9 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -204,7 +204,7 @@ static const struct option show_ref_options[] = {
 
 int cmd_show_ref(int argc, const char **argv, const char *prefix)
 {
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage_with_options(show_ref_usage, show_ref_options);
 
 	argc = parse_options(argc, argv, prefix, show_ref_options,
-- 
1.7.0.2.445.gcbdb3

Re: [PATCH 2/3] builtins: utilize startup_info->help where possible

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> It helps reduce false alarms while I'm looking for "git foo -h" code
> path that accesses repository.

Not sure I understand.  Is the idea that use of startup_info->help
is a marker for "I've checked this code path"?

If that were the only reason, I don't think I'd like the idea.

As it is, I'm a bit conflicted: what if we decide to short-circuit
"git foo --help-all" in the future just like we short-circuit
"git foo -h" now?  Would that require a separate flag?

In other words, I'm not sure startup_info->help is a good abstraction.
Maybe (modulo names) it would be better to do

struct startup_info {
	...
	const char *short_circuit;	/* "-h", "--help-all", "--no-index", or NULL */
};

and use

	if (!strcmp(startup_info->short_circuit, "-h"))

to allow relaxing the argc == 2 check later?

Re: [PATCH 2/3] builtins: utilize startup_info->help where possible

[Jonathan Nieder]

Jonathan Nieder wrote:
> Nguyễn Thái Ngọc Duy wrote:

>> It helps reduce false alarms while I'm looking for "git foo -h" code
>> path that accesses repository.
>
> Not sure I understand.  Is the idea that use of startup_info->help
> is a marker for "I've checked this code path"?
> 
> If that were the only reason, I don't think I'd like the idea.
> 
> As it is, I'm a bit conflicted: what if we decide to short-circuit
> "git foo --help-all" in the future just like we short-circuit
> "git foo -h" now?  Would that require a separate flag?

To be clear: I'm a bit conflicted but not extremely so.  The help
flag has the benefit of simplicity and of allowing expansion to

	git foo -h --gobbledegook

if later someone cares and a code audit shows it to be safe.

Re: [PATCH 2/3] builtins: utilize startup_info->help where possible

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> In other words, I'm not sure startup_info->help is a good abstraction.
> Maybe (modulo names) it would be better to do
>
> struct startup_info {
> 	...
> 	const char *short_circuit;	/* "-h", "--help-all", "--no-index", or NULL */
> };

I am not sure short-circuit is a good abstraction either; will we have
only one aspect of whatever we can short-circuit forever?

Re: [PATCH 2/3] builtins: utilize startup_info->help where possible

[Nguyen Thai Ngoc Duy]

2010/10/20 Jonathan Nieder <jrnieder@gmail.com>:
> Nguyễn Thái Ngọc Duy wrote:
>
>> It helps reduce false alarms while I'm looking for "git foo -h" code
>> path that accesses repository.
>
> Not sure I understand.  Is the idea that use of startup_info->help
> is a marker for "I've checked this code path"?
>
> If that were the only reason, I don't think I'd like the idea.

That. And simpler check. I mean "if (startup_info->help)" takes a tiny
bit less energy for me to understand than "if (argc == 2 && argv[1] ==
"-h")". It's also good for grepping.

> As it is, I'm a bit conflicted: what if we decide to short-circuit
> "git foo --help-all" in the future just like we short-circuit
> "git foo -h" now?  Would that require a separate flag?

As long as it's help related, startup_info->help can be turned to a
bit set. Although I'm not sure if --help-all or any other option will
be implemented anytime soon. Remember it has to be implemented for
_all_ builtin commands, or we need to introduce NO_DASH_H in
run_builtin() to skip the shortcut for commands that do not support
it.

> In other words, I'm not sure startup_info->help is a good abstraction.
> Maybe (modulo names) it would be better to do
>
> struct startup_info {
>        ...
>        const char *short_circuit;      /* "-h", "--help-all", "--no-index", or NULL */
> };
>
> and use
>
>        if (!strcmp(startup_info->short_circuit, "-h"))
>
> to allow relaxing the argc == 2 check later?

Can it be relaxed later when someone comes up with "--help-all" or something?
-- 
Duy

Re: [PATCH 2/3] builtins: utilize startup_info->help where possible

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> That. And simpler check. I mean "if (startup_info->help)" takes a tiny
> bit less energy for me to understand than "if (argc == 2 && argv[1] ==
> "-h")". It's also good for grepping.

Mm, that still does not motivate the churn to me.  But "we can change
the meaning to 'if (argc >= 2 && !strcmp(argv[1], "-h"))' some day"
motivates it okay, I think.

> 2010/10/20 Jonathan Nieder <jrnieder@gmail.com>:

>> Maybe (modulo names) it would be better to do
[...]
> Can it be relaxed later when someone comes up with "--help-all" or something?

Yes, I was just thinking out loud.  By "relaxed" I meant the above
(changing == to >= in one place).  Now that I've thought it over,
I don't mind startup_info->help so much.

Hope that helps, and sorry for the noise.
Jonathan

[PATCH 3/3] builtins: check for startup_info->help, print and exit early

[Nguyễn Thái Ngọc Duy]

These commands need more than just git_config() before parsing
commmand line arguments. Some of these activities will unconditionally
look into a repository. When startup_info->help is TRUE, no repository
is set up and the caller expects callees to print help usage and exit,
no more.

Do as expected.
---
 builtin/branch.c         |    3 +++
 builtin/checkout-index.c |    3 +++
 builtin/commit.c         |    6 ++++++
 builtin/gc.c             |    3 +++
 builtin/ls-files.c       |    3 +++
 builtin/merge.c          |    3 +++
 builtin/update-index.c   |    3 +++
 builtin/upload-archive.c |    7 ++++---
 t/t3905-help.sh          |   20 ++++++++++++++++++++
 9 files changed, 48 insertions(+), 3 deletions(-)
 create mode 100755 t/t3905-help.sh

diff --git a/builtin/branch.c b/builtin/branch.c
index 87976f0..9f152ed 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -667,6 +667,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	if (startup_info->help)
+		usage_with_options(builtin_branch_usage, options);
+
 	git_config(git_branch_config, NULL);
 
 	if (branch_use_color == -1)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index a7a5ee1..7f25cd7 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -241,6 +241,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	if (startup_info->help)
+		usage_with_options(builtin_checkout_index_usage, builtin_checkout_index_options);
+
 	git_config(git_default_config, NULL);
 	state.base_dir = "";
 	prefix_length = prefix ? strlen(prefix) : 0;
diff --git a/builtin/commit.c b/builtin/commit.c
index 66fdd22..8b086f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1070,6 +1070,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	if (startup_info->help)
+		usage_with_options(builtin_status_usage, builtin_status_options);
+
 	if (null_termination && status_format == STATUS_FORMAT_LONG)
 		status_format = STATUS_FORMAT_PORCELAIN;
 
@@ -1255,6 +1258,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	int allow_fast_forward = 1;
 	struct wt_status s;
 
+	if (startup_info->help)
+		usage_with_options(builtin_commit_usage, builtin_commit_options);
+
 	wt_status_prepare(&s);
 	git_config(git_commit_config, &s);
 	in_merge = file_exists(git_path("MERGE_HEAD"));
diff --git a/builtin/gc.c b/builtin/gc.c
index c304638..f040171 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -191,6 +191,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 
 	git_config(gc_config, NULL);
 
+	if (startup_info->help)
+		usage_with_options(builtin_gc_usage, builtin_gc_options);
+
 	if (pack_refs < 0)
 		pack_refs = !is_bare_repository();
 
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index bb4f612..814da51 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -530,6 +530,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		OPT_END()
 	};
 
+	if (startup_info->help)
+		usage_with_options(ls_files_usage, builtin_ls_files_options);
+
 	memset(&dir, 0, sizeof(dir));
 	prefix = cmd_prefix;
 	if (prefix)
diff --git a/builtin/merge.c b/builtin/merge.c
index 2dba3b9..0169694 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -917,6 +917,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
 
+	if (startup_info->help)
+		usage_with_options(builtin_merge_usage, builtin_merge_options);
+
 	if (read_cache_unmerged()) {
 		die_resolve_conflict("merge");
 	}
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 3ab214d..46a53f5 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -589,6 +589,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	int lock_error = 0;
 	struct lock_file *lock_file;
 
+	if (startup_info->help)
+		usage(update_index_usage);
+
 	git_config(git_default_config, NULL);
 
 	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 73f788e..d4f4741 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -26,9 +26,6 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)
 	int sent_argc;
 	int len;
 
-	if (argc != 2)
-		usage(upload_archive_usage);
-
 	if (strlen(argv[1]) + 1 > sizeof(buf))
 		die("insanely long repository name");
 
@@ -98,6 +95,10 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 {
 	pid_t writer;
 	int fd1[2], fd2[2];
+
+	if (startup_info->help || argc != 2)
+		usage(upload_archive_usage);
+
 	/*
 	 * Set up sideband subprocess.
 	 *
diff --git a/t/t3905-help.sh b/t/t3905-help.sh
new file mode 100755
index 0000000..c7e80c5
--- /dev/null
+++ b/t/t3905-help.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+test_description='tests that git foo -h should work even in broken repos'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	git config core.repositoryformatversion 99 &&
+	test_must_fail git rev-parse HEAD
+'
+
+CMDS="branch checkout-index commit gc ls-files merge update-index upload-archive"
+for cmd in $CMDS; do
+	test_expect_success "$cmd -h" "
+		test_must_fail git $cmd -h &&
+		test \$exit_code = 129
+	"
+done
+
+test_done
-- 
1.7.0.2.445.gcbdb3

Re: [PATCH 3/3] builtins: check for startup_info->help, print and exit early

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> These commands need more than just git_config() before parsing
> commmand line arguments. Some of these activities will unconditionally
> look into a repository. When startup_info->help is TRUE, no repository
> is set up and the caller expects callees to print help usage and exit,
> no more.
>
> Do as expected.
> ---

No sign-off given to any of the three patches...

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 87976f0..9f152ed 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -667,6 +667,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		OPT_END(),
>  	};
>  
> +	if (startup_info->help)
> +		usage_with_options(builtin_branch_usage, options);
> +
>  	git_config(git_branch_config, NULL);

I can just say:

    $ cd / && git branch -h
    usage: git branch [options] ...

without your patch just fine (and no I am not insane to make / controlled
by git).

The same for checkout-index, commit, ls-files, etc.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index c304638..f040171 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -191,6 +191,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>  
>  	git_config(gc_config, NULL);
>  
> +	if (startup_info->help)
> +		usage_with_options(builtin_gc_usage, builtin_gc_options);
> +
>  	if (pack_refs < 0)
>  		pack_refs = !is_bare_repository();

This one is curious.  Why do you need a call to git_config() in "gc" only?
You don't do that for e.g. "branch".

While I do not see anything glaringly wrong with this change, I am not
sure I am getting the point of these patches.

Re: [PATCH 3/3] builtins: check for startup_info->help, print and exit early

[Nguyen Thai Ngoc Duy]

2010/10/19 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> These commands need more than just git_config() before parsing
>> commmand line arguments. Some of these activities will unconditionally
>> look into a repository. When startup_info->help is TRUE, no repository
>> is set up and the caller expects callees to print help usage and exit,
>> no more.
>>
>> Do as expected.
>> ---
>
> No sign-off given to any of the three patches...

Oops, I removed old signoffs and accidentally mine too.

>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 87976f0..9f152ed 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -667,6 +667,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>               OPT_END(),
>>       };
>>
>> +     if (startup_info->help)
>> +             usage_with_options(builtin_branch_usage, options);
>> +
>>       git_config(git_branch_config, NULL);
>
> I can just say:
>
>    $ cd / && git branch -h
>    usage: git branch [options] ...
>
> without your patch just fine (and no I am not insane to make / controlled
> by git).
>
> The same for checkout-index, commit, ls-files, etc.
>
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> index c304638..f040171 100644
>> --- a/builtin/gc.c
>> +++ b/builtin/gc.c
>> @@ -191,6 +191,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
>>
>>       git_config(gc_config, NULL);
>>
>> +     if (startup_info->help)
>> +             usage_with_options(builtin_gc_usage, builtin_gc_options);
>> +
>>       if (pack_refs < 0)
>>               pack_refs = !is_bare_repository();
>
> This one is curious.  Why do you need a call to git_config() in "gc" only?
> You don't do that for e.g. "branch".

Hmm.. I don't think that git_config() is needed. It does no harm, but
no gain either.

> While I do not see anything glaringly wrong with this change, I am not
> sure I am getting the point of these patches.

As Jonathan pointed out, the series makes "git foo -h" works even in
special cases where setup code may terminate program early. Apparently
my test in hurry does not work great.
-- 
Duy

Re: [PATCH 3/3] builtins: check for startup_info->help, print and exit early

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -667,6 +667,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  		OPT_END(),
>  	};
>  
> +	if (startup_info->help)
> +		usage_with_options(builtin_branch_usage, options);
> +
>  	git_config(git_branch_config, NULL);
>  
>  	if (branch_use_color == -1)

One has to work hard to trigger this one.

	git init foo
	cd foo
	>.git/refs/heads/master
	git branch -h

Produces:

	fatal: Failed to resolve HEAD as a valid ref.

> --- a/builtin/checkout-index.c
> +++ b/builtin/checkout-index.c
> @@ -241,6 +241,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>  
> +	if (startup_info->help)
> +		usage_with_options(builtin_checkout_index_usage, builtin_checkout_index_options);
> +
>  	git_config(git_default_config, NULL);
>  	state.base_dir = "";
>  	prefix_length = prefix ? strlen(prefix) : 0;

	git init foo
	cd foo
	>.git/index
	git checkout-index -h

Produces:

	fatal: index file smaller than expected

> --- a/builtin/commit.c
> +++ b/builtin/commit.c

I'm stopping here.  But maybe some test cases like this would make the
series clearer.

Hope that helps,
Jonathan

Re: [PATCH 1/3] run_builtin(): save "-h" detection result for later use

[Jonathan Nieder]

Hi,

Nguyễn Thái Ngọc Duy wrote:

> When run_builtin() sees "-h" as the first argument, it assumes:
> 
>  - this is the call for help usage
>  - the real git command will only print help usage then exit
> 
> So it skips all setup in this case.  Unfortunately, some commands do
> other things before calling parse_options()

and even during parse_options() in some weird cases.

Taking this patch as a cover letter, I wonder about the impact
(because I forgot).  Is this to avoid, e.g., erroring out from a
repository with invalid HEAD when the user just wanted to get help?
Example commands with improved behavior?  It would be nice to be able
to add a test or two. [*]

> --- a/cache.h
> +++ b/cache.h
> @@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
>  /* git.c */
>  struct startup_info {
>  	int have_repository;
> +	int help; /* print help and exit, except git_config(), repo must not be touched */
>  };

Since this is data, not code, I think it is clearer to just explain
what the value represents.  The commit message and access sites can
explain why it matters.  Maybe something like this?

	unsigned have_repository:1;
	unsigned help:1;	/* git <command> -h? */

A better technical writer could probably find a better way to say it.

> --- a/git.c
> +++ b/git.c
> @@ -246,13 +246,13 @@ struct cmd_struct {
>  
>  static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>  {
> -	int status, help;
> +	int status;
>  	struct stat st;
>  	const char *prefix;
>  
>  	prefix = NULL;
> -	help = argc == 2 && !strcmp(argv[1], "-h");
> -	if (!help) {
> +	startup_info->help = argc == 2 && !strcmp(argv[1], "-h");
> +	if (!startup_info->help) {
[...]

For what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

[*] The reader might recall a long-term goal of clarifying .git dir
access semantics:
http://thread.gmane.org/gmane.comp.version-control.git/149771/focus=152745
Given that, what need is there to ask about the patch's impact?  One
answer: I am asking about side-effects rather than the goal of the
patch.  Positive side-effects would be indicators of a good design.