Re: [PATCH] merge-file: add --diff-algorithm option

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 19, 2023, 16:43 UTC
Message-ID: <de04aec0-a195-45da-8951-bb30f2a629a3@gmail.com>
In-Reply-To: <653b08fd-2df3-4a7a-8082-fdb809e87784@delpeuch.eu>

Hi Antonin

On 17/11/2023 21:42, Antonin Delpeuch wrote:

Show 15 quoted lines

> Hi all,
> 
> Here a few more thoughts about this patch, to explain what brought me to 
> needing that. If this need is misguided, perhaps you could redirect me 
> to a better solution.
> 
> I am writing a custom merge driver for Java files. This merge driver 
> internally calls git-merge-file and then solves the merge conflicts 
> which only consist of import statements (there might be cases where it 
> gets it wrong, but I can then use other tools to cleanup those import 
> statements). When testing this, I noticed that the merge driver 
> performed more poorly on other sorts of conflicts, compared to the 
> standard "ort" merge strategy. This is because "ort" uses the 
> "histogram" diff algorithm, which gives better results than the "myers" 
> diff algorithm that merge-file uses.

I cannot comment on this particular use but I think in general calling "git merge-file" from a custom merge driver is perfectly sensible. Have you tested your driver with this patch to see if you get better results with the histogram diff algorithm?

Show 5 quoted lines

> Intuitively, if "histogram" is the default diff algorithm used by "git 
> merge", then it would also make sense to have the same default for "git 
> merge-file", but I assume that changing this default could be considered 
> a bad breaking change. So I thought that making this diff algorithm 
> configurable would be an acceptable move, hence my patch.

I can see there's an argument for changing the default algorithm of "git merge-file" to match what "ort" uses. I know Elijah found the histogram algorithm gave better results in his testing when he was developing "ort". While it would be a breaking change if on the average the new default gives better conflicts it might be worth it. This patch would mean that someone wanting to use the "myers" algorithm could still do so.

Show 7 quoted lines

> Of course, the diffing could be configured in other ways, for instance 
> with its handling of whitespace or EOL (similarly to what the "git-diff" 
> command offers). I think those options would definitely be worth 
> exposing in merge-file as well. If you think this makes sense, then I 
> would be happy to work on a new version of this patch which would 
> attempt to include all the relevant options. I could also try to add the 
> corresponding tests.

It would be nice to see some tests for this patch, ideally using a test case that gives different conflicts for "myers" and "histogram". We could add the other options later if there is a demand.

Best Wishes

Phillip

Show 11 quoted lines

> But perhaps my need is misguided? Could it be that I should not be 
> writing a custom merge driver, but instead use another extension point 
> to only process the conflicting hunks after execution of the existing 
> merge driver? I couldn't find such an extension point, but it can well 
> be that I missed it.
> 
> Thank you,
> 
> Antonin
> 
>