{"thread":{"id":"66679","subject":"[PATCH] test-chmtime: use parse_options() to parse command-line flags","startedAt":"2026-10-27T11:14:48Z","lastAt":"2026-11-02T17:07:55Z","messageCount":9,"participants":["Shashank S via GitGitGadget","Junio C Hamano","Shashank S","Cynthia Uche"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"556484","messageId":"pull.2280.git.1793099688076.gitgitgadget@gmail.com","threadId":"66679","inReplyTo":null,"subject":"[PATCH] test-chmtime: use parse_options() to parse command-line flags","fromName":"Shashank S via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-10-27T11:14:48Z","receivedAt":"2026-10-27T11:14:48Z","isPatch":true,"sender":{"key":"shashu8660@gmail.com","avatar":null},"body":"From: Shashank S Mogaveera \n\ncmd__chmtime() currently parses the '-v'/'--verbose' and '-g'/'--get'\nflags manually using strcmp() checks and manual index tracking.\n\nRefactor cmd__chmtime() to use parse_options() and usage_with_options()\ninstead. Pass PARSE_OPT_KEEP_UNKNOWN_OPT so that negative relative time\noffsets (such as '-86400') are not rejected as unknown short options\nand remain in argv for timespec_arg() to parse.\n\nSigned-off-by: Shashank S Mogaveera \n---\n [Outreachy]test-chmtime: use parse_options() to parse command-line flags\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2280%2Fshashu8660%2Foutreachy3-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2280/shashu8660/outreachy3-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2280\n\n t/helper/test-chmtime.c | 46 +++++++++++++++++++----------------------\n 1 file changed, 21 insertions(+), 25 deletions(-)\n\ndiff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c\nindex 0e5538833a..dc6281b1d6 100644\n--- a/t/helper/test-chmtime.c\n+++ b/t/helper/test-chmtime.c\n@@ -38,10 +38,13 @@\n */\n #include \"test-tool.h\"\n #include \"git-compat-util.h\"\n+#include \"parse-options.h\"\n #include \n \n-static const char usage_str[] =\n-\t\"(-v|--verbose|-g|--get) (+|=|=+|=-|-) ...\";\n+static const char * const chmtime_usage[] = {\n+\t\"test-tool chmtime (-v|--verbose|-g|--get) (+|=|=+|=-|-) ...\",\n+\tNULL\n+};\n \n static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n {\n@@ -68,40 +71,37 @@ static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n \n int cmd__chmtime(int argc, const char **argv)\n {\n-\tstatic int verbose;\n-\tstatic int get;\n+\tint verbose = 0;\n+\tint get = 0;\n \n-\tint i = 1;\n+\tint i = 0;\n \t/* no mtime change by default */\n \tint set_eq = 0;\n \tlong int set_time = 0;\n \n-\tif (argc < 3)\n-\t\tgoto usage;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL('v', \"verbose\", &verbose, \"print the mtime and the file name\"),\n+\t\tOPT_BOOL('g', \"get\", &get, \"print only the mtime\"),\n+\t\tOPT_END()\n+\t};\n \n-\tif (strcmp(argv[i], \"--get\") == 0 || strcmp(argv[i], \"-g\") == 0) {\n-\t\tget = 1;\n-\t\t++i;\n-\t} else if (strcmp(argv[i], \"--verbose\") == 0 || strcmp(argv[i], \"-v\") == 0) {\n-\t\tverbose = 1;\n-\t\t++i;\n-\t}\n+\targc = parse_options(argc, argv, NULL, options, chmtime_usage,\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n \n-\tif (i == argc) {\n-\t\tgoto usage;\n-\t}\n+\tif (argc < 1 || (!get && !verbose && argc < 2))\n+\t\tusage_with_options(chmtime_usage, options);\n \n \tif (timespec_arg(argv[i], &set_time, &set_eq)) {\n \t\t++i;\n \t} else {\n \t\tif (get == 0 && verbose == 0) {\n \t\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n-\t\t\tgoto usage;\n+\t\t\tusage_with_options(chmtime_usage, options);\n \t\t}\n \t}\n \n \tif (i == argc)\n-\t\tgoto usage;\n+\t\tusage_with_options(chmtime_usage, options);\n \n \tfor (; i < argc; i++) {\n \t\tstruct stat sb;\n@@ -110,7 +110,7 @@ int cmd__chmtime(int argc, const char **argv)\n \n \t\tif (stat(argv[i], &sb) < 0) {\n \t\t\tfprintf(stderr, \"Failed to stat %s: %s. Skipping\\n\",\n-\t\t\t argv[i], strerror(errno));\n+\t\t\t\targv[i], strerror(errno));\n \t\t\tcontinue;\n \t\t}\n \n@@ -150,14 +150,10 @@ int cmd__chmtime(int argc, const char **argv)\n \t\t\t}\n #endif\n \t\t\tfprintf(stderr, \"Failed to modify time on %s: %s\\n\",\n-\t\t\t argv[i], strerror(errno));\n+\t\t\t\targv[i], strerror(errno));\n \t\t\treturn 1;\n \t\t}\n \t}\n \n \treturn 0;\n-\n-usage:\n-\tfprintf(stderr, \"usage: %s %s\\n\", argv[0], usage_str);\n-\treturn 1;\n }\n\nbase-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e\n-- \ngitgitgadget\n\n"},{"id":"556556","messageId":"xmqqzevzj8f7.fsf@gitster.g","threadId":"66679","inReplyTo":"pull.2280.git.1793099688076.gitgitgadget@gmail.com","subject":"Re: [PATCH] test-chmtime: use parse_options() to parse command-line flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-27T18:38:20Z","receivedAt":"2026-10-27T18:38:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shashank S via GitGitGadget\" writes:\n\n> From: Shashank S Mogaveera \n>\n> cmd__chmtime() currently parses the '-v'/'--verbose' and '-g'/'--get'\n> flags manually using strcmp() checks and manual index tracking.\n>\n> Refactor cmd__chmtime() to use parse_options() and usage_with_options()\n> instead. Pass PARSE_OPT_KEEP_UNKNOWN_OPT so that negative relative time\n> offsets (such as '-86400') are not rejected as unknown short options\n> and remain in argv for timespec_arg() to parse.\n\nI somehow find the verb \"refactor\" used to describe a change like\nthis slightly awkward, but it is excellent to reminder readers why\nit is necessary to use PARSE_OPT_KEEP_UNKNOWN_OPT.\n\n> t/helper/test-chmtime.c | 46 +++++++++++++++++++----------------------\n> 1 file changed, 21 insertions(+), 25 deletions(-)\n>\n> diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c\n> index 0e5538833a..dc6281b1d6 100644\n> --- a/t/helper/test-chmtime.c\n> +++ b/t/helper/test-chmtime.c\n> @@ -38,10 +38,13 @@\n> */\n> #include \"test-tool.h\"\n> #include \"git-compat-util.h\"\n> +#include \"parse-options.h\"\n> #include \n> \n> -static const char usage_str[] =\n> -\t\"(-v|--verbose|-g|--get) (+|=|=+|=-|-) ...\";\n> +static const char * const chmtime_usage[] = {\n> +\t\"test-tool chmtime (-v|--verbose|-g|--get) (+|=|=+|=-|-) ...\",\n> +\tNULL\n> +};\n\nIs it necessary to spell out the options like verbose and get, once\nyou switch to the parse-options API? What does \"test-tool chmtime -h\"\nsay with this patch applied? I think it is common to say []\ninstead and let the list of options produced by the parse-options API\nto take it from there.\n\n> static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n> {\n> @@ -68,40 +71,37 @@ static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n> \n> int cmd__chmtime(int argc, const char **argv)\n> {\n> -\tstatic int verbose;\n> -\tstatic int get;\n> +\tint verbose = 0;\n> +\tint get = 0;\n\nThere is no reason why these two need to be static, but as a\n\"main()\" function for a run-once-and-exit program, there is no\nreason why they should not be static, either. With this change,\nyou are forced to explicitly initialize them to 0.\n\n> -\tint i = 1;\n> \t/* no mtime change by default */\n> \tint set_eq = 0;\n> \tlong int set_time = 0;\n> \n> -\tif (argc < 3)\n> -\t\tgoto usage;\n> \n> -\tif (strcmp(argv[i], \"--get\") == 0 || strcmp(argv[i], \"-g\") == 0) {\n> -\t\tget = 1;\n> -\t\t++i;\n> -\t} else if (strcmp(argv[i], \"--verbose\") == 0 || strcmp(argv[i], \"-v\") == 0) {\n> -\t\tverbose = 1;\n> -\t\t++i;\n> -\t}\n> \n> -\tif (i == argc) {\n> -\t\tgoto usage;\n> -\t}\n> \n> \tif (timespec_arg(argv[i], &set_time, &set_eq)) {\n> \t\t++i;\n> \t} else {\n> \t\tif (get == 0 && verbose == 0) {\n> \t\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n> -\t\t\tgoto usage;\n> \t\t}\n> \t}\n> \n> \tif (i == argc)\n> -\t\tgoto usage;\n> \n> \tfor (; i < argc; i++) {\n> \t\tstruct stat sb;\n\nI agree that the original was horrible in that it did not allow -v\nto come before -g etc.\n\nNow, let's read the updated code.\n\n> +\tint i = 0;\n\n> \t/* no mtime change by default */\n> \tint set_eq = 0;\n> \tlong int set_time = 0;\n> \n> +\tstruct option options[] = {\n> +\t\tOPT_BOOL('v', \"verbose\", &verbose, \"print the mtime and the file name\"),\n> +\t\tOPT_BOOL('g', \"get\", &get, \"print only the mtime\"),\n> +\t\tOPT_END()\n> +\t};\n> \n> +\targc = parse_options(argc, argv, NULL, options, chmtime_usage,\n> +\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n> \n> +\tif (argc < 1 || (!get && !verbose && argc < 2))\n> +\t\tusage_with_options(chmtime_usage, options);\n\nI am not sure what is going on here. The command, when parsing out\nthe -g and -v options, expects to have and 1 or more\n arguments. Why should it reject argc==2 when neither -g nor\n-v is given, but allow argc==2 when -g and/or -v is?\n\nOK, the usage of this command is a bit opaque. The command works in\ntwo ways.\n\n * When -v/-g is given, it is a request to show the timestamp of the\n file, so becomes optional (i.e., when given, it sets\n and then shows). We still want at least one .\n\n * Otherwise, it is a request to set the timestamp, so is\n required, and we want at least one .\n\nNot the best way to design an end-user UI, but this is merely a test\nhelper, so let's not complain too much ;-).\n\n> \tif (timespec_arg(argv[i], &set_time, &set_eq)) {\n> \t\t++i;\n> \t} else {\n> \t\tif (get == 0 && verbose == 0) {\n> \t\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n> +\t\t\tusage_with_options(chmtime_usage, options);\n> \t\t}\n> \t}\n\nThis is largely the fault of the original, but I wonder if we can\navoid the \"without -v/-g, timespec is required\" logic in two\nseparate places like this (and the earlier one that puzzled me)?\n\n\n> \tif (i == argc)\n> +\t\tusage_with_options(chmtime_usage, options);\n\nAnd we want at least one in any case.\n\n> \tfor (; i < argc; i++) {\n> \t\tstruct stat sb;\n\nOK. We will iterate over the arguments given.\n\n> @@ -110,7 +110,7 @@ int cmd__chmtime(int argc, const char **argv)\n> \n> \t\tif (stat(argv[i], &sb) < 0) {\n> \t\t\tfprintf(stderr, \"Failed to stat %s: %s. Skipping\\n\",\n> -\t\t\t argv[i], strerror(errno));\n> +\t\t\t\targv[i], strerror(errno));\n> \t\t\tcontinue;\n> \t\t}\n\nThis fixes a whitespace breakage, which is an unrelated change, but\nI'll let it pass.\n\n> @@ -150,14 +150,10 @@ int cmd__chmtime(int argc, const char **argv)\n> \t\t\t}\n> #endif\n> \t\t\tfprintf(stderr, \"Failed to modify time on %s: %s\\n\",\n> -\t\t\t argv[i], strerror(errno));\n> +\t\t\t\targv[i], strerror(errno));\n> \t\t\treturn 1;\n> \t\t}\n> \t}\n\nLikewise.\n\n> \n> \treturn 0;\n> -\n> -usage:\n> -\tfprintf(stderr, \"usage: %s %s\\n\", argv[0], usage_str);\n> -\treturn 1;\n> }\n\nWe exit via usage_with_options() with 129 now, but used to exit(1)\nupon a command line error.\n\nLooking good. Thanks.\n\n"},{"id":"556558","messageId":"xmqqv76nj7ys.fsf@gitster.g","threadId":"66679","inReplyTo":"pull.2280.git.1793099688076.gitgitgadget@gmail.com","subject":"Re: [PATCH] test-chmtime: use parse_options() to parse command-line flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-27T18:48:11Z","receivedAt":"2026-10-27T18:48:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shashank S via GitGitGadget\" writes:\n\n>\n> t/helper/test-chmtime.c | 46 +++++++++++++++++++----------------------\n> 1 file changed, 21 insertions(+), 25 deletions(-)\n\nForgot to mention one thing. Do we have any tests to cover the\nbehaviour of this test helper? If not, should we add some (not a\nrhetorical question)?\n\nAlso, one thing I missed is that the original ignored --verbose\nwhen --get was given, since they contradict with each other.\nShouldn't we make them explicitly incompatible with each other,\nperhaps before where we call timespec_arg()?\n\n> @@ -68,40 +71,37 @@ static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n> \n> int cmd__chmtime(int argc, const char **argv)\n> {\n> ...\n> +\targc = parse_options(argc, argv, NULL, options, chmtime_usage,\n> +\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n> \n> +\tif (argc < 1 || (!get && !verbose && argc < 2))\n> +\t\tusage_with_options(chmtime_usage, options);\n\n\tdie_for_incompatible_opt2(get, \"--get\", verbose, \"--verbose\");\n\n> \tif (timespec_arg(argv[i], &set_time, &set_eq)) {\n> \t\t++i;\n> ...\n> }\n>\n> base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e\n\n"},{"id":"556729","messageId":"CANHmUQSEoz=NFH0KV9bNyTz9_gUVPhicUeHdJr2yp1q6iVaDWA@mail.gmail.com","threadId":"66679","inReplyTo":"xmqqzevzj8f7.fsf@gitster.g","subject":"Re: [PATCH] test-chmtime: use parse_options() to parse command-line flags","fromName":"Shashank S","fromEmail":"shashu8660@gmail.com","sentAt":"2026-10-29T05:52:35Z","receivedAt":"2026-10-29T05:52:35Z","isPatch":true,"sender":{"key":"shashu8660@gmail.com","avatar":null},"body":"On Wed, 28 Oct 2026 at 00:08, Junio C Hamano wrote:\n\nThank you for the detailed review and for pointing these out!\n\n> > and remain in argv for timespec_arg() to parse.\n>\n> I somehow find the verb \"refactor\" used to describe a change like\n> this slightly awkward, but it is excellent to reminder readers why\n> it is necessary to use PARSE_OPT_KEEP_UNKNOWN_OPT.\n>\nUnderstood! In my v2, I will change the commit message to use \"update\"\ninstead of \"refactor\".\n\n> > t/helper/test-chmtime.c | 46 +++++++++++++++++++----------------------\n> > 1 file changed, 21 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c\n> > index 0e5538833a..dc6281b1d6 100644\n> > --- a/t/helper/test-chmtime.c\n> > +++ b/t/helper/test-chmtime.c\n> > @@ -38,10 +38,13 @@\n> > */\n> > #include \"test-tool.h\"\n> > #include \"git-compat-util.h\"\n> > +#include \"parse-options.h\"\n> > #include \n> >\n> > -static const char usage_str[] =\n> > - \"(-v|--verbose|-g|--get) (+|=|=+|=-|-) ...\";\n> > +static const char * const chmtime_usage[] = {\n> > + \"test-tool chmtime (-v|--verbose|-g|--get) (+|=|=+|=-|-) ...\",\n> > + NULL\n> > +};\n>\n> Is it necessary to spell out the options like verbose and get, once\n> you switch to the parse-options API? What does \"test-tool chmtime -h\"\n> say with this patch applied? I think it is common to say []\n> instead and let the list of options produced by the parse-options API\n> to take it from there.\n>\nYou are completely right. Hardcoding the flags in the string is\nredundant now that `parse_options()` handles the `-h` output. I will\nupdate the usage string to `[]` in v2.\n\n> > static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n> > {\n> > @@ -68,40 +71,37 @@ static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n> >\n> > int cmd__chmtime(int argc, const char **argv)\n> > {\n> > - static int verbose;\n> > - static int get;\n> > + int verbose = 0;\n> > + int get = 0;\n>\n> There is no reason why these two need to be static, but as a\n> \"main()\" function for a run-once-and-exit program, there is no\n> reason why they should not be static, either. With this change,\n> you are forced to explicitly initialize them to 0.\n>\nThat is a great point about the automatic initialization of static\nvariables. Since it isn't strictly necessary to change them for\n`parse_options()` to work, I will revert this specific change in v2 to\nkeep the diff as small and focused as possible.\n\n> > - int i = 1;\n> > /* no mtime change by default */\n> > int set_eq = 0;\n> > long int set_time = 0;\n> >\n> > - if (argc < 3)\n> > - goto usage;\n> >\n> > - if (strcmp(argv[i], \"--get\") == 0 || strcmp(argv[i], \"-g\") == 0) {\n> > - get = 1;\n> > - ++i;\n> > - } else if (strcmp(argv[i], \"--verbose\") == 0 || strcmp(argv[i], \"-v\") == 0) {\n> > - verbose = 1;\n> > - ++i;\n> > - }\n> >\n> > - if (i == argc) {\n> > - goto usage;\n> > - }\n> >\n> > if (timespec_arg(argv[i], &set_time, &set_eq)) {\n> > ++i;\n> > } else {\n> > if (get == 0 && verbose == 0) {\n> > fprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n> > - goto usage;\n> > }\n> > }\n> >\n> > if (i == argc)\n> > - goto usage;\n> >\n> > for (; i < argc; i++) {\n> > struct stat sb;\n>\n> I agree that the original was horrible in that it did not allow -v\n> to come before -g etc.\n>\n> Now, let's read the updated code.\n>\n> > + int i = 0;\n>\n> > /* no mtime change by default */\n> > int set_eq = 0;\n> > long int set_time = 0;\n> >\n> > + struct option options[] = {\n> > + OPT_BOOL('v', \"verbose\", &verbose, \"print the mtime and the file name\"),\n> > + OPT_BOOL('g', \"get\", &get, \"print only the mtime\"),\n> > + OPT_END()\n> > + };\n> >\n> > + argc = parse_options(argc, argv, NULL, options, chmtime_usage,\n> > + PARSE_OPT_KEEP_UNKNOWN_OPT);\n> >\n> > + if (argc < 1 || (!get && !verbose && argc < 2))\n> > + usage_with_options(chmtime_usage, options);\n>\n> I am not sure what is going on here. The command, when parsing out\n> the -g and -v options, expects to have and 1 or more\n> arguments. Why should it reject argc==2 when neither -g nor\n> -v is given, but allow argc==2 when -g and/or -v is?\n>\n> OK, the usage of this command is a bit opaque. The command works in\n> two ways.\n>\n> * When -v/-g is given, it is a request to show the timestamp of the\n> file, so becomes optional (i.e., when given, it sets\n> and then shows). We still want at least one .\n>\n> * Otherwise, it is a request to set the timestamp, so is\n> required, and we want at least one .\n>\n> Not the best way to design an end-user UI, but this is merely a test\n> helper, so let's not complain too much ;-).\n>\nI completely agree, the original logic was very opaque. By removing\nthe initial `(!get && !verbose && argc < 2)` check, we can rely\nentirely on the `timespec_arg()` check to validate the ``\nargument, and then simply use `if (i == argc)` at the end to ensure\n`` arguments were provided. I will clean this flow up\nsignificantly in v2.\n\n> > if (timespec_arg(argv[i], &set_time, &set_eq)) {\n> > ++i;\n> > } else {\n> > if (get == 0 && verbose == 0) {\n> > fprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n> > + usage_with_options(chmtime_usage, options);\n> > }\n> > }\n>\n> This is largely the fault of the original, but I wonder if we can\n> avoid the \"without -v/-g, timespec is required\" logic in two\n> separate places like this (and the earlier one that puzzled me)?\n>\n>\n> > if (i == argc)\n> > + usage_with_options(chmtime_usage, options);\n>\n> And we want at least one in any case.\n>\n> > for (; i < argc; i++) {\n> > struct stat sb;\n>\n> OK. We will iterate over the arguments given.\n>\n> > @@ -110,7 +110,7 @@ int cmd__chmtime(int argc, const char **argv)\n> >\n> > if (stat(argv[i], &sb) < 0) {\n> > fprintf(stderr, \"Failed to stat %s: %s. Skipping\\n\",\n> > - argv[i], strerror(errno));\n> > + argv[i], strerror(errno));\n> > continue;\n> > }\n>\n> This fixes a whitespace breakage, which is an unrelated change, but\n> I'll let it pass.\n>\n> > @@ -150,14 +150,10 @@ int cmd__chmtime(int argc, const char **argv)\n> > }\n> > #endif\n> > fprintf(stderr, \"Failed to modify time on %s: %s\\n\",\n> > - argv[i], strerror(errno));\n> > + argv[i], strerror(errno));\n> > return 1;\n> > }\n> > }\n>\n> Likewise.\n>\n> >\n> > return 0;\n> > -\n> > -usage:\n> > - fprintf(stderr, \"usage: %s %s\\n\", argv[0], usage_str);\n> > - return 1;\n> > }\n>\n> We exit via usage_with_options() with 129 now, but used to exit(1)\n> upon a command line error.\n>\nwhich is a nice bonus of migrating to the parse-options API.\n\n> Looking good. Thanks.\n\nThanks\nShashank S Mogaveera\n\n"},{"id":"556730","messageId":"CANHmUQRO1z1hL-L-To3_5n77P9V=GQAhN5cJ8BdDewP9sNqEYA@mail.gmail.com","threadId":"66679","inReplyTo":"xmqqv76nj7ys.fsf@gitster.g","subject":"Re: [PATCH] test-chmtime: use parse_options() to parse command-line flags","fromName":"Shashank S","fromEmail":"shashu8660@gmail.com","sentAt":"2026-10-29T05:54:42Z","receivedAt":"2026-10-29T05:54:42Z","isPatch":true,"sender":{"key":"shashu8660@gmail.com","avatar":null},"body":"On Wed, 28 Oct 2026 at 00:18, Junio C Hamano wrote:\n>\n> \"Shashank S via GitGitGadget\" writes:\n>\n> >\n> > t/helper/test-chmtime.c | 46 +++++++++++++++++++----------------------\n> > 1 file changed, 21 insertions(+), 25 deletions(-)\n>\n> Forgot to mention one thing. Do we have any tests to cover the\n> behaviour of this test helper? If not, should we add some (not a\n> rhetorical question)?\n>\n> Also, one thing I missed is that the original ignored --verbose\n> when --get was given, since they contradict with each other.\n> Shouldn't we make them explicitly incompatible with each other,\n> perhaps before where we call timespec_arg()?\n>\nThis is a great catch. While `test-chmtime` is heavily utilized in\nscripts like `t5304-prune.sh`, there are no tests covering the\nhelper's actual argument parsing.\n\nIn my v2, I will add `die_for_incompatible_opt2()` immediately after\n`parse_options()`, and I will include a second patch in the series\nthat adds a dedicated `t/t00xx-test-chmtime.sh` script to explicitly\ntest the `--get`/`--verbose` incompatibility and the standard usage.\n\nThanks again for the excellent guidance\n\nShashank s mogaveera\n\n"},{"id":"556862","messageId":"pull.2280.v2.git.1793350341743.gitgitgadget@gmail.com","threadId":"66679","inReplyTo":"pull.2280.git.1793099688076.gitgitgadget@gmail.com","subject":"[PATCH v2] test-chmtime: use parse_options() to parse command-line flags","fromName":"Shashank S via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-10-30T08:52:21Z","receivedAt":"2026-10-30T08:52:21Z","isPatch":true,"sender":{"key":"shashu8660@gmail.com","avatar":null},"body":"From: Shashank S Mogaveera \n\ncmd__chmtime() currently parses the '-v'/'--verbose' and '-g'/'--get'\nflags manually using strcmp() checks and manual index tracking.\n\nMigrate cmd__chmtime() to use parse_options() and usage_with_options()\ninstead. Pass PARSE_OPT_KEEP_UNKNOWN_OPT so that negative relative time\noffsets (such as '-86400') are not rejected as unknown short options\nand remain in argv for timespec_arg() to parse.\n\nAdditionally, strictly prevent combining mutually exclusive options\n('--get' and '--verbose') by utilizing die_for_incompatible_opt2(),\nand introduce a new test script to verify both this restriction and\nthe standard usage.\n\nSigned-off-by: Shashank S Mogaveera \n---\n [Outreachy]test-chmtime: use parse_options() to parse command-line flags\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2280%2Fshashu8660%2Foutreachy3-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2280/shashu8660/outreachy3-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/2280\n\nRange-diff vs v1:\n\n 1: 4f702e3f0b ! 1: 43c96de13e test-chmtime: use parse_options() to parse command-line flags\n @@ Commit message\n cmd__chmtime() currently parses the '-v'/'--verbose' and '-g'/'--get'\n flags manually using strcmp() checks and manual index tracking.\n \n - Refactor cmd__chmtime() to use parse_options() and usage_with_options()\n + Migrate cmd__chmtime() to use parse_options() and usage_with_options()\n instead. Pass PARSE_OPT_KEEP_UNKNOWN_OPT so that negative relative time\n offsets (such as '-86400') are not rejected as unknown short options\n and remain in argv for timespec_arg() to parse.\n \n + Additionally, strictly prevent combining mutually exclusive options\n + ('--get' and '--verbose') by utilizing die_for_incompatible_opt2(),\n + and introduce a new test script to verify both this restriction and\n + the standard usage.\n +\n Signed-off-by: Shashank S Mogaveera \n \n ## t/helper/test-chmtime.c ##\n @@\n + *\n + * The mtime can be changed to an absolute value:\n + *\n +- *\ttest-tool chmtime = file...\n ++ * test-tool chmtime = file...\n + *\n + * Relative to the current time as returned by time(3):\n + *\n +- *\ttest-tool chmtime =+ (or =-) file...\n ++ * test-tool chmtime =+ (or =-) file...\n + *\n + * Or relative to the current mtime of the file:\n + *\n +- *\ttest-tool chmtime file...\n +- *\ttest-tool chmtime + (or -) file...\n ++ * test-tool chmtime file...\n ++ * test-tool chmtime + (or -) file...\n + *\n + * Examples:\n + *\n + * To print the mtime and the file name use --verbose and set\n + * the file mtime offset to 0:\n + *\n +- *\ttest-tool chmtime -v +0 file\n ++ * test-tool chmtime -v +0 file\n + *\n + * To print only the mtime use --get:\n + *\n +- *\ttest-tool chmtime --get file\n ++ * test-tool chmtime --get file\n + *\n + * To set the mtime to current time:\n + *\n +- *\ttest-tool chmtime =+0 file\n ++ * test-tool chmtime =+0 file\n + *\n + * To set the file mtime offset to +1 and print the new value:\n + *\n +- *\ttest-tool chmtime --get +1 file\n ++ * test-tool chmtime --get +1 file\n + *\n */\n #include \"test-tool.h\"\n #include \"git-compat-util.h\"\n @@ t/helper/test-chmtime.c\n -static const char usage_str[] =\n -\t\"(-v|--verbose|-g|--get) (+|=|=+|=-|-) ...\";\n +static const char * const chmtime_usage[] = {\n -+\t\"test-tool chmtime (-v|--verbose|-g|--get) (+|=|=+|=-|-) ...\",\n ++\t\"test-tool chmtime [] (+|=|=+|=-|-) ...\",\n +\tNULL\n +};\n \n static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n {\n -@@ t/helper/test-chmtime.c: static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n - \n - int cmd__chmtime(int argc, const char **argv)\n - {\n --\tstatic int verbose;\n --\tstatic int get;\n -+\tint verbose = 0;\n -+\tint get = 0;\n +@@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)\n + \tstatic int verbose;\n + \tstatic int get;\n \n -\tint i = 1;\n +\tint i = 0;\n @@ t/helper/test-chmtime.c: static int timespec_arg(const char *arg, long int *set_\n -\tif (i == argc) {\n -\t\tgoto usage;\n -\t}\n -+\tif (argc < 1 || (!get && !verbose && argc < 2))\n ++\tdie_for_incompatible_opt2(get, \"--get\", verbose, \"--verbose\");\n ++\n ++\tif (argc < 1)\n +\t\tusage_with_options(chmtime_usage, options);\n \n \tif (timespec_arg(argv[i], &set_time, &set_eq)) {\n - \t\t++i;\n - \t} else {\n - \t\tif (get == 0 && verbose == 0) {\n - \t\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n +-\t\t++i;\n +-\t} else {\n +-\t\tif (get == 0 && verbose == 0) {\n +-\t\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n -\t\t\tgoto usage;\n -+\t\t\tusage_with_options(chmtime_usage, options);\n - \t\t}\n +-\t\t}\n ++\t\ti++;\n ++\t} else if (!get && !verbose) {\n ++\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n ++\t\tusage_with_options(chmtime_usage, options);\n \t}\n \n \tif (i == argc)\n @@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)\n \t\t\tcontinue;\n \t\t}\n \n + #ifdef GIT_WINDOWS_NATIVE\n + \t\tif (!(sb.st_mode & S_IWUSR) &&\n +-\t\t\t\tchmod(argv[i], sb.st_mode | S_IWUSR)) {\n ++\t\t chmod(argv[i], sb.st_mode | S_IWUSR)) {\n + \t\t\tfprintf(stderr, \"Could not make user-writable %s: %s\",\n + \t\t\t\targv[i], strerror(errno));\n + \t\t\treturn 1;\n +@@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)\n + \t\tutb.actime = sb.st_atime;\n + \t\tutb.modtime = set_eq ? set_time : sb.st_mtime + set_time;\n + \n +-\t\tmtime = utb.modtime < 0 ? 0: utb.modtime;\n ++\t\tmtime = utb.modtime < 0 ? 0 : utb.modtime;\n + \t\tif (get) {\n + \t\t\tprintf(\"%\"PRIuMAX\"\\n\", mtime);\n + \t\t} else if (verbose) {\n @@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)\n \t\t\t}\n #endif\n @@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)\n -\tfprintf(stderr, \"usage: %s %s\\n\", argv[0], usage_str);\n -\treturn 1;\n }\n +\n + ## t/meson.build ##\n +@@ t/meson.build: integration_tests = [\n + 't0071-sort.sh',\n + 't0080-unit-test-output.sh',\n + 't0081-find-pack.sh',\n ++ 't0082-test-chmtime.sh',\n + 't0090-cache-tree.sh',\n + 't0091-bugreport.sh',\n + 't0092-diagnose.sh',\n +\n + ## t/t0082-test-chmtime.sh (new) ##\n +@@\n ++#!/bin/sh\n ++\n ++test_description='tests for test-tool chmtime'\n ++\n ++TEST_PASSES_SANITIZE_LEAK=true\n ++. ./test-lib.sh\n ++\n ++test_expect_success 'setup' '\n ++\techo hello >file\n ++'\n ++\n ++test_expect_success 'mutually exclusive options are rejected' '\n ++\ttest_must_fail test-tool chmtime --get --verbose +0 file 2>err &&\n ++\ttest_grep \"cannot be used together\" err\n ++'\n ++\n ++test_expect_success 'missing timespec without -g or -v is rejected' '\n ++\ttest_must_fail test-tool chmtime file 2>err &&\n ++\ttest_grep \"usage: test-tool chmtime\" err\n ++'\n ++\n ++test_done\n\n\n t/helper/test-chmtime.c | 72 +++++++++++++++++++----------------------\n t/meson.build | 1 +\n t/t0082-test-chmtime.sh | 22 +++++++++++++\n 3 files changed, 57 insertions(+), 38 deletions(-)\n create mode 100755 t/t0082-test-chmtime.sh\n\ndiff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c\nindex 0e5538833a..cfcbca75a4 100644\n--- a/t/helper/test-chmtime.c\n+++ b/t/helper/test-chmtime.c\n@@ -5,43 +5,46 @@\n *\n * The mtime can be changed to an absolute value:\n *\n- *\ttest-tool chmtime = file...\n+ * test-tool chmtime = file...\n *\n * Relative to the current time as returned by time(3):\n *\n- *\ttest-tool chmtime =+ (or =-) file...\n+ * test-tool chmtime =+ (or =-) file...\n *\n * Or relative to the current mtime of the file:\n *\n- *\ttest-tool chmtime file...\n- *\ttest-tool chmtime + (or -) file...\n+ * test-tool chmtime file...\n+ * test-tool chmtime + (or -) file...\n *\n * Examples:\n *\n * To print the mtime and the file name use --verbose and set\n * the file mtime offset to 0:\n *\n- *\ttest-tool chmtime -v +0 file\n+ * test-tool chmtime -v +0 file\n *\n * To print only the mtime use --get:\n *\n- *\ttest-tool chmtime --get file\n+ * test-tool chmtime --get file\n *\n * To set the mtime to current time:\n *\n- *\ttest-tool chmtime =+0 file\n+ * test-tool chmtime =+0 file\n *\n * To set the file mtime offset to +1 and print the new value:\n *\n- *\ttest-tool chmtime --get +1 file\n+ * test-tool chmtime --get +1 file\n *\n */\n #include \"test-tool.h\"\n #include \"git-compat-util.h\"\n+#include \"parse-options.h\"\n #include \n \n-static const char usage_str[] =\n-\t\"(-v|--verbose|-g|--get) (+|=|=+|=-|-) ...\";\n+static const char * const chmtime_usage[] = {\n+\t\"test-tool chmtime [] (+|=|=+|=-|-) ...\",\n+\tNULL\n+};\n \n static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n {\n@@ -71,37 +74,34 @@ int cmd__chmtime(int argc, const char **argv)\n \tstatic int verbose;\n \tstatic int get;\n \n-\tint i = 1;\n+\tint i = 0;\n \t/* no mtime change by default */\n \tint set_eq = 0;\n \tlong int set_time = 0;\n \n-\tif (argc < 3)\n-\t\tgoto usage;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL('v', \"verbose\", &verbose, \"print the mtime and the file name\"),\n+\t\tOPT_BOOL('g', \"get\", &get, \"print only the mtime\"),\n+\t\tOPT_END()\n+\t};\n \n-\tif (strcmp(argv[i], \"--get\") == 0 || strcmp(argv[i], \"-g\") == 0) {\n-\t\tget = 1;\n-\t\t++i;\n-\t} else if (strcmp(argv[i], \"--verbose\") == 0 || strcmp(argv[i], \"-v\") == 0) {\n-\t\tverbose = 1;\n-\t\t++i;\n-\t}\n+\targc = parse_options(argc, argv, NULL, options, chmtime_usage,\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n \n-\tif (i == argc) {\n-\t\tgoto usage;\n-\t}\n+\tdie_for_incompatible_opt2(get, \"--get\", verbose, \"--verbose\");\n+\n+\tif (argc < 1)\n+\t\tusage_with_options(chmtime_usage, options);\n \n \tif (timespec_arg(argv[i], &set_time, &set_eq)) {\n-\t\t++i;\n-\t} else {\n-\t\tif (get == 0 && verbose == 0) {\n-\t\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n-\t\t\tgoto usage;\n-\t\t}\n+\t\ti++;\n+\t} else if (!get && !verbose) {\n+\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n+\t\tusage_with_options(chmtime_usage, options);\n \t}\n \n \tif (i == argc)\n-\t\tgoto usage;\n+\t\tusage_with_options(chmtime_usage, options);\n \n \tfor (; i < argc; i++) {\n \t\tstruct stat sb;\n@@ -110,13 +110,13 @@ int cmd__chmtime(int argc, const char **argv)\n \n \t\tif (stat(argv[i], &sb) < 0) {\n \t\t\tfprintf(stderr, \"Failed to stat %s: %s. Skipping\\n\",\n-\t\t\t argv[i], strerror(errno));\n+\t\t\t\targv[i], strerror(errno));\n \t\t\tcontinue;\n \t\t}\n \n #ifdef GIT_WINDOWS_NATIVE\n \t\tif (!(sb.st_mode & S_IWUSR) &&\n-\t\t\t\tchmod(argv[i], sb.st_mode | S_IWUSR)) {\n+\t\t chmod(argv[i], sb.st_mode | S_IWUSR)) {\n \t\t\tfprintf(stderr, \"Could not make user-writable %s: %s\",\n \t\t\t\targv[i], strerror(errno));\n \t\t\treturn 1;\n@@ -126,7 +126,7 @@ int cmd__chmtime(int argc, const char **argv)\n \t\tutb.actime = sb.st_atime;\n \t\tutb.modtime = set_eq ? set_time : sb.st_mtime + set_time;\n \n-\t\tmtime = utb.modtime < 0 ? 0: utb.modtime;\n+\t\tmtime = utb.modtime < 0 ? 0 : utb.modtime;\n \t\tif (get) {\n \t\t\tprintf(\"%\"PRIuMAX\"\\n\", mtime);\n \t\t} else if (verbose) {\n@@ -150,14 +150,10 @@ int cmd__chmtime(int argc, const char **argv)\n \t\t\t}\n #endif\n \t\t\tfprintf(stderr, \"Failed to modify time on %s: %s\\n\",\n-\t\t\t argv[i], strerror(errno));\n+\t\t\t\targv[i], strerror(errno));\n \t\t\treturn 1;\n \t\t}\n \t}\n \n \treturn 0;\n-\n-usage:\n-\tfprintf(stderr, \"usage: %s %s\\n\", argv[0], usage_str);\n-\treturn 1;\n }\ndiff --git a/t/meson.build b/t/meson.build\nindex 11b3752f00..004bda52c1 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -128,6 +128,7 @@ integration_tests = [\n 't0071-sort.sh',\n 't0080-unit-test-output.sh',\n 't0081-find-pack.sh',\n+ 't0082-test-chmtime.sh',\n 't0090-cache-tree.sh',\n 't0091-bugreport.sh',\n 't0092-diagnose.sh',\ndiff --git a/t/t0082-test-chmtime.sh b/t/t0082-test-chmtime.sh\nnew file mode 100755\nindex 0000000000..277df4ff1a\n--- /dev/null\n+++ b/t/t0082-test-chmtime.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+\n+test_description='tests for test-tool chmtime'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo hello >file\n+'\n+\n+test_expect_success 'mutually exclusive options are rejected' '\n+\ttest_must_fail test-tool chmtime --get --verbose +0 file 2>err &&\n+\ttest_grep \"cannot be used together\" err\n+'\n+\n+test_expect_success 'missing timespec without -g or -v is rejected' '\n+\ttest_must_fail test-tool chmtime file 2>err &&\n+\ttest_grep \"usage: test-tool chmtime\" err\n+'\n+\n+test_done\n\nbase-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e\n-- \ngitgitgadget\n\n"},{"id":"557112","messageId":"20261102024623.24897-1-herroyalpriestess@gmail.com","threadId":"66679","inReplyTo":"pull.2280.v2.git.1793350341743.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] test-chmtime: use parse_options() to parse command-line flags","fromName":"Cynthia Uche","fromEmail":"herroyalpriestess@gmail.com","sentAt":"2026-11-02T02:46:23Z","receivedAt":"2026-11-02T02:46:23Z","isPatch":true,"sender":{"key":"herroyalpriestess@gmail.com","avatar":null},"body":"Hi Shashank,\n\nI built v2 (43c96de13e) with NO_RUST=1 and compared it with a build\nof the base commit (7e34d8174d). t0082 passes 3 of 3. Thanks for\ntaking Junio's three suggestions from v1.\n\n> instead. Pass PARSE_OPT_KEEP_UNKNOWN_OPT so that negative relative time\n> offsets (such as '-86400') are not rejected as unknown short options\n> and remain in argv for timespec_arg() to parse.\n\nNo test covers this. With that argument set to 0, t0082 still passed\n3 of 3, while \"test-tool chmtime --get -86400 file\" failed with\n\"unknown switch `8'\" (exit 129). Could you add a test like this one?\nIt passes with v2 and fails without the flag:\n\n test_expect_success 'negative offset is not taken as an option' '\n \ttest-tool chmtime =1000000000 file &&\n \ttest-tool chmtime --get -86400 file >actual &&\n \techo 999913600 >expect &&\n \ttest_cmp expect actual\n '\n\nCompared with the base build, I also saw three changes that the\ncommit message does not mention:\n\n - \"+0 --get f\": base tried to stat \"--get\"; v2 accepts the flag\n after the timespec and prints the mtime.\n - \"-g -v f\": base tried to stat \"-v\" [and still printed the mtime]\n and exited 0; v2 dies (exit 128).\n - usage errors exit with 129 instead of 1.\n\nCould you mention them in the commit message, or let me know if any\nof them is not intended?\n\nCynthia\n\n"},{"id":"557114","messageId":"CANHmUQS15GGDOXm-WZ9M-UyKf2N0K+VixowL_heNKW0vjE0vJw@mail.gmail.com","threadId":"66679","inReplyTo":"20261102024623.24897-1-herroyalpriestess@gmail.com","subject":"Re: [PATCH v2] test-chmtime: use parse_options() to parse command-line flags","fromName":"Shashank S","fromEmail":"shashu8660@gmail.com","sentAt":"2026-11-02T04:56:04Z","receivedAt":"2026-11-02T04:56:04Z","isPatch":true,"sender":{"key":"shashu8660@gmail.com","avatar":null},"body":"On Mon, 2 Nov 2026 at 08:17, Cynthia Uche wrote:\n\nHi Cynthia,\n\nThank you so much for taking the time to test and review this\n\n>\n> Hi Shashank\n> No test covers this. With that argument set to 0, t0082 still passed\n> 3 of 3, while \"test-tool chmtime --get -86400 file\" failed with\n> \"unknown switch `8'\" (exit 129). Could you add a test like this one?\n> It passes with v2 and fails without the flag:\n>\n> test_expect_success 'negative offset is not taken as an option' '\n> test-tool chmtime =1000000000 file &&\n> test-tool chmtime --get -86400 file >actual &&\n> echo 999913600 >expect &&\n> test_cmp expect actual\n> '\n>\nThis is an excellent catch. Adding a dedicated test to prevent a\nregression here is a great idea, and I appreciate you providing the\nsnippet. I will include this test in the v3 iteration.\n\n> Compared with the base build, I also saw three changes that the\n> commit message does not mention:\n>\n> - \"+0 --get f\": base tried to stat \"--get\"; v2 accepts the flag\n> after the timespec and prints the mtime.\n> - \"-g -v f\": base tried to stat \"-v\" [and still printed the mtime]\n> and exited 0; v2 dies (exit 128).\n> - usage errors exit with 129 instead of 1.\n>\n> Could you mention them in the commit message, or let me know if any\n> of them is not intended?\n>\nAll of these behavioral shifts are intended! Since `parse_options()`\nhandles arguments independent of their position and uses standard\nerror codes (129 for usage, 128 for `die`), the tool now behaves much\nmore like a standard Git command. You are completely right that I\nshould document these side-effects explicitly in the commit message.\n\nI will add a breakdown of these behavioral improvements to the commit\nmessage in v3. Thanks again for the meticulous review!\n\nShashank S Mogaveera\n\n"},{"id":"557209","messageId":"pull.2280.v3.git.1793639275978.gitgitgadget@gmail.com","threadId":"66679","inReplyTo":"pull.2280.git.1793099688076.gitgitgadget@gmail.com","subject":"[PATCH v3] test-chmtime: use parse_options() to parse command-line flags","fromName":"Shashank S via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-11-02T17:07:55Z","receivedAt":"2026-11-02T17:07:55Z","isPatch":true,"sender":{"key":"shashu8660@gmail.com","avatar":null},"body":"From: Shashank S Mogaveera \n\ncmd__chmtime() currently parses the '-v'/'--verbose' and '-g'/'--get'\nflags manually using strcmp() checks and manual index tracking.\n\nMigrate cmd__chmtime() to use parse_options() and usage_with_options()\ninstead. Pass PARSE_OPT_KEEP_UNKNOWN_OPT so that negative relative time\noffsets (such as '-86400') are not rejected as unknown short options\nand remain in argv for timespec_arg() to parse.\n\nAdditionally, strictly prevent combining mutually exclusive options\n('--get' and '--verbose') by utilizing die_for_incompatible_opt2(),\nand introduce a new test script to verify both this restriction and\nthe standard usage.\n\nThis migration also introduces the following intended behavioral\nimprovements over the original manual parsing:\n- Position-independent flags: Flags can now be safely passed after the\n timespec (e.g., `+0 --get file` now works, whereas it previously tried\n to stat a file literally named `--get`).\n- Incompatible flags strictly abort: Passing both `-g` and `-v` now\n triggers a fatal error (exit 128), whereas it previously tried to stat\n `-v` and exited 0.\n- Standardized error codes: Usage errors now properly exit with 129\n instead of 1.\n\nSigned-off-by: Shashank S Mogaveera \n---\n [Outreachy]test-chmtime: use parse_options() to parse command-line flags\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2280%2Fshashu8660%2Foutreachy3-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2280/shashu8660/outreachy3-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/2280\n\nRange-diff vs v2:\n\n 1: 43c96de13e ! 1: 512b17b39a test-chmtime: use parse_options() to parse command-line flags\n @@ Commit message\n and introduce a new test script to verify both this restriction and\n the standard usage.\n \n + This migration also introduces the following intended behavioral\n + improvements over the original manual parsing:\n + - Position-independent flags: Flags can now be safely passed after the\n + timespec (e.g., `+0 --get file` now works, whereas it previously tried\n + to stat a file literally named `--get`).\n + - Incompatible flags strictly abort: Passing both `-g` and `-v` now\n + triggers a fatal error (exit 128), whereas it previously tried to stat\n + `-v` and exited 0.\n + - Standardized error codes: Usage errors now properly exit with 129\n + instead of 1.\n +\n Signed-off-by: Shashank S Mogaveera \n \n ## t/helper/test-chmtime.c ##\n @@ t/t0082-test-chmtime.sh (new)\n +\ttest_grep \"usage: test-tool chmtime\" err\n +'\n +\n ++test_expect_success 'negative offset is not taken as an option' '\n ++\ttest-tool chmtime =1000000000 file &&\n ++\ttest-tool chmtime --get -86400 file >actual &&\n ++\techo 999913600 >expect &&\n ++\ttest_cmp expect actual\n ++'\n ++\n +test_done\n\n\n t/helper/test-chmtime.c | 72 +++++++++++++++++++----------------------\n t/meson.build | 1 +\n t/t0082-test-chmtime.sh | 29 +++++++++++++++++\n 3 files changed, 64 insertions(+), 38 deletions(-)\n create mode 100755 t/t0082-test-chmtime.sh\n\ndiff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c\nindex 0e5538833a..cfcbca75a4 100644\n--- a/t/helper/test-chmtime.c\n+++ b/t/helper/test-chmtime.c\n@@ -5,43 +5,46 @@\n *\n * The mtime can be changed to an absolute value:\n *\n- *\ttest-tool chmtime = file...\n+ * test-tool chmtime = file...\n *\n * Relative to the current time as returned by time(3):\n *\n- *\ttest-tool chmtime =+ (or =-) file...\n+ * test-tool chmtime =+ (or =-) file...\n *\n * Or relative to the current mtime of the file:\n *\n- *\ttest-tool chmtime file...\n- *\ttest-tool chmtime + (or -) file...\n+ * test-tool chmtime file...\n+ * test-tool chmtime + (or -) file...\n *\n * Examples:\n *\n * To print the mtime and the file name use --verbose and set\n * the file mtime offset to 0:\n *\n- *\ttest-tool chmtime -v +0 file\n+ * test-tool chmtime -v +0 file\n *\n * To print only the mtime use --get:\n *\n- *\ttest-tool chmtime --get file\n+ * test-tool chmtime --get file\n *\n * To set the mtime to current time:\n *\n- *\ttest-tool chmtime =+0 file\n+ * test-tool chmtime =+0 file\n *\n * To set the file mtime offset to +1 and print the new value:\n *\n- *\ttest-tool chmtime --get +1 file\n+ * test-tool chmtime --get +1 file\n *\n */\n #include \"test-tool.h\"\n #include \"git-compat-util.h\"\n+#include \"parse-options.h\"\n #include \n \n-static const char usage_str[] =\n-\t\"(-v|--verbose|-g|--get) (+|=|=+|=-|-) ...\";\n+static const char * const chmtime_usage[] = {\n+\t\"test-tool chmtime [] (+|=|=+|=-|-) ...\",\n+\tNULL\n+};\n \n static int timespec_arg(const char *arg, long int *set_time, int *set_eq)\n {\n@@ -71,37 +74,34 @@ int cmd__chmtime(int argc, const char **argv)\n \tstatic int verbose;\n \tstatic int get;\n \n-\tint i = 1;\n+\tint i = 0;\n \t/* no mtime change by default */\n \tint set_eq = 0;\n \tlong int set_time = 0;\n \n-\tif (argc < 3)\n-\t\tgoto usage;\n+\tstruct option options[] = {\n+\t\tOPT_BOOL('v', \"verbose\", &verbose, \"print the mtime and the file name\"),\n+\t\tOPT_BOOL('g', \"get\", &get, \"print only the mtime\"),\n+\t\tOPT_END()\n+\t};\n \n-\tif (strcmp(argv[i], \"--get\") == 0 || strcmp(argv[i], \"-g\") == 0) {\n-\t\tget = 1;\n-\t\t++i;\n-\t} else if (strcmp(argv[i], \"--verbose\") == 0 || strcmp(argv[i], \"-v\") == 0) {\n-\t\tverbose = 1;\n-\t\t++i;\n-\t}\n+\targc = parse_options(argc, argv, NULL, options, chmtime_usage,\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT);\n \n-\tif (i == argc) {\n-\t\tgoto usage;\n-\t}\n+\tdie_for_incompatible_opt2(get, \"--get\", verbose, \"--verbose\");\n+\n+\tif (argc < 1)\n+\t\tusage_with_options(chmtime_usage, options);\n \n \tif (timespec_arg(argv[i], &set_time, &set_eq)) {\n-\t\t++i;\n-\t} else {\n-\t\tif (get == 0 && verbose == 0) {\n-\t\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n-\t\t\tgoto usage;\n-\t\t}\n+\t\ti++;\n+\t} else if (!get && !verbose) {\n+\t\tfprintf(stderr, \"Not a base-10 integer: %s\\n\", argv[i] + 1);\n+\t\tusage_with_options(chmtime_usage, options);\n \t}\n \n \tif (i == argc)\n-\t\tgoto usage;\n+\t\tusage_with_options(chmtime_usage, options);\n \n \tfor (; i < argc; i++) {\n \t\tstruct stat sb;\n@@ -110,13 +110,13 @@ int cmd__chmtime(int argc, const char **argv)\n \n \t\tif (stat(argv[i], &sb) < 0) {\n \t\t\tfprintf(stderr, \"Failed to stat %s: %s. Skipping\\n\",\n-\t\t\t argv[i], strerror(errno));\n+\t\t\t\targv[i], strerror(errno));\n \t\t\tcontinue;\n \t\t}\n \n #ifdef GIT_WINDOWS_NATIVE\n \t\tif (!(sb.st_mode & S_IWUSR) &&\n-\t\t\t\tchmod(argv[i], sb.st_mode | S_IWUSR)) {\n+\t\t chmod(argv[i], sb.st_mode | S_IWUSR)) {\n \t\t\tfprintf(stderr, \"Could not make user-writable %s: %s\",\n \t\t\t\targv[i], strerror(errno));\n \t\t\treturn 1;\n@@ -126,7 +126,7 @@ int cmd__chmtime(int argc, const char **argv)\n \t\tutb.actime = sb.st_atime;\n \t\tutb.modtime = set_eq ? set_time : sb.st_mtime + set_time;\n \n-\t\tmtime = utb.modtime < 0 ? 0: utb.modtime;\n+\t\tmtime = utb.modtime < 0 ? 0 : utb.modtime;\n \t\tif (get) {\n \t\t\tprintf(\"%\"PRIuMAX\"\\n\", mtime);\n \t\t} else if (verbose) {\n@@ -150,14 +150,10 @@ int cmd__chmtime(int argc, const char **argv)\n \t\t\t}\n #endif\n \t\t\tfprintf(stderr, \"Failed to modify time on %s: %s\\n\",\n-\t\t\t argv[i], strerror(errno));\n+\t\t\t\targv[i], strerror(errno));\n \t\t\treturn 1;\n \t\t}\n \t}\n \n \treturn 0;\n-\n-usage:\n-\tfprintf(stderr, \"usage: %s %s\\n\", argv[0], usage_str);\n-\treturn 1;\n }\ndiff --git a/t/meson.build b/t/meson.build\nindex 11b3752f00..004bda52c1 100644\n--- a/t/meson.build\n+++ b/t/meson.build\n@@ -128,6 +128,7 @@ integration_tests = [\n 't0071-sort.sh',\n 't0080-unit-test-output.sh',\n 't0081-find-pack.sh',\n+ 't0082-test-chmtime.sh',\n 't0090-cache-tree.sh',\n 't0091-bugreport.sh',\n 't0092-diagnose.sh',\ndiff --git a/t/t0082-test-chmtime.sh b/t/t0082-test-chmtime.sh\nnew file mode 100755\nindex 0000000000..f1a9d6c466\n--- /dev/null\n+++ b/t/t0082-test-chmtime.sh\n@@ -0,0 +1,29 @@\n+#!/bin/sh\n+\n+test_description='tests for test-tool chmtime'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo hello >file\n+'\n+\n+test_expect_success 'mutually exclusive options are rejected' '\n+\ttest_must_fail test-tool chmtime --get --verbose +0 file 2>err &&\n+\ttest_grep \"cannot be used together\" err\n+'\n+\n+test_expect_success 'missing timespec without -g or -v is rejected' '\n+\ttest_must_fail test-tool chmtime file 2>err &&\n+\ttest_grep \"usage: test-tool chmtime\" err\n+'\n+\n+test_expect_success 'negative offset is not taken as an option' '\n+\ttest-tool chmtime =1000000000 file &&\n+\ttest-tool chmtime --get -86400 file >actual &&\n+\techo 999913600 >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n\nbase-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e\n-- \ngitgitgadget\n\n"}]}