git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Derrick Stolee <dstolee@microsoft.com>, Jeff King <peff@peff.net>,
	Jonathan Nieder <jrnieder@gmail.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Taylor Blau <me@ttaylorr.com>
Subject: Re: [PATCH v2 0/1] And so it begins...merge/rename performance work
Date: Thu, 14 Jan 2021 12:18:17 -0800	[thread overview]
Message-ID: <CABPp-BHhXXAX7QJeD6jGckozL9DHf5UZ=UXzaogyv-F4_xdYVQ@mail.gmail.com> (raw)
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:
>
> 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...)

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

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

  reply	other threads:[~2021-01-14 20:19 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-08 20:51 [PATCH 0/1] And so it begins...merge/rename performance work Elijah Newren
2021-01-08 20:51 ` [PATCH 1/1] merge-ort: begin performance work; instrument with trace2_region_* calls Elijah Newren
2021-01-08 20:59   ` Taylor Blau
2021-01-08 21:50     ` Elijah Newren
2021-01-08 21:55       ` Taylor Blau
2021-01-09  0:52         ` Elijah Newren
2021-01-13 22:11 ` [PATCH v2 0/1] And so it begins...merge/rename performance work Elijah Newren
2021-01-13 22:11   ` [PATCH v2 1/1] merge-ort: begin performance work; instrument with trace2_region_* calls Elijah Newren
2021-01-14 19:08   ` [PATCH v2 0/1] And so it begins...merge/rename performance work Junio C Hamano
2021-01-14 20:18     ` Elijah Newren [this message]
2021-01-15 19:29   ` [PATCH v3 " Elijah Newren
2021-01-15 19:29     ` [PATCH v3 1/1] merge-ort: begin performance work; instrument with trace2_region_* calls Elijah Newren
2021-01-24  6:01     ` [PATCH v4 0/3] And so it begins...merge/rename performance work Elijah Newren
2021-01-24  6:01       ` [PATCH v4 1/3] merge-ort: fix massive leak Elijah Newren
2021-01-24 19:11         ` Derrick Stolee
2021-01-24  6:01       ` [PATCH v4 2/3] merge-ort: ignore the directory rename split conflict for now Elijah Newren
2021-01-24  6:01       ` [PATCH v4 3/3] merge-ort: begin performance work; instrument with trace2_region_* calls Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BHhXXAX7QJeD6jGckozL9DHf5UZ=UXzaogyv-F4_xdYVQ@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=jrnieder@gmail.com \
    --cc=me@ttaylorr.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).