{"thread":{"id":"40840","subject":"[PATCH 1/2] rebase: test broken behavior with --keep-empty","startedAt":"2015-11-20T12:04:38Z","lastAt":"2015-12-14T15:39:45Z","messageCount":5,"participants":["Patrick Steinhardt","Michael Blume"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"273540","messageId":"1448021080-4994-1-git-send-email-ps@pks.im","threadId":"40840","inReplyTo":null,"subject":"[PATCH 0/2] rebase: broken behavior with --keep-empty","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-11-20T12:04:38Z","receivedAt":"2015-11-20T12:04:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"I've recently run into broken behavior with `git-rebase` and\n`--keep-empty`. As soon as `--keep-empty` is specified, we fall\nback on using `git cherry-pick` instead of `git am` for rebasing\nthe commits, which seems to not work correctly.\n\nIn fact I guess there are two different bugs in here. The first\nbug is in using `git cherry-pick --allow-empty` instead of `git\ncherry-pick --keep-redundant-commits`, as judging from\ngit-rebase(1) we also want to keep commits that are already\nincluded in the branch that is being rebased upon whe we use\n`--keep-empty`.\n\nThe second bug is that we fail to correctly record the\nrebasing-state when using cherry-pick. This causes us to end up\nin cherry-picking mode as soon as the invoked cherry-pick command\nruns into any error (e.g. due to a conflict or the broken\nbehavior with `--allow-empty`). There is also no possibility to\nget back into rebase-mode from here.\n\nI've written two tests that document these breakages, as well as\na fix to the first described breakage. I did not fix the second\nbreakage of not recording rebase-state, though, as this is\nsomewhat more involved.\n\nPatrick\n\nPatrick Steinhardt (2):\n  rebase: test broken behavior with --keep-empty\n  rebase: fix preserving commits with --keep-empty\n\n git-rebase--am.sh |  2 +-\n t/t3400-rebase.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\n-- \n2.6.3\n"},{"id":"273539","messageId":"1448021080-4994-2-git-send-email-ps@pks.im","threadId":"40840","inReplyTo":"1448021080-4994-1-git-send-email-ps@pks.im","subject":"[PATCH 1/2] rebase: test broken behavior with --keep-empty","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-11-20T12:04:39Z","receivedAt":"2015-11-20T12:04:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When a commit is already present in the branch that is being\nrebased upon we get an empty commit. Usually we just drop this\ncommit, but with `--keep-empty` we want to preserve the commit's\nmessage.\n\nInstead of simply applying the empty commit, though, we\nerroneously end up in cherry-picking mode without any ability to\nget back to the previous rebase.\n\nThe same error happens when we try to rebase a commit with\n`--keep-empty` that causes a conflict.\n\nAdd tests that document this breakage.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t3400-rebase.sh | 26 ++++++++++++++++++++++++++\n 1 file changed, 26 insertions(+)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 47b5682..6cca319 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -255,4 +255,30 @@ test_expect_success 'rebase commit with an ancient timestamp' '\n \tgrep \"author .* 34567 +0600$\" actual\n '\n \n+test_expect_failure 'rebase duplicated commit with --keep-empty' '\n+\tgit reset --hard &&\n+\tgit checkout master &&\n+\n+\t>x && git add x && git commit x -mx &&\n+\techo x > x && git commit x -mx1 &&\n+\n+\tgit checkout -b duplicated HEAD~ &&\n+\techo x > x && git commit x -mx2 &&\n+\tgit rebase --keep-empty master\n+'\n+\n+test_expect_failure 'rebase conflicting commit with --keep-empty' '\n+\tgit reset --hard &&\n+\tgit checkout master &&\n+\n+\techo y > x && git commit x -my &&\n+\n+\tgit checkout -b conflict HEAD~ &&\n+\techo z > x && git commit x -mz &&\n+\ttest_must_fail git rebase --keep-empty master &&\n+\n+\tgit add x &&\n+\tgit rebase --continue\n+'\n+\n test_done\n-- \n2.6.3\n"},{"id":"273541","messageId":"1448021080-4994-3-git-send-email-ps@pks.im","threadId":"40840","inReplyTo":"1448021080-4994-1-git-send-email-ps@pks.im","subject":"[PATCH 2/2] rebase: fix preserving commits with --keep-empty","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-11-20T12:04:40Z","receivedAt":"2015-11-20T12:04:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When rebasing commits where one or several commits are redundant\nto commits on the branch that is being rebased upon we error out.\nThis is due to the usage of `--allow-empty` for the invoked\ncherry-pick command, which will only cause _empty_ commits to be\npicked instead of also allowing redundant commits. As\ngit-rebase(1) mentions, though, we also want to keep commits that\ndo not change anything from its parents, that is also redundant\ncommits.\n\nFix this by invoking `git cherry-pick --keep-redundant-commits`\ninstead, which will cause redundant commits to be rebased\ncorrectly.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n git-rebase--am.sh | 2 +-\n t/t3400-rebase.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex 9ae898b..ea7b897 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -44,7 +44,7 @@ then\n \t# empty commits and even if it didn't the format doesn't really lend\n \t# itself well to recording empty patches.  fortunately, cherry-pick\n \t# makes this easy\n-\tgit cherry-pick ${gpg_sign_opt:+\"$gpg_sign_opt\"} --allow-empty \\\n+\tgit cherry-pick ${gpg_sign_opt:+\"$gpg_sign_opt\"} --keep-redundant-commits \\\n \t\t--right-only \"$revisions\" \\\n \t\t${restrict_revision+^$restrict_revision}\n \tret=$?\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 6cca319..f43b202 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -255,7 +255,7 @@ test_expect_success 'rebase commit with an ancient timestamp' '\n \tgrep \"author .* 34567 +0600$\" actual\n '\n \n-test_expect_failure 'rebase duplicated commit with --keep-empty' '\n+test_expect_success 'rebase duplicated commit with --keep-empty' '\n \tgit reset --hard &&\n \tgit checkout master &&\n \n-- \n2.6.3\n"},{"id":"274253","messageId":"CAO2U3QiFXaOCb114NMgGxDOV3FqHN6HyNDJBWT6ZeqTvQ+i0Zg@mail.gmail.com","threadId":"40840","inReplyTo":"1448021080-4994-3-git-send-email-ps@pks.im","subject":"Re: [PATCH 2/2] rebase: fix preserving commits with --keep-empty","fromName":"Michael Blume","fromEmail":"blume.mike@gmail.com","sentAt":"2015-12-10T22:58:06Z","receivedAt":"2015-12-10T22:58:06Z","isPatch":true,"sender":{"key":"blume.mike@gmail.com","avatar":"https://gravatar.com/avatar/1a7b440e1d942425ff4098ac7fc15b86b30cecaa56e1692a7ef8b5939ba25ea7?d=mp&s=160"},"body":"This test does not seem to pass on my mac.\n\nI've placed the verbose output here:\nhttps://gist.github.com/MichaelBlume/db7ba222be001d502e57\n\nOn Fri, Nov 20, 2015 at 4:04 AM, Patrick Steinhardt <ps@pks.im> wrote:\n> When rebasing commits where one or several commits are redundant\n> to commits on the branch that is being rebased upon we error out.\n> This is due to the usage of `--allow-empty` for the invoked\n> cherry-pick command, which will only cause _empty_ commits to be\n> picked instead of also allowing redundant commits. As\n> git-rebase(1) mentions, though, we also want to keep commits that\n> do not change anything from its parents, that is also redundant\n> commits.\n>\n> Fix this by invoking `git cherry-pick --keep-redundant-commits`\n> instead, which will cause redundant commits to be rebased\n> correctly.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  git-rebase--am.sh | 2 +-\n>  t/t3400-rebase.sh | 2 +-\n>  2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-rebase--am.sh b/git-rebase--am.sh\n> index 9ae898b..ea7b897 100644\n> --- a/git-rebase--am.sh\n> +++ b/git-rebase--am.sh\n> @@ -44,7 +44,7 @@ then\n>         # empty commits and even if it didn't the format doesn't really lend\n>         # itself well to recording empty patches.  fortunately, cherry-pick\n>         # makes this easy\n> -       git cherry-pick ${gpg_sign_opt:+\"$gpg_sign_opt\"} --allow-empty \\\n> +       git cherry-pick ${gpg_sign_opt:+\"$gpg_sign_opt\"} --keep-redundant-commits \\\n>                 --right-only \"$revisions\" \\\n>                 ${restrict_revision+^$restrict_revision}\n>         ret=$?\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 6cca319..f43b202 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -255,7 +255,7 @@ test_expect_success 'rebase commit with an ancient timestamp' '\n>         grep \"author .* 34567 +0600$\" actual\n>  '\n>\n> -test_expect_failure 'rebase duplicated commit with --keep-empty' '\n> +test_expect_success 'rebase duplicated commit with --keep-empty' '\n>         git reset --hard &&\n>         git checkout master &&\n>\n> --\n> 2.6.3\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"274381","messageId":"20151214153945.GA1414@pks-xps.fritz.box","threadId":"40840","inReplyTo":"CAO2U3QiFXaOCb114NMgGxDOV3FqHN6HyNDJBWT6ZeqTvQ+i0Zg@mail.gmail.com","subject":"Re: [PATCH 2/2] rebase: fix preserving commits with --keep-empty","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-12-14T15:39:45Z","receivedAt":"2015-12-14T15:39:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Dec 10, 2015 at 02:58:06PM -0800, Michael Blume wrote:\n> This test does not seem to pass on my mac.\n> \n> I've placed the verbose output here:\n> https://gist.github.com/MichaelBlume/db7ba222be001d502e57\n> \n> On Fri, Nov 20, 2015 at 4:04 AM, Patrick Steinhardt <ps@pks.im> wrote:\n> > When rebasing commits where one or several commits are redundant\n> > to commits on the branch that is being rebased upon we error out.\n> > This is due to the usage of `--allow-empty` for the invoked\n> > cherry-pick command, which will only cause _empty_ commits to be\n> > picked instead of also allowing redundant commits. As\n> > git-rebase(1) mentions, though, we also want to keep commits that\n> > do not change anything from its parents, that is also redundant\n> > commits.\n> >\n> > Fix this by invoking `git cherry-pick --keep-redundant-commits`\n> > instead, which will cause redundant commits to be rebased\n> > correctly.\n> >\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  git-rebase--am.sh | 2 +-\n> >  t/t3400-rebase.sh | 2 +-\n> >  2 files changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/git-rebase--am.sh b/git-rebase--am.sh\n> > index 9ae898b..ea7b897 100644\n> > --- a/git-rebase--am.sh\n> > +++ b/git-rebase--am.sh\n> > @@ -44,7 +44,7 @@ then\n> >         # empty commits and even if it didn't the format doesn't really lend\n> >         # itself well to recording empty patches.  fortunately, cherry-pick\n> >         # makes this easy\n> > -       git cherry-pick ${gpg_sign_opt:+\"$gpg_sign_opt\"} --allow-empty \\\n> > +       git cherry-pick ${gpg_sign_opt:+\"$gpg_sign_opt\"} --keep-redundant-commits \\\n> >                 --right-only \"$revisions\" \\\n> >                 ${restrict_revision+^$restrict_revision}\n> >         ret=$?\n> > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> > index 6cca319..f43b202 100755\n> > --- a/t/t3400-rebase.sh\n> > +++ b/t/t3400-rebase.sh\n> > @@ -255,7 +255,7 @@ test_expect_success 'rebase commit with an ancient timestamp' '\n> >         grep \"author .* 34567 +0600$\" actual\n> >  '\n> >\n> > -test_expect_failure 'rebase duplicated commit with --keep-empty' '\n> > +test_expect_success 'rebase duplicated commit with --keep-empty' '\n> >         git reset --hard &&\n> >         git checkout master &&\n> >\n> > --\n> > 2.6.3\n\nThanks for letting me know. I'll keep this in mind when there is\nsome consensus reached wether this option actually makes any\nsense. Until then I'll just leave it as is.\n\nPatrick\n"}]}