On Fri, Feb 17, 2023 at 12:21 PM John Cai via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: John Cai <johncai86@xxxxxxxxx>
>
> A subsequent commit will need the ability to tell if the diff algorithm
> was set through the command line through setting a new member of
> diff_options. While this logic can be added to the
> diff_opt_diff_algorithm() callback, the `--minimal` and `--histogram`
> options are handled via OPT_BIT without a callback.
>
> Remedy this by consolidating the options parsing logic for --minimal and
> --histogram into one callback. This way we can modify `diff_options` in
> that function.
>
> As an additional refactor, 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.
>
> Signed-off-by: John Cai <johncai86@xxxxxxxxx>
> ---
> diff.c | 57 +++++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 329eebf16a0..5efc22ca06b 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,28 @@ 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 (set_diff_algorithm(options, opt->long_name))
> + BUG("available diff algorithms include \"myers\", "
> + "\"minimal\", \"patience\" and \"histogram\"");
> +
> return 0;
> }
>
> @@ -5250,7 +5277,6 @@ static int diff_opt_patience(const struct option *opt,
>
> 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
> @@ -5259,7 +5285,8 @@ static int diff_opt_patience(const struct option *opt,
> for (i = 0; i < options->anchors_nr; i++)
> free(options->anchors[i]);
> options->anchors_nr = 0;
> - return 0;
> +
> + return set_diff_algorithm(options, "patience");
> }
>
> static int diff_opt_ignore_regex(const struct option *opt,
> @@ -5562,9 +5589,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),
> @@ -5590,9 +5618,10 @@ struct option *add_diff_options(const struct option *opts,
> 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),
> + 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),
> --
> gitgitgadget
This patch looks good to me.
