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

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

Edson <edsonwasswa54@gmail.com> writes:

> The cmd_diff_files() function manually parses --base, --ours, --theirs,
> and -q options using a hand-rolled loop with strcmp() calls. This is
> error-prone and inconsistent with the rest of Git's codebase.

The first sentence is correct, but is it error-prone how?

> Convert this to use the parse-options API by:
> - Adding option definitions using OPT_* macros
> - Removing the manual parsing loop
> - Using parse_options() for consistent argument handling
>
> The --base, --ours, and --theirs options use PARSE_OPT_NONEG to prevent
> negated forms (--no-base, etc.) which are not meaningful for merge stage
> selection. This follows the pattern used in builtin/checkout.c for similar
> stage selection options.
>
> This makes the code more maintainable and brings it in line with Git's coding standards. It also enables auto-generated help text, git completion support, and option abbreviation.
>
> Signed-off-by: Edson <edsonwasswa54@gmail.com>

Earlier messages to the list from this e-mail address identified the
sender as "Wasswa Edson".  Are these the same person?

See Documentation/SubmittingPatches [[dco]] and [[real-name]].


> ---
>  builtin/diff-files.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/diff-files.c b/builtin/diff-files.c
> index ea91347ce2..a0c3981407 100644
> --- a/builtin/diff-files.c
> +++ b/builtin/diff-files.c
> @@ -14,6 +14,7 @@
>  #include "commit.h"
>  #include "preload-index.h"
>  #include "revision.h"
> +#include "parse-options.h"
>  
>  static const char diff_files_usage[] =
>  "git diff-files [-q] [-0 | -1 | -2 | -3 | -c | --cc] [<common-diff-options>] [<path>...]"

Using parse-options API means you'd need to be careful about how
you write the usage string, as the parse_options() call itself will
list options and its meanings.  Doesn't the above need adjusting?

> @@ -48,18 +49,21 @@ int cmd_diff_files(int argc,
>  	prefix = precompose_argv_prefix(argc, argv, prefix);
>  
>  	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--;
> +
> +	{
> +		static struct option diff_files_options[] = {
> +			OPT_SET_INT_F(0, "base", &rev.max_count,
> +				      N_("show base version"), 1, PARSE_OPT_NONEG),
> +			OPT_SET_INT_F(0, "ours", &rev.max_count,
> +				      N_("show our version"), 2, PARSE_OPT_NONEG),
> +			OPT_SET_INT_F(0, "theirs", &rev.max_count,
> +				      N_("show their version"), 3, PARSE_OPT_NONEG),
> +			OPT_BIT('q', NULL, &options,
> +			       N_("be quiet"), DIFF_SILENT_ON_REMOVED),
> +			OPT_END()
> +		};
> +		argc = parse_options(argc, argv, prefix, diff_files_options,
> +				     diff_files_usage, 0);
>  	}

Is it necessary to introduce an extra block that only increases the
indent level?  I do not think so.  Move the "struct option []" up in
the beginning of the function.  If we really have to use the
parse_options() API, that is.

It also is unusual in modern code to have "struct option[]" array as
a function scope static, even though older code like cmd_fsck()
makes it static.

>  	if (!rev.diffopt.output_format)
>  		rev.diffopt.output_format = DIFF_FORMAT_RAW;