Re: [PATCH] check-ref-format: use parse-options API

[Junio C Hamano <gitster@pobox.com>]

"Husain Sitamuwala via GitGitGadget" <gitgitgadget@gmail.com>
writes:

Didn't I read very recently that somebody else was updating
check-ref-format?  Perhaps I am hallucinating.

Anyway.

>  static const char builtin_check_ref_format_usage[] =
> -"git check-ref-format [--normalize] [<options>] <refname>\n"
> -"   or: git check-ref-format --branch <branchname-shorthand>";
> +	"git check-ref-format [--normalize] [<options>] <refname>\n"
> +	"   or: git check-ref-format --branch <branchname-shorthand>";

What is this change about?  More importantly, is it sensible to have
this and ...

> +static const char * const builtin_check_ref_format_usages[] = {
> +	"git check-ref-format [--normalize] [<options>] <refname>",
> +	"   or: git check-ref-format --branch <branchname-shorthand>",
> +	NULL
> +};

... another variable with very similar and confusing name with very
similar contents?  How do we plan to pay for the cost maintaining
them in sync?  Do we even need both of them?

> @@ -58,13 +65,26 @@ int cmd_check_ref_format(int argc,
>  			 const char *prefix,
>  			 struct repository *repo UNUSED)
>  {
> -	int i;
>  	int normalize = 0;
>  	int flags = 0;
>  	const char *refname;
>  	char *to_free = NULL;
>  	int ret = 1;
>  
> +	struct option options[] = {
> +		OPT_BOOL(0, "normalize", &normalize,
> +			 N_("normalize refname")),
> +		OPT_BOOL(0, "print", &normalize,
> +			 N_("print normalized refname")),

As can be seen in the usage string and in the "git blame" output for
the code before this patch to parse "--print", which points at
a40e6fb67a (Change check_refname_format() to reject unnormalized
refnames, 2011-09-15), we still accept "--print" as a deprecated
synonym but we do not advertise it.  A good candidate for using
PARSE_OPT_HIDDEN, probably.

> +		OPT_BIT(0, "allow-onelevel", &flags,
> +			N_("allow one-level refnames"),
> +			REFNAME_ALLOW_ONELEVEL),
> +		OPT_BIT(0, "refspec-pattern", &flags,
> +			N_("allow refspec patterns"),
> +			REFNAME_REFSPEC_PATTERN),
> +		OPT_END()
> +	};

Do we want to barf when the user says

    $ git check-ref-format --no-print --no-normalize --no-refspec-pattern ...

by the way?  With the above, we suddenly start taking these options,
and I offhand do not know what the ramifications of doing so are.

>  	BUG_ON_NON_EMPTY_PREFIX(prefix);
>  
>  	show_usage_if_asked(argc, argv,

The call to show_usage_if_asked() is still here, which means

    $ git check-ref-format -h

is intercepted by this call and the control will never come back.

We will never reach parse_options() call below, so you lose 80% of
the value of using parse-options API, which is that you get the list
of options and its descriptions for free (well not free, you
carefully prepared them in the option[] array above, and they are
wasted).

> @@ -73,22 +93,13 @@ int cmd_check_ref_format(int argc,
>  	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;
> -		else if (!strcmp(argv[i], "--allow-onelevel"))
> -			flags |= REFNAME_ALLOW_ONELEVEL;
> -		else if (!strcmp(argv[i], "--no-allow-onelevel"))
> -			flags &= ~REFNAME_ALLOW_ONELEVEL;
> -		else if (!strcmp(argv[i], "--refspec-pattern"))
> -			flags |= REFNAME_REFSPEC_PATTERN;
> -		else
> -			usage(builtin_check_ref_format_usage);
> -	}
> -	if (! (i == argc - 1))
> +	argc = parse_options(argc, argv, prefix, options,
> +			     builtin_check_ref_format_usages, 0);
> +
> +	if (argc != 1)
>  		usage(builtin_check_ref_format_usage);
>  
> -	refname = argv[i];
> +	refname = argv[0];
>  	if (normalize)
>  		refname = to_free = collapse_slashes(refname);
>  	if (check_refname_format(refname, flags))
>
> base-commit: 37b3475fb530c14109211faca33716a173db61ed