* [PATCH v2] builtin/diff-files: convert ad hoc option parsing to parse-options API
[not found] <20261028170432.3524465-1-gitster@pobox.com>
@ 2026-10-28 19:14 ` Edson
2026-10-28 19:26 ` [PATCH v3] " Edson
2026-10-29 20:39 ` [PATCH v4] " Edson
2 siblings, 0 replies; 8+ messages in thread
From: Edson @ 2026-10-28 19:14 UTC (permalink / raw)
To: git; +Cc: edsonwasswa54
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3] builtin/diff-files: convert ad hoc option parsing to parse-options API
[not found] <20261028170432.3524465-1-gitster@pobox.com>
2026-10-28 19:14 ` [PATCH v2] builtin/diff-files: convert ad hoc option parsing to parse-options API Edson
@ 2026-10-28 19:26 ` Edson
2026-10-28 22:24 ` Junio C Hamano
2026-10-29 20:39 ` [PATCH v4] " Edson
2 siblings, 1 reply; 8+ messages in thread
From: Edson @ 2026-10-28 19:26 UTC (permalink / raw)
To: git; +Cc: edsonwasswa54
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] builtin/diff-files: convert ad hoc option parsing to parse-options API
2026-10-28 19:26 ` [PATCH v3] " Edson
@ 2026-10-28 22:24 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2026-10-28 22:24 UTC (permalink / raw)
To: Edson; +Cc: git
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;
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4] builtin/diff-files: convert ad hoc option parsing to parse-options API
[not found] <20261028170432.3524465-1-gitster@pobox.com>
2026-10-28 19:14 ` [PATCH v2] builtin/diff-files: convert ad hoc option parsing to parse-options API Edson
2026-10-28 19:26 ` [PATCH v3] " Edson
@ 2026-10-29 20:39 ` Edson
2026-10-29 21:40 ` Junio C Hamano
2026-11-01 19:25 ` [PATCH v6] " Edson
2 siblings, 2 replies; 8+ messages in thread
From: Edson @ 2026-10-29 20:39 UTC (permalink / raw)
To: git; +Cc: edsonwasswa54
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4] builtin/diff-files: convert ad hoc option parsing to parse-options API
2026-10-29 20:39 ` [PATCH v4] " Edson
@ 2026-10-29 21:40 ` Junio C Hamano
2026-11-01 19:25 ` [PATCH v6] " Edson
1 sibling, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2026-10-29 21:40 UTC (permalink / raw)
To: Edson; +Cc: git
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v6] builtin/diff-files: convert ad hoc option parsing to parse-options API
2026-10-29 20:39 ` [PATCH v4] " Edson
2026-10-29 21:40 ` Junio C Hamano
@ 2026-11-01 19:25 ` Edson
2026-11-02 10:01 ` Junio C Hamano
1 sibling, 1 reply; 8+ messages in thread
From: Edson @ 2026-11-01 19:25 UTC (permalink / raw)
To: git; +Cc: edsonwasswa54
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v6] builtin/diff-files: convert ad hoc option parsing to parse-options API
2026-11-01 19:25 ` [PATCH v6] " Edson
@ 2026-11-02 10:01 ` Junio C Hamano
2026-11-02 14:16 ` Wasswa Edson
0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2026-11-02 10:01 UTC (permalink / raw)
To: Edson; +Cc: git
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v6] builtin/diff-files: convert ad hoc option parsing to parse-options API
2026-11-02 10:01 ` Junio C Hamano
@ 2026-11-02 14:16 ` Wasswa Edson
0 siblings, 0 replies; 8+ messages in thread
From: Wasswa Edson @ 2026-11-02 14:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Hi Junio,
Thanks for the review. You make a good point about the value trade-off
for a plumbing command - the loss of COMMON_DIFF_OPTIONS_HELP in the
help output isn't worth the benefits parse-options would bring i had
not seen that coming ......lol.
I'll drop this patch and look for a better target to work on.
Thanks for the guidance.
Best regards,
Wasswa Edson
On Mon, 2 Nov 2026 at 13:01, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-11-02 14:16 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20261028170432.3524465-1-gitster@pobox.com>
2026-10-28 19:14 ` [PATCH v2] builtin/diff-files: convert ad hoc option parsing to parse-options API Edson
2026-10-28 19:26 ` [PATCH v3] " Edson
2026-10-28 22:24 ` Junio C Hamano
2026-10-29 20:39 ` [PATCH v4] " Edson
2026-10-29 21:40 ` Junio C Hamano
2026-11-01 19:25 ` [PATCH v6] " Edson
2026-11-02 10:01 ` Junio C Hamano
2026-11-02 14:16 ` Wasswa Edson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox