Re: [PATCH] test-chmtime: use parse_options() to parse command-line flags

> > 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.
>

> >  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.
>

> >  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.
>

> > -     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 <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 ;-).
>

> >       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.
>
> > @@ -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.
>