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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 7, 2025, 15:52 UTC
Message-ID: <xmqqh5v5hmat.fsf@gitster.g>
In-Reply-To: <e81a5d2bd23add19e04184f6b37910bc89a514a5.1762468914.git.gitgitgadget@gmail.com>

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

Show 10 quoted lines

> 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.

Show 11 quoted lines

> 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.

Show 11 quoted lines

> 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.