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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 15, 2022, 15:32 UTC
Message-ID: <221215.86a63o3c0i.gmgdl@evledraar.gmail.com>
In-Reply-To: <c9c2c979-82b3-8be1-1edb-343661cf4b32@dunelm.org.uk>

On Thu, Dec 15 2022, Phillip Wood wrote:

Show 38 quoted lines

> 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 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/