Hi Junio,
On 14 Feb 2023, at 21:38, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> From: John Cai <johncai86@xxxxxxxxx>
>>
>> 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?
>
>> 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.
>
>> 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.
>
>> +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.
>
>> @@ -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".
>
>> - 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.
Yeah, it's not great to pull things out from a bit flip to a callback but I
needed some way of setting the xdl_opts_command_line member in the next commit
when we know that the diff algorithm was set via options on the command line so
that we can honor the precedence. If there's a different way to do that other
than using callbacks for command options parsing, I'd agree that keeping these
as bit flips would be ideal.
>
> Thanks.
Thanks!
John
