From: Shashank S Date: Thu, 29 Oct 2026 05:52:35 GMT Subject: Re: [PATCH] test-chmtime: use parse_options() to parse command-line flags Message-ID: In-Reply-To: On Wed, 28 Oct 2026 at 00:08, Junio C Hamano wrote: Thank you for the detailed review and for pointing these out! > > and remain in argv for timespec_arg() to parse. > > I somehow find the verb "refactor" used to describe a change like > this slightly awkward, but it is excellent to reminder readers why > it is necessary to use PARSE_OPT_KEEP_UNKNOWN_OPT. > Understood! In my v2, I will change the commit message to use "update" instead of "refactor". > > t/helper/test-chmtime.c | 46 +++++++++++++++++++---------------------- > > 1 file changed, 21 insertions(+), 25 deletions(-) > > > > diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c > > index 0e5538833a..dc6281b1d6 100644 > > --- a/t/helper/test-chmtime.c > > +++ b/t/helper/test-chmtime.c > > @@ -38,10 +38,13 @@ > > */ > > #include "test-tool.h" > > #include "git-compat-util.h" > > +#include "parse-options.h" > > #include > > > > -static const char usage_str[] = > > - "(-v|--verbose|-g|--get) (+|=|=+|=-|-) ..."; > > +static const char * const chmtime_usage[] = { > > + "test-tool chmtime (-v|--verbose|-g|--get) (+|=|=+|=-|-) ...", > > + NULL > > +}; > > Is it necessary to spell out the options like verbose and get, once > you switch to the parse-options API? What does "test-tool chmtime -h" > say with this patch applied? I think it is common to say [] > instead and let the list of options produced by the parse-options API > to take it from there. > You are completely right. Hardcoding the flags in the string is redundant now that `parse_options()` handles the `-h` output. I will update the usage string to `[]` in v2. > > static int timespec_arg(const char *arg, long int *set_time, int *set_eq) > > { > > @@ -68,40 +71,37 @@ static int timespec_arg(const char *arg, long int *set_time, int *set_eq) > > > > int cmd__chmtime(int argc, const char **argv) > > { > > - static int verbose; > > - static int get; > > + int verbose = 0; > > + int get = 0; > > There is no reason why these two need to be static, but as a > "main()" function for a run-once-and-exit program, there is no > reason why they should not be static, either. With this change, > you are forced to explicitly initialize them to 0. > That is a great point about the automatic initialization of static variables. Since it isn't strictly necessary to change them for `parse_options()` to work, I will revert this specific change in v2 to keep the diff as small and focused as possible. > > - int i = 1; > > /* no mtime change by default */ > > int set_eq = 0; > > long int set_time = 0; > > > > - if (argc < 3) > > - goto usage; > > > > - if (strcmp(argv[i], "--get") == 0 || strcmp(argv[i], "-g") == 0) { > > - get = 1; > > - ++i; > > - } else if (strcmp(argv[i], "--verbose") == 0 || strcmp(argv[i], "-v") == 0) { > > - verbose = 1; > > - ++i; > > - } > > > > - if (i == argc) { > > - goto usage; > > - } > > > > if (timespec_arg(argv[i], &set_time, &set_eq)) { > > ++i; > > } else { > > if (get == 0 && verbose == 0) { > > fprintf(stderr, "Not a base-10 integer: %s\n", argv[i] + 1); > > - goto usage; > > } > > } > > > > if (i == argc) > > - goto usage; > > > > for (; i < argc; i++) { > > struct stat sb; > > I agree that the original was horrible in that it did not allow -v > to come before -g etc. > > Now, let's read the updated code. > > > + int i = 0; > > > /* no mtime change by default */ > > int set_eq = 0; > > long int set_time = 0; > > > > + struct option options[] = { > > + OPT_BOOL('v', "verbose", &verbose, "print the mtime and the file name"), > > + OPT_BOOL('g', "get", &get, "print only the mtime"), > > + OPT_END() > > + }; > > > > + argc = parse_options(argc, argv, NULL, options, chmtime_usage, > > + PARSE_OPT_KEEP_UNKNOWN_OPT); > > > > + if (argc < 1 || (!get && !verbose && argc < 2)) > > + usage_with_options(chmtime_usage, options); > > I am not sure what is going on here. The command, when parsing out > the -g and -v options, expects to have and 1 or more > arguments. Why should it reject argc==2 when neither -g nor > -v is given, but allow argc==2 when -g and/or -v is? > > OK, the usage of this command is a bit opaque. The command works in > two ways. > > * When -v/-g is given, it is a request to show the timestamp of the > file, so becomes optional (i.e., when given, it sets > and then shows). We still want at least one . > > * Otherwise, it is a request to set the timestamp, so is > required, and we want at least one . > > Not the best way to design an end-user UI, but this is merely a test > helper, so let's not complain too much ;-). > I completely agree, the original logic was very opaque. By removing the initial `(!get && !verbose && argc < 2)` check, we can rely entirely on the `timespec_arg()` check to validate the `` argument, and then simply use `if (i == argc)` at the end to ensure `` arguments were provided. I will clean this flow up significantly in v2. > > if (timespec_arg(argv[i], &set_time, &set_eq)) { > > ++i; > > } else { > > if (get == 0 && verbose == 0) { > > fprintf(stderr, "Not a base-10 integer: %s\n", argv[i] + 1); > > + usage_with_options(chmtime_usage, options); > > } > > } > > This is largely the fault of the original, but I wonder if we can > avoid the "without -v/-g, timespec is required" logic in two > separate places like this (and the earlier one that puzzled me)? > > > > if (i == argc) > > + usage_with_options(chmtime_usage, options); > > And we want at least one in any case. > > > for (; i < argc; i++) { > > struct stat sb; > > OK. We will iterate over the arguments given. > > > @@ -110,7 +110,7 @@ int cmd__chmtime(int argc, const char **argv) > > > > if (stat(argv[i], &sb) < 0) { > > fprintf(stderr, "Failed to stat %s: %s. Skipping\n", > > - argv[i], strerror(errno)); > > + argv[i], strerror(errno)); > > continue; > > } > > This fixes a whitespace breakage, which is an unrelated change, but > I'll let it pass. > > > @@ -150,14 +150,10 @@ int cmd__chmtime(int argc, const char **argv) > > } > > #endif > > fprintf(stderr, "Failed to modify time on %s: %s\n", > > - argv[i], strerror(errno)); > > + argv[i], strerror(errno)); > > return 1; > > } > > } > > Likewise. > > > > > return 0; > > - > > -usage: > > - fprintf(stderr, "usage: %s %s\n", argv[0], usage_str); > > - return 1; > > } > > We exit via usage_with_options() with 129 now, but used to exit(1) > upon a command line error. > which is a nice bonus of migrating to the parse-options API. > Looking good. Thanks. Thanks Shashank S Mogaveera