Re: [PATCH 2/2] rebase: don't rebase linear topology with --fork-point

From: Jeff King <peff@peff.net>
Date: Feb 24, 2019, 10:10 UTC
Message-ID: <20190224101029.GA13438@sigill.intra.peff.net>
In-Reply-To: <871s3z6a4q.fsf@evledraar.gmail.com>

On Fri, Feb 22, 2019 at 05:49:57PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 30 quoted lines

> Yes. I didn't dig far enough into this and will re-word & re-submit,
> also with the feedback you had on 1/2.
> 
> So here's my current understanding of this.
> 
> It's b6266dc88b ("rebase--am: use --cherry-pick instead of
> --ignore-if-in-upstream", 2014-07-15) that broke this in the general
> case.
> 
> I.e. if you set a tracking branch within the same repo (which I'd
> betnobody does) but *also* if you have an established clone you have a
> reflog for the upstream. Then we'll find the fork point, and we'll
> always redundantly rebase.
> 
> But this hung on by a thread until your 4f21454b55 ("merge-base: handle
> --fork-point without reflog", 2016-10-12). In particular when you:
> 
>  1. Clone some *new* repo
>  2. commit on top
>  3. git pull --rebase
> 
> You'll redundantly rebase on top, even though you have nothing to
> do. Since there's no reflog.
> 
> This is why I ran into this most of the time, because my "patch some
> random thing" is that, and I have pull.rebase=true in my config.
> 
> What had me confused about this being the primary cause was that when
> trying to test this I was re-cloning, so I'd always get this empty
> reflog case.

OK, thanks, that all makes sense to me and matches what I'm seeing. I think we're on the same page now.

Show 7 quoted lines

> > Your fix looks plausibly correct to me, but I admit I don't quite grok
> > all the details of that conditional.
> 
> We just consider whether we can fast-forward now, and then don't need to
> rebase (unless "git rebase -i" etc.). I.e. that --fork-point was
> considered for "do we need to do stuff" was a bug introduced in
> b6266dc88b.

Right, that makes sense. But I'm just not clear on the new oidcmp() rule that's put in place.

When we're not using fork point, in the old code we'd check whether upstream and onto are the same. Which makes sense; if we're rebuilding onto the same spot, then it's a noop.

And in the fork-point case, we'd instead want to do something similar with restrict_revision. But you compare upstream and restrict_revision, and my understanding is that with --fork-point the latter basically replaces the former. Shouldn't we be comparing restrict_revision and onto?

E.g., if I do:

  git checkout -b foo origin
  echo content >file && git add file && git commit -m foo
  git rebase --onto origin^ --fork-point origin

we should do an actual rebase, but with your patch we get "Current branch foo is up to date". Whereas dropping --fork-point, it works.

-Peff