threads / patch / 25479

patch, 3 partsrun_builtin(): save "-h" detection result for later use

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

## tl;dr

12 messages between Oct 19, 2010 and Oct 20, 2010. Diffs are folded; open one to read it.

replies: 11people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Oct 19, 2010, 13:35 UTC · lore
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(-)
Show changes to 2 files +5 −4

cache.h, git.c

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
Nguyễn Thái Ngọc Duy· Oct 19, 2010, 13:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 7 files +7 −11

builtin/check-ref-format.c, builtin/grep.c, builtin/index-pack.c, builtin/log.c, builtin/merge-ours.c, builtin/pack-redundant.c, builtin/show-ref.c

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
Jonathan Nieder· Oct 19, 2010, 17:29 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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?
Jonathan Nieder· Oct 19, 2010, 18:50 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
> Nguyễn Thái Ngọc Duy wrote:
Show 11 quoted lines
>> 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.
Junio C Hamano· Oct 19, 2010, 19:10 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 7 quoted lines
> 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?

Nguyen Thai Ngoc Duy· Oct 20, 2010, 01:13 UTC · re: Jonathan Nieder · lore

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

2010/10/20 Jonathan Nieder <jrnieder@gmail.com>:
Show 9 quoted lines
> 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.

Show 13 quoted lines
> 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
Jonathan Nieder· Oct 20, 2010, 01:18 UTC · re: Nguyen Thai Ngoc Duy · lore

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

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

Nguyễn Thái Ngọc Duy· Oct 19, 2010, 13:35 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Show changes to 9 files +48 −3

builtin/branch.c, builtin/checkout-index.c, builtin/commit.c, builtin/gc.c, builtin/ls-files.c, builtin/merge.c, builtin/update-index.c, builtin/upload-archive.c, 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
Junio C Hamano· Oct 19, 2010, 16:33 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 8 quoted lines
> 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...
Show 12 quoted lines
> 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.
Show 13 quoted lines
> 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.

Nguyen Thai Ngoc Duy· Oct 20, 2010, 01:18 UTC · re: Junio C Hamano · lore

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

2010/10/19 Junio C Hamano <gitster@pobox.com>:
Show 12 quoted lines
> 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.
Show 39 quoted lines
>> 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
Jonathan Nieder· Oct 19, 2010, 17:55 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy wrote:
Show 12 quoted lines
> --- 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.
Show 12 quoted lines
> --- 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

Jonathan Nieder· Oct 19, 2010, 16:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Hi,
Nguyễn Thái Ngọc Duy wrote:
Show 7 quoted lines
> 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. [*]

Show 8 quoted lines
> --- 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.
Show 16 quoted lines
> --- 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.

← back to recent threads