[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