{"thread":{"id":"36339","subject":"git-rebase-todo gets popped prematurely","startedAt":"2014-04-02T22:37:48Z","lastAt":"2014-06-10T19:17:02Z","messageCount":13,"participants":["Phil Hord","Fabian Ruch","Michael Haggerty","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"238302","messageId":"533C913C.20106@cisco.com","threadId":"36339","inReplyTo":null,"subject":"git-rebase-todo gets popped prematurely","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2014-04-02T22:37:48Z","receivedAt":"2014-04-02T22:37:48Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"During a 'git rebase --continue', I got an error about having left a\nfile in place which the next commit intended to add as new.  Stupid me.\n\nSo I rm'ed the file and tried again.  This time, git rebase --continue\nsucceeded.  But it accidentally left out the next commit in my rebase-todo.\n\nI looked in the code and it seems that when the \"pick\" returns an error,\nrebase--interactive stops and lets the user clean up.  But it assumes\nthe index  already tracks a conflicted merge, and so it removes the\ncommit from the todo list.  In this case, however, the conflicted merge\nwas avoided by detecting it in advance.  The result is that the \"would\nbe overwritten\" conflict evicts the entire commit from the rebase action.\n\nI think the code needs to detect the difference between \"merge failed;\nconflicted index\" and \"merge failed; no change\".  I think I can do this\nwith 'git-status -s -uno', maybe.  But I haven't tried it yet and it\nfeels like I'm missing a case or two also.\n\nI tried to bisect this to some specific change, but it fails the same\nway as far back as 1.6.5. \n\nTest script follows in case anyone has a better idea how to approach\nthis and wants to understand it better.\n\n    #!/bin/sh\n\n    set -x\n    git --version\n    rm -rf baz\n    git init baz && cd baz\n    echo initial>initial && git add initial && git commit -minitial\n    echo foo>foo && git add foo && git commit -mfoo\n    echo bar>bar && git add bar && git commit -mbar\n    git log --oneline\n\n    GIT_EDITOR='sed -i -e s/^pick/edit/ -e /^#/d' git rebase -i HEAD^^\n    touch bar\n    git rebase --continue\n    rm bar\n    git rebase --continue\n    git log --oneline\n\n\nAnd the tail of the output (note the missing \"bar\" commit even though\n\"Successfully rebased\"):\n\n    + git log --oneline\n    fcc9b6e bar\n    8121f15 foo\n    a521fa1 initial\n    + GIT_EDITOR='sed -i -e s/^pick/edit/ -e /^#/d'\n    + git rebase -i 'HEAD^^'\n    Stopped at 8121f1593ea5c66dc7e9af7719100c1fcf4ab721... foo\n    You can amend the commit now, with\n\n        git commit --amend\n\n    Once you are satisfied with your changes, run\n\n        git rebase --continue\n\n    + touch bar\n    + git rebase --continue\n    error: The following untracked working tree files would be\n    overwritten by merge:\n        bar\n    Please move or remove them before you can merge.\n    Aborting\n    Could not apply fcc9b6ef2941e870f88362edbe0f1078cebb20e5... bar\n    + rm bar\n    + git rebase --continue\n    Successfully rebased and updated refs/heads/master.\n    + git log --oneline\n    8121f15 foo\n    a521fa1 initial\n"},{"id":"242702","messageId":"5383BDE4.9000704@gmail.com","threadId":"36339","inReplyTo":"533C913C.20106@cisco.com","subject":"[RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-05-26T22:19:16Z","receivedAt":"2014-05-26T22:19:16Z","isPatch":false,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"`do_pick_commit` handles three situations if it is not fast-forwarding.\nIn order for `do_pick_commit` to identify the situation, it examines the\nreturn value of the selected merge command.\n\n1. return value 0 stands for a clean merge\n2. 1 is passed in case of a failed merge due to conflict\n3. any other return value means that the merge did not even start\n\nSo far, the sequencer returns 1 in case of a failed fast-forward, which\nwould mean \"failed merge due to conflict\". However, a fast-forward\neither starts and succeeds or does not start at all. In particular, it\ncannot fail in between like a merge with a dirty index due to conflicts.\n\nIn order to signal the three possible situations (not only success and\nfailure to complete) after a pick through porcelain commands such as\n`cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was\nchosen in line with the other situations in which the sequencer\nencounters an error.\n\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 90cac7b..97cecca 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \n \tread_cache();\n \tif (checkout_fast_forward(from, to, 1))\n-\t\texit(1); /* the callee should have complained already */\n+\t\texit(-1); /* the callee should have complained already */\n \tref_lock = lock_any_ref_for_update(\"HEAD\", unborn ? null_sha1 : from,\n \t\t\t\t\t   0, NULL);\n \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n-- \n1.9.3\n"},{"id":"242703","messageId":"5383BDEA.9070908@gmail.com","threadId":"36339","inReplyTo":"533C913C.20106@cisco.com","subject":"[RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-05-26T22:19:22Z","receivedAt":"2014-05-26T22:19:22Z","isPatch":false,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"When `rebase--interactive` processes a task, it removes the item from\nthe todo list and appends it to another list of executed tasks. If a\n`pick` (this includes `squash` and `fixup`) fails before the index has\nrecorded the changes, take the corresponding item and put it on the todo\nlist again. Otherwise, the changes introduced by the scheduled commit\nwould be lost.\n\nThat kind of decision is possible since the `cherry-pick` command\nsignals why it failed to apply the changes of the given commit. Either\nthe changes are recorded in the index using a conflict (return value 1)\nand `rebase` does not continue until they are resolved or the changes\nare not recorded in the index (return value neither 0 nor 1) and\n`rebase` has to try again with the same task.\n\nReported-by: Phil Hord <hordp@cisco.com>\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\n git-rebase--interactive.sh | 27 ++++++++++++++++++++++-----\n 1 file changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 9e1dd1e..bba4f3a 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -132,6 +132,16 @@ mark_action_done () {\n \tfi\n }\n \n+# Put the last action marked done at the beginning of the todo list\n+# again. If there has not been an action marked done yet, the list of\n+# items on the todo list is left unchanged.\n+reschedule_last_action () {\n+\ttail -n 1 \"$done\" | cat - \"$todo\" >\"$todo\".new\n+\tsed -e \\$d <\"$done\" >\"$done\".new\n+\tmv -f \"$todo\".new \"$todo\"\n+\tmv -f \"$done\".new \"$done\"\n+}\n+\n append_todo_help () {\n \tgit stripspace --comment-lines >>\"$todo\" <<\\EOF\n \n@@ -470,11 +480,15 @@ do_pick () {\n \t\t\t   --no-post-rewrite -n -q -C $1 &&\n \t\t\tpick_one -n $1 &&\n \t\t\tgit commit --allow-empty --allow-empty-message \\\n-\t\t\t\t   --amend --no-post-rewrite -n -q -C $1 ||\n-\t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n+\t\t\t\t   --amend --no-post-rewrite -n -q -C $1\n \telse\n-\t\tpick_one $1 ||\n-\t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n+\t\tpick_one $1\n+\tfi\n+\tret=$?\n+\tif test $ret -ne 0\n+\tthen\n+\t\ttest $ret -ne 1 && reschedule_last_action\n+\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n \tfi\n }\n \n@@ -533,8 +547,11 @@ do_next () {\n \t\tauthor_script_content=$(get_author_ident_from_commit HEAD)\n \t\techo \"$author_script_content\" > \"$author_script\"\n \t\teval \"$author_script_content\"\n-\t\tif ! pick_one -n $sha1\n+\t\tpick_one -n $sha1\n+\t\tret=$?\n+\t\tif test $ret -ne 0\n \t\tthen\n+\t\t\ttest $ret -ne 1 && reschedule_last_action\n \t\t\tgit rev-parse --verify HEAD >\"$amend\"\n \t\t\tdie_failed_squash $sha1 \"$rest\"\n \t\tfi\n-- \n1.9.3\n"},{"id":"242704","messageId":"5383BDF0.2040904@gmail.com","threadId":"36339","inReplyTo":"533C913C.20106@cisco.com","subject":"[RFC 3/3] tests: Add 'rebase -i commits that overwrite untracked files'","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-05-26T22:19:28Z","receivedAt":"2014-05-26T22:19:28Z","isPatch":false,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"If a todo list will cherry-pick a commit that adds some file and the\nworking tree already contains a file with the same name, the rebase\nsequence for that todo list will be interrupted and the cherry-picked\ncommit will be lost after the rebasing process is resumed.\n\nThis is fixed.\n\nAdd as a test case for regression testing to the \"rebase-interactive\"\ntest suite.\n\nReported-by: Phil Hord <hordp@cisco.com>\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\n t/t3404-rebase-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 44 insertions(+)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 50e22b1..7f5ac18 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1074,4 +1074,48 @@ test_expect_success 'short SHA-1 collide' '\n \t)\n '\n \n+test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '\n+\tgit checkout branch2 &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"edit 1 2\" git rebase -i A &&\n+\ttest_cmp_rev HEAD F &&\n+\ttest_path_is_missing file6 &&\n+\ttouch file6 &&\n+\ttest_must_fail git rebase --continue &&\n+\ttest_cmp_rev HEAD F &&\n+\trm file6 &&\n+\tgit rebase --continue &&\n+\ttest_cmp_rev HEAD I\n+'\n+\n+test_expect_success 'rebase -i commits that overwrite untracked files (squash)' '\n+\tgit checkout branch2 &&\n+\tgit tag original-branch2 &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"edit 1 squash 2\" git rebase -i A &&\n+\ttest_cmp_rev HEAD F &&\n+\ttest_path_is_missing file6 &&\n+\ttouch file6 &&\n+\ttest_must_fail git rebase --continue &&\n+\ttest_cmp_rev HEAD F &&\n+\trm file6 &&\n+\tgit rebase --continue &&\n+\ttest $(git cat-file commit HEAD | sed -ne \\$p) = I &&\n+\tgit reset --hard original-branch2\n+'\n+\n+test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '\n+\tgit checkout branch2 &&\n+\tset_fake_editor &&\n+\tFAKE_LINES=\"edit 1 2\" git rebase -i --no-ff A &&\n+\ttest $(git cat-file commit HEAD | sed -ne \\$p) = F &&\n+\ttest_path_is_missing file6 &&\n+\ttouch file6 &&\n+\ttest_must_fail git rebase --continue &&\n+\ttest $(git cat-file commit HEAD | sed -ne \\$p) = F &&\n+\trm file6 &&\n+\tgit rebase --continue &&\n+\ttest $(git cat-file commit HEAD | sed -ne \\$p) = I\n+'\n+\n test_done\n-- \n1.9.3\n"},{"id":"242740","messageId":"53847D7D.5050000@alum.mit.edu","threadId":"36339","inReplyTo":"5383BDEA.9070908@gmail.com","subject":"Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-05-27T11:56:45Z","receivedAt":"2014-05-27T11:56:45Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Hi,\n\nOverall, this approach seems reasonable.\n\nPlease see the inline comments below.\n\nOn 05/27/2014 12:19 AM, Fabian Ruch wrote:\n> When `rebase--interactive` processes a task, it removes the item from\n> the todo list and appends it to another list of executed tasks. If a\n> `pick` (this includes `squash` and `fixup`) fails before the index has\n> recorded the changes, take the corresponding item and put it on the todo\n> list again. Otherwise, the changes introduced by the scheduled commit\n> would be lost.\n> \n> That kind of decision is possible since the `cherry-pick` command\n> signals why it failed to apply the changes of the given commit. Either\n> the changes are recorded in the index using a conflict (return value 1)\n> and `rebase` does not continue until they are resolved or the changes\n> are not recorded in the index (return value neither 0 nor 1) and\n> `rebase` has to try again with the same task.\n> \n> Reported-by: Phil Hord <hordp@cisco.com>\n> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n> ---\n>  git-rebase--interactive.sh | 27 ++++++++++++++++++++++-----\n>  1 file changed, 22 insertions(+), 5 deletions(-)\n> \n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 9e1dd1e..bba4f3a 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -132,6 +132,16 @@ mark_action_done () {\n>  \tfi\n>  }\n>  \n> +# Put the last action marked done at the beginning of the todo list\n> +# again. If there has not been an action marked done yet, the list of\n> +# items on the todo list is left unchanged.\n\nThe comment would read better if the second sentence were also in active\nvoice, like the first sentence:\n\n    If there has not been an action marked done yet, leave the list of\n    items on the todo list unchanged.\n\n> +reschedule_last_action () {\n> +\ttail -n 1 \"$done\" | cat - \"$todo\" >\"$todo\".new\n> +\tsed -e \\$d <\"$done\" >\"$done\".new\n> +\tmv -f \"$todo\".new \"$todo\"\n> +\tmv -f \"$done\".new \"$done\"\n> +}\n> +\n>  append_todo_help () {\n>  \tgit stripspace --comment-lines >>\"$todo\" <<\\EOF\n>  \n> @@ -470,11 +480,15 @@ do_pick () {\n>  \t\t\t   --no-post-rewrite -n -q -C $1 &&\n>  \t\t\tpick_one -n $1 &&\n>  \t\t\tgit commit --allow-empty --allow-empty-message \\\n> -\t\t\t\t   --amend --no-post-rewrite -n -q -C $1 ||\n> -\t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n> +\t\t\t\t   --amend --no-post-rewrite -n -q -C $1\n\n\"git cherry-pick\" indicates its error status specifically as 1 or some\nother value.  But here it could be that pick_one succeeds but \"git\ncommit\" fails; in that case ret is set to the return code of \"git\ncommit\".  So, if \"git commit\" fails with a retcode different than 1,\nthen reschedule_last_action will be called a few lines later.  This\nseems incorrect to me.\n\n>  \telse\n> -\t\tpick_one $1 ||\n> -\t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n> +\t\tpick_one $1\n> +\tfi\n> +\tret=$?\n> +\tif test $ret -ne 0\n> +\tthen\n> +\t\ttest $ret -ne 1 && reschedule_last_action\n> +\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n>  \tfi\n>  }\n>  \n> @@ -533,8 +547,11 @@ do_next () {\n>  \t\tauthor_script_content=$(get_author_ident_from_commit HEAD)\n>  \t\techo \"$author_script_content\" > \"$author_script\"\n>  \t\teval \"$author_script_content\"\n> -\t\tif ! pick_one -n $sha1\n> +\t\tpick_one -n $sha1\n> +\t\tret=$?\n> +\t\tif test $ret -ne 0\n>  \t\tthen\n> +\t\t\ttest $ret -ne 1 && reschedule_last_action\n>  \t\t\tgit rev-parse --verify HEAD >\"$amend\"\n>  \t\t\tdie_failed_squash $sha1 \"$rest\"\n>  \t\tfi\n> \n\nI suggest that you add a comment for pick_one explaining that if it\nfails, its failure code is like that of cherry-pick, namely ...etc...\nThis will warn future developers to preserve the error code semantics.\n\nIt is preferable to squash the next commit, containing the tests,\ntogether with this commit.\n\nThanks,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"242742","messageId":"53849002.7060502@alum.mit.edu","threadId":"36339","inReplyTo":"5383BDF0.2040904@gmail.com","subject":"Re: [RFC 3/3] tests: Add 'rebase -i commits that overwrite untracked files'","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-05-27T13:15:46Z","receivedAt":"2014-05-27T13:15:46Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/27/2014 12:19 AM, Fabian Ruch wrote:\n> If a todo list will cherry-pick a commit that adds some file and the\n> working tree already contains a file with the same name, the rebase\n> sequence for that todo list will be interrupted and the cherry-picked\n> commit will be lost after the rebasing process is resumed.\n> \n> This is fixed.\n> \n> Add as a test case for regression testing to the \"rebase-interactive\"\n> test suite.\n\nThe tests look fine.  (I assume you tested the tests against the\npre-fixed version of the software to make sure that they caught the\nproblem that you fixed.)\n\nAs I mentioned in patch 2/3, I think it would be better to add the tests\nin the same commit where you fix the problem.\n\nThe commit message is, I think, confusing because the first paragraph is\nin future tense even though it is describing a problem that was just\nfixed.  It will probably change completely when you squash this with the\nprevious commit, but for future reference, I would have suggested\nsomething more like\n\n    t3404: \"rebase -i\" retries pick when blocked by untracked file\n\n    If a commit that is being \"pick\"ed adds a file that already exists\n    as an untracked file in the working tree, cherry-pick fails before\n    even trying to apply the change.  \"rebase --interactive\" didn't\n    distinguish this error from a conflict, and when \"rebase --continue\"\n    was run it thought that the user had just resolved and committed\n    the conflict.  The result was that the commit was omitted entirely\n    from the rebased series.\n\n    This problem was fixed in the previous commit.  Add tests.\n\n> Reported-by: Phil Hord <hordp@cisco.com>\n> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n> ---\n>  t/t3404-rebase-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 44 insertions(+)\n> \n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 50e22b1..7f5ac18 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1074,4 +1074,48 @@ test_expect_success 'short SHA-1 collide' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '\n> +\tgit checkout branch2 &&\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"edit 1 2\" git rebase -i A &&\n> +\ttest_cmp_rev HEAD F &&\n> +\ttest_path_is_missing file6 &&\n> +\ttouch file6 &&\n> +\ttest_must_fail git rebase --continue &&\n> +\ttest_cmp_rev HEAD F &&\n> +\trm file6 &&\n> +\tgit rebase --continue &&\n> +\ttest_cmp_rev HEAD I\n> +'\n> +\n> +test_expect_success 'rebase -i commits that overwrite untracked files (squash)' '\n> +\tgit checkout branch2 &&\n> +\tgit tag original-branch2 &&\n\nIt might be easier (and better decoupled from other tests) to\n\n        git checkout -b squash-overwrite branch2 &&\n\nrather than moving branch2 then resetting it.  That way you can also\nleave the branch in the repo when you are done rather than having to\nclean it up.\n\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"edit 1 squash 2\" git rebase -i A &&\n> +\ttest_cmp_rev HEAD F &&\n> +\ttest_path_is_missing file6 &&\n> +\ttouch file6 &&\n> +\ttest_must_fail git rebase --continue &&\n> +\ttest_cmp_rev HEAD F &&\n> +\trm file6 &&\n> +\tgit rebase --continue &&\n> +\ttest $(git cat-file commit HEAD | sed -ne \\$p) = I &&\n> +\tgit reset --hard original-branch2\n> +'\n> +\n> +test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' '\n> +\tgit checkout branch2 &&\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"edit 1 2\" git rebase -i --no-ff A &&\n> +\ttest $(git cat-file commit HEAD | sed -ne \\$p) = F &&\n> +\ttest_path_is_missing file6 &&\n> +\ttouch file6 &&\n> +\ttest_must_fail git rebase --continue &&\n> +\ttest $(git cat-file commit HEAD | sed -ne \\$p) = F &&\n> +\trm file6 &&\n> +\tgit rebase --continue &&\n> +\ttest $(git cat-file commit HEAD | sed -ne \\$p) = I\n> +'\n> +\n>  test_done\n> \n\nThanks!\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"242760","messageId":"5384D8DA.40005@cisco.com","threadId":"36339","inReplyTo":"53847D7D.5050000@alum.mit.edu","subject":"Re: [RFC 2/3] rebase -i: Reschedule tasks that failed before the index was touched","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2014-05-27T18:26:34Z","receivedAt":"2014-05-27T18:26:34Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"Hi Fabian,\n\nThanks for looking into this.\n\nOn 05/27/2014 07:56 AM, Michael Haggerty wrote:\n>> +reschedule_last_action () {\n>> +\ttail -n 1 \"$done\" | cat - \"$todo\" >\"$todo\".new\n>> +\tsed -e \\$d <\"$done\" >\"$done\".new\n>> +\tmv -f \"$todo\".new \"$todo\"\n>> +\tmv -f \"$done\".new \"$done\"\n>> +}\n>> +\n>>  append_todo_help () {\n>>  \tgit stripspace --comment-lines >>\"$todo\" <<\\EOF\n>>  \n>> @@ -470,11 +480,15 @@ do_pick () {\n>>  \t\t\t   --no-post-rewrite -n -q -C $1 &&\n>>  \t\t\tpick_one -n $1 &&\n>>  \t\t\tgit commit --allow-empty --allow-empty-message \\\n>> -\t\t\t\t   --amend --no-post-rewrite -n -q -C $1 ||\n>> -\t\t\tdie_with_patch $1 \"Could not apply $1... $2\"\n>> +\t\t\t\t   --amend --no-post-rewrite -n -q -C $1\n> \"git cherry-pick\" indicates its error status specifically as 1 or some\n> other value.  But here it could be that pick_one succeeds but \"git\n> commit\" fails; in that case ret is set to the return code of \"git\n> commit\".  So, if \"git commit\" fails with a retcode different than 1,\n> then reschedule_last_action will be called a few lines later.  This\n> seems incorrect to me.\n\nI agree.  I was thinking that pick_one should get this new behavior\ninstead of do_pick, but some callers may not appreciate the new behavior\n(i.e. squash/fixup), though I expect those callers have similar problems\nas this commit is trying to fix.\n\nI think adding a pick_one_with_reschedule function (to be called in both\nplaces from do_pick) could solve the issue Michael mentioned without\nbreaking other pick_one callers.\n\nI have not tested doing so, but I think it would look like this:\n\n===================\n\ndiff --git i/git-rebase--interactive.sh w/git-rebase--interactive.sh\nindex fe813ba..ae5603a 100644\n--- i/git-rebase--interactive.sh\n+++ w/git-rebase--interactive.sh\n@@ -235,6 +235,17 @@ git_sequence_editor () {\n     eval \"$GIT_SEQUENCE_EDITOR\" '\"$@\"'\n }\n \n+pick_one_with_reschedule () {\n+    pick_one $1\n+    ret=$?\n+    case \"$ret\" in\n+        0) ;;\n+        1) ;;\n+        *) reschedule_last_action ;;\n+    esac\n+    return $ret\n+}\n+\n pick_one () {\n     ff=--ff\n \n@@ -474,13 +485,13 @@ do_pick () {\n         # rebase --continue.\n         git commit --allow-empty --allow-empty-message --amend \\\n                --no-post-rewrite -n -q -C $1 &&\n-            pick_one -n $1 &&\n+            pick_one_with_reschedule -n $1 &&\n             git commit --allow-empty --allow-empty-message \\\n                    --amend --no-post-rewrite -n -q -C $1 \\\n                    ${gpg_sign_opt:+\"$gpg_sign_opt\"} ||\n             die_with_patch $1 \"Could not apply $1... $2\"\n     else\n-        pick_one $1 ||\n+        pick_one_with_reschedule $1 ||\n             die_with_patch $1 \"Could not apply $1... $2\"\n     fi\n }\n\n===================\n\nI don't much like the name 'pick_one_with_reschedule', but I didn't like\nmy other choices, either.\n\nI'll try to look at this and your patches more closely when I have a bit\nmore time.\n\nPhil\n"},{"id":"242762","messageId":"xmqqvbsrf4hd.fsf@gitster.dls.corp.google.com","threadId":"36339","inReplyTo":"5383BDE4.9000704@gmail.com","subject":"Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-27T18:42:22Z","receivedAt":"2014-05-27T18:42:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Ruch <bafain@gmail.com> writes:\n\n> `do_pick_commit` handles three situations if it is not fast-forwarding.\n> In order for `do_pick_commit` to identify the situation, it examines the\n> return value of the selected merge command.\n>\n> 1. return value 0 stands for a clean merge\n> 2. 1 is passed in case of a failed merge due to conflict\n> 3. any other return value means that the merge did not even start\n>\n> So far, the sequencer returns 1 in case of a failed fast-forward, which\n> would mean \"failed merge due to conflict\". However, a fast-forward\n> either starts and succeeds or does not start at all. In particular, it\n> cannot fail in between like a merge with a dirty index due to conflicts.\n>\n> In order to signal the three possible situations (not only success and\n> failure to complete) after a pick through porcelain commands such as\n> `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was\n> chosen in line with the other situations in which the sequencer\n> encounters an error.\n\nHmph... do we still pass negative to exit() anywhere in our codebase?\n\n>\n> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n> ---\n>  sequencer.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 90cac7b..97cecca 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n>  \n>  \tread_cache();\n>  \tif (checkout_fast_forward(from, to, 1))\n> -\t\texit(1); /* the callee should have complained already */\n> +\t\texit(-1); /* the callee should have complained already */\n>  \tref_lock = lock_any_ref_for_update(\"HEAD\", unborn ? null_sha1 : from,\n>  \t\t\t\t\t   0, NULL);\n>  \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n"},{"id":"242763","messageId":"xmqqr43ff490.fsf@gitster.dls.corp.google.com","threadId":"36339","inReplyTo":"5383BDF0.2040904@gmail.com","subject":"Re: [RFC 3/3] tests: Add 'rebase -i commits that overwrite untracked files'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-27T18:47:23Z","receivedAt":"2014-05-27T18:47:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Ruch <bafain@gmail.com> writes:\n\n> If a todo list will cherry-pick a commit that adds some file and the\n> working tree already contains a file with the same name, the rebase\n> sequence for that todo list will be interrupted and the cherry-picked\n> commit will be lost after the rebasing process is resumed.\n>\n> This is fixed.\n>\n> Add as a test case for regression testing to the \"rebase-interactive\"\n> test suite.\n>\n> Reported-by: Phil Hord <hordp@cisco.com>\n> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n> ---\n>  t/t3404-rebase-interactive.sh | 44 +++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 44 insertions(+)\n>\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 50e22b1..7f5ac18 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1074,4 +1074,48 @@ test_expect_success 'short SHA-1 collide' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '\n> +\tgit checkout branch2 &&\n> +\tset_fake_editor &&\n> +\tFAKE_LINES=\"edit 1 2\" git rebase -i A &&\n> +\ttest_cmp_rev HEAD F &&\n> +\ttest_path_is_missing file6 &&\n> +\ttouch file6 &&\n\nUnless you care deeply about updating the timestamp file6 has, use\nof \"touch\" is misleading.  Use something like this instead:\n\n\t>file6 &&\n\nwhen it is the existence of \"file6\" that you care about.\n\nThanks.\n"},{"id":"243626","messageId":"5395CD04.2050303@gmail.com","threadId":"36339","inReplyTo":"xmqqvbsrf4hd.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2014-06-09T15:04:36Z","receivedAt":"2014-06-09T15:04:36Z","isPatch":false,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Hi Junio,\n\nOn 05/27/2014 08:42 PM, Junio C Hamano wrote:\n> Fabian Ruch <bafain@gmail.com> writes:\n>> [..]\n>>\n>> In order to signal the three possible situations (not only success and\n>> failure to complete) after a pick through porcelain commands such as\n>> `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was\n>> chosen in line with the other situations in which the sequencer\n>> encounters an error.\n> \n> Hmph... do we still pass negative to exit() anywhere in our codebase?\n\nNo, but I thought `cmd_cherry_pick` would just forward the `return -1` from the\nsequencer to the shell. I had another look and found that `cmd_cherry_pick`\ncalls `die` instead. Now, the commit inserts 128 as exit status in\n`fast_forward_to`.\n\nWould it be appropriate to mention the choice of exit status in the coding\nguidelines? I didn't know that the int-argument to exit(3) gets truncated and\nthat this is why it is a general rule to only use values in the range from 0 to\n255 with exit(3).\n\nKind regards,\n   Fabian\n\n-- >8 --\nSubject: sequencer: Signal failed ff as an aborted, not a conflicted merge\n\n`do_pick_commit` handles three situations if it is not fast-forwarding.\nIn order for `do_pick_commit` to identify the situation, it examines the\nreturn value of the selected merge command.\n\n1. return value 0 stands for a clean merge\n2. 1 is passed in case of a failed merge due to conflict\n3. any other return value means that the merge did not even start\n\nSo far, the sequencer returns 1 in case of a failed fast-forward, which\nwould mean \"failed merge due to conflict\". However, a fast-forward\neither starts and succeeds or does not start at all. In particular, it\ncannot fail in between like a merge with a dirty index due to conflicts.\n\nIn order to signal the three possible situations (not only success and\nfailure to complete) after a pick through porcelain commands such as\n`cherry-pick`, exit with a return value that is neither 0 nor 1. 128 was\nchosen in line with the other situations in which the sequencer\nencounters an error. In such situations, the sequencer returns a\nnegative value and `cherry-pick` translates this into a call to `die`.\n`die` then terminates the process with exit status 128.\n\nSigned-off-by: Fabian Ruch <bafain@gmail.com>\n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 90cac7b..225afcb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \n \tread_cache();\n \tif (checkout_fast_forward(from, to, 1))\n-\t\texit(1); /* the callee should have complained already */\n+\t\texit(128); /* the callee should have complained already */\n \tref_lock = lock_any_ref_for_update(\"HEAD\", unborn ? null_sha1 : from,\n \t\t\t\t\t   0, NULL);\n \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n-- \n2.0.0\n"},{"id":"243748","messageId":"xmqq8up4abs3.fsf@gitster.dls.corp.google.com","threadId":"36339","inReplyTo":"5395CD04.2050303@gmail.com","subject":"Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T17:56:44Z","receivedAt":"2014-06-10T17:56:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Fabian Ruch <bafain@gmail.com> writes:\n\n> On 05/27/2014 08:42 PM, Junio C Hamano wrote:\n>> Fabian Ruch <bafain@gmail.com> writes:\n>>> [..]\n>>>\n>>> In order to signal the three possible situations (not only success and\n>>> failure to complete) after a pick through porcelain commands such as\n>>> `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was\n>>> chosen in line with the other situations in which the sequencer\n>>> encounters an error.\n>> \n>> Hmph... do we still pass negative to exit() anywhere in our codebase?\n>\n> No, but I thought `cmd_cherry_pick` would just forward the `return -1` from the\n> sequencer to the shell. I had another look and found that `cmd_cherry_pick`\n> calls `die` instead. Now, the commit inserts 128 as exit status in\n> `fast_forward_to`.\n>\n> Would it be appropriate to mention the choice of exit status in the coding\n> guidelines? I didn't know that the int-argument to exit(3) gets truncated and\n> that this is why it is a general rule to only use values in the range from 0 to\n> 255 with exit(3).\n\nI personally do not think of a reason why it is necessary to mention\nhow the argument to exit(3) is expected to be used by the system, but\nif it is common not to know it, I guess it would not hurt to have a\nsingle paragraph with at most two lines.\n\nIn any case, I agree that exiting with 1 that signals \"failed with\nconflict\" can be confusing to the caller.  Can we have a test to\ndemonstrate when this fix matters?\n\nThanks.\n\n> -- >8 --\n> Subject: sequencer: Signal failed ff as an aborted, not a conflicted merge\n>\n> `do_pick_commit` handles three situations if it is not fast-forwarding.\n> In order for `do_pick_commit` to identify the situation, it examines the\n> return value of the selected merge command.\n>\n> 1. return value 0 stands for a clean merge\n> 2. 1 is passed in case of a failed merge due to conflict\n> 3. any other return value means that the merge did not even start\n>\n> So far, the sequencer returns 1 in case of a failed fast-forward, which\n> would mean \"failed merge due to conflict\". However, a fast-forward\n> either starts and succeeds or does not start at all. In particular, it\n> cannot fail in between like a merge with a dirty index due to conflicts.\n>\n> In order to signal the three possible situations (not only success and\n> failure to complete) after a pick through porcelain commands such as\n> `cherry-pick`, exit with a return value that is neither 0 nor 1. 128 was\n> chosen in line with the other situations in which the sequencer\n> encounters an error. In such situations, the sequencer returns a\n> negative value and `cherry-pick` translates this into a call to `die`.\n> `die` then terminates the process with exit status 128.\n>\n> Signed-off-by: Fabian Ruch <bafain@gmail.com>\n> ---\n>  sequencer.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 90cac7b..225afcb 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n>  \n>  \tread_cache();\n>  \tif (checkout_fast_forward(from, to, 1))\n> -\t\texit(1); /* the callee should have complained already */\n> +\t\texit(128); /* the callee should have complained already */\n>  \tref_lock = lock_any_ref_for_update(\"HEAD\", unborn ? null_sha1 : from,\n>  \t\t\t\t\t   0, NULL);\n>  \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n"},{"id":"243755","messageId":"539753C3.2020101@cisco.com","threadId":"36339","inReplyTo":"xmqq8up4abs3.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2014-06-10T18:51:47Z","receivedAt":"2014-06-10T18:51:47Z","isPatch":false,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"\nOn 06/10/2014 01:56 PM, Junio C Hamano wrote:\n> Fabian Ruch <bafain@gmail.com> writes:\n>\n>> On 05/27/2014 08:42 PM, Junio C Hamano wrote:\n>>> Fabian Ruch <bafain@gmail.com> writes:\n>>>> [..]\n>>>>\n>>>> In order to signal the three possible situations (not only success and\n>>>> failure to complete) after a pick through porcelain commands such as\n>>>> `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was\n>>>> chosen in line with the other situations in which the sequencer\n>>>> encounters an error.\n>>> Hmph... do we still pass negative to exit() anywhere in our codebase?\n>> No, but I thought `cmd_cherry_pick` would just forward the `return -1` from the\n>> sequencer to the shell. I had another look and found that `cmd_cherry_pick`\n>> calls `die` instead. Now, the commit inserts 128 as exit status in\n>> `fast_forward_to`.\n>>\n>> Would it be appropriate to mention the choice of exit status in the coding\n>> guidelines? I didn't know that the int-argument to exit(3) gets truncated and\n>> that this is why it is a general rule to only use values in the range from 0 to\n>> 255 with exit(3).\n> I personally do not think of a reason why it is necessary to mention\n> how the argument to exit(3) is expected to be used by the system, but\n> if it is common not to know it, I guess it would not hurt to have a\n> single paragraph with at most two lines.\n>\n> In any case, I agree that exiting with 1 that signals \"failed with\n> conflict\" can be confusing to the caller.  Can we have a test to\n> demonstrate when this fix matters?\n\nI think you are asking for a test and not for clarification.  But a test\nwas provided in 3/3 in this series.  Was it not related directly enough?\n\nFor clarification, this tri-state return value matters when the caller\nis planning to do some cleanup and needs to handle the fallout\ncorrectly.  Maybe changing this return value is not the correct way\nforward, though.  It might be better if the caller could examine the\nresult after-the-fact instead.  This would require some reliable state\nfunctions which I recall were somewhat scattered last time I looked. \nAlso I cannot think of a reliable test for \"the previous cherry-pick\nfailed during pre-condition checks\" and I'm not sure anything should\nexist to track this in .git outside of the return value for this function. \n"},{"id":"243760","messageId":"xmqqvbs88tht.fsf@gitster.dls.corp.google.com","threadId":"36339","inReplyTo":"539753C3.2020101@cisco.com","subject":"Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-10T19:17:02Z","receivedAt":"2014-06-10T19:17:02Z","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 any case, I agree that exiting with 1 that signals \"failed with\n>> conflict\" can be confusing to the caller.  Can we have a test to\n>> demonstrate when this fix matters?\n>\n> I think you are asking for a test and not for clarification.  But a test\n> was provided in 3/3 in this series.  Was it not related directly enough?\n\nX-<  Somehow I missed the \"3\" in \"1/3\" above and did not look beyond\nthis first patch.\n\n> For clarification, this tri-state return value matters when the caller\n> is planning to do some cleanup and needs to handle the fallout\n> correctly.  Maybe changing this return value is not the correct way\n> forward, though.  It might be better if the caller could examine the\n> result after-the-fact instead.\n\nI am not sure about that.  For merge strategies \"exit with 1 iff you\nleft the conflict in the index\" is the contract between \"git merge\"\nfrontend and the strategies backend; if a similar contract is needed\nbetween the sequencer and its users, it is good to follow the same\npattern for consistency.  The resulting index and/or the working\ntree may or may not match the contents recorded in the HEAD commit\nbut without the backend telling the caller, the caller cannot tell\nif the difference was from before the operation or created by the\noperation.\n"}]}