[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;