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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 19, 2010, 16:48 UTC
Message-ID: <20101019164826.GA25139@burratino>
In-Reply-To: <1287495320-27278-1-git-send-email-pclouds@gmail.com>

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.