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

From: Phillip Wood <[email-masked]>
Date: Mon, 3 Nov 2025 14:32:33 +0000

Hi Antonin

On 01/11/2025 21:57, Antonin Delpeuch via GitGitGadget wrote:
> From: Antonin Delpeuch <[email-masked]>
> 
> 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.

Nicely explained. This is a useful improvement that should prevent 
errors in the future. After this patch there are no users of 
DIFF_XDL_CLR() so we should probably remove that macro. I'm not sure it 
makes sense to remove the comments that have been deleted below as we're 
still clearing the old setting. Apart from that this all looks good.

Thanks

Phillip

> Signed-off-by: Antonin Delpeuch <[email-masked]>
> ---
>   diff.c        | 2 --
>   merge-ort.c   | 2 --
>   xdiff/xdiff.h | 2 +-
>   3 files changed, 1 insertion(+), 5 deletions(-)
> 
> 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;
>   	opts->xdl_opts |= value;
>   
> diff --git a/merge-ort.c b/merge-ort.c
> index 29858074f9..9b2b0fce7e 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -5495,8 +5495,6 @@ int parse_merge_opt(struct merge_options *opt, const char *s)
>   		long value = parse_algorithm_value(arg);
>   		if (value < 0)
>   			return -1;
> -		/* clear out previous settings */
> -		DIFF_XDL_CLR(opt, NEED_MINIMAL);
>   		opt->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
>   		opt->xdl_opts |= value;
>   	}
> 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)
>   
>   #define XDF_INDENT_HEURISTIC (1 << 23)