{"thread":{"id":"30602","subject":"Cherry-pick dangles and forgets helpful advice in next","startedAt":"2012-05-23T20:31:00Z","lastAt":"2012-05-31T17:29:57Z","messageCount":10,"participants":["Phil Hord","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"192014","messageId":"4FBD4904.9090000@cisco.com","threadId":"30602","inReplyTo":null,"subject":"Cherry-pick dangles and forgets helpful advice in next","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2012-05-23T20:31:00Z","receivedAt":"2012-05-23T20:31:00Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"In git.git 'master' when I cherry-pick a commit which is eventually\nempty, git gives me a friendly description of my supposed error, leaves\nmy cherry-pick \"pending\" and exits with an error code.\n\n\n$ git cherry-pick a0aff2d                          \n# On branch master\nnothing to commit (working directory clean)\nThe previous cherry-pick is now empty, possibly due to conflict resolution.\nIf you wish to commit it anyway, use:\n\n    git commit --allow-empty\n\nOtherwise, please use 'git reset'\n\n\nIn 'next' this is broken.  Now git does not tell me anything and it does\nnot exit with an error code.  It does still leave the cherry-pick\npending, though.\n\n\n$ git cherry-pick a0aff2d\n$ cat /tmp/repo/.git/CHERRY_PICK_HEAD\na0aff2da3d12a9a097c02e39570611f359433c23\n\n\n\nIt bisects to this commit:\n\ncommit b27cfb0d8d4cbb6d079c70ffeadac9c0dcfff250\nAuthor: Neil Horman <nhorman@tuxdriver.com>\nDate:   Fri Apr 20 10:36:15 2012 -0400\n\n    git-cherry-pick: Add keep-redundant-commits option\n   \n    The git-cherry-pick --allow-empty command by default only preserves\nempty\n    commits that were originally empty, i.e only those commits for which\n    <commit>^{tree} and <commit>^^{tree} are equal.  By default commits\nwhich are\n    non-empty, but were made empty by the inclusion of a prior commit on\nthe current\n    history are filtered out.  This option allows us to override that\nbehavior and\n    include redundant commits as empty commits in the change history.\n   \n    Note that this patch changes the default behavior of git cherry-pick\nslightly.\n    Prior to this patch all commits in a cherry-pick sequence were\napplied and git\n    commit was run.  The implication here was that, if a commit was\nredundant, and\n    the commit did not trigger the fast forward logic, the git commit\noperation, and\n    therefore the git cherry-pick operation would fail, displaying the\ncherry pick\n    advice (i.e. run git commit --allow-empty).  With this patch\nhowever, such\n    redundant commits are automatically skipped without stopping, unless\n    --keep-redundant-commits is specified, in which case, they are\nautomatically\n    applied as empty commits.\n   \n    Signed-off-by: Neil Horman <nhorman@tuxdriver.com>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\n\nI don't have time to chase it any further today.  Hopefully someone can\nsee the flub and straighten it out before I get a chance to look again. \nIf not, I'll probably forget anyway.\n\nPhil\n"},{"id":"192020","messageId":"7vr4uazjb3.fsf@alter.siamese.dyndns.org","threadId":"30602","inReplyTo":"4FBD4904.9090000@cisco.com","subject":"Re: Cherry-pick dangles and forgets helpful advice in next","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-23T21:20:00Z","receivedAt":"2012-05-23T21:20:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <hordp@cisco.com> writes:\n\n> In git.git 'master' when I cherry-pick a commit which is eventually\n> empty, git gives me a friendly description of my supposed error, leaves\n> my cherry-pick \"pending\" and exits with an error code.\n>\n>\n> $ git cherry-pick a0aff2d                          \n> # On branch master\n> nothing to commit (working directory clean)\n> The previous cherry-pick is now empty, possibly due to conflict resolution.\n> If you wish to commit it anyway, use:\n>\n>     git commit --allow-empty\n>\n> Otherwise, please use 'git reset'\n>\n>\n> In 'next' this is broken.  Now git does not tell me anything and it does\n> not exit with an error code.  It does still leave the cherry-pick\n> pending, though.\n>\n>\n> $ git cherry-pick a0aff2d\n> $ cat /tmp/repo/.git/CHERRY_PICK_HEAD\n> a0aff2da3d12a9a097c02e39570611f359433c23\n>\n> It bisects to this commit:\n>\n> commit b27cfb0d8d4cbb6d079c70ffeadac9c0dcfff250\n> Author: Neil Horman <nhorman@tuxdriver.com>\n> Date:   Fri Apr 20 10:36:15 2012 -0400\n>\n>     git-cherry-pick: Add keep-redundant-commits option\n> ...\n> I don't have time to chase it any further today.  Hopefully someone can\n> see the flub and straighten it out before I get a chance to look again. \n> If not, I'll probably forget anyway.\n\nGiven that I am currently very deep in today's integration round, I\nwouldn't have a chance to look into this.  I am OK with reverting the\nwhole topic (and I am also very much tempted to do so), as nobody other\nthan Neil seemed to have wanted this \"feature\". and even though it is\n\"optional\", I doubt it encourages a good workflow.\n\nLet's see if somebody comes up with a fix quickly enough before the -rc1\nis tagged.\n\nThanks for a report.\n"},{"id":"192027","messageId":"7vipfmzfel.fsf@alter.siamese.dyndns.org","threadId":"30602","inReplyTo":"4FBD4904.9090000@cisco.com","subject":"Re: Cherry-pick dangles and forgets helpful advice in next","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-23T22:44:18Z","receivedAt":"2012-05-23T22:44:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <hordp@cisco.com> writes:\n\n> In git.git 'master' when I cherry-pick a commit which is eventually\n> empty, git gives me a friendly description of my supposed error, leaves\n> my cherry-pick \"pending\" and exits with an error code.\n>\n>\n> $ git cherry-pick a0aff2d                          \n> # On branch master\n> nothing to commit (working directory clean)\n> The previous cherry-pick is now empty, possibly due to conflict resolution.\n> If you wish to commit it anyway, use:\n>\n>     git commit --allow-empty\n>\n> Otherwise, please use 'git reset'\n>\n> In 'next' this is broken.  Now git does not tell me anything and it does\n> not exit with an error code.\n\nThere probably is something else that is broken in _your_ build.  The test\nt3505.2 is about failing an attempt to cherry-pick an empty commit:\n\n        test_expect_success 'cherry-pick an empty commit' '\n                git checkout master && {\n                        git cherry-pick empty-branch^\n                        test \"$?\" = 1\n                }\n        '\n\nIf I insert an \"exit\" immediately after this test and run the test with\n\"-i -v\" option, it ends like this:\n\n    $ make && cd t && sh t3505-cherry-pick-empty.sh -i -v\n    ...\n    ok 1 - setup\n\n    expecting success:\n            git checkout master && {\n                    git cherry-pick empty-branch^\n                    test \"$?\" = 1\n            }\n\n    Switched to branch 'master'\n    Already up-to-date!\n    # On branch master\n    nothing to commit (working directory clean)\n    The previous cherry-pick is now empty, possibly due to conflict\n    resolution.\n    If you wish to commit it anyway, use:\n\n        git commit --allow-empty\n\n    Otherwise, please use 'git reset'\n    ok 2 - cherry-pick an empty commit\n\n    FATAL: Unexpected exit with code 0\n\nIt does fail with non-zero exit code (it might be better to test with\ntest_expect_failure, but that is a minor point here), and we see the error\nmessage.\n"},{"id":"192028","messageId":"7vehqazf9d.fsf@alter.siamese.dyndns.org","threadId":"30602","inReplyTo":"7vipfmzfel.fsf@alter.siamese.dyndns.org","subject":"Re: Cherry-pick dangles and forgets helpful advice in next","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-23T22:47:26Z","receivedAt":"2012-05-23T22:47:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Phil Hord <hordp@cisco.com> writes:\n>\n>> In git.git 'master' when I cherry-pick a commit which is eventually\n>> empty, git gives me a friendly description of my supposed error, leaves\n>> my cherry-pick \"pending\" and exits with an error code.\n>>\n>>\n>> $ git cherry-pick a0aff2d                          \n>> # On branch master\n>> nothing to commit (working directory clean)\n>> The previous cherry-pick is now empty, possibly due to conflict resolution.\n>> If you wish to commit it anyway, use:\n>>\n>>     git commit --allow-empty\n>>\n>> Otherwise, please use 'git reset'\n>>\n>> In 'next' this is broken.  Now git does not tell me anything and it does\n>> not exit with an error code.\n>\n> ...  The test\n> t3505.2 is about failing an attempt to cherry-pick an empty commit:\n\nAhh, disregard that one.  It is not testing the case where a cherry-pick\nresults in empty.\n"},{"id":"192029","messageId":"7vaa0yzeqr.fsf@alter.siamese.dyndns.org","threadId":"30602","inReplyTo":"7vehqazf9d.fsf@alter.siamese.dyndns.org","subject":"Re: Cherry-pick dangles and forgets helpful advice in next","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-23T22:58:36Z","receivedAt":"2012-05-23T22:58:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Phil Hord <hordp@cisco.com> writes:\n>>\n>>> In git.git 'master' when I cherry-pick a commit which is eventually\n>>> empty, git gives me a friendly description of my supposed error, leaves\n>>> my cherry-pick \"pending\" and exits with an error code.\n>>> ...\n>>> In 'next' this is broken.\n\nThe above was written back when nh/empty-rebase topic was not in 'master'\nyet.\n\nThe attached will help reproducing it.\n\n t/t3505-cherry-pick-empty.sh | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh\nindex 92f00cd..bd06981 100755\n--- a/t/t3505-cherry-pick-empty.sh\n+++ b/t/t3505-cherry-pick-empty.sh\n@@ -23,8 +23,19 @@ test_expect_success setup '\n \tgit checkout master &&\n \tgit checkout -b empty-branch2 &&\n \ttest_tick &&\n-\tgit commit --allow-empty -m \"empty\"\n+\tgit commit --allow-empty -m \"empty\" &&\n+\n+\tgit checkout master &&\n+\techo second >>file1 &&\n+\tgit add file1 &&\n+\ttest_tick &&\n+\tgit commit -m \"second\" &&\n \n+\tgit checkout -b merge-to-empty master^ &&\n+\techo second >>file1 &&\n+\tgit add file1 &&\n+\ttest_tick &&\n+\tgit commit -m \"will merge to empty\"\n '\n \n test_expect_success 'cherry-pick an empty commit' '\n@@ -40,7 +51,13 @@ test_expect_success 'index lockfile was removed' '\n \n '\n \n+test_expect_failure 'cherry-pick to result in empty' '\n+\tgit checkout master^0 &&\n+\ttest_must_fail git cherry-pick merge-to-empty\n+'\n+\n test_expect_success 'cherry-pick a commit with an empty message' '\n+\tgit reset --hard &&\n \tgit checkout master && {\n \t\tgit cherry-pick empty-branch\n \t\ttest \"$?\" = 1\n"},{"id":"192030","messageId":"4FBD6EC2.3000804@cisco.com","threadId":"30602","inReplyTo":"7vehqazf9d.fsf@alter.siamese.dyndns.org","subject":"Re: Cherry-pick dangles and forgets helpful advice in next","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2012-05-23T23:12:02Z","receivedAt":"2012-05-23T23:12:02Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Phil Hord <hordp@cisco.com> writes:\n>>\n>>> In git.git 'master' when I cherry-pick a commit which is eventually\n>>> empty, git gives me a friendly description of my supposed error, leaves\n>>> my cherry-pick \"pending\" and exits with an error code.\n>>>\n>>>\n>>> $ git cherry-pick a0aff2d                          \n>>> # On branch master\n>>> nothing to commit (working directory clean)\n>>> The previous cherry-pick is now empty, possibly due to conflict resolution.\n>>> If you wish to commit it anyway, use:\n>>>\n>>>     git commit --allow-empty\n>>>\n>>> Otherwise, please use 'git reset'\n>>>\n>>> In 'next' this is broken.  Now git does not tell me anything and it does\n>>> not exit with an error code.\n>> ...  The test\n>> t3505.2 is about failing an attempt to cherry-pick an empty commit:\n> Ahh, disregard that one.  It is not testing the case where a cherry-pick\n> results in empty.\n>\n\nYes, but the last one checks for something similar.  However, I tried\nthis and it does not trigger the same failure.\n\n$ git cherry-pick HEAD^                              \n# On branch master\nnothing to commit (working directory clean)\nThe previous cherry-pick is now empty, possibly due to conflict resolution.\nIf you wish to commit it anyway, use:\n\n    git commit --allow-empty\n\nOtherwise, please use 'git reset'\n\n\n\nSo I think I need to be more specific about the error condition I am seeing.\n\nmkdir repo && cd repo && git init &&\ntouch foo && git add foo && git commit -mfoo &&\necho foo > foo && git add foo && git commit -mnewfoo && git branch newfoo &&\ngit commit --amend -m\"new foo\" &&\ngit cherry-pick newfoo\n\nThis sequence fails to report an error as of this commit:\n\ncommit b27cfb0d8d4cbb6d079c70ffeadac9c0dcfff250\nAuthor: Neil Horman <nhorman@tuxdriver.com>\nDate:   Fri Apr 20 10:36:15 2012 -0400\n\n    git-cherry-pick: Add keep-redundant-commits option\n\nBut it does report the problem and 'exit 1' prior to that.\n\nPhil\n"},{"id":"192031","messageId":"7v62bmzdmy.fsf@alter.siamese.dyndns.org","threadId":"30602","inReplyTo":"4FBD6EC2.3000804@cisco.com","subject":"Re: Cherry-pick dangles and forgets helpful advice in next","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-23T23:22:29Z","receivedAt":"2012-05-23T23:22:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <hordp@cisco.com> writes:\n\n> This sequence fails to report an error as of this commit:\n>\n> commit b27cfb0d8d4cbb6d079c70ffeadac9c0dcfff250\n> Author: Neil Horman <nhorman@tuxdriver.com>\n> Date:   Fri Apr 20 10:36:15 2012 -0400\n>\n>     git-cherry-pick: Add keep-redundant-commits option\n\nYes, because it deliberately breaks sequencer.c::do_pick_commit() in this\nparticular case:\n\n\tif (!empty_commit && !opts->keep_redundant_commits && index_unchanged)\n\t\t/*\n\t\t * The head tree and the index match\n\t\t * meaning the commit is empty.  Since it wasn't created\n\t\t * empty (based on the previous test), we can conclude\n\t\t * the commit has been made redundant.  Since we don't\n\t\t * want to keep redundant commits, we can just return\n\t\t * here, skipping this commit\n\t\t */\n\t\treturn 0;\n\nwhich is not quite right.  We do not want to keep redundant commits, so\nthis needs to error out as before.\n\nI will queue the following in the meantime.\n\n-- >8 --\nSubject: [PATCH] Revert nh/empty-rebase topic\n\nThe topic breaks \"git cherry-pick $commit\" that results in no change\nin the tree by not erroring out, which is a grave regression.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/git-cherry-pick.txt |  19 -------\n Documentation/git-rebase.txt      |   4 --\n builtin/revert.c                  |   8 ---\n git-rebase--am.sh                 |  19 ++-----\n git-rebase--interactive.sh        |  35 ++-----------\n git-rebase.sh                     |   5 --\n sequencer.c                       | 103 ++++----------------------------------\n sequencer.h                       |   2 -\n t/t3505-cherry-pick-empty.sh      |  25 +--------\n 9 files changed, 19 insertions(+), 201 deletions(-)\n\ndiff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt\nindex 9f3dae6..06a0bfd 100644\n--- a/Documentation/git-cherry-pick.txt\n+++ b/Documentation/git-cherry-pick.txt\n@@ -103,25 +103,6 @@ effect to your index in a row.\n \tcherry-pick'ed commit, then a fast forward to this commit will\n \tbe performed.\n \n---allow-empty::\n-\tBy default, cherry-picking an empty commit will fail,\n-\tindicating that an explicit invocation of `git commit\n-\t--allow-empty` is required. This option overrides that\n-\tbehavior, allowing empty commits to be preserved automatically\n-\tin a cherry-pick. Note that when \"--ff\" is in effect, empty\n-\tcommits that meet the \"fast-forward\" requirement will be kept\n-\teven without this option.  Note also, that use of this option only\n-\tkeeps commits that were initially empty (i.e. the commit recorded the\n-\tsame tree as its parent).  Commits which are made empty due to a\n-\tprevious commit are dropped.  To force the inclusion of those commits\n-\tuse `--keep-redundant-commits`.\n-\n---keep-redundant-commits::\n-\tIf a commit being cherry picked duplicates a commit already in the\n-\tcurrent history, it will become empty.  By default these\n-\tredundant commits are ignored.  This option overrides that behavior and\n-\tcreates an empty commit object.  Implies `--allow-empty`.\n-\n --strategy=<strategy>::\n \tUse the given merge strategy.  Should only be used once.\n \tSee the MERGE STRATEGIES section in linkgit:git-merge[1]\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 147fa1a..b0e13e5 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -238,10 +238,6 @@ leave out at most one of A and B, in which case it defaults to HEAD.\n \twill be reset to where it was when the rebase operation was\n \tstarted.\n \n---keep-empty::\n-\tKeep the commits that do not change anything from its\n-\tparents in the result.\n-\n --skip::\n \tRestart the rebasing process by skipping the current patch.\n \ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 82d1bf8..5462e67 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -115,16 +115,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tOPT_END(),\n \t\tOPT_END(),\n \t\tOPT_END(),\n-\t\tOPT_END(),\n-\t\tOPT_END(),\n \t};\n \n \tif (opts->action == REPLAY_PICK) {\n \t\tstruct option cp_extra[] = {\n \t\t\tOPT_BOOLEAN('x', NULL, &opts->record_origin, \"append commit name\"),\n \t\t\tOPT_BOOLEAN(0, \"ff\", &opts->allow_ff, \"allow fast-forward\"),\n-\t\t\tOPT_BOOLEAN(0, \"allow-empty\", &opts->allow_empty, \"preserve initially empty commits\"),\n-\t\t\tOPT_BOOLEAN(0, \"keep-redundant-commits\", &opts->keep_redundant_commits, \"keep redundant, empty commits\"),\n \t\t\tOPT_END(),\n \t\t};\n \t\tif (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))\n@@ -142,10 +138,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\t\t\t\"--abort\", rollback,\n \t\t\t\tNULL);\n \n-\t/* implies allow_empty */\n-\tif (opts->keep_redundant_commits)\n-\t\topts->allow_empty = 1;\n-\n \t/* Set the subcommand */\n \tif (remove_state)\n \t\topts->subcommand = REPLAY_REMOVE_STATE;\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex 04d8941..c815a24 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -20,20 +20,11 @@ esac\n \n test -n \"$rebase_root\" && root_flag=--root\n \n-if test -n \"$keep_empty\"\n-then\n-\t# we have to do this the hard way.  git format-patch completely squashes\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 --allow-empty \"$revisions\"\n-else\n-\tgit format-patch -k --stdout --full-index --ignore-if-in-upstream \\\n-\t\t--src-prefix=a/ --dst-prefix=b/ \\\n-\t\t--no-renames $root_flag \"$revisions\" |\n-\tgit am $git_am_opt --rebasing --resolvemsg=\"$resolvemsg\"\n-fi && move_to_original_branch\n-\n+git format-patch -k --stdout --full-index --ignore-if-in-upstream \\\n+\t--src-prefix=a/ --dst-prefix=b/ \\\n+\t--no-renames $root_flag \"$revisions\" |\n+git am $git_am_opt --rebasing --resolvemsg=\"$resolvemsg\" &&\n+move_to_original_branch\n ret=$?\n test 0 != $ret -a -d \"$state_dir\" && write_basic_state\n exit $ret\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 0c19b7c..2e13258 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -167,14 +167,6 @@ has_action () {\n \tsane_grep '^[^#]' \"$1\" >/dev/null\n }\n \n-is_empty_commit() {\n-\ttree=$(git rev-parse -q --verify \"$1\"^{tree} 2>/dev/null ||\n-\t\tdie \"$1: not a commit that can be picked\")\n-\tptree=$(git rev-parse -q --verify \"$1\"^^{tree} 2>/dev/null ||\n-\t\tptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)\n-\ttest \"$tree\" = \"$ptree\"\n-}\n-\n # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n # GIT_AUTHOR_DATE exported from the current environment.\n do_with_author () {\n@@ -199,19 +191,12 @@ git_sequence_editor () {\n \n pick_one () {\n \tff=--ff\n-\n \tcase \"$1\" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac\n \tcase \"$force_rebase\" in '') ;; ?*) ff= ;; esac\n \toutput git rev-parse --verify $sha1 || die \"Invalid commit name: $sha1\"\n-\n-\tif is_empty_commit \"$sha1\"\n-\tthen\n-\t\tempty_args=\"--allow-empty\"\n-\tfi\n-\n \ttest -d \"$rewritten\" &&\n \t\tpick_one_preserving_merges \"$@\" && return\n-\toutput git cherry-pick $empty_args $ff \"$@\"\n+\toutput git cherry-pick $ff \"$@\"\n }\n \n pick_one_preserving_merges () {\n@@ -795,17 +780,9 @@ git rev-list $merges_option --pretty=oneline --abbrev-commit \\\n \tsed -n \"s/^>//p\" |\n while read -r shortsha1 rest\n do\n-\n-\tif test -z \"$keep_empty\" && is_empty_commit $shortsha1\n-\tthen\n-\t\tcomment_out=\"# \"\n-\telse\n-\t\tcomment_out=\n-\tfi\n-\n \tif test t != \"$preserve_merges\"\n \tthen\n-\t\tprintf '%s\\n' \"${comment_out}pick $shortsha1 $rest\" >>\"$todo\"\n+\t\tprintf '%s\\n' \"pick $shortsha1 $rest\" >> \"$todo\"\n \telse\n \t\tsha1=$(git rev-parse $shortsha1)\n \t\tif test -z \"$rebase_root\"\n@@ -824,7 +801,7 @@ do\n \t\tif test f = \"$preserve\"\n \t\tthen\n \t\t\ttouch \"$rewritten\"/$sha1\n-\t\t\tprintf '%s\\n' \"${comment_out}pick $shortsha1 $rest\" >>\"$todo\"\n+\t\t\tprintf '%s\\n' \"pick $shortsha1 $rest\" >> \"$todo\"\n \t\tfi\n \tfi\n done\n@@ -876,12 +853,6 @@ cat >> \"$todo\" << EOF\n #\n EOF\n \n-if test -z \"$keep_empty\"\n-then\n-\techo \"# Note that empty commits are commented out\" >>\"$todo\"\n-fi\n-\n-\n has_action \"$todo\" ||\n \tdie_abort \"Nothing to do\"\n \ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 24a2840..69c1374 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -43,7 +43,6 @@ s,strategy=!       use the given merge strategy\n no-ff!             cherry-pick all commits, even if unchanged\n m,merge!           use merging strategies to rebase\n i,interactive!     let the user edit the list of commits to rebase\n-k,keep-empty\t   preserve empty commits during rebase\n f,force-rebase!    force rebase even if branch is up to date\n X,strategy-option=! pass the argument through to the merge strategy\n stat!              display a diffstat of what changed upstream\n@@ -98,7 +97,6 @@ state_dir=\n action=\n preserve_merges=\n autosquash=\n-keep_empty=\n test \"$(git config --bool rebase.autosquash)\" = \"true\" && autosquash=t\n \n read_basic_state () {\n@@ -222,9 +220,6 @@ do\n \t-i)\n \t\tinteractive_rebase=explicit\n \t\t;;\n-\t-k)\n-\t\tkeep_empty=yes\n-\t\t;;\n \t-p)\n \t\tpreserve_merges=t\n \t\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\ndiff --git a/sequencer.c b/sequencer.c\nindex 3c384b9..81d8ace 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -13,7 +13,6 @@\n #include \"rerere.h\"\n #include \"merge-recursive.h\"\n #include \"refs.h\"\n-#include \"argv-array.h\"\n \n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n@@ -252,38 +251,6 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \treturn !clean;\n }\n \n-static int is_index_unchanged(void)\n-{\n-\tunsigned char head_sha1[20];\n-\tstruct commit *head_commit;\n-\n-\tif (!resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL))\n-\t\treturn error(_(\"Could not resolve HEAD commit\\n\"));\n-\n-\thead_commit = lookup_commit(head_sha1);\n-\n-\t/*\n-\t * If head_commit is NULL, check_commit, called from\n-\t * lookup_commit, would have indicated that head_commit is not\n-\t * a commit object already.  parse_commit() will return failure\n-\t * without further complaints in such a case.  Otherwise, if\n-\t * the commit is invalid, parse_commit() will complain.  So\n-\t * there is nothing for us to say here.  Just return failure.\n-\t */\n-\tif (parse_commit(head_commit))\n-\t\treturn -1;\n-\n-\tif (!active_cache_tree)\n-\t\tactive_cache_tree = cache_tree();\n-\n-\tif (!cache_tree_fully_valid(active_cache_tree))\n-\t\tif (cache_tree_update(active_cache_tree, active_cache,\n-\t\t\t\t  active_nr, 0))\n-\t\t\treturn error(_(\"Unable to update cache tree\\n\"));\n-\n-\treturn !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);\n-}\n-\n /*\n  * If we are cherry-pick, and if the merge did not result in\n  * hand-editing, we will hit this commit and inherit the original\n@@ -293,46 +260,21 @@ static int is_index_unchanged(void)\n  */\n static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n {\n-\tstruct argv_array array;\n-\tint rc;\n-\n-\targv_array_init(&array);\n-\targv_array_push(&array, \"commit\");\n-\targv_array_push(&array, \"-n\");\n+\t/* 6 is max possible length of our args array including NULL */\n+\tconst char *args[6];\n+\tint i = 0;\n \n+\targs[i++] = \"commit\";\n+\targs[i++] = \"-n\";\n \tif (opts->signoff)\n-\t\targv_array_push(&array, \"-s\");\n+\t\targs[i++] = \"-s\";\n \tif (!opts->edit) {\n-\t\targv_array_push(&array, \"-F\");\n-\t\targv_array_push(&array, defmsg);\n+\t\targs[i++] = \"-F\";\n+\t\targs[i++] = defmsg;\n \t}\n+\targs[i] = NULL;\n \n-\tif (opts->allow_empty)\n-\t\targv_array_push(&array, \"--allow-empty\");\n-\n-\trc = run_command_v_opt(array.argv, RUN_GIT_CMD);\n-\targv_array_clear(&array);\n-\treturn rc;\n-}\n-\n-static int is_original_commit_empty(struct commit *commit)\n-{\n-\tconst unsigned char *ptree_sha1;\n-\n-\tif (parse_commit(commit))\n-\t\treturn error(_(\"Could not parse commit %s\\n\"),\n-\t\t\t     sha1_to_hex(commit->object.sha1));\n-\tif (commit->parents) {\n-\t\tstruct commit *parent = commit->parents->item;\n-\t\tif (parse_commit(parent))\n-\t\t\treturn error(_(\"Could not parse parent commit %s\\n\"),\n-\t\t\t\tsha1_to_hex(parent->object.sha1));\n-\t\tptree_sha1 = parent->tree->object.sha1;\n-\t} else {\n-\t\tptree_sha1 = EMPTY_TREE_SHA1_BIN; /* commit is root */\n-\t}\n-\n-\treturn !hashcmp(ptree_sha1, commit->tree->object.sha1);\n+\treturn run_command_v_opt(args, RUN_GIT_CMD);\n }\n \n static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n@@ -344,8 +286,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint res;\n-\tint empty_commit;\n-\tint index_unchanged;\n \n \tif (opts->no_commit) {\n \t\t/*\n@@ -471,10 +411,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tfree_commit_list(remotes);\n \t}\n \n-\tempty_commit = is_original_commit_empty(commit);\n-\tif (empty_commit < 0)\n-\t\treturn empty_commit;\n-\n \t/*\n \t * If the merge was clean or if it failed due to conflict, we write\n \t * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.\n@@ -495,25 +431,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tprint_advice(res == 1, opts);\n \t\trerere(opts->allow_rerere_auto);\n \t} else {\n-\t\tindex_unchanged = is_index_unchanged();\n-\t\t/*\n-\t\t * If index_unchanged is less than 0, that indicates we either\n-\t\t * couldn't parse HEAD or the index, so error out here.\n-\t\t */\n-\t\tif (index_unchanged < 0)\n-\t\t\treturn index_unchanged;\n-\n-\t\tif (!empty_commit && !opts->keep_redundant_commits && index_unchanged)\n-\t\t\t/*\n-\t\t\t * The head tree and the index match\n-\t\t\t * meaning the commit is empty.  Since it wasn't created\n-\t\t\t * empty (based on the previous test), we can conclude\n-\t\t\t * the commit has been made redundant.  Since we don't\n-\t\t\t * want to keep redundant commits, we can just return\n-\t\t\t * here, skipping this commit\n-\t\t\t */\n-\t\t\treturn 0;\n-\n \t\tif (!opts->no_commit)\n \t\t\tres = run_git_commit(defmsg, opts);\n \t}\ndiff --git a/sequencer.h b/sequencer.h\nindex aa5f17c..bb4b138 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -29,8 +29,6 @@ struct replay_opts {\n \tint signoff;\n \tint allow_ff;\n \tint allow_rerere_auto;\n-\tint allow_empty;\n-\tint keep_redundant_commits;\n \n \tint mainline;\n \ndiff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh\nindex 92f00cd..c10b28c 100755\n--- a/t/t3505-cherry-pick-empty.sh\n+++ b/t/t3505-cherry-pick-empty.sh\n@@ -18,12 +18,7 @@ test_expect_success setup '\n \techo third >> file1 &&\n \tgit add file1 &&\n \ttest_tick &&\n-\tgit commit --allow-empty-message -m \"\" &&\n-\n-\tgit checkout master &&\n-\tgit checkout -b empty-branch2 &&\n-\ttest_tick &&\n-\tgit commit --allow-empty -m \"empty\"\n+\tgit commit --allow-empty-message -m \"\"\n \n '\n \n@@ -53,22 +48,4 @@ test_expect_success 'index lockfile was removed' '\n \n '\n \n-test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '\n-\tgit checkout master &&\n-\techo fourth >>file2 &&\n-\tgit add file2 &&\n-\tgit commit -m \"fourth\" &&\n-\ttest_must_fail git cherry-pick empty-branch2\n-'\n-\n-test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '\n-\tgit checkout master &&\n-\tgit cherry-pick --allow-empty empty-branch2\n-'\n-\n-test_expect_success 'cherry pick with --keep-redundant-commits' '\n-\tgit checkout master &&\n-\tgit cherry-pick --keep-redundant-commits HEAD^\n-'\n-\n test_done\n-- \n1.7.10.2.627.g7c93d77\n"},{"id":"192453","messageId":"7vipfe1ihg.fsf@alter.siamese.dyndns.org","threadId":"30602","inReplyTo":"7v62bmzdmy.fsf@alter.siamese.dyndns.org","subject":"Re: Cherry-pick dangles and forgets helpful advice in next","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-30T02:59:55Z","receivedAt":"2012-05-30T02:59:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Instead of reverting the entire thing, perhaps we can fix the\nregression like this.\n\nWith this, we no longer unconditionally give \"--allow-empty\" when we\nrun \"git commit\", when --allow-empty (which is only about commits\nthat are originally empty) is given to cherry-pick; specifically,\nwhen the user did not ask for --keep-redundant-commit, we do not\ngive \"--allow-empty\" if the original commit is not.\n\nThinking about it again, I _think_ we do not even have to check if\nthe result is an empty commit ourselves (\"git commit\" will do that\nfor us anyway), so we might want to rip \"is_empty_commit()\" out of\nthe problematic patch and keep only \"is_index_unmodified()\" bit, but\nfor now I think this may be good enough.\n\nPhil, does it fix your issue?\n\nNeil?\n\n-- >8 --\n\nSubject: [PATCH] cherry-pick: regression fix for empty commits\n\nThe earlier \"--keep-redundant-commit\" series broke \"cherry-pick\"\nthat is given a commit whose change is already in the current\nhistory. Such a cherry-pick would result in an empty change, and\nshould stop with an error, telling the user that conflict resolution\nmay have made the result empty (which is exactly what is happening),\nbut we silently dropped the change on the floor without any message\nnor non-zero exit code.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n sequencer.c                  | 73 +++++++++++++++++++++++++++-----------------\n t/t3505-cherry-pick-empty.sh | 30 ++++++++++++++++++\n 2 files changed, 75 insertions(+), 28 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 72cb4ff..1b2168c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -291,7 +291,8 @@ static int is_index_unchanged(void)\n  * If we are revert, or if our cherry-pick results in a hand merge,\n  * we had better say that the current user is responsible for that.\n  */\n-static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n+static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n+\t\t\t  int allow_empty)\n {\n \tstruct argv_array array;\n \tint rc;\n@@ -307,7 +308,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n \t\targv_array_push(&array, defmsg);\n \t}\n \n-\tif (opts->allow_empty)\n+\tif (allow_empty)\n \t\targv_array_push(&array, \"--allow-empty\");\n \n \trc = run_command_v_opt(array.argv, RUN_GIT_CMD);\n@@ -335,6 +336,44 @@ static int is_original_commit_empty(struct commit *commit)\n \treturn !hashcmp(ptree_sha1, commit->tree->object.sha1);\n }\n \n+/*\n+ * Do we run \"git commit\" with \"--allow-empty\"?\n+ */\n+static int allow_empty(struct replay_opts *opts, struct commit *commit)\n+{\n+\tint index_unchanged, empty_commit;\n+\n+\t/*\n+\t * Three cases:\n+\t *\n+\t * (1) we do not allow empty at all and error out.\n+\t *\n+\t * (2) we allow ones that were initially empty, but\n+\t * forbid the ones that become empty;\n+\t *\n+\t * (3) we allow both.\n+\t */\n+\tif (!opts->allow_empty)\n+\t\treturn 0; /* let \"git commit\" barf as necessary */\n+\n+\tindex_unchanged = is_index_unchanged();\n+\tif (index_unchanged < 0)\n+\t\treturn index_unchanged;\n+\tif (!index_unchanged)\n+\t\treturn 0; /* we do not have to say --allow-empty */\n+\n+\tif (opts->keep_redundant_commits)\n+\t\treturn 1;\n+\n+\tempty_commit = is_original_commit_empty(commit);\n+\tif (empty_commit < 0)\n+\t\treturn empty_commit;\n+\tif (!empty_commit)\n+\t\treturn 0;\n+\telse\n+\t\treturn 1;\n+}\n+\n static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n {\n \tunsigned char head[20];\n@@ -344,8 +383,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint res;\n-\tint empty_commit;\n-\tint index_unchanged;\n \n \tif (opts->no_commit) {\n \t\t/*\n@@ -471,10 +508,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tfree_commit_list(remotes);\n \t}\n \n-\tempty_commit = is_original_commit_empty(commit);\n-\tif (empty_commit < 0)\n-\t\treturn empty_commit;\n-\n \t/*\n \t * If the merge was clean or if it failed due to conflict, we write\n \t * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.\n@@ -495,27 +528,11 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tprint_advice(res == 1, opts);\n \t\trerere(opts->allow_rerere_auto);\n \t} else {\n-\t\tindex_unchanged = is_index_unchanged();\n-\t\t/*\n-\t\t * If index_unchanged is less than 0, that indicates we either\n-\t\t * couldn't parse HEAD or the index, so error out here.\n-\t\t */\n-\t\tif (index_unchanged < 0)\n-\t\t\treturn index_unchanged;\n-\n-\t\tif (!empty_commit && !opts->keep_redundant_commits && index_unchanged)\n-\t\t\t/*\n-\t\t\t * The head tree and the index match\n-\t\t\t * meaning the commit is empty.  Since it wasn't created\n-\t\t\t * empty (based on the previous test), we can conclude\n-\t\t\t * the commit has been made redundant.  Since we don't\n-\t\t\t * want to keep redundant commits, we can just return\n-\t\t\t * here, skipping this commit\n-\t\t\t */\n-\t\t\treturn 0;\n-\n+\t\tint allow = allow_empty(opts, commit);\n+\t\tif (allow < 0)\n+\t\t\treturn allow;\n \t\tif (!opts->no_commit)\n-\t\t\tres = run_git_commit(defmsg, opts);\n+\t\t\tres = run_git_commit(defmsg, opts, allow);\n \t}\n \n \tfree_message(&msg);\ndiff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh\nindex 92f00cd..5a1340c 100755\n--- a/t/t3505-cherry-pick-empty.sh\n+++ b/t/t3505-cherry-pick-empty.sh\n@@ -71,4 +71,34 @@ test_expect_success 'cherry pick with --keep-redundant-commits' '\n \tgit cherry-pick --keep-redundant-commits HEAD^\n '\n \n+test_expect_success 'cherry-pick a commit that becomes no-op (prep)' '\n+\tgit checkout master &&\n+\tgit branch fork &&\n+\techo foo >file2 &&\n+\tgit add file2 &&\n+\ttest_tick &&\n+\tgit commit -m \"add file2 on master\" &&\n+\n+\tgit checkout fork &&\n+\techo foo >file2 &&\n+\tgit add file2 &&\n+\ttest_tick &&\n+\tgit commit -m \"add file2 on the side\"\n+'\n+\n+test_expect_success 'cherry-pick a no-op without --keep-redundant' '\n+\tgit reset --hard &&\n+\tgit checkout fork^0 &&\n+\ttest_must_fail git cherry-pick master\n+'\n+\n+test_expect_success 'cherry-pick a no-op with --keep-redundant' '\n+\tgit reset --hard &&\n+\tgit checkout fork^0 &&\n+\tgit cherry-pick --keep-redundant-commits master &&\n+\tgit show -s --format='%s' >actual &&\n+\techo \"add file2 on master\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.11.rc0.54.g0680f74\n"},{"id":"192526","messageId":"4FC6AFE3.5070805@cisco.com","threadId":"30602","inReplyTo":"7vipfe1ihg.fsf@alter.siamese.dyndns.org","subject":"Re: Cherry-pick dangles and forgets helpful advice in next","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2012-05-30T23:40:19Z","receivedAt":"2012-05-30T23:40:19Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"Junio C Hamano wrote:\n\n> Instead of reverting the entire thing, perhaps we can fix the\n> regression like this.\n>\n> With this, we no longer unconditionally give \"--allow-empty\" when we\n> run \"git commit\", when --allow-empty (which is only about commits\n> that are originally empty) is given to cherry-pick; specifically,\n> when the user did not ask for --keep-redundant-commit, we do not\n> give \"--allow-empty\" if the original commit is not.\n>\n> Thinking about it again, I _think_ we do not even have to check if\n> the result is an empty commit ourselves (\"git commit\" will do that\n> for us anyway), so we might want to rip \"is_empty_commit()\" out of\n> the problematic patch and keep only \"is_index_unmodified()\" bit, but\n> for now I think this may be good enough.\n>\n> Phil, does it fix your issue?\nYes, it appears to fix my issue.  I don't have the original condition in\nplay anymore, but it fixes the test case I cooked up earlier.\n\nThanks,\nPhil\n"},{"id":"192601","messageId":"7vy5o86yy2.fsf@alter.siamese.dyndns.org","threadId":"30602","inReplyTo":"4FC6AFE3.5070805@cisco.com","subject":"Re: Cherry-pick dangles and forgets helpful advice in next","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-31T17:29:57Z","receivedAt":"2012-05-31T17:29:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <hordp@cisco.com> writes:\n\n> Junio C Hamano wrote:\n>\n>> Instead of reverting the entire thing, perhaps we can fix the\n>> regression like this.\n>>\n>> With this, we no longer unconditionally give \"--allow-empty\" when we\n>> run \"git commit\", when --allow-empty (which is only about commits\n>> that are originally empty) is given to cherry-pick; specifically,\n>> when the user did not ask for --keep-redundant-commit, we do not\n>> give \"--allow-empty\" if the original commit is not.\n>>\n>> Thinking about it again, I _think_ we do not even have to check if\n>> the result is an empty commit ourselves (\"git commit\" will do that\n>> for us anyway), so we might want to rip \"is_empty_commit()\" out of\n>> the problematic patch and keep only \"is_index_unmodified()\" bit, but\n>> for now I think this may be good enough.\n>>\n>> Phil, does it fix your issue?\n>\n> Yes, it appears to fix my issue.  I don't have the original condition in\n> play anymore, but it fixes the test case I cooked up earlier.\n\nOK, I'm planning to merge the fix down before 1.7.11 final. It may\nhave broken the use case Neil wanted to support as a side effect (I\ntried to be careful but I did not do anything beyond the test cases\nas I do not deliberately add empty commits to my history); Neil may\nwant to double check the result.\n\nThanks.\n"}]}