[Outreachy PATCH] var: refactor to use parse-options API

[Outreachy PATCH] var: refactor to use parse-options API

[kerylefondji7]

From: kerylefondji <kerylefondji7@gmail.com>

Refactor 'git var' command handling to use an existing API (parse-options API)
instead of ad hoc code for argument parsing.

Previously, the ad hoc code lacked proper validation for option
combinations. Specifically, listing all variables (-l) and requesting a
specific variable name are mutually exclusive operations, but the code
did not enforce this restriction, causing test failures.

This change:
- Includes the parse-options.h header and defines builtin_var_options.
- Adds an explicit check to ensure that combining '-l' with additional
  positional arguments correctly triggers a usage error.

All 27 automated tests in t0007-git-var.sh pass successfully.
---
 builtin/var.c | 50 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 15 deletions(-)

diff --git a/builtin/var.c b/builtin/var.c
index cc3a43cde2..20ea4e0489 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -18,8 +18,11 @@
 #include "path.h"
 #include "strbuf.h"
 #include "run-command.h"
-
-static const char var_usage[] = "git var (-l | <variable>)";
+#include "parse-options.h"
+static const char * const var_usage[] = {
+	N_("git var (-l | <variable>)"),
+	NULL
+};
 
 static char *committer(int ident_flag)
 {
@@ -220,22 +223,39 @@ int cmd_var(int argc,
 	    struct repository *repo UNUSED)
 {
 	const struct git_var *git_var;
-	char *val;
+        char *val;
+        int list = 0; /* Flag to track if -l/--list option is used */
 
-	show_usage_if_asked(argc, argv, var_usage);
-	if (argc != 2)
-		usage(var_usage);
+        /* Define the accepted command-line options */
+        struct option builtin_var_options[] = {
+                OPT_BOOL('l', "list", &list, N_("list builtin variables")),
+                OPT_END()
+        };
 
-	if (strcmp(argv[1], "-l") == 0) {
-		repo_config(the_repository, show_config, NULL);
-		list_vars();
-		return 0;
-	}
-	repo_config(the_repository, git_default_config, NULL);
+        /* Parse options using the standard API */
+        argc = parse_options(argc, argv, prefix, builtin_var_options,
+                             var_usage, 0);
+
+        /* If the user requested to list all variables (-l) */
+       
+if (list) {
+    if (argc > 0)
+        usage(var_usage[0]);
+    repo_config(the_repository, show_config, NULL);
+    list_vars();
+    return 0;
+}
+
+        /* Ensure only one positional argument (the variable name) remains */
+        if (argc != 1)
+                usage(var_usage[0]);
+
+        repo_config(the_repository, git_default_config, NULL);
 
-	git_var = get_git_var(argv[1]);
-	if (!git_var)
-		usage(var_usage);
+        /* Retrieve the specific git variable (note: now at argv[0] after parsing) */
+        git_var = get_git_var(argv[0]);
+        if (!git_var)
+                usage(var_usage[0]);
 
 	val = git_var->read(IDENT_STRICT);
 	if (!val)
-- 
2.53.0

Re: [Outreachy PATCH] var: refactor to use parse-options API

[Junio C Hamano]

kerylefondji7@gmail.com writes:

> From: kerylefondji <kerylefondji7@gmail.com>
>
> Refactor 'git var' command handling to use an existing API (parse-options API)
> instead of ad hoc code for argument parsing.
>
> Previously, the ad hoc code lacked proper validation for option
> combinations. Specifically, listing all variables (-l) and requesting a
> specific variable name are mutually exclusive operations, but the code
> did not enforce this restriction, causing test failures.

Everything below this line up to the three-dash line should be moved
below the three-dash line, and replaced with your sign-off, I think

> This change:
> - Includes the parse-options.h header and defines builtin_var_options.
> - Adds an explicit check to ensure that combining '-l' with additional
>   positional arguments correctly triggers a usage error.
>
> All 27 automated tests in t0007-git-var.sh pass successfully.
> ---
>  builtin/var.c | 50 +++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/var.c b/builtin/var.c
> index cc3a43cde2..20ea4e0489 100644
> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -18,8 +18,11 @@
>  #include "path.h"
>  #include "strbuf.h"
>  #include "run-command.h"
> -
> -static const char var_usage[] = "git var (-l | <variable>)";
> +#include "parse-options.h"
> +static const char * const var_usage[] = {
> +	N_("git var (-l | <variable>)"),
> +	NULL
> +};
>  
>  static char *committer(int ident_flag)
>  {
> @@ -220,22 +223,39 @@ int cmd_var(int argc,
>  	    struct repository *repo UNUSED)
>  {
>  	const struct git_var *git_var;
> -	char *val;
> +        char *val;
> +        int list = 0; /* Flag to track if -l/--list option is used */

Useless comment that is too obvious from a properly named variable
with its use in a "struct option" array.

Either the patch is heavily whitespace damaged, or you indented
incorrectly with spaces, or perhaps little bit of both?

> -	show_usage_if_asked(argc, argv, var_usage);
> -	if (argc != 2)
> -		usage(var_usage);
> +        /* Define the accepted command-line options */
> +        struct option builtin_var_options[] = {
> +                OPT_BOOL('l', "list", &list, N_("list builtin variables")),
> +                OPT_END()
> +        };

We did not take "git var --list", now we do and in addition we
probably take "git var --lis".  That is notable enough change.

> -	if (strcmp(argv[1], "-l") == 0) {
> -		repo_config(the_repository, show_config, NULL);
> -		list_vars();
> -		return 0;
> -	}
> -	repo_config(the_repository, git_default_config, NULL);
> +        /* Parse options using the standard API */
> +        argc = parse_options(argc, argv, prefix, builtin_var_options,
> +                             var_usage, 0);
> +
> +        /* If the user requested to list all variables (-l) */
> +       
> +if (list) {
> +    if (argc > 0)
> +        usage(var_usage[0]);
> +    repo_config(the_repository, show_config, NULL);
> +    list_vars();
> +    return 0;
> +}

Again, the indentation, or lack thereof in this case, is
problematic.

> +        /* Ensure only one positional argument (the variable name) remains */
> +        if (argc != 1)
> +                usage(var_usage[0]);
> +
> +        repo_config(the_repository, git_default_config, NULL);
>  
> -	git_var = get_git_var(argv[1]);
> -	if (!git_var)
> -		usage(var_usage);
> +        /* Retrieve the specific git variable (note: now at argv[0] after parsing) */
> +        git_var = get_git_var(argv[0]);
> +        if (!git_var)
> +                usage(var_usage[0]);
>  
>  	val = git_var->read(IDENT_STRICT);
>  	if (!val)