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

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

[Husain Sitamuwala via GitGitGadget]

From: Hussain Sitamouwala <husainsitamuwala21@gmail.com>

Use the parse-options API to handle command-line options
consistently with other Git builtins.

The command currently parses options manually, duplicating
argument-handling logic. Using parse-options makes option
handling consistent with the rest of Git.

Replace manual option parsing while preserving support for
normalization, branch names, and refname validation.

Signed-off-by: Hussain Sitamouwala <husainsitamuwala21@gmail.com>
---
    check-ref-format: use parse-options API

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2470%2FCodeWithHusain%2Foutreachy-microproject-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2470/CodeWithHusain/outreachy-microproject-v1
Pull-Request: https://github.com/git/git/pull/2470

 builtin/check-ref-format.c | 45 ++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index fd1c9c0e0c..58e60eecd2 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -5,13 +5,20 @@
 #define USE_THE_REPOSITORY_VARIABLE
 
 #include "builtin.h"
+#include "parse-options.h"
 #include "refs.h"
 #include "setup.h"
 #include "strbuf.h"
 
 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>";
+
+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
+};
 
 /*
  * Return a copy of refname but with leading slashes removed and runs
@@ -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")),
+		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()
+	};
+
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
 	show_usage_if_asked(argc, argv,
@@ -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
-- 
gitgitgadget

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

[Junio C Hamano]

"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

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

[Husain Sitamuwala via GitGitGadget]

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

Signed-off-by: Hussain Sitamouwala <husainsitamuwala21@gmail.com>
---
    check-ref-format: use parse-options API

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2470%2FCodeWithHusain%2Foutreachy-microproject-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2470/CodeWithHusain/outreachy-microproject-v2
Pull-Request: https://github.com/git/git/pull/2470

Range-diff vs v1:

 1:  9f05296454 ! 1:  991ca870fe check-ref-format: use parse-options API
     @@ Metadata
       ## Commit message ##
          check-ref-format: use parse-options API
      
     -    Use the parse-options API to handle command-line options
     -    consistently with other Git builtins.
     +    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.
      
     -    The command currently parses options manually, duplicating
     -    argument-handling logic. Using parse-options makes option
     -    handling consistent with the rest of Git.
     +    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.
      
     -    Replace manual option parsing while preserving support for
     -    normalization, branch names, and refname validation.
     +    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>.
      
          Signed-off-by: Hussain Sitamouwala <husainsitamuwala21@gmail.com>
      
     @@ builtin/check-ref-format.c
       #include "setup.h"
       #include "strbuf.h"
       
     - static const char builtin_check_ref_format_usage[] =
     +-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>";
     -+
     -+static const char * const builtin_check_ref_format_usages[] = {
     -+	"git check-ref-format [--normalize] [<options>] <refname>",
     -+	"   or: git check-ref-format --branch <branchname-shorthand>",
     ++static const char * const builtin_check_ref_format_usage[] = {
     ++	N_("git check-ref-format [--normalize] [<options>] <refname>"),
     ++	N_("git check-ref-format --branch <branchname-shorthand>"),
      +	NULL
      +};
       
     @@ builtin/check-ref-format.c: int cmd_check_ref_format(int argc,
       	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")),
     ++		OPT_BOOL_F(0, "normalize", &normalize,
     ++			   N_("normalize the refname and print it"),
     ++			   PARSE_OPT_NONEG),
     ++		OPT_BOOL_F(0, "print", &normalize,
     ++			   N_("deprecated synonym for --normalize"),
     ++			   PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
      +		OPT_BIT(0, "allow-onelevel", &flags,
     -+			N_("allow one-level refnames"),
     ++			N_("accept a refname with a single level"),
      +			REFNAME_ALLOW_ONELEVEL),
     -+		OPT_BIT(0, "refspec-pattern", &flags,
     -+			N_("allow refspec patterns"),
     -+			REFNAME_REFSPEC_PATTERN),
     ++		OPT_BIT_F(0, "refspec-pattern", &flags,
     ++			  N_("interpret <refname> as a reference pattern for a refspec"),
     ++			  REFNAME_REFSPEC_PATTERN, PARSE_OPT_NONEG),
      +		OPT_END()
      +	};
     -+
     + 
       	BUG_ON_NON_EMPTY_PREFIX(prefix);
       
     - 	show_usage_if_asked(argc, argv,
     -@@ builtin/check-ref-format.c: int cmd_check_ref_format(int argc,
     +-	show_usage_if_asked(argc, argv,
     +-			    builtin_check_ref_format_usage);
     +-
       	if (argc == 3 && !strcmp(argv[1], "--branch"))
       		return check_ref_format_branch(argv[2]);
       
     @@ builtin/check-ref-format.c: int cmd_check_ref_format(int argc,
      -			usage(builtin_check_ref_format_usage);
      -	}
      -	if (! (i == argc - 1))
     +-		usage(builtin_check_ref_format_usage);
      +	argc = parse_options(argc, argv, prefix, options,
     -+			     builtin_check_ref_format_usages, 0);
     -+
     ++			     builtin_check_ref_format_usage,
     ++			     PARSE_OPT_STOP_AT_NON_OPTION);
      +	if (argc != 1)
     - 		usage(builtin_check_ref_format_usage);
     ++		usage_with_options(builtin_check_ref_format_usage, options);
       
      -	refname = argv[i];
      +	refname = argv[0];


 builtin/check-ref-format.c | 49 +++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index fd1c9c0e0c..d698da99d9 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -5,13 +5,16 @@
 #define USE_THE_REPOSITORY_VARIABLE
 
 #include "builtin.h"
+#include "parse-options.h"
 #include "refs.h"
 #include "setup.h"
 #include "strbuf.h"
 
-static const char builtin_check_ref_format_usage[] =
-"git check-ref-format [--normalize] [<options>] <refname>\n"
-"   or: git check-ref-format --branch <branchname-shorthand>";
+static const char * const builtin_check_ref_format_usage[] = {
+	N_("git check-ref-format [--normalize] [<options>] <refname>"),
+	N_("git check-ref-format --branch <branchname-shorthand>"),
+	NULL
+};
 
 /*
  * Return a copy of refname but with leading slashes removed and runs
@@ -58,37 +61,39 @@ 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_F(0, "normalize", &normalize,
+			   N_("normalize the refname and print it"),
+			   PARSE_OPT_NONEG),
+		OPT_BOOL_F(0, "print", &normalize,
+			   N_("deprecated synonym for --normalize"),
+			   PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
+		OPT_BIT(0, "allow-onelevel", &flags,
+			N_("accept a refname with a single level"),
+			REFNAME_ALLOW_ONELEVEL),
+		OPT_BIT_F(0, "refspec-pattern", &flags,
+			  N_("interpret <refname> as a reference pattern for a refspec"),
+			  REFNAME_REFSPEC_PATTERN, PARSE_OPT_NONEG),
+		OPT_END()
+	};
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
-	show_usage_if_asked(argc, argv,
-			    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;
-		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))
-		usage(builtin_check_ref_format_usage);
+	argc = parse_options(argc, argv, prefix, options,
+			     builtin_check_ref_format_usage,
+			     PARSE_OPT_STOP_AT_NON_OPTION);
+	if (argc != 1)
+		usage_with_options(builtin_check_ref_format_usage, options);
 
-	refname = argv[i];
+	refname = argv[0];
 	if (normalize)
 		refname = to_free = collapse_slashes(refname);
 	if (check_refname_format(refname, flags))

base-commit: b9e5cff1803ab24582201ef059516eb16014b9d9
-- 
gitgitgadget

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1728 bytes --]

"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]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Junio C Hamano]

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.