Re: [RFC PATCH] add t3420-rebase-topology

From: Martin von Zweigbergk <martinvonz@gmail.com>
Date: Sep 21, 2012, 17:06 UTC
Message-ID: <CANiSa6iQsxWYHTRDGNg_h77rr3Y1cL_di-Z3zzR4gZvcRHtVqQ@mail.gmail.com>
In-Reply-To: <7vzk4nojkd.fsf@alter.siamese.dyndns.org>

On Tue, Sep 18, 2012 at 12:51 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> Martin von Zweigbergk <martinvonz@gmail.com> writes:
>
>> do you agree
>> that 'rebase --onto does not re-apply patches in onto' is desirable?
>
> This depends on how you look at --onto.  Recall the most typical and
> the original use case of rebase:
>
>                                A'--C' your rebased work
>                               /
>   ---o---o---o---F---B'--o---T master
>      ^            \
>      v1.7.12       A---B---C your work
>
> You could view "git rebase master" as a short-hand for
>
>         $ git rebase --onto $(git merge-base master HEAD) master

Exactly. I frequently consider it a short-hand for that. It might be worth pointing out that 'git pull --rebase', which might be one of the most frequent uses of rebase, internally often does

  git rebase --onto upstream upstream@{...} branch

where upstream@{...} is the most recent upstream that is an ancestor of "branch". For example, if your work is based on origin/pu and you send the bottom-most patch ("B" in the figure below) to the maintainer and and it gets applied to pu. Running "git pull --rebase" would then lead to "git rebase --onto T A". You would want this to drop B.

                               C' your rebased work
                              /
  ---o---o---o---F---B'--o---T origin/pu
                  \
                   A origin/pu@{1}
                    \
                     B---C your work

Show 5 quoted lines

> The intended use case for "--onto", however, is primarily to replay
> a history to older base, i.e. [...] a moral equivalent of
>
>         $ git checkout v1.7.12
>         $ git cherry-pick A B C ;# or git cherry-pick master..HEAD

Yes, this is the alternative way of looking it at and exactly why I, too, was not sure how it should behave.

> You could argue that you can compute the patch equivalence between
> the commits in "onto..master" and commits in "master..HEAD" and
> filter out the equivalent commits

I'm not sure if you meant "master..onto" rather than "onto..master". Rebase (well, all flavors of rebase but "-m") currently drops patches from "master..HEAD" that are also in "HEAD..master". This is what the "rebase --onto does not lose patches in upstream" test is about. It is also one of the main problems that I try to fix in my long-stalled rebase-range series. I think we should drop patches in "master..HEAD" that are also in "HEAD..onto" (which is almost the same as "master..onto").

> The "replay to an updated base" case (i.e. without "--onto")

Or _with_ --onto as in the above example from "git pull --rebase".

Show 5 quoted lines

> On the other hand, when the user replays to an older base, she has
> some idea what constitutes "a series" that she is replaying (i.e.
> "$(git merge-base master HEAD)..HEAD").  It smells to go against the
> user's world model if the command silently filtered commits by patch
> equivalence.

If it's truly about rebasing onto an older base, there can't possibly be any patches in "HEAD..onto", so assuming you agree with my reasoning above that those are the patches we should drop, rebasing onto older history would be safe.

Show 6 quoted lines

> Besides, the whole point of a separate "onto" is to allow the user
> to specify a commit that does not have a straightforward ancestry
> relationship with the bottom of the series (i.e. either "master" or
> "F"), and computation of patch equivalence is expected to be much
> higher.  Given that it is unlikely to find any match, it feels
> doubly wrong to always run "git cherry" equivalent in that case.

Yes, this was my only concern (apart from it possibly being conceptually wrong to do, depending on what the user meant by issuing the command).

Show 14 quoted lines

>> How about 'rebase --root is not a no-op'?
>
>   ---o---o---o---F---B'--o---T master
>      ^            \
>      v1.7.12       A---B---C your work
>
> If "git rebase F" when you are at C in the above illustration
> (reproduced only the relevant parts) is a no-op (and I think it
> should be), "git rebase --root" in the illustration below ought to
> be as well, no?
>
>                  F---B'--o---T master
>                   \
>                    A---B---C your work

Yeah, that's what I thought as well at first. I think my test case even started out as "rebase --root _is_ a no-op".

When thinking about how to handle roots in general, I often imagine a single virtual root commit (parent of all "initial" commits), and that reasoning also implies that "git rebase --root" should be a no-op. Then I saw that the test case failed (or perhaps I remembered how it is implemented with the clever fake root/initial commit) and started thinking about why anyone would use "git rebase --root" if it was a no-op. I could only think of using it to linearize history, but that doesn't seem like a very likely use case. So it seems like weighing purity/correctness against usefulness to me. I'm not sure which way to go.

Thanks for quick and detailed feedback on an RFC patch.

Martin