Re: [PATCH] rebase --preserve-merges keeps empty merge commits

Martin von Zweigbergk wrote:

I'm working on a re-roll of
http://thread.gmane.org/gmane.comp.version-control.git/205796

and finally got around to including test cases for what you fixed in
this patch. I want to make sure I'm testing what you fixed here. See
questions below.

Thanks for that.  I should have done this myself.

On Sat, Jan 12, 2013 at 12:46 PM, Phil Hord [off-list ref] wrote:

quoted

Since 90e1818f9a  (git-rebase: add keep_empty flag, 2012-04-20)
'git rebase --preserve-merges' fails to preserve empty merge commits
unless --keep-empty is also specified.  Merge commits should be
preserved in order to preserve the structure of the rebased graph,
even if the merge commit does not introduce changes to the parent.

Teach rebase not to drop merge commits only because they are empty.

Consider a history like

# a---b---c
#      \   \
#       d---l
#        \
#         e
#          \
#           C

where 'l' is tree-same with 'd' and 'C' introduces the same change as 'c'.

My test case runs 'git rebase -p e l' and expects the result to look like

# a---b---c
#      \   \
#       d   \
#        \   \
#         e---l

This is probably right, but it is not exactly the case that caused my itch.
I think my branch looked like this:

# a---b---c
#      \   
#       d---f
#        \   \
#         e---g
#              \
#               l

where g is tree-same with f.  That is, e merged with f, but all of e's
changes were dropped in the merge.

So when I ran 'git rebase -p c l', I expected to end up with this:

# a---b---c
#          \   
#           d---f
#            \   \
#             e---g
#                  \
#                   l

But instead, I got an error because git-rebase--interactive.sh decided
that g was empty, so it dropped it by commenting it out of the todo
list:

pick d
pick e
pick f
#pick g
pick l

At the end of this attempt, I got some odd error about a cherry-pick
have incorrect parameters or somesuch.  I bisected the problem to a
commit that clued me in to one of my commits being silently dropped.
And that is specifically what I fixed.

This happened only because 'is_empty_commit' checks for tree-sameness
with the first parent; it does not consider whether there are multiple
parents.  Perhaps it should.

quoted

A special case which is not handled by this change is for a merge commit
whose parents are now the same commit because all the previous different
parents have been dropped as a result of this rebase or some previous
operation.

And for this case, the test case runs 'git rebase -p C l'. Is that
what you meant here?

Before your patch, git would just say "Nothing to do"

Huh.  That is worse than I thought.

and after your
patch, we get

# a---b---c
#      \   \
#       d   \
#        \   \
#         e   \
#          \   \
#           C---l

As you say, your patch doesn't try to handle this case, but at least
the new behavior seems better. I think we would ideally want the
recreated 'l' to have only 'C' as parent in this case. Does that make
sense?

This is not what I meant, but it is a very interesting corner case.  I
am not sure I have a solid opinion on what the result should be here.
I feel like it should look the same as you show here, since neither
'c' nor 'C' is a candidate for collapsing during this rebase.  But I may
be missing some subtlety here.


Here is the corner case I was thinking of.  I did not test this to see
if this will happen, but I conceived that it might.  Suppose you have
this tree where

# a---b---c
#      \   
#       d---g---l
#        \ /
#         C

where 'C' introduced the same changes as 'c'.

When I execute 'git rebase -p l c', I expect that I will end up with
this:

# a---b---c---d---
#              \  \
#               ---g---l

That is, 'C' gets skipped because it introduces the same changes already
seen in 'c'.  So 'g' now has two parents: 'd' and 'C^'.  But 'C^' is 'd',
so 'g' now has two parents, both of whom are 'd'.  

I think it should collapse to this instead:

# a---b---c---d---g---l

I don't think this occurs because of my patch, and I am not sure it
occurs at all.  It is something that I considered when I was thinking of
failure scenarios for my patch.

I expect it also may happen if 'C' is an already-empty commit, or if
it is made empty after conflict resolution involving the user. I
mentioned it because I thought my patch _could_ address this if my
is_merge_commit test would also consider whether the parents are
distinct from each other or not.

I hope this is clear, but please let me know if I made it too confusing.

Phil