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

[Junio C Hamano <gitster@pobox.com>]

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)