check-ref-format: use parse-options API

From: Hussain Sitamouwala <[email protected]>

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.

---
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(-)

index fd1c9c0e0c..58e60eecd2 100644
@@ -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

"Husain Sitamuwala via GitGitGadget" <[email protected]> writes:

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

Anyway.

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

    $ git check-ref-format -h

From: Hussain Sitamouwala <[email protected]>

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

---
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 <[email protected]>
      
     @@ 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(-)

index fd1c9c0e0c..d698da99d9 100644
@@ -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

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]

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.

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.

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.

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

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.

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