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

[Jeff King <peff@peff.net>]

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

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

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