Hi John
On 05/02/2023 03:46, John Cai via GitGitGadget wrote:
From: John Cai <jcai@xxxxxxxxxx>
The diff option parsing for --minimal, --patience, --histgoram can all
be consolidated into one function. This is a preparatory step for the
subsequent commit which teaches diff to keep track of whether or not a
diff algorithm has been set via the command line.
While we're at it, the logic that sets the diff algorithm in
diff_opt_diff_algorithm() can be refactored into a helper that will
allow multiple callsites to set the diff algorithm.
You say "while we're at it" but isn't it a wholly necessary change for
what you want to do?
This patch basically looks good, I've left a couple of comments below,
thanks for separating it out as a preparatory step
Signed-off-by: John Cai <johncai86@xxxxxxxxx>
---
diff.c | 87 ++++++++++++++++++++++++++++++++++++----------------------
1 file changed, 54 insertions(+), 33 deletions(-)
diff --git a/diff.c b/diff.c
index 329eebf16a0..a8a31c81fe7 100644
--- a/diff.c
+++ b/diff.c
@@ -3437,6 +3437,22 @@ static int diff_filepair_is_phoney(struct diff_filespec *one,
return !DIFF_FILE_VALID(one) && !DIFF_FILE_VALID(two);
}
+static int set_diff_algorithm(struct diff_options *opts,
+ const char *alg)
+{
+ long value = parse_algorithm_value(alg);
+
+ if (value < 0)
+ return 1;
+
+ /* clear out previous settings */
+ DIFF_XDL_CLR(opts, NEED_MINIMAL);
+ opts->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
+ opts->xdl_opts |= value;
+
+ return 0;
+}
+
static void builtin_diff(const char *name_a,
const char *name_b,
struct diff_filespec *one,
@@ -5107,17 +5123,40 @@ static int diff_opt_diff_algorithm(const struct option *opt,
const char *arg, int unset)
{
struct diff_options *options = opt->value;
- long value = parse_algorithm_value(arg);
BUG_ON_OPT_NEG(unset);
- if (value < 0)
+
+ if (set_diff_algorithm(options, arg))
return error(_("option diff-algorithm accepts \"myers\", "
"\"minimal\", \"patience\" and \"histogram\""));
- /* clear out previous settings */
- DIFF_XDL_CLR(options, NEED_MINIMAL);
- options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
- options->xdl_opts |= value;
+ return 0;
+}
+
+static int diff_opt_diff_algorithm_no_arg(const struct option *opt,
+ const char *arg, int unset)
+{
+ struct diff_options *options = opt->value;
+
+ BUG_ON_OPT_NEG(unset);
+ BUG_ON_OPT_ARG(arg);
+
+ if (!strcmp(opt->long_name, "patience")) {
+ int i;
This is copied from the existing code but as `options->anchors_nr` is a
size_t it is probably worth converting `i` to a size_t here.
+ /*
+ * Both --patience and --anchored use PATIENCE_DIFF
+ * internally, so remove any anchors previously
+ * specified.
+ */
+ for (i = 0; i < options->anchors_nr; i++)
+ free(options->anchors[i]);
+ options->anchors_nr = 0;
+ }
+
+ if (set_diff_algorithm(options, opt->long_name))
+ return error(_("available diff algorithms include \"myers\", "
+ "\"minimal\", \"patience\" and \"histogram\""));
I think this should be a BUG() as it is a programming error if we reach
this point.
Best Wishes
Phillip
+
return 0;
}
@@ -5242,26 +5281,6 @@ static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,
return 0;
}
-static int diff_opt_patience(const struct option *opt,
- const char *arg, int unset)
-{
- struct diff_options *options = opt->value;
- int i;
-
- BUG_ON_OPT_NEG(unset);
- BUG_ON_OPT_ARG(arg);
- options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
- /*
- * Both --patience and --anchored use PATIENCE_DIFF
- * internally, so remove any anchors previously
- * specified.
- */
- for (i = 0; i < options->anchors_nr; i++)
- free(options->anchors[i]);
- options->anchors_nr = 0;
- return 0;
-}
-
static int diff_opt_ignore_regex(const struct option *opt,
const char *arg, int unset)
{
@@ -5562,9 +5581,10 @@ struct option *add_diff_options(const struct option *opts,
N_("prevent rename/copy detection if the number of rename/copy targets exceeds given limit")),
OPT_GROUP(N_("Diff algorithm options")),
- OPT_BIT(0, "minimal", &options->xdl_opts,
- N_("produce the smallest possible diff"),
- XDF_NEED_MINIMAL),
+ OPT_CALLBACK_F(0, "minimal", options, NULL,
+ N_("produce the smallest possible diff"),
+ PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+ diff_opt_diff_algorithm_no_arg),
OPT_BIT_F('w', "ignore-all-space", &options->xdl_opts,
N_("ignore whitespace when comparing lines"),
XDF_IGNORE_WHITESPACE, PARSE_OPT_NONEG),
@@ -5589,10 +5609,11 @@ struct option *add_diff_options(const struct option *opts,
OPT_CALLBACK_F(0, "patience", options, NULL,
N_("generate diff using the \"patience diff\" algorithm"),
PARSE_OPT_NONEG | PARSE_OPT_NOARG,
- diff_opt_patience),
- OPT_BITOP(0, "histogram", &options->xdl_opts,
- N_("generate diff using the \"histogram diff\" algorithm"),
- XDF_HISTOGRAM_DIFF, XDF_DIFF_ALGORITHM_MASK),
+ diff_opt_diff_algorithm_no_arg),
+ OPT_CALLBACK_F(0, "histogram", options, NULL,
+ N_("generate diff using the \"histogram diff\" algorithm"),
+ PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+ diff_opt_diff_algorithm_no_arg),
OPT_CALLBACK_F(0, "diff-algorithm", options, N_("<algorithm>"),
N_("choose a diff algorithm"),
PARSE_OPT_NONEG, diff_opt_diff_algorithm),
