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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 22, 2019, 16:49 UTC
Message-ID: <871s3z6a4q.fsf@evledraar.gmail.com>
In-Reply-To: <20190222150852.GB5090@sigill.intra.peff.net>

On Fri, Feb 22 2019, Jeff King wrote:

Show 40 quoted lines

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

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.

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