Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Dec 19, 2016, 11:07 UTC
Message-ID: <e93ee78a-aa5e-27f6-9703-6efa385f487b@alum.mit.edu>
In-Reply-To: <20161104191358.28812-3-ijackson@chiark.greenend.org.uk>

On 11/04/2016 08:13 PM, Ian Jackson wrote:

Show 27 quoted lines

> We are going to want to permit other options with --branch.
> 
> So, replace the special case with just an entry for --branch in the
> parser for ordinary options, and check for option compatibility at the
> end.
> 
> No overall functional change.
> 
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
>  builtin/check-ref-format.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index 4d56caa..f12c19c 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -49,13 +49,19 @@ static int check_ref_format_branch(const char *arg)
>  }
>  
>  static int normalize = 0;
> +static int check_branch = 0;
>  static int flags = 0;
>  
>  static int check_one_ref_format(const char *refname)
>  {
> +	int got;

`got` is an unusual name for this variable, and I don't really understand what the word means in this context. Is there a reason not to use the more usual `err`?

Show 32 quoted lines

> +
>  	if (normalize)
>  		refname = collapse_slashes(refname);
> -	if (check_refname_format(refname, flags))
> +	got = check_branch
> +		? check_ref_format_branch(refname)
> +		: check_refname_format(refname, flags);
> +	if (got)
>  		return 1;
>  	if (normalize)
>  		printf("%s\n", refname);
> @@ -68,9 +74,6 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
>  	if (argc == 2 && !strcmp(argv[1], "-h"))
>  		usage(builtin_check_ref_format_usage);
>  
> -	if (argc == 3 && !strcmp(argv[1], "--branch"))
> -		return check_ref_format_branch(argv[2]);
> -
>  	for (i = 1; i < argc && argv[i][0] == '-'; i++) {
>  		if (!strcmp(argv[i], "--normalize") || !strcmp(argv[i], "--print"))
>  			normalize = 1;
> @@ -80,9 +83,15 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
>  			flags &= ~REFNAME_ALLOW_ONELEVEL;
>  		else if (!strcmp(argv[i], "--refspec-pattern"))
>  			flags |= REFNAME_REFSPEC_PATTERN;
> +		else if (!strcmp(argv[i], "--branch"))
> +			check_branch = 1;
>  		else
>  			usage(builtin_check_ref_format_usage);
>  	}
> +
> +	if (check_branch && (flags || normalize))

Is there a reason not to allow `--normalize` with `--branch`? (Currently, `git check-ref-format --branch` *does* allow input like `refs/heads/foo`.)

But note that simply allowing `--branch --normalize` without changing `check_one_ref_format()` would mean generating *two* lines of output per reference, so something else would have to change, too.

Show 6 quoted lines

> +		usage(builtin_check_ref_format_usage);
> +
>  	if (! (i == argc - 1))
>  		usage(builtin_check_ref_format_usage);
>  
>

Michael