Re: [PATCH v6] builtin/diff-files: convert ad hoc option parsing to parse-options API

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

Edson <edsonwasswa54@gmail.com> writes:

> -static const char diff_files_usage[] =
> -"git diff-files [-q] [-0 | -1 | -2 | -3 | -c | --cc] [<common-diff-options>] [<path>...]"
> -"\n"
> -COMMON_DIFF_OPTIONS_HELP;
> +static const char * const diff_files_usage[] = {
> +	"git diff-files [-q] [-0 | -1 | -2 | -3 | -c | --cc] [<common-diff-options>] [<path>...]",
> +	NULL
> +};

The loss of COMMON_DIFF_OPTIONS_HELP here gives us a regression in
the help output ("git diff-files -h").  By replacing the single string
with a string array and passing it to parse_options() or
show_usage_with_options_if_asked(), the COMMON_DIFF_OPTIONS_HELP
macro is omitted.

Since parse_options() or show_usage_with_options_if_asked() only
knows about the options explicitly defined in the diff_files_options[]
array (leaving standard diff options to be parsed by
setup_revisions()), the common diff options (like -p, --stat, and -z)
will no longer be documented when the user runs "git diff-files -h".

You probably could shove COMMON_DIFF_OPTIONS_HELP into OPT_GROUP()
or something that produces static text in the help output to avoid
the regression if you really wanted to, but given that "diff-files"
is plumbing meant for use by scripts, I am not sure how much value
we get by rewriting its command-line parsing.  Being able to say
"git diff-files --bas" and get it understood as "--base" is
something we may appreciate if it were a Porcelain command, but is
it worth the hassle for a plumbing?  I have to wonder.

Especially when the parsing loop the patch wants to replace ...

>  	argc = setup_revisions(argc, argv, &rev, NULL);
> -	while (1 < argc && argv[1][0] == '-') {
> -		if (!strcmp(argv[1], "--base"))
> -			rev.max_count = 1;
> -		else if (!strcmp(argv[1], "--ours"))
> -			rev.max_count = 2;
> -		else if (!strcmp(argv[1], "--theirs"))
> -			rev.max_count = 3;
> -		else if (!strcmp(argv[1], "-q"))
> -			options |= DIFF_SILENT_ON_REMOVED;
> -		else
> -			usage(diff_files_usage);
> -		argv++; argc--;
> -	}

... is not anything that is ugly or overly hard to understand.

I dunno.


> +	argc = parse_options(argc, argv, prefix, diff_files_options,
> +			     diff_files_usage, 0);
>  	if (!rev.diffopt.output_format)
>  		rev.diffopt.output_format = DIFF_FORMAT_RAW;
>  	rev.diffopt.rotate_to_strict = 1;
> @@ -73,7 +74,7 @@ int cmd_diff_files(int argc,
>  	if (rev.pending.nr ||
>  	    rev.min_age != -1 || rev.max_age != -1 ||
>  	    3 < rev.max_count)
> -		usage(diff_files_usage);
> +		usage_with_options(diff_files_usage, diff_files_options);
>  
>  	/*
>  	 * "diff-files --base -p" should not combine merges because it