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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Dec 15 2022, Phillip Wood wrote:

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

That seems to implicitly assume that the outstanding feedback on the v8
was the last word on outstanding issues with the series.

The v9..v10 combines addressing those, with issues that I discovered
along the way. I had not reviewed this series in any detail before.

So, I could submit such a v8.5, and then comment on it, and address
those comments, and then we'd be back to the same v9 in the end?

Since I didn't see the point of doing that I submitted the v9 directly.

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

Yes, part 2 will address that, i.e. there are callers that need to not
die() in the same process, but the v8 took the approach of changing the
entirety of the API to error(), but as it turns out the surface where we
needed to error() or die() is much smaller than that in the end.

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