Re: [PATCH v2 0/1] And so it begins...merge/rename performance work

From: Elijah Newren <newren@gmail.com>
Date: Jan 14, 2021, 20:18 UTC
Message-ID: <CABPp-BHhXXAX7QJeD6jGckozL9DHf5UZ=UXzaogyv-F4_xdYVQ@mail.gmail.com>
In-Reply-To: <xmqqo8hrxrnh.fsf@gitster.c.googlers.com>

Hi Junio,

On Thu, Jan 14, 2021 at 11:08 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > This depends on a merge of en/ort-conflict-handling, en/diffcore-rename,
> > and en/ort-directory-rename.
>
> Thanks.
>
> How ready is the foundation to accept this change?  This will depend
> on all of the above three topics and I am not sure what the status
> of them is---I think that I've read through the diffcore-rename one
> and it was a pleasant read, but I do not recall the reviewer
> reactions to the other two.

Good questions.  Let me go through the three in order:

en/ort-conflict-handling: Has already been reviewed; v2 included the fixes to the issues Stolee noted in v1 and Stolee was happy with v2 (https://lore.kernel.org/git/5f6d5428-36ce-3e91-4916-8968ac1b8686@gmail.com/) as of a week and a half ago. I am unaware of any issues; I think it's ready as-is to merge down to next and then to master.

en/diffcore-rename: Taylor and you both reviewed it; Christian skimmed over it and noted a typo. I fixed up the issues all three of you noted by v3 at the end of December. I was assuming based on your comments (at https://lore.kernel.org/git/xmqqwnwnglim.fsf@gitster.c.googlers.com/) that you are happy with it now, which seems to be supported by the fact that you've already merged it to next. I think it's ready to continue merging on to master.

en/ort-directory-rename: This one is the only question mark in my mind. Most of the ort patches I wanted folks to review because I'm doing things significantly differently than merge-recursive.c does. This series is an exception; patches 1-16 are just porting over the exact same algorithm from merge-recursive.c using the new data structures. It's a lot of code, because directory rename detection has a lot of logic to it, but there's not anything new or tricky to it in relation to what's in merge-recursive.c. However, even though the algorithm is essentially just being copied, the differences in data structures means there are lots of tiny changes all over that make a direct comparison difficult (unless folks are familiar with the old logic, but I think only Stefan Beller and I were). Honestly, I'm not so sure this series is worth reviewers' time given all those details, with the exception of patch 17/17. If folks can review everything, that's great, but if we have limited review resources, I'd rather people review the series before this one, and the performance related ones I'll be posting later. The testcases are pretty thorough in this area, in part thanks to additional suggestions from Stefan a few years back, and the similarity of the logic makes me less concerned about this topic than any of the others in the merge-ort and diffcore-rename work that I have and will be doing.

In summary:
  * en/ort-conflict-handling and en/diffcore-rename are ready to merge
down (and you already started on one of them)
  * I'm not sure if anyone will review en/ort-directory-rename (it's
been a week and a half with no review so far), but I'm tempted to
encourage people to save their review effort for other topics.
There's just not much different here than what's found in
merge-recursive.c.  (With the exception of patch 17...)

Show 6 quoted lines

> It does not instill a lot of confidence in these topics that nobody
> commented on things like [v2 17/17] of en/ort-directory-rename
> (which fixes the code introduced in [v2 06/17] instead of fixing it
> at the source before 06/17 copies it from elsewhere [*1*]).  It
> looks to me like a sign that these collection of series are moving
> too fast than any reviewer to catch up.

I considered just moving 17/17 earlier, and even started that way, but there are a couple problems with that:

1) The comparison of directory rename detection code between
merge-recursive.c and merge-ort.c, if folks want to compare the two,
is much easier if I first implement the same algorithm

2) 06/17 introduces the concept of counting renames from a directory
and picking the highest count; the idea is simple but but with the
extra complications from 17/17 the combined patch just looks too
obtuse to follow done all in one step.  Splitting 17/17 into a
separate later step was something that I felt would make it easier to
review.

3) 06/17 consists of well-understood code from merge-recursive.c.
17/17 is new.  If we ever want to port the fix from merge-ort to
merge-recursive, having it as a separate change will help with that.

So, although it may look weird, I intentionally changed from a combined early commit, as you seem to be suggesting here, into splitting it out this way. I also considered removing 17/17 from the series and then submitting it later.

Show 12 quoted lines

> Thanks.
>
>
> [Footnote]
>
> *1* I do not particularly think [06/17] that copies and leaves the
>     recursive backend behind is necessarily bad.  What I find more
>     disturbing is that nobody seems to have a chance to give any
>     look to the two iterations of the series (as far as I can see,
>     v2 is just in name only---it removed 18/17 in v1 that were not
>     meant to be sent), and we are about to build on top of the
>     foundation that nobody knows how solid it is.

What if we dropped 17/17 from en/ort-directory-rename (the merge/rename performance work doesn't depend upon it), and then merged that series down? Without 17/17, the series is just a port of code that exists in merge-recursive.c that has been battle tested for years. I could submit the final patch separately later.