Re: [PATCH v5 1/2] xdiff: add 'minimal' to XDF_DIFF_ALGORITHM_MASK

[Junio C Hamano <gitster@pobox.com>]

"Antonin Delpeuch via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Antonin Delpeuch <antonin@delpeuch.eu>
>
> The XDF_DIFF_ALGORITHM_MASK bit mask only includes bits for the patience
> and histogram diffs, not for the minimal one. This means that when
> reseting the diff algorithm to the default one, one needs to separately
> clear the bit for the minimal diff. There are places in the code that fail
> to do that: merge-ort.c and builtin/merge-file.c.
>
> Add the XDF_NEED_MINIMAL bit to the bit mask, and remove the separate
> clearing of this bit in the places where it hasn't been forgotten.

Makes sense.  In other words, lack of any algorithm-mask bit means
the code uses myers.

> diff --git a/diff.c b/diff.c
> index 87fa16b730..6ce3591c5b 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3526,8 +3526,6 @@ static int set_diff_algorithm(struct diff_options *opts,
>  	if (value < 0)
>  		return -1;
>  
> -	/* clear out previous settings */
> -	DIFF_XDL_CLR(opts, NEED_MINIMAL);
>  	opts->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;

The comment still accurately describes what the surviving line does,
though.  It is borderline if it needs commenting, but the topic of
this patch not being "remove overly obvious comments", I'd probably
vote for retaining the comment.

> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
> index 2cecde5afe..dc370712e9 100644
> --- a/xdiff/xdiff.h
> +++ b/xdiff/xdiff.h
> @@ -43,7 +43,7 @@ extern "C" {
>  
>  #define XDF_PATIENCE_DIFF (1 << 14)
>  #define XDF_HISTOGRAM_DIFF (1 << 15)
> -#define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF)
> +#define XDF_DIFF_ALGORITHM_MASK (XDF_PATIENCE_DIFF | XDF_HISTOGRAM_DIFF | XDF_NEED_MINIMAL)
>  #define XDF_DIFF_ALG(x) ((x) & XDF_DIFF_ALGORITHM_MASK)

Given the definition of XDF_DIFF_ALG(), I wondered how it is used.

    $ git grep -n -e 'XDF_DIFF_ALG(' \*.c
    xdiff/xdiffi.c:324:	if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) {
    xdiff/xdiffi.c:329:	if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) {
    xdiff/xprepare.c:170:	if ((XDF_DIFF_ALG(xpp->flags) != XDF_PATIENCE_DIFF) &&
    xdiff/xprepare.c:171:	    (XDF_DIFF_ALG(xpp->flags) != XDF_HISTOGRAM_DIFF)) {
    xdiff/xprepare.c:396:	sample = (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF
    xdiff/xprepare.c:417:	if ((XDF_DIFF_ALG(xpp->flags) != XDF_PATIENCE_DIFF) &&
    xdiff/xprepare.c:418:	    (XDF_DIFF_ALG(xpp->flags) != XDF_HISTOGRAM_DIFF) &&

They say "if the specified algorithm is (or is not) patience (or
histogram), do this".  Now the original code, because the mask did
not include the need-minimal bit, would have chosen patience code
path even if xpp->flags had XDF_PATIENCE_DIFF and XDF_NEED_MINIMAL
bits set at the same time.  The code would no longer do so.

What is keeping us safe and not making this change a bug is that
among XDF_DIFF_ALGORITHM_MASK bits, we intend to set at most one of
them at a time.  I wonder if we want the command line option and
configuration parser to have an explicit check (and BUG("")) to
ensure this constraint.

Also, in the longer term as #leftoverbits clean-up, perhaps these
bits can be removed from xpp->flags and diff_options->xdl_opts, xpp
structure can gain a separate member that is an enum of the
algorithm names instead, and XDF_DIFF_ALGORITHM_MASK can be dropped?

Then set_diff_algorithm() we saw earlier can become

	if (value < 0)
		return -1;
	opts->xdl_algo = value;
	return 0;

And since there is no "clean out prvious settings" required (now we
can simply overwrite), we can truly lose that old comment once we do
so.

As a part of this topic, I think that a new code to sanity check
that there are at most one bit in XDF_DIFF_ALG(xpp->flags) may be a
good safety measure to have.  Moving the algorithm bits out of the
flags is a larger change, and it may be better left outside the
topic, but I do not personaly mind seeing such a clean-up included
as a preparatory change for this series, either.

Thanks.