public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Phillip Wood <phillip.wood123@gmail•com>
To: Antonin Delpeuch <antonin@delpeuch•eu>,
	phillip.wood@dunelm•org.uk,
	Antonin Delpeuch via GitGitGadget <gitgitgadget@gmail•com>,
	git@vger•kernel.org
Cc: Elijah Newren <newren@gmail•com>
Subject: Re: [PATCH v3] blame: make diff algorithm configurable
Date: Thu, 30 Oct 2025 10:47:43 +0000	[thread overview]
Message-ID: <fa9c953b-a03f-44ab-962e-3eb4ee335b5b@gmail.com> (raw)
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
> 

next prev parent reply	other threads:[~2025-10-30 10:47 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-10-20 14:56 [PATCH] blame: make diff algorithm configurable Antonin Delpeuch via GitGitGadget
2025-10-20 16:05 ` Junio C Hamano
2025-10-22  9:37   ` Antonin Delpeuch
2025-10-22 20:39     ` Junio C Hamano
2025-10-23 16:03 ` Phillip Wood
2025-10-28 13:37 ` [PATCH v2] " Antonin Delpeuch via GitGitGadget
2025-10-28 15:22   ` Junio C Hamano
2025-10-28 16:00     ` Antonin Delpeuch
2025-10-28 21:14   ` [PATCH v3] " Antonin Delpeuch via GitGitGadget
2025-10-29 10:16     ` Phillip Wood
2025-10-29 18:46       ` Junio C Hamano
2025-10-30  9:22       ` Antonin Delpeuch
2025-10-30 10:47         ` Phillip Wood [this message]
2025-11-01 21:57     ` [PATCH v4 0/2] " Antonin Delpeuch via GitGitGadget
2025-11-01 21:57       ` [PATCH v4 1/2] xdiff: add 'minimal' to XDF_DIFF_ALGORITHM_MASK Antonin Delpeuch via GitGitGadget
2025-11-03 14:32         ` Phillip Wood
2025-11-01 21:57       ` [PATCH v4 2/2] blame: make diff algorithm configurable Antonin Delpeuch via GitGitGadget
2025-11-03 14:32         ` Phillip Wood
2025-11-03 16:15           ` Junio C Hamano
2025-11-06 20:29             ` Junio C Hamano
2025-11-06 22:41       ` [PATCH v5 0/2] " Antonin Delpeuch via GitGitGadget
2025-11-06 22:41         ` [PATCH v5 1/2] xdiff: add 'minimal' to XDF_DIFF_ALGORITHM_MASK Antonin Delpeuch via GitGitGadget
2025-11-07 15:52           ` Junio C Hamano
2025-11-06 22:41         ` [PATCH v5 2/2] blame: make diff algorithm configurable Antonin Delpeuch via GitGitGadget
2025-11-07 15:57           ` Junio C Hamano
2025-11-07 15:49         ` [PATCH v5 0/2] " Phillip Wood
2025-11-17  1:12           ` Junio C Hamano
2025-11-17  8:04         ` [PATCH v6 " Antonin Delpeuch via GitGitGadget
2025-11-17  8:04           ` [PATCH v6 1/2] xdiff: add 'minimal' to XDF_DIFF_ALGORITHM_MASK Antonin Delpeuch via GitGitGadget
2025-11-17  8:04           ` [PATCH v6 2/2] blame: make diff algorithm configurable Antonin Delpeuch via GitGitGadget
2025-11-17 14:13           ` [PATCH v6 0/2] " Phillip Wood
2025-11-17 18:24           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fa9c953b-a03f-44ab-962e-3eb4ee335b5b@gmail.com \
    --to=phillip.wood123@gmail$(echo .)com \
    --cc=antonin@delpeuch$(echo .)eu \
    --cc=git@vger$(echo .)kernel.org \
    --cc=gitgitgadget@gmail$(echo .)com \
    --cc=newren@gmail$(echo .)com \
    --cc=phillip.wood@dunelm$(echo .)org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox