{"thread":{"id":"39182","subject":"[PATCH] rebase -i: redo tasks that die during cherry-pick","startedAt":"2015-04-28T22:55:20Z","lastAt":"2015-04-30T09:54:50Z","messageCount":7,"participants":["Phil Hord","Johannes Schindelin","Junio C Hamano","Fabian Ruch"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"260150","messageId":"1430261720-9051-1-git-send-email-hordp@cisco.com","threadId":"39182","inReplyTo":null,"subject":"[PATCH] rebase -i: redo tasks that die during cherry-pick","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2015-04-28T22:55:20Z","receivedAt":"2015-04-28T22:55:20Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?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\npick (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\nrebase has to try again with the same task.\n\nAdd a test cases for regression testing to the \"rebase-interactive\"\ntest suite.\n\nSigned-off-by: Fabian Ruch \nSigned-off-by: Phil Hord \n---\n\nNotes:\n Last year in ${gmane}/250126 Fabian Ruch helpfully provided a patch\n to fix a rebase bug I complained about. I have simplified it a bit\n and merged in the tests which had been in a separate commit.\n \n It has bitten me twice since the original discussion and has also\n been reported by others, though I haven't found those emails to\n add them to the CC list yet.\n \n CC: Michael Haggerty \n\n git-rebase--interactive.sh | 16 +++++++++++++++\n t/t3404-rebase-interactive.sh | 47 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 63 insertions(+)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 08e5d86..bab0dcc 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, leave the list of\n+# items on the todo list 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@@ -252,6 +262,12 @@ pick_one () {\n \toutput eval git cherry-pick \\\n \t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n \t\t\t\"$strategy_args\" $empty_args $ff \"$@\"\n+\n+\t# If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule\n+\t# previous task so this commit is not lost.\n+\tret=$?\n+\tcase \"$ret\" in [01]) ;; *) reschedule_last_action ;; esac\n+\treturn $ret\n }\n \n pick_one_preserving_merges () {\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex eed76cc..ac429a0 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1055,4 +1055,51 @@ test_expect_success 'todo count' '\n \tgrep \"^# Rebase ..* onto ..* ([0-9]\" actual\n '\n \n+test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '\n+\tgit checkout --force branch2 &&\n+\tgit clean -f &&\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+\t>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 --force branch2 &&\n+\tgit clean -f &&\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+\t>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 --force branch2 &&\n+\tgit clean -f &&\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+\t>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-- \n2.4.0.rc3.329.gd1f7d3b\n"},{"id":"260151","messageId":"42512fad738e0ec47a8cdb6e6e92e994@www.dscho.org","threadId":"39182","inReplyTo":"1430261720-9051-1-git-send-email-hordp@cisco.com","subject":"Re: [PATCH] rebase -i: redo tasks that die during cherry-pick","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-28T23:17:39Z","receivedAt":"2015-04-28T23:17:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn 2015-04-29 00:55, Phil Hord 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> Add a test cases for regression testing to the \"rebase-interactive\"\n> test suite.\n> \n> Signed-off-by: Fabian Ruch \n> Signed-off-by: Phil Hord \n> ---\n\nACK.\n\nIt would be even nicer to avoid removing the task from the `todo` list until it has been performed correctly, of course, but I believe that would require a much more invasive patch. So this here patch is fine with me.\n\nCiao,\nDscho\n"},{"id":"260186","messageId":"xmqq7fsu6ek4.fsf@gitster.dls.corp.google.com","threadId":"39182","inReplyTo":"42512fad738e0ec47a8cdb6e6e92e994@www.dscho.org","subject":"Re: [PATCH] rebase -i: redo tasks that die during cherry-pick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-29T17:15:55Z","receivedAt":"2015-04-29T17:15:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On 2015-04-29 00:55, Phil Hord 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>> Add a test cases for regression testing to the \"rebase-interactive\"\n>> test suite.\n>> \n>> Signed-off-by: Fabian Ruch \n>> Signed-off-by: Phil Hord \n>> ---\n>\n> ACK.\n>\n> It would be even nicer to avoid removing the task from the `todo` list\n> until it has been performed correctly, of course, but I believe that\n> would require a much more invasive patch. So this here patch is fine\n> with me.\n\nThanks, will queue.\n\nAside from the \"much more invasive\" possibility, the patch makes me\nwonder if it would have been a better design to have a static \"todo\"\nwith a \"current\" pointer as two state files. Then reschedule would\nhave been just the matter of decrementing the number in \"current\",\ninstead of \"grab the last line of one file and prepend to the other\nfile, and then lose the last line\".\n"},{"id":"260194","messageId":"CABURp0odQWJC9LfQMywHKUarKAe7EY45fHET8xVVEsoBYDzBPg@mail.gmail.com","threadId":"39182","inReplyTo":"xmqq7fsu6ek4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] rebase -i: redo tasks that die during cherry-pick","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2015-04-29T19:32:33Z","receivedAt":"2015-04-29T19:32:33Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Wed, Apr 29, 2015 at 1:15 PM, Junio C Hamano wrote:\n>\n> Thanks, will queue.\n>\n> Aside from the \"much more invasive\" possibility, the patch makes me\n> wonder if it would have been a better design to have a static \"todo\"\n> with a \"current\" pointer as two state files. Then reschedule would\n> have been just the matter of decrementing the number in \"current\",\n> instead of \"grab the last line of one file and prepend to the other\n> file, and then lose the last line\".\n\nThat's an interesting idea. Changing it now would impact anyone who\nnow depends on the current todo/done behavior, and I imagine there\nare many.\n"},{"id":"260197","messageId":"xmqqtwvy4t97.fsf@gitster.dls.corp.google.com","threadId":"39182","inReplyTo":"CABURp0odQWJC9LfQMywHKUarKAe7EY45fHET8xVVEsoBYDzBPg@mail.gmail.com","subject":"Re: [PATCH] rebase -i: redo tasks that die during cherry-pick","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-29T19:41:24Z","receivedAt":"2015-04-29T19:41:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord writes:\n\n> On Wed, Apr 29, 2015 at 1:15 PM, Junio C Hamano wrote:\n>>\n>> Thanks, will queue.\n>>\n>> Aside from the \"much more invasive\" possibility, the patch makes me\n>> wonder if it would have been a better design to have a static \"todo\"\n>> with a \"current\" pointer as two state files. Then reschedule would\n>> have been just the matter of decrementing the number in \"current\",\n>> instead of \"grab the last line of one file and prepend to the other\n>> file, and then lose the last line\".\n>\n> That's an interesting idea. Changing it now would impact anyone who\n> now depends on the current todo/done behavior, and I imagine there\n> are many.\n\nYeah, in case it wasn't clear, I was merely lamenting over water\nunder the bridge, not seriously suggesting to break users to\nsimplify our logic.\n"},{"id":"260260","messageId":"66567923994761429d29715508bc415f@www.dscho.org","threadId":"39182","inReplyTo":"xmqq7fsu6ek4.fsf@gitster.dls.corp.google.com","subject":"rebase -i's todo/done list, was Re: [PATCH] rebase -i: redo tasks that die during cherry-pick","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-04-30T07:49:40Z","receivedAt":"2015-04-30T07:49:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-04-29 19:15, Junio C Hamano wrote:\n\n> [I] wonder if it would have been a better design to have a static \"todo\" with a \"current\" pointer as two state files.\n\nI decided against this in my original design because I frequently edited the todo list during the rebase run, already back when I had still called it `edit-patch-series.sh` (you will remember that \"Saturday project\" I reported on IRC).\n\nOne example for such an editing happens to me quite frequently: I reorder patches into a more logical order, except that I make mistakes and try to pull a patch too early, i.e. before the code it touches was added. In such a case, I obviously get a merge conflict, and after analysing the problem I have a better idea where to put that patch, which I do by adding a \"pick deadbeef\" into the `git-rebase-todo` list (these days, I use `git rebase --edit-todo`, of course).\n\nIf the list and the \"current\" pointer would be two different things, it would be way too easy to incur inconsistencies. Even for the inventor of rebase -i.\n\n> Then reschedule would have been just the matter of decrementing the number in \"current\", instead of \"grab the last line of one file and prepend to the other file, and then lose the last line\".\n\nI think that is only an implementation detail: whether we decrement a pointer or put back a line we moved to another file is essentially the same operation: it is a roll-back. The \"more invasive\" change I implied was to *avoid* the need for a roll-back, i.e. only move that line (or in your example, increment the pointer) after the operation completed successfully.\n\nMy implied change would mean that all the `mark_action_done` calls would have to be moved just before the `record_in_rewritten` calls -- but not all actions have that call. The tricky bit is that there are side effects: `peek_next_command` relies on the current behavior and would have to be adjusted. And that is only the side effect that I can think of off the top of my head.\n\nCiao,\nDscho\n"},{"id":"260267","messageId":"5541FBEA.6080804@gmail.com","threadId":"39182","inReplyTo":"1430261720-9051-1-git-send-email-hordp@cisco.com","subject":"Re: [PATCH] rebase -i: redo tasks that die during cherry-pick","fromName":"Fabian Ruch","fromEmail":"bafain@gmail.com","sentAt":"2015-04-30T09:54:50Z","receivedAt":"2015-04-30T09:54:50Z","isPatch":true,"sender":{"key":"bafain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1150972?v=4"},"body":"Hi,\n\nPhil Hord writes:\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> Add a test cases for regression testing to the \"rebase-interactive\"\n> test suite.\n> \n> Signed-off-by: Fabian Ruch \n> Signed-off-by: Phil Hord \n> ---\n> \n> Notes:\n> Last year in ${gmane}/250126 Fabian Ruch helpfully provided a patch\n> to fix a rebase bug I complained about. I have simplified it a bit\n> and merged in the tests which had been in a separate commit.\n\nThanks for picking this up again.\n\nIt seems like the bug still shows when git-rebase is asked to\nrecreate merges (option -p):\n\n set_fake_editor &&\n # 1 (A) adds file4, 2 (I) adds file6\n FAKE_LINES=\"edit 1 2\" git rebase -i -p A &&\n git commit --amend &&\n >file6 &&\n # merge fails because file6 would be overwritten\n test_must_fail git rebase --continue &&\n # succeeds because 2 was popped and is not tried again\n git rebase --continue &&\n # ERROR: top commit is A because I was skipped\n test $(git cat-file commit HEAD | sed -ne \\$p) = I\n\n(The git-commit command in the third line forces\npick_one_preserving_merges to recreate rather than fast-forward to\nthe commit that adds file6. Normally, one would instruct git-rebase\nto do so by specifying the --no-ff option. However, another bug\n(thread 252946) causes the --no-ff to be ignored in conjunction with\n-p; this example case then succeeds for the wrong reasons.)\n\n> It has bitten me twice since the original discussion and has also\n> been reported by others, though I haven't found those emails to\n> add them to the CC list yet.\n> \n> CC: Michael Haggerty \n> \n> git-rebase--interactive.sh | 16 +++++++++++++++\n> t/t3404-rebase-interactive.sh | 47 +++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 63 insertions(+)\n> \n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 08e5d86..bab0dcc 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, leave the list of\n> +# items on the todo list 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> @@ -252,6 +262,12 @@ pick_one () {\n> \toutput eval git cherry-pick \\\n> \t\t\t${gpg_sign_opt:+$(git rev-parse --sq-quote \"$gpg_sign_opt\")} \\\n> \t\t\t\"$strategy_args\" $empty_args $ff \"$@\"\n> +\n> +\t# If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule\n> +\t# previous task so this commit is not lost.\n> +\tret=$?\n> +\tcase \"$ret\" in [01]) ;; *) reschedule_last_action ;; esac\n> +\treturn $ret\n> }\n\nIf -p is specified, pick_one doesn't reach the git-cherry-pick\ncommand line shown here. Instead, it enters\npick_one_preserving_merges one line above and returns from pick_one\nimmediately afterwards.\n\n Fabian\n\n> pick_one_preserving_merges () {\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index eed76cc..ac429a0 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1055,4 +1055,51 @@ test_expect_success 'todo count' '\n> \tgrep \"^# Rebase ..* onto ..* ([0-9]\" actual\n> '\n> \n> +test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '\n> +\tgit checkout --force branch2 &&\n> +\tgit clean -f &&\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> +\t>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 --force branch2 &&\n> +\tgit clean -f &&\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> +\t>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 --force branch2 &&\n> +\tgit clean -f &&\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> +\t>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"}]}