From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 30 Oct 2025 10:47:43 GMT
Subject: Re: [PATCH v3] blame: make diff algorithm configurable
Message-ID: <fa9c953b-a03f-44ab-962e-3eb4ee335b5b@gmail.com>
In-Reply-To: <33d44dc6-36b3-4736-b3ed-96861a3c4003@delpeuch.eu>

Hi Antonin

On 30/10/2025 09:22, Antonin Delpeuch wrote:
> On 29/10/2025 11:16, Phillip Wood wrote:
>> Unfortunately XDF_DIFF_ALGORITHM_MASK does not include 
>> XDF_NEED_MINIMAL so if the user has a config file that looks like
>>
>>     [diff]
>>         algorithm = minimal
>>         algorithm = myers
>>
>> We'll parse it as "minimal" rather than "myers"
>>
>> As we need to reset the diff algorithm in a number of places I think 
>> it would be best to define a macro
>>
>>         #define CLEAR_DIFF_ALGORITHM(flags) \
>>         flags &= ~(XDF_DIFF_ALGORITHM_MASK | XDF_NEED_MINIMAL)
> 
> Ouch, good catch! This problem is affecting other places as well.
> 
> I'm wondering if we couldn't even add XDF_NEED_MINIMAL to 
> XDF_DIFF_ALGORITHM_MASK. I've reviewed all the places where 
> XDF_DIFF_ALGORITHM_MASK is used, and it seems that in all cases it would 
> either preserve the existing behaviour (potentially allowing us to 
> remove an accompanying "DIFF_XDL_CLR(opts, NEED_MINIMAL);" macro which 
> becomes redundant), or in some other cases it would fix a similar issue 
> (for instance, in merge-file.c).

Thanks for taking the time to look at the other uses of 
XDF_DIFF_ALGORITHM_MASK. I think adding XDF_NEED_MINIMAL to 
XDF_DIFF_ALGORITHM_MASK is a good idea as I think we always treat 
"minimal" as another diff algorithm, rather than a variant of "myers" 
which is how it is implemented in xdiff.

> Is your suggestion to introduce a new macro motivated by stability 
> concerns? I'm aware that xdiff is used in other code bases as a library, 
> so I guess changing XDF_DIFF_ALGORITHM_MASK can indeed be seen as a 
> breaking change.

While it is a breaking change, tweaking XDF_DIFF_ALGORITHM_MASK seems 
like a better idea to me. I wondered about it yesterday but worried it 
would end up being too much work to review all the existing uses.

Thanks

Phillip

> Antonin
>