threads / patch / 40840

patch, 2 partsrebase: test broken behavior with --keep-empty

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

## tl;dr

5 messages between Nov 20, 2015 and Dec 14, 2015. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Patrick Steinhardt· Nov 20, 2015, 12:04 UTC · lore

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

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

Patrick Steinhardt· Nov 20, 2015, 12:04 UTC · re: Patrick Steinhardt · lore

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(+)

Show changes to t/t3400-rebase.sh +26 −0

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

Patrick Steinhardt· Nov 20, 2015, 12:04 UTC · re: Patrick Steinhardt · lore

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

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

Show changes to 2 files +2 −2

git-rebase--am.sh, t/t3400-rebase.sh

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

Michael Blume· Dec 10, 2015, 22:58 UTC · re: Patrick Steinhardt · lore

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

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:

Show 52 quoted lines

> 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

Patrick Steinhardt· Dec 14, 2015, 15:39 UTC · re: Michael Blume · lore

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

On Thu, Dec 10, 2015 at 02:58:06PM -0800, Michael Blume wrote:

Show 53 quoted lines

> 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