Re: [PATCHv2] Teach --no-ff option to 'rebase -i'.

From: Marc Branchaud <marcnarc@xiplink.com>
Date: Mar 17, 2010, 15:56 UTC
Message-ID: <4BA0FBC7.3080305@xiplink.com>
In-Reply-To: <20100316214717.GA24880@progeny.tock>

Jonathan Nieder wrote:

Show 20 quoted lines

> Hi Marc,
> 
> Marc Branchaud wrote:
> 
>> This option tells rebase--interactive to cherry-pick all the commits in the
>> rebased branch, instead of fast-forwarding over any unchanged commits.
>>
>> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
> [...]
>> This offers an alterntive way to deal with reverted merges (the
>> revert-a-faulty-merge.txt howto recommends reverting the initial reversion
>> before re-merging a modified topic branch).
>>
>> With this change, you can instead use the --no-ff option to recreate the
>> branch with entirely new commits (they're new because at the very least the
>> committer time is different).  This obviates the need to revert the
>> reversion, as you can re-merge the new topic branch directly.
> 
> I think this rationale should go in the commit message, for the sake of
> future readers studying the code you’ve changed.

Easy to do, but how necessary? The rationale is already in the description of --no-ff in rebase's man page.

There's an extra aspect to my motivation that I failed to mention, which is that I would like the topic branch to stay rooted at its original commit. This is because in our workflow that original commit is a synchronization point for several maintenance and "next"-style branches, and keeping the topic rooted there makes it easy to merge into different branches.

Show 34 quoted lines

> But let me make sure I understand it.
> 
> If I am understanding properly, your idea is that this would be used on
> a branch after “unmerging” it from master:
> 
>     B --- C --- D [topic]
>   /              \
>  A ---  ...   --- M ... --- U [master]
> 
> Here M is a merge commit and U a commit reverting the change from M^
> to M.
> 
> As howto/revert-a-faulty-merge explains, this is an accident waiting to
> happen: if you merge master into topic, then it will pull in the revert
> along with the rest of the changes.  Similarly, if you merge topic into
> master, it will be a no-op (“already up to date”).
> 
> Your solution, to rewrite the branch, should address both of these
> problems.  The new history looks like this:
> 
>     B' --- C' --- D' [topic]
>   /
>  |  B --- C --- D
>  |/              \
>  A ---   ...  --- M ... --- U [master]
> 
> What happens now if you merge topic into master (or master into topic)?
> 
> Git will try to reconcile the changes from the two branches since when
> they diverged.  master gained the changes from the old topic branch and
> then reverted them since it diverged from topic; since merge algorithm
> is a simple three-way merge, all that history is ignored and Git will
> just combine any new changes from master with the changes from topic.
> Success.

Yup, you got it!

> Side note:
> 
> After this maneuver, I would want to merge ‘master’ into ‘topic’ as
> soon as possible.

Well, I wouldn't want to do that because it would make it impossible to merge the topic into some other branch without also bringing in all of master.

Show 5 quoted lines

> Why?  Because there is still a failure mode possible
> after this: if you merge any commits from the old topic into the new
> one before then, you’re back in trouble.  This could happen if some
> topic2 that topic ends up needing is based on a point in master after M
> and before U.

If the re-cast topic1 doesn't rewrite those commits, then the merge will simply succeed because the code is already identical in both branches.

But if topic1 does rewrite those commits then there'll be a conflict. IMO that's correct, because with the merging of topic2 the code in master really did diverge in a relevant way from what's in topic1, so that conflict should get resolved in the normal way.

> After merging master into topic, the _meaning_ of the history is much
> clearer: by making U an ancestor, we are saying that as far as this
> branch is concerned, the state at the tip of the topic branch is to be
> preferred over the reverted state.

Yes, but it also means that all the history of master since commit A is now part of the topic, which isn't acceptable to me.

Show 21 quoted lines

> This suggests an alternative method to achieve your goal:
> 
>  git checkout master
>  git revert -m 1 M;	# (1)
>  git checkout topic
>  git merge master^;	# (2)
>  git merge -s ours master;	# (3)
> 
> 1. Add a commit U removing the changes introduced by topic from master.
> 
> 2. Merge all changes from master before the revert into topic.
> 
> 3. Declare the changes from topic to supersede the master branch.
>    This way, if topic is ever merged back to master, the changes will be
>    reintroduced again.
> 
> Resulting history:
> 
>     B --- C --- D --- ... --- T^ --- T [topic]
>   /              \            /     /
>  A ---   ...  --- M ... --- U^ --- U [master]

I understand how this achieves your goal but, aside from dragging all of master into the topic, this is way more complicated than recreating the topic or even doing the double-revert. By creating T with "-s ours" U becomes like a surrogate ancestor of the topic -- it's there, but it doesn't share any genetic material with the topic. I think that's a recipe for confusion to future explorers of the repo's history.

Besides, I do expect at least one of the topic's original commits to actually be different in the recast topic -- the original merge was reverted because the topic was faulty in some way, so there needs to be a real change made somewhere. If the topic could simply be fixed with an extra commit or two after D then re-merged, there wouldn't be a need to revert the original merge in the first place.

Show 16 quoted lines

> You can avoid the perhaps pointless commit T^ by using --no-commit in
> the command (2).  If not all changes from M..master are suitable for
> merging into topic, you can use the same trick on a temporary side
> branch:
> 
>  git checkout -b revert-topic M
>  git revert -m 1 M
>  git checkout topic
>  git merge --no-commit revert-topic^
>  git merge -s ours revert-topic
>  git checkout master
>  git merge revert-topic
> 
> If not all changes from topic..M are suitable for merging into topic,
> then things are harder.  I’d suggest making a special unrevert-topic
> branch as above to keep in the wings until its time.

This still brings all of the master's A..M^ commits into the topic.

Show 12 quoted lines

>> (Honestly, I wouldn't say that this approach is vastly superior to
>> reverting the reversion.  I just find it a little less messy and a little
>> more intuitive.  It's also a bit easier to explain to people to "use --no-ff
>> after reverting a merge" instead of making sure they get the double-
>> reversion right.)
> 
> Hope this helps clarify things.
> 
> Your patch itself might still be a good idea.  I think plain
> ‘git rebase’ already has something like it, in the form of the
> --force-rebase option.  I have used it before, though I don’t remember
> why.

No, the man page says --force-rebase does something else.

Thanks for the feedback!

M.