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

From: Jeff King <peff@peff.net>
Date: Feb 22, 2019, 15:08 UTC
Message-ID: <20190222150852.GB5090@sigill.intra.peff.net>
In-Reply-To: <20190221214059.9195-3-avarab@gmail.com>

On Thu, Feb 21, 2019 at 10:40:59PM +0100, Ævar Arnfjörð Bjarmason wrote:

> Fix a regression introduced in 4f21454b55 ("merge-base: handle
> --fork-point without reflog", 2016-10-12).
> [...]

OK, your explanation mostly makes sense to me, except for one thing.

Show 5 quoted lines

> Then in 4f21454b55 ("merge-base: handle --fork-point without reflog",
> 2016-10-12) which introduced the regression being fixed here, a bug
> fix for "git merge-base --fork-point" being run stand-alone by proxy
> broke this use-case git-rebase.sh was relying on, since it was still
> assuming that if we didn't have divergent history we'd have no output.

I still don't quite see how 4f21454b55 is involved here, except by returning a fork-point value when there is no reflog, and thus triggering the bug in more cases.

In particular, imagine this case:

  git init
  for i in $(seq 1 3); do echo $i >$i; git add $i; git commit -m $i; done
  git checkout -t -b other
  for i in $(seq 4 6); do echo $i >$i; git add $i; git commit -m $i; done
  git rebase

With the current code, that will rewind and replay 4-6, and I understand that to be a bug from your description. And it happens at 4f21454b55, too. But it _also_ happens at 4f21454b55^.

I.e., I still think that the only thing that commit changed is that we found a fork-point in more cases. But the bug was still demonstrably there when you actually have a reflog entry.

With the fix you have here, that case now produces "Current branch other is up to date".

This is splitting hairs a little (and of course I'm trying to exonerate the commit I'm responsible for ;) ), but I just want to make sure we understand fully what's going on. Your fix looks plausibly correct to me, but I admit I don't quite grok all the details of that conditional.

-Peff