Re: [PATCH v2 1/2] diff: consolidate diff algorithm option parsing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 15, 2023, 02:38 UTC
- Message-ID
- <xmqqk00j3b6r.fsf@gitster.g>
- In-Reply-To
- <0c5e1fc6c2651e39bcefa27ee0976c9519671969.1676410819.git.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> From: John Cai <johncai86@gmail.com> > > 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.
Everybody other than patience used to be just a bit-op but now everybody is a callback?
Show 23 quoted lines
> diff --git a/diff.c b/diff.c
> index 329eebf16a0..92a0eab942e 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;
> +}The above is a faithful copy of diff_opt_diff_algorithm(), except that it returns 1 (not -1) on failure, which is unexpected in this codebase, and should be corrected if this patch gets rerolled.
Show 22 quoted lines
> 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;
> +}This version of diff_opt_diff_algorithm() behaves identically from the version before this patch, which is excellent.
Show 26 quoted lines
> +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")) {
> + size_t i;
> + /*
> + * 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))
> + BUG("available diff algorithms include \"myers\", "
> + "\"minimal\", \"patience\" and \"histogram\"");
> +
> return 0;
> }Calling this instead of diff_opt_patience() would make "--patience" parsed identically as before without this patch, which is excellent.
Show 11 quoted lines
> @@ -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),I offhand cannot say that these two are equivalent, even though they ought to be (otherwise this patch would break things). The callback seems to do much more than just a simple "flip the NEED_MINIMAL bit on".
Show 8 quoted lines
> - 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),Likewise.
By nature, patience (and anchored) needs to do much more than everybody else, so it almost feels that it is OK (and preferable, even) to leave it a special case to make the distinction stand out. Consolidating everybody else who are much simpler to share the more complex callback does not look like a good change to me, at least at the first glance.
Thanks.