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

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

[Phil Hord]

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.

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.
---
 git-rebase--interactive.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 44901d5..8ed7fcc 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -190,6 +190,11 @@ is_empty_commit() {
 	test "$tree" = "$ptree"
 }
 
+is_merge_commit()
+{
+	git rev-parse --verify --quiet "$1"^2 >/dev/null 2>&1
+}
+
 # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
 # GIT_AUTHOR_DATE exported from the current environment.
 do_with_author () {
@@ -874,7 +879,7 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \
 while read -r shortsha1 rest
 do
 
-	if test -z "$keep_empty" && is_empty_commit $shortsha1
+	if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
 	then
 		comment_out="# "
 	else
-- 
1.8.1.dirty

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

[Neil Horman]

On Sat, Jan 12, 2013 at 03:46:01PM -0500, Phil Hord wrote:
> 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.
> 
> 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.
> ---
>  git-rebase--interactive.sh | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 44901d5..8ed7fcc 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -190,6 +190,11 @@ is_empty_commit() {
>  	test "$tree" = "$ptree"
>  }
>  
> +is_merge_commit()
> +{
> +	git rev-parse --verify --quiet "$1"^2 >/dev/null 2>&1
> +}
> +
>  # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
>  # GIT_AUTHOR_DATE exported from the current environment.
>  do_with_author () {
> @@ -874,7 +879,7 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \
>  while read -r shortsha1 rest
>  do
>  
> -	if test -z "$keep_empty" && is_empty_commit $shortsha1
> +	if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
>  	then
>  		comment_out="# "
>  	else
> -- 
> 1.8.1.dirty
> 
> 
Seems reasonable 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

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

[Matthieu Moy]

Phil Hord <hordp@cisco.com> writes:

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

I would rephrase it as

  rebase --preserve-merges: keep empty merge commits

we usually give orders in commit messages, not state facts (it's not
clear from the existing subject line whether keeping merge commit is the
new behavior or a bug that the commit tries to fix).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Phil Hord <hordp@cisco.com> writes:
>
>> Subject: [PATCH] rebase --preserve-merges keeps empty merge commits
>
> I would rephrase it as
>
>   rebase --preserve-merges: keep empty merge commits
>
> we usually give orders in commit messages, not state facts (it's not
> clear from the existing subject line whether keeping merge commit is the
> new behavior or a bug that the commit tries to fix).

Thanks for giving a concise rationale on our use of imperative mood.

Phil, I think you meant to and forgot to sign-off; here is what I'll
queue.

Thanks.

-- >8 --
From: Phil Hord <hordp@cisco.com>
Date: Sat, 12 Jan 2013 15:46:01 -0500
Subject: [PATCH] rebase --preserve-merges: keep all merge commits including empty ones

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.

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.

Signed-off-by: Phil Hord <hordp@cisco.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-rebase--interactive.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0c19b7c..2fed92f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -175,6 +175,11 @@ is_empty_commit() {
 	test "$tree" = "$ptree"
 }
 
+is_merge_commit()
+{
+	git rev-parse --verify --quiet "$1"^2 >/dev/null 2>&1
+}
+
 # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
 # GIT_AUTHOR_DATE exported from the current environment.
 do_with_author () {
@@ -796,7 +801,7 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \
 while read -r shortsha1 rest
 do
 
-	if test -z "$keep_empty" && is_empty_commit $shortsha1
+	if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1
 	then
 		comment_out="# "
 	else
-- 
1.8.1.1.338.g126d652

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

[Phil Hord]

Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Phil Hord <hordp@cisco.com> writes:
>>
>>> Subject: [PATCH] rebase --preserve-merges keeps empty merge commits
>> I would rephrase it as
>>
>>   rebase --preserve-merges: keep empty merge commits
>>
>> we usually give orders in commit messages, not state facts (it's not
>> clear from the existing subject line whether keeping merge commit is the
>> new behavior or a bug that the commit tries to fix).
> Thanks for giving a concise rationale on our use of imperative mood.
>
> Phil, I think you meant to and forgot to sign-off; here is what I'll
> queue.
>
> Thanks.
>

Looks good.  Thanks for the help.

Phil

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

[Martin von Zweigbergk]

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.

On Sat, Jan 12, 2013 at 12:46 PM, Phil Hord <hordp@cisco.com> wrote:
> 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

> 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" 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?

Martin

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

[Phil Hord]

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 <hordp@cisco.com> wrote:
>> 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.

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

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

[Martin von Zweigbergk]

On Fri, Feb 1, 2013 at 1:05 PM, Phil Hord <hordp@cisco.com> wrote:
>
> This is probably right, but it is not exactly the case that caused my itch.
> I think my branch looked like [...]

That also makes sense. I'll add tests for both cases. Your patch makes
both of them pass.

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

Neither do I, so I'll just drop the test case. Thanks.

> 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 think this is actually what you will get. But I think it will only
be linearized if the branch that should be dropped is the second
parent. I have two tests for this, but I need to simplify them a
little to see that that (parent number) is the only difference.

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

Very clear. Thanks.

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

[Junio C Hamano]

Martin von Zweigbergk <martinvonz@gmail.com> writes:

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

Did anything further happen to this topic?