"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

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

> From: Hussain Sitamouwala <husainsitamuwala21@gmail.com>
>
> The command parses its options by hand, comparing each argument
> against string literals. Use the parse-options API instead so that
> option handling is consistent with other builtins and the help
> output is generated from the option table.
>
> Keep the existing behavior: --print stays a hidden, deprecated
> synonym for --normalize, only --allow-onelevel can be negated, and
> parsing stops at the first non-option argument so that options
> after <refname> are still rejected.
>
> Drop the early show_usage_if_asked() call, so that -h is handled by
> parse_options().
>
> Two minor differences come with parse_options(): unambiguous
> abbreviations of long options, such as --norm, are accepted, and
> "--" may now precede <refname>.
>

If you follow our 'Documentation/SubmittingPatches' guidelines, it
states:

  It is often beneficial to allow some time for reviewers to provide
  feedback before sending a new version, rather than sending an updated
  series immediately after receiving a review. This helps collect
  broader input, gives reviewers in different time zones a fair chance
  to comment, and avoids unnecessary churn from many rapid iterations.
  Waiting also encourages you to polish each version before sending it,
  so reviewers can focus on substantial issues rather than typos or
  other small mistakes.

Seems like Junio's review went unanswered. Which makes it hard for me to
consider dedicating time to do another review of the newer version. I
would suggest that you engage in the process before sending new
versions. I'll hold off reviewing till then :)

[snip]

Karthik Nayak <karthik.188@gmail.com> writes:

> If you follow our 'Documentation/SubmittingPatches' guidelines, it
> states:
>
>   It is often beneficial to allow some time for reviewers to provide
>   feedback before sending a new version, rather than sending an updated
>   series immediately after receiving a review. This helps collect
>   broader input, gives reviewers in different time zones a fair chance
>   to comment, and avoids unnecessary churn from many rapid iterations.
>   Waiting also encourages you to polish each version before sending it,
>   so reviewers can focus on substantial issues rather than typos or
>   other small mistakes.

Thanks for bringing it up.  Also MyFirstContribution has this:

    After a few days, you may receive comments on your patch series.
    Carefully read them to understand the reviewers' perspective, and do
    not hesitate to ask for clarification.

    Reviewers may also ask about your proposed commit log or the changes.
    Answer these in your response.  They often ask because the patch needs
    clarification; use this feedback to improve your writing, and update
    the patch so the next reader does not have to ask the same question.

    Do not start coding, much less send the next version, until you finish
    discussing the current one and both sides know what to expect.  This
    saves time, lets reviewers respond before you write a patch they have
    to reject, and avoids forcing them to inspect 'v2' only to find their
    concerns unaddressed.

Karthik Nayak <karthik.188@gmail.com> writes:

> Seems like Junio's review went unanswered. Which makes it hard for me
> to consider dedicating time to do another review of the newer version.

Hi Karthik, Junio,

Sorry about that. I sent v2 before replying to Junio's review of v1,
and I should have discussed it first. I have now re-read
Documentation/SubmittingPatches and MyFirstContribution, and I will
wait for feedback before sending any further version.

> What is this change about?  More importantly, is it sensible to have
> this and ... another variable with very similar and confusing name

You are right, keeping two usage variables in sync made no sense. v2
has a single builtin_check_ref_format_usage[] array, and
usage_with_options() is used for the error path, so the old string
is gone.

> As can be seen in the usage string ... we still accept "--print" as
> a deprecated synonym but we do not advertise it.

In v2, --print is marked PARSE_OPT_HIDDEN and its help text says it
is a deprecated synonym for --normalize.

> Do we want to barf when the user says
>
>     $ git check-ref-format --no-print --no-normalize
>       --no-refspec-pattern ...

No. v2 adds PARSE_OPT_NONEG to --normalize, --print and
--refspec-pattern. Only --no-allow-onelevel is still accepted, as
before, so the set of accepted options is unchanged.

> The call to show_usage_if_asked() is still here, which means
> "git check-ref-format -h" is intercepted ...

v2 drops that call, so "-h" now goes through parse_options() and the
option list is generated from the table.

I also used PARSE_OPT_STOP_AT_NON_OPTION so that options placed after
<refname> are still rejected, as they were before.

Two behavior differences come with parse_options(), both mentioned in
the commit message: unambiguous abbreviations such as --norm are now
accepted, and "--" may now precede <refname>. If either is unwanted,
I am happy to change it. I ran t1402-check-ref-format.sh and it passes.

I will wait for further comments before preparing v3.

Thanks,
Hussain Sitamouwala