From: Jonathan Nieder <jrnieder@gmail.com>
Date: Tue, 19 Oct 2010 16:48:26 GMT
Subject: Re: [PATCH 1/3] run_builtin(): save "-h" detection result for later use
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:

> 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.