Subject: Re: [PATCH v3 0/4] rebase: teach rebase --keep-base

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Sat, 13 Apr 2019 23:10:43 +0200

On Sat, Apr 06 2019, Denton Liu wrote:

> On Sat, Apr 06, 2019 at 09:44:49PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Mon, Apr 01 2019, Denton Liu wrote:
>>
>> > Thanks again for your feedback, Ævar! I think we're both on the same page now.
>> > Hopefully I've addressed all of your high-level concerns with this patchset and
>> > we can move into a discussion on implementation detail.
>>
>> Late in replying to this, have been off-list. This also applies for your
>> v4.
>>
>> The current version you have still doesn't explain the "Why would we
>> redundantly rebase every time in this case..." question I had in
>> https://public-inbox.org/git/[email-masked]/
>>
>> I *think* it's closer to "it was easier to implement this in terms of
>> --onto, which happens to behave that way now" than "it must work this
>> way for --keep-base", which is fair enough.
>
> Correct, the reason why --keep-base was not lazy initially was because
> "--onto did it that way". You are correct in that --keep-base should be
> lazily rebasing so I changed --onto's behaviour in 3/4 because it would
> also benefit from laziness. Thus, now --keep-base lazily rebases because
> --onto also does.
>
>>
>> Although I see when I forward-port my POC patch from that E-Mail that
>> one test fails now, which is good, that wasn't the case before, but it
>> looks like that might be testing something else than just the lazy
>> behavior.
>
> The test fails because the patch disables fork_point if --keep-base is
> set. So, with the patch applied, C is rebased even though it is excluded
> when fork_point is set.
>
>>
>> It would be good to know in terms of commit message or (better) explicit
>> tests so that if we teach these various rebase modes the same lazyness
>> --fork-point uses in the future it's clear if that's OK or not.
>
> Sorry, could you please clarify what you mean by the "lazyness
> --fork-point uses"? I don't understand what laziness is introduced by
> using --fork-point. Also, are the tests in t3432 not sufficient for
> testing fast-forwarding (aka lazy) behaviour?

Late reply, sorry. I mean if I do e.g. on git.git:

    git checkout -b avar/test 041f5ea1cf

I.e. branch of Junio's "The third batch" (we're now on the 4th) and
then:

    git branch --set-upstream-to origin/master

And commit a dummy change, I'm now:

    On branch avar/test
    Your branch and 'origin/master' have diverged,
    and have 1 and 33 different commits each, respectively.

Then I run a --keep-base rebase twice:

    $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --keep-base; git rev-parse HEAD
    fc3e916c5f foo
    041f5ea1cf The third batch
    First, rewinding head to replay your work on top of it...
    Applying: foo
    b10e672074dfee6b6e8b2901c9bb49f856a13708
    $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --keep-base; git rev-parse HEAD
    b10e672074 foo
    041f5ea1cf The third batch
    First, rewinding head to replay your work on top of it...
    Applying: foo
    fd3029e73b89f5a799653ff17199d00f2a6ee2e2

I.e. I'll keep on getting a new commit, even though by any criteria I
can think of for this type of case there's no work to do. I.e. we're
already based on 041f5ea1cf, no need to rebase again.

Of course this is also currently the case with --fork-point without
argument, which'll settle on (note fourth, not third):

    $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --fork-point; git rev-parse HEAD
    85a1861cec foo
    e35b8cb8e2 The fourth batch
    First, rewinding head to replay your work on top of it...
    Applying: foo
    c6046e97af29d71bf6270080acf188c095e0cb7c
    $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --fork-point; git rev-parse HEAD
    c6046e97af foo
    e35b8cb8e2 The fourth batch
    First, rewinding head to replay your work on top of it...
    Applying: foo
    672d22d58e9aa9b6a28054531c21e1f1b598b013

Whereas a --keep-base in *this* case should be identical to:

    $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase $(git merge-base --fork-point origin/master HEAD); git rev-parse HEAD
    fc3e916c5f foo
    041f5ea1cf The third batch
    Current branch avar/test is up to date.
    fc3e916c5fc707cfc83f28b3faca81046306b305

The reason I'm asking is that as noted in the thread starting at
https://public-inbox.org/git/[email-masked]/
we used to in *some* cases do the same for:

    rebase $(git merge-base --fork-point origin/master HEAD)

As for just:

    rebase --fork-point

I.e. say "Current branch is up-to-date". I'm planning to loop back to
that and fix it for the --fork-point case. So I was wondering if you
could think of a reason for why --keep-base couldn't also do the same,
or if there was some intrinsic difference I'm missing.

Anyway. Don't worry about it. When I poke at it it'll likely shake out
of the respective tests.

I was just wondering if you having looked at many of the same things
recently could understand my ramblings and knew if this was a case of
"yup, we could add a bit more to that can_fast_forward(...) case and
make it lazy too", or "no, --keep-base is special for reasons you're
missing...".