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@xxxxxxxxx>
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@xxxxxxxxx/
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@xxxxxxxxxxxxxxxxxxx
[2] https://lore.kernel.org/git/o759r3qn-nqn9-oq22-p90o-2nrn24085n80@xxxxxx
[3] https://lore.kernel.org/git/2r992r19-or36-733r-1139-4575n9o6o23s@xxxxxx
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@xxxxxxxxx/
