[PATCH 0/2] rebase: broken behavior with --keep-empty

[PATCH 0/2] rebase: broken behavior with --keep-empty

[Patrick Steinhardt]

I've recently run into broken behavior with `git-rebase` and
`--keep-empty`. As soon as `--keep-empty` is specified, we fall
back on using `git cherry-pick` instead of `git am` for rebasing
the commits, which seems to not work correctly.

In fact I guess there are two different bugs in here. The first
bug is in using `git cherry-pick --allow-empty` instead of `git
cherry-pick --keep-redundant-commits`, as judging from
git-rebase(1) we also want to keep commits that are already
included in the branch that is being rebased upon whe we use
`--keep-empty`.

The second bug is that we fail to correctly record the
rebasing-state when using cherry-pick. This causes us to end up
in cherry-picking mode as soon as the invoked cherry-pick command
runs into any error (e.g. due to a conflict or the broken
behavior with `--allow-empty`). There is also no possibility to
get back into rebase-mode from here.

I've written two tests that document these breakages, as well as
a fix to the first described breakage. I did not fix the second
breakage of not recording rebase-state, though, as this is
somewhat more involved.

Patrick

Patrick Steinhardt (2):
  rebase: test broken behavior with --keep-empty
  rebase: fix preserving commits with --keep-empty

 git-rebase--am.sh |  2 +-
 t/t3400-rebase.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

-- 
2.6.3

[PATCH 1/2] rebase: test broken behavior with --keep-empty

[Patrick Steinhardt]

When a commit is already present in the branch that is being
rebased upon we get an empty commit. Usually we just drop this
commit, but with `--keep-empty` we want to preserve the commit's
message.

Instead of simply applying the empty commit, though, we
erroneously end up in cherry-picking mode without any ability to
get back to the previous rebase.

The same error happens when we try to rebase a commit with
`--keep-empty` that causes a conflict.

Add tests that document this breakage.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 t/t3400-rebase.sh | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 47b5682..6cca319 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -255,4 +255,30 @@ test_expect_success 'rebase commit with an ancient timestamp' '
 	grep "author .* 34567 +0600$" actual
 '
 
+test_expect_failure 'rebase duplicated commit with --keep-empty' '
+	git reset --hard &&
+	git checkout master &&
+
+	>x && git add x && git commit x -mx &&
+	echo x > x && git commit x -mx1 &&
+
+	git checkout -b duplicated HEAD~ &&
+	echo x > x && git commit x -mx2 &&
+	git rebase --keep-empty master
+'
+
+test_expect_failure 'rebase conflicting commit with --keep-empty' '
+	git reset --hard &&
+	git checkout master &&
+
+	echo y > x && git commit x -my &&
+
+	git checkout -b conflict HEAD~ &&
+	echo z > x && git commit x -mz &&
+	test_must_fail git rebase --keep-empty master &&
+
+	git add x &&
+	git rebase --continue
+'
+
 test_done
-- 
2.6.3

[PATCH 2/2] rebase: fix preserving commits with --keep-empty

[Patrick Steinhardt]

When rebasing commits where one or several commits are redundant
to commits on the branch that is being rebased upon we error out.
This is due to the usage of `--allow-empty` for the invoked
cherry-pick command, which will only cause _empty_ commits to be
picked instead of also allowing redundant commits. As
git-rebase(1) mentions, though, we also want to keep commits that
do not change anything from its parents, that is also redundant
commits.

Fix this by invoking `git cherry-pick --keep-redundant-commits`
instead, which will cause redundant commits to be rebased
correctly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 git-rebase--am.sh | 2 +-
 t/t3400-rebase.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 9ae898b..ea7b897 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -44,7 +44,7 @@ then
 	# empty commits and even if it didn't the format doesn't really lend
 	# itself well to recording empty patches.  fortunately, cherry-pick
 	# makes this easy
-	git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --allow-empty \
+	git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --keep-redundant-commits \
 		--right-only "$revisions" \
 		${restrict_revision+^$restrict_revision}
 	ret=$?
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6cca319..f43b202 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -255,7 +255,7 @@ test_expect_success 'rebase commit with an ancient timestamp' '
 	grep "author .* 34567 +0600$" actual
 '
 
-test_expect_failure 'rebase duplicated commit with --keep-empty' '
+test_expect_success 'rebase duplicated commit with --keep-empty' '
 	git reset --hard &&
 	git checkout master &&
 
-- 
2.6.3

Re: [PATCH 2/2] rebase: fix preserving commits with --keep-empty

[Michael Blume]

This test does not seem to pass on my mac.

I've placed the verbose output here:
https://gist.github.com/MichaelBlume/db7ba222be001d502e57

On Fri, Nov 20, 2015 at 4:04 AM, Patrick Steinhardt <ps@pks.im> wrote:
> When rebasing commits where one or several commits are redundant
> to commits on the branch that is being rebased upon we error out.
> This is due to the usage of `--allow-empty` for the invoked
> cherry-pick command, which will only cause _empty_ commits to be
> picked instead of also allowing redundant commits. As
> git-rebase(1) mentions, though, we also want to keep commits that
> do not change anything from its parents, that is also redundant
> commits.
>
> Fix this by invoking `git cherry-pick --keep-redundant-commits`
> instead, which will cause redundant commits to be rebased
> correctly.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  git-rebase--am.sh | 2 +-
>  t/t3400-rebase.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> index 9ae898b..ea7b897 100644
> --- a/git-rebase--am.sh
> +++ b/git-rebase--am.sh
> @@ -44,7 +44,7 @@ then
>         # empty commits and even if it didn't the format doesn't really lend
>         # itself well to recording empty patches.  fortunately, cherry-pick
>         # makes this easy
> -       git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --allow-empty \
> +       git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --keep-redundant-commits \
>                 --right-only "$revisions" \
>                 ${restrict_revision+^$restrict_revision}
>         ret=$?
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 6cca319..f43b202 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -255,7 +255,7 @@ test_expect_success 'rebase commit with an ancient timestamp' '
>         grep "author .* 34567 +0600$" actual
>  '
>
> -test_expect_failure 'rebase duplicated commit with --keep-empty' '
> +test_expect_success 'rebase duplicated commit with --keep-empty' '
>         git reset --hard &&
>         git checkout master &&
>
> --
> 2.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] rebase: fix preserving commits with --keep-empty

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2517 bytes --]

On Thu, Dec 10, 2015 at 02:58:06PM -0800, Michael Blume wrote:
> This test does not seem to pass on my mac.
> 
> I've placed the verbose output here:
> https://gist.github.com/MichaelBlume/db7ba222be001d502e57
> 
> On Fri, Nov 20, 2015 at 4:04 AM, Patrick Steinhardt <ps@pks.im> wrote:
> > When rebasing commits where one or several commits are redundant
> > to commits on the branch that is being rebased upon we error out.
> > This is due to the usage of `--allow-empty` for the invoked
> > cherry-pick command, which will only cause _empty_ commits to be
> > picked instead of also allowing redundant commits. As
> > git-rebase(1) mentions, though, we also want to keep commits that
> > do not change anything from its parents, that is also redundant
> > commits.
> >
> > Fix this by invoking `git cherry-pick --keep-redundant-commits`
> > instead, which will cause redundant commits to be rebased
> > correctly.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  git-rebase--am.sh | 2 +-
> >  t/t3400-rebase.sh | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> > index 9ae898b..ea7b897 100644
> > --- a/git-rebase--am.sh
> > +++ b/git-rebase--am.sh
> > @@ -44,7 +44,7 @@ then
> >         # empty commits and even if it didn't the format doesn't really lend
> >         # itself well to recording empty patches.  fortunately, cherry-pick
> >         # makes this easy
> > -       git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --allow-empty \
> > +       git cherry-pick ${gpg_sign_opt:+"$gpg_sign_opt"} --keep-redundant-commits \
> >                 --right-only "$revisions" \
> >                 ${restrict_revision+^$restrict_revision}
> >         ret=$?
> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> > index 6cca319..f43b202 100755
> > --- a/t/t3400-rebase.sh
> > +++ b/t/t3400-rebase.sh
> > @@ -255,7 +255,7 @@ test_expect_success 'rebase commit with an ancient timestamp' '
> >         grep "author .* 34567 +0600$" actual
> >  '
> >
> > -test_expect_failure 'rebase duplicated commit with --keep-empty' '
> > +test_expect_success 'rebase duplicated commit with --keep-empty' '
> >         git reset --hard &&
> >         git checkout master &&
> >
> > --
> > 2.6.3

Thanks for letting me know. I'll keep this in mind when there is
some consensus reached wether this option actually makes any
sense. Until then I'll just leave it as is.

Patrick

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]