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

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

[Edson]

From: Wasswa Edson <edsonwasswa54@gmail.com>

The cmd_diff_files() function manually parses --base, --ours, --theirs,
and -q options using a hand-rolled loop with strcmp() calls. This is
inconsistent with the rest of Git's codebase and lacks features provided
by the parse-options API such as auto-generated help text, git completion
support, and option abbreviation.

Changes since v1:
- Fixed author name to "Wasswa Edson" for DCO compliance
- Removed vague "error-prone" claim, replaced with specific justification
- Removed unnecessary block scope around option array
- Changed from function-scope static to regular local variable

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.

Signed-off-by: Wasswa Edson <edsonwasswa54@gmail.com>
---
 builtin/diff-files.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index ea91347ce2..bd761443b9 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>...]"
@@ -48,19 +49,20 @@ 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--;
-	}
+
+	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);
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 	rev.diffopt.rotate_to_strict = 1;
-- 
2.46.0.windows.1

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

[Edson]

From: Wasswa Edson <edsonwasswa54@gmail.com>

The cmd_diff_files() function manually parses --base, --ours, --theirs,
and -q options using a hand-rolled loop with strcmp() calls. This is
inconsistent with the rest of Git's codebase and lacks features provided
by the parse-options API such as auto-generated help text, git completion
support, and option abbreviation.

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.

Signed-off-by: Wasswa Edson <edsonwasswa54@gmail.com>
---
 builtin/diff-files.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index ea91347ce2..bd761443b9 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>...]"
@@ -48,19 +49,20 @@ 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--;
-	}
+
+	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);
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 	rev.diffopt.rotate_to_strict = 1;
-- 
2.46.0.windows.1

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

[Junio C Hamano]

Edson <edsonwasswa54@gmail.com> writes:

> From: Wasswa Edson <edsonwasswa54@gmail.com>
>
> The cmd_diff_files() function manually parses --base, --ours, --theirs,
> and -q options using a hand-rolled loop with strcmp() calls. This is
> inconsistent with the rest of Git's codebase and lacks features provided
> by the parse-options API such as auto-generated help text, git completion
> support, and option abbreviation.
>
> 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.
>
> Signed-off-by: Wasswa Edson <edsonwasswa54@gmail.com>

Nicely described.

$ make DEVELOPER=YesPlease builtin/diff-files.o

is your friend.

>  
>  static const char diff_files_usage[] =
>  "git diff-files [-q] [-0 | -1 | -2 | -3 | -c | --cc] [<common-diff-options>] [<path>...]"
> @@ -48,19 +49,20 @@ 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--;
> -	}
> +
> +	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()
> +	};

Move this a lot higher.

builtin/diff-files.c:53:9: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
   53 |         struct option diff_files_options[] = {


> +	argc = parse_options(argc, argv, prefix, diff_files_options,
> +			     diff_files_usage, 0);

parse_options() takes a NULL-terminated array of constant strings,
but diff_files_usage is a single constant string.

builtin/diff-files.c:65:30: error: passing argument 5 of ‘parse_options’ from incompatible pointer type [-Wincompatible-pointer-types]
   65 |                              diff_files_usage, 0);
      |                              ^~~~~~~~~~~~~~~~
      |                              |
      |                              const char *
In file included from builtin/diff-files.c:17:
./parse-options.h:424:38: note: expected ‘const char * const*’ but argument is of type ‘const char *’
  424 |                   const char * const usagestr[],
      |                   ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~

As I said in my earlier message, conversion of parse-options API
requires adjustment of usage string.

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

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

[Edson]

From: Wasswa Edson <edsonwasswa54@gmail.com>

The cmd_diff_files() function manually parses --base, --ours, --theirs,
and -q options using a hand-rolled loop with strcmp() calls. This is
inconsistent with the rest of Git's codebase and lacks features provided
by the parse-options API such as auto-generated help text, git completion
support, and option abbreviation.

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.

Signed-off-by: Wasswa Edson <edsonwasswa54@gmail.com>
---
 builtin/diff-files.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index ea91347ce2..e9e4907311 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -14,11 +14,12 @@
 #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>...]"
-"\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
+};
 
 int cmd_diff_files(int argc,
 		   const char **argv,
@@ -28,8 +29,19 @@ int cmd_diff_files(int argc,
 	struct rev_info rev;
 	int result;
 	unsigned options = 0;
+	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()
+	};
 
-	show_usage_if_asked(argc, argv, diff_files_usage);
+	show_usage_with_options_if_asked(argc, argv, diff_files_usage, diff_files_options);
 
 	repo_config(the_repository, git_diff_basic_config, NULL); /* no "diff" UI options */
 
@@ -48,19 +60,8 @@ 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--;
-	}
+	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
-- 
2.46.0.windows.1

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

[Junio C Hamano]

Edson <edsonwasswa54@gmail.com> writes:

Your message has these headers

    In-Reply-To: <20261028170432.3524465-1-gitster@pobox.com>
    References: <20261028170432.3524465-1-gitster@pobox.com>

but that does not look anything like the message ID of the previous
round of your patch.  I do not see any such messages on the mailing
list.

This hurts both automation and human readers, as people cannot find
and compare the previous round (v3) from the message of v4.  It seems
that your v3 had exactly the same issue.

Please sort it out before sending more patches, as messages with
broken threading will take more resources and time out of your
recipients.

Thanks.