Re: ag/merge-strategies-in-c (was: What's cooking in git.git (Dec 2022, #05; Wed, 14))

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Ævar

On 15/12/2022 15:32, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Dec 15 2022, Phillip Wood wrote:
> 
>> On 15/12/2022 09:14, Ævar Arnfjörð Bjarmason wrote:
>>> On Wed, Dec 14 2022, Junio C Hamano wrote:
>>>
>>>> * ag/merge-strategies-in-c (2022-08-10) 14 commits
>>>>    . sequencer: use the "octopus" strategy without forking
>>>>    . sequencer: use the "resolve" strategy without forking
>>>>    . merge: use the "octopus" strategy without forking
>>>>    . merge: use the "resolve" strategy without forking
>>>>    . merge-octopus: rewrite in C
>>>>    . merge-recursive: move better_branch_name() to merge.c
>>>>    . merge-resolve: rewrite in C
>>>>    . merge-one-file: rewrite in C
>>>>    . update-index: move add_cacheinfo() to read-cache.c
>>>>    . merge-index: add a new way to invoke `git-merge-one-file'
>>>>    . merge-index: drop the index
>>>>    . merge-index: libify merge_one_path() and merge_all()
>>>>    . t6060: add tests for removed files
>>>>    . t6060: modify multiple files to expose a possible issue with merge-index
>>>>
>>>>    An attempt to rewrite remaining merge strategies from shell to C.
>>>>
>>>>    Tired of waiting for too long.
>>>>    source: <20220809185429.20098-1-alban.gruin@gmail.com>
>>> I submitted a v9 of this during Taylor's maintainership, but it fell
>>> between the cracks. I've submitted a rebased-on-master v10 now (there
>>> were some conflicts):
>>> https://lore.kernel.org/git/cover-v10-00.12-00000000000-20221215T084803Z-avarab@gmail.com/
>>> It's just the "prep" patches, the real meaty part is converting the
>>> merge drivers, which will come after. Some of the performance numbers
>>> for those are really impressive...
>>
>> I think splitting this in two is a good idea as there were only a
>> couple of outstanding issues with the first half of Alban's V8. When
>> you posted V9 I looked at the range-diff hoping to see a couple of
>> localized changes addressing those issues. Instead it looks like
>> you've rewritten most of the patches that people have already spent a
>> considerable time reviewing. I don't think it is a good use of
>> reviewers' time to essentially start reviewing again from scratch.
> 
> What do you consider a good way to turn this comment into something
> actionable?

To address the issues noted in [1-3] by tweaking patches 3, 5 & 7 of v8 
and not rewrite all the other patches. Note that the sequencer cannot 
die() while merging so we need to be careful when fixing patch 3 - it 
should bail out early rather than dying when there is an error.

Best Wishes

Phillip

[1] https://lore.kernel.org/git/220817.86lernaa9i.gmgdl@evledraar.gmail.com
[2] https://lore.kernel.org/git/o759r3qn-nqn9-oq22-p90o-2nrn24085n80@tzk.qr
[3] https://lore.kernel.org/git/2r992r19-or36-733r-1139-4575n9o6o23s@tzk.qr

> To have a minimal re-submission of the v8 which simply fixes the
> semnatic & textual merge conflicts we've had on "master" in the interim?
> 
> I think such a re-submission would need to address the issues I noted in
> the v9 CL[1]. E.g. that in over-libifying merge-index its behavior was
> changed, e.g. emitting N error() instead of die()-ing on the 1st
> one. Addressing that is going to need to require around the same code
> changes.
> 
> This is also a case where the range-diff looks overly scary, aside from
> such specific fixes the end result is substantially the same, but I did
> split up (and mostly not rewrite) the existing patches to:
> 
> * Cleanly separate those bits that were changing behavior, from those
>    that were not (and precede them with tests to assert that)
> 
> * Make the eventual libification change as small as possible, now it
>    really benefits from the diff rename detection.
> 
> If you have some more specific suggestions for how to move forward I'm
> all ears.
> 
> 1. https://lore.kernel.org/git/cover-v9-00.12-00000000000-20221118T110058Z-avarab@gmail.com/