{"thread":{"id":"66557","subject":"[Outreachy PATCH] var: refactor to use parse-options API","startedAt":"2026-10-16T20:56:59Z","lastAt":"2026-10-16T21:47:40Z","messageCount":2,"participants":["kerylefondji7@gmail.com","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"555275","messageId":"20261016205659.4417-1-kerylefondji7@gmail.com","threadId":"66557","inReplyTo":null,"subject":"[Outreachy PATCH] var: refactor to use parse-options API","fromName":"","fromEmail":"kerylefondji7@gmail.com","sentAt":"2026-10-16T20:56:59Z","receivedAt":"2026-10-16T20:56:59Z","isPatch":true,"sender":{"key":"kerylefondji7@gmail.com","avatar":null},"body":"From: kerylefondji <kerylefondji7@gmail.com>\n\nRefactor 'git var' command handling to use an existing API (parse-options API)\ninstead of ad hoc code for argument parsing.\n\nPreviously, the ad hoc code lacked proper validation for option\ncombinations. Specifically, listing all variables (-l) and requesting a\nspecific variable name are mutually exclusive operations, but the code\ndid not enforce this restriction, causing test failures.\n\nThis change:\n- Includes the parse-options.h header and defines builtin_var_options.\n- Adds an explicit check to ensure that combining '-l' with additional\n  positional arguments correctly triggers a usage error.\n\nAll 27 automated tests in t0007-git-var.sh pass successfully.\n---\n builtin/var.c | 50 +++++++++++++++++++++++++++++++++++---------------\n 1 file changed, 35 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/var.c b/builtin/var.c\nindex cc3a43cde2..20ea4e0489 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -18,8 +18,11 @@\n #include \"path.h\"\n #include \"strbuf.h\"\n #include \"run-command.h\"\n-\n-static const char var_usage[] = \"git var (-l | <variable>)\";\n+#include \"parse-options.h\"\n+static const char * const var_usage[] = {\n+\tN_(\"git var (-l | <variable>)\"),\n+\tNULL\n+};\n \n static char *committer(int ident_flag)\n {\n@@ -220,22 +223,39 @@ int cmd_var(int argc,\n \t    struct repository *repo UNUSED)\n {\n \tconst struct git_var *git_var;\n-\tchar *val;\n+        char *val;\n+        int list = 0; /* Flag to track if -l/--list option is used */\n \n-\tshow_usage_if_asked(argc, argv, var_usage);\n-\tif (argc != 2)\n-\t\tusage(var_usage);\n+        /* Define the accepted command-line options */\n+        struct option builtin_var_options[] = {\n+                OPT_BOOL('l', \"list\", &list, N_(\"list builtin variables\")),\n+                OPT_END()\n+        };\n \n-\tif (strcmp(argv[1], \"-l\") == 0) {\n-\t\trepo_config(the_repository, show_config, NULL);\n-\t\tlist_vars();\n-\t\treturn 0;\n-\t}\n-\trepo_config(the_repository, git_default_config, NULL);\n+        /* Parse options using the standard API */\n+        argc = parse_options(argc, argv, prefix, builtin_var_options,\n+                             var_usage, 0);\n+\n+        /* If the user requested to list all variables (-l) */\n+       \n+if (list) {\n+    if (argc > 0)\n+        usage(var_usage[0]);\n+    repo_config(the_repository, show_config, NULL);\n+    list_vars();\n+    return 0;\n+}\n+\n+        /* Ensure only one positional argument (the variable name) remains */\n+        if (argc != 1)\n+                usage(var_usage[0]);\n+\n+        repo_config(the_repository, git_default_config, NULL);\n \n-\tgit_var = get_git_var(argv[1]);\n-\tif (!git_var)\n-\t\tusage(var_usage);\n+        /* Retrieve the specific git variable (note: now at argv[0] after parsing) */\n+        git_var = get_git_var(argv[0]);\n+        if (!git_var)\n+                usage(var_usage[0]);\n \n \tval = git_var->read(IDENT_STRICT);\n \tif (!val)\n-- \n2.53.0\n\n\n"},{"id":"555286","messageId":"xmqq1p9pe2rn.fsf@gitster.g","threadId":"66557","inReplyTo":"20261016205659.4417-1-kerylefondji7@gmail.com","subject":"Re: [Outreachy PATCH] var: refactor to use parse-options API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-16T21:47:40Z","receivedAt":"2026-10-16T21:47:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"kerylefondji7@gmail.com writes:\n\n> From: kerylefondji <kerylefondji7@gmail.com>\n>\n> Refactor 'git var' command handling to use an existing API (parse-options API)\n> instead of ad hoc code for argument parsing.\n>\n> Previously, the ad hoc code lacked proper validation for option\n> combinations. Specifically, listing all variables (-l) and requesting a\n> specific variable name are mutually exclusive operations, but the code\n> did not enforce this restriction, causing test failures.\n\nEverything below this line up to the three-dash line should be moved\nbelow the three-dash line, and replaced with your sign-off, I think\n\n> This change:\n> - Includes the parse-options.h header and defines builtin_var_options.\n> - Adds an explicit check to ensure that combining '-l' with additional\n>   positional arguments correctly triggers a usage error.\n>\n> All 27 automated tests in t0007-git-var.sh pass successfully.\n> ---\n>  builtin/var.c | 50 +++++++++++++++++++++++++++++++++++---------------\n>  1 file changed, 35 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/var.c b/builtin/var.c\n> index cc3a43cde2..20ea4e0489 100644\n> --- a/builtin/var.c\n> +++ b/builtin/var.c\n> @@ -18,8 +18,11 @@\n>  #include \"path.h\"\n>  #include \"strbuf.h\"\n>  #include \"run-command.h\"\n> -\n> -static const char var_usage[] = \"git var (-l | <variable>)\";\n> +#include \"parse-options.h\"\n> +static const char * const var_usage[] = {\n> +\tN_(\"git var (-l | <variable>)\"),\n> +\tNULL\n> +};\n>  \n>  static char *committer(int ident_flag)\n>  {\n> @@ -220,22 +223,39 @@ int cmd_var(int argc,\n>  \t    struct repository *repo UNUSED)\n>  {\n>  \tconst struct git_var *git_var;\n> -\tchar *val;\n> +        char *val;\n> +        int list = 0; /* Flag to track if -l/--list option is used */\n\nUseless comment that is too obvious from a properly named variable\nwith its use in a \"struct option\" array.\n\nEither the patch is heavily whitespace damaged, or you indented\nincorrectly with spaces, or perhaps little bit of both?\n\n> -\tshow_usage_if_asked(argc, argv, var_usage);\n> -\tif (argc != 2)\n> -\t\tusage(var_usage);\n> +        /* Define the accepted command-line options */\n> +        struct option builtin_var_options[] = {\n> +                OPT_BOOL('l', \"list\", &list, N_(\"list builtin variables\")),\n> +                OPT_END()\n> +        };\n\nWe did not take \"git var --list\", now we do and in addition we\nprobably take \"git var --lis\".  That is notable enough change.\n\n> -\tif (strcmp(argv[1], \"-l\") == 0) {\n> -\t\trepo_config(the_repository, show_config, NULL);\n> -\t\tlist_vars();\n> -\t\treturn 0;\n> -\t}\n> -\trepo_config(the_repository, git_default_config, NULL);\n> +        /* Parse options using the standard API */\n> +        argc = parse_options(argc, argv, prefix, builtin_var_options,\n> +                             var_usage, 0);\n> +\n> +        /* If the user requested to list all variables (-l) */\n> +       \n> +if (list) {\n> +    if (argc > 0)\n> +        usage(var_usage[0]);\n> +    repo_config(the_repository, show_config, NULL);\n> +    list_vars();\n> +    return 0;\n> +}\n\nAgain, the indentation, or lack thereof in this case, is\nproblematic.\n\n> +        /* Ensure only one positional argument (the variable name) remains */\n> +        if (argc != 1)\n> +                usage(var_usage[0]);\n> +\n> +        repo_config(the_repository, git_default_config, NULL);\n>  \n> -\tgit_var = get_git_var(argv[1]);\n> -\tif (!git_var)\n> -\t\tusage(var_usage);\n> +        /* Retrieve the specific git variable (note: now at argv[0] after parsing) */\n> +        git_var = get_git_var(argv[0]);\n> +        if (!git_var)\n> +                usage(var_usage[0]);\n>  \n>  \tval = git_var->read(IDENT_STRICT);\n>  \tif (!val)\n\n"}]}