Re: [PATCH] test-chmtime: use parse_options() to parse command-line flags
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 27, 2026, 18:38 UTC
- Message-ID
- <xmqqzevzj8f7.fsf@gitster.g>
- In-Reply-To
- <pull.2280.git.1793099688076.gitgitgadget@gmail.com>
"Shashank S via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 9 quoted lines
> From: Shashank S Mogaveera <shashu8660@gmail.com> > > cmd__chmtime() currently parses the '-v'/'--verbose' and '-g'/'--get' > flags manually using strcmp() checks and manual index tracking. > > Refactor cmd__chmtime() to use parse_options() and usage_with_options() > instead. Pass PARSE_OPT_KEEP_UNKNOWN_OPT so that negative relative time > offsets (such as '-86400') are not rejected as unknown short options > 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.
Show 20 quoted lines
> 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 <utime.h>
>
> -static const char usage_str[] =
> - "(-v|--verbose|-g|--get) (+|=|=+|=-|-)<seconds> <file>...";
> +static const char * const chmtime_usage[] = {
> + "test-tool chmtime (-v|--verbose|-g|--get) (+|=|=+|=-|-)<seconds> <file>...",
> + 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 [<options>] instead and let the list of options produced by the parse-options API to take it from there.
Show 10 quoted lines
> 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.
Show 34 quoted lines
> - 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;
Show 15 quoted lines
> /* 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 <seconds> and 1 or more <file> 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 <seconds> becomes optional (i.e., when given, it sets and then shows). We still want at least one <file>.
* Otherwise, it is a request to set the timestamp, so <seconds> is required, and we want at least one <file>.
Not the best way to design an end-user UI, but this is merely a test helper, so let's not complain too much ;-).
Show 8 quoted lines
> 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 <file> in any case.
> for (; i < argc; i++) {
> struct stat sb;OK. We will iterate over the <file> arguments given.
Show 8 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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.
Show 7 quoted lines
> > 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.
Looking good. Thanks.