Re: A rebase regression in Git 2.18.0

From: Elijah Newren <newren@gmail.com>
Date: Aug 31, 2018, 19:37 UTC
Message-ID: <CABPp-BEJ-JPa8g8G1ELsjqf3EDxe+aCBdwERBi5hr5ukrDjJJg@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1808311158540.71@tvgsbejvaqbjf.bet>

Hi Dscho,

On Fri, Aug 31, 2018 at 3:12 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> On Thu, 30 Aug 2018, Elijah Newren wrote:
> > On Tue, Aug 28, 2018 at 9:58 AM Junio C Hamano <gitster@pobox.com> wrote:
> > > Elijah Newren <newren@gmail.com> writes:

...

Show 11 quoted lines

> > > I'd say this is the only practical solution, before you deprecate
> > > the "pipe format-patch output to am -3" style of "git rebase" (and
> > > optionally replace with something else).
> >
> > I posted a patch a while back to add an --am flag to "git rebase",
> > make "--am" be implied by options which are still am-specific
> > (--whitespace, --committer-date-is-author-date, and -C), and change
> > --merge to be the default.
>
> Didn't you also post a patch to fold --merge into the --interactive
> backend? What's your current state of thinking about this?

Yes. I updated it once or twice, but it had conflicts with the GSoC projects each time, so I decided to just hold off on it a bit longer. I'm still planning to resubmit this once the GSoC projects merge down.

Show 5 quoted lines

> As to switching from --am as the default: I still think that --am has
> serious speed advantages over --merge (or for that matter, --interactive).
> I have no numbers to back that up, though, and I am currently really busy
> with working on the CI, so I won't be able to measure these numbers,
> either...

Yep, we talked about this before and you mentioned that the rewrite in C should bring some performance improvements, and we agreed that merge-recursive is probably the next issue performance-wise. I think it's at least worth measuring what the approximate performance differences are with the rewrite of rebase in C, and posting an RFC with that info. If the answer comes back that we need to do more optimization before we switch the default, that's fine.

> Also please note: I converted the `am` backend to pure C (it is waiting at
> https://github.com/gitgitgadget/git/pull/24, to be submitted after the
> v2.19.0 RC period). Switching to `--merge` as the default would force me
> to convert that backend, too ;-)

Not if git-rebase--merge is deleted and --merge is implemented on top of the interactive backend as an implicitly_interactive case. In fact, that's probably the simplest way to "convert" that backend to C. Anyway, since I plan to submit that change first, we should be good.

Show 7 quoted lines

> > I'll post it as an RFC again after the various rebase-rewrite series
> > have settled and merged down...along with my other rebase cleanups
> > that I was waiting on to avoid conflicts with GSoC stuff.
>
> Thanks for waiting! Please note that I am interested, yet I will be on
> vacation for a couple of weeks in September. Don't let that stop you,
> though!

Enjoy your vacation!

Show 11 quoted lines

> > > The whole point of "am -3" is to do _better_ than just "patch" with
> > > minimum amount of information available on the pre- and post- image
> > > blobs, without knowing the remainder of the tree that the patch did
> > > not touch.  It is not surprising that the heuristics that look at
> > > the unchanging part of the tree to infer renames that may or may not
> > > exist guesses incorrectly, either with false positive or negative.
> > > In the context of "rebase", we always have all the trees that are
> > > involved.  We should be able to do better than "am -3".
>
> Right. I think that Elijah's right, and --merge is that "do better"
> solution.

Cool, good to see others seem to agree on the direction I'd like to see things move.