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

[Junio C Hamano <gitster@pobox.com>]

"Shashank S via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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

Looking good.  Thanks.