{"thread":{"id":"50839","subject":"[PATCH 0/2] A couple of cherry-pick related fixes","startedAt":"2019-03-29T16:32:27Z","lastAt":"2019-04-17T15:04:24Z","messageCount":15,"participants":["Phillip Wood","Junio C Hamano","Duy Nguyen","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"372713","messageId":"20190329163009.493-1-phillip.wood123@gmail.com","threadId":"50839","inReplyTo":null,"subject":"[PATCH 0/2] A couple of cherry-pick related fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-03-29T16:30:07Z","receivedAt":"2019-03-29T16:32:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nSee the actual patches for a description of the fixes, I'm not sure\nabout the second one, see the notes on it. I'm going to be mostly\noff-line for the next couple of weeks so it might take me a while to\nrespond to any comments.\n\nPhillip Wood (2):\n  commit/reset: try to clean up sequencer state\n  fix cherry-pick/revert status after commit\n\n branch.c                        |  7 ++--\n builtin/commit.c                |  7 ++--\n sequencer.c                     | 60 +++++++++++++++++++++++++++++++++\n sequencer.h                     |  3 ++\n t/t3507-cherry-pick-conflict.sh | 19 +++++++++++\n t/t7512-status-help.sh          | 19 +++++++++++\n wt-status.c                     | 12 ++++++-\n 7 files changed, 122 insertions(+), 5 deletions(-)\n\n-- \n2.21.0\n\n"},{"id":"372714","messageId":"20190329163009.493-2-phillip.wood123@gmail.com","threadId":"50839","inReplyTo":"20190329163009.493-1-phillip.wood123@gmail.com","subject":"[PATCH 1/2] commit/reset: try to clean up sequencer state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-03-29T16:30:08Z","receivedAt":"2019-03-29T16:32:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen cherry-picking or reverting a sequence of commits and if the final\npick/revert has conflicts and the user uses `git commit` to commit the\nconflict resolution and does not run `git cherry-pick --continue` then\nthe sequencer state is left behind. This can cause problems later. In my\ncase I cherry-picked a sequence of commits the last one of which I\ncommitted with `git commit` after resolving some conflicts, then a while\nlater, on a different branch I aborted a revert which rewound my HEAD to\nthe end of the cherry-pick sequence on the previous branch. Avoid this\npotential problem by removing the sequencer state if we're committing or\nresetting the final pick in a sequence.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n branch.c                        |  7 +++++--\n builtin/commit.c                |  7 +++++--\n sequencer.c                     | 23 +++++++++++++++++++++++\n sequencer.h                     |  1 +\n t/t3507-cherry-pick-conflict.sh | 19 +++++++++++++++++++\n 5 files changed, 53 insertions(+), 4 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 28b81a7e02..9ed60081c1 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -5,6 +5,7 @@\n #include \"refs.h\"\n #include \"refspec.h\"\n #include \"remote.h\"\n+#include \"sequencer.h\"\n #include \"commit.h\"\n #include \"worktree.h\"\n \n@@ -339,8 +340,10 @@ void create_branch(struct repository *r,\n \n void remove_branch_state(struct repository *r)\n {\n-\tunlink(git_path_cherry_pick_head(r));\n-\tunlink(git_path_revert_head(r));\n+\tif (!unlink(git_path_cherry_pick_head(r)))\n+\t\tsequencer_post_commit_cleanup();\n+\tif (!unlink(git_path_revert_head(r)))\n+\t\tsequencer_post_commit_cleanup();\n \tunlink(git_path_merge_head(r));\n \tunlink(git_path_merge_rr(r));\n \tunlink(git_path_merge_msg(r));\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2986553d5f..422b7d62a5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1657,8 +1657,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tdie(\"%s\", err.buf);\n \t}\n \n-\tunlink(git_path_cherry_pick_head(the_repository));\n-\tunlink(git_path_revert_head(the_repository));\n+\tif (!unlink(git_path_cherry_pick_head(the_repository)))\n+\t\tsequencer_post_commit_cleanup();\n+\tif (!unlink(git_path_revert_head(the_repository)))\n+\t\tsequencer_post_commit_cleanup();\n \tunlink(git_path_merge_head(the_repository));\n \tunlink(git_path_merge_msg(the_repository));\n \tunlink(git_path_merge_mode(the_repository));\n@@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tif (amend && !no_post_rewrite) {\n \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n \t}\n+\n \tif (!quiet) {\n \t\tunsigned int flags = 0;\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 0db410d590..028699209f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)\n \treturn len;\n }\n \n+void sequencer_post_commit_cleanup(void)\n+{\n+\tstruct replay_opts opts = REPLAY_OPTS_INIT;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *eol;\n+\tconst char *todo_path = git_path_todo_file();\n+\n+\tif (strbuf_read_file(&buf, todo_path, 0) < 0) {\n+\t\tif (errno == ENOENT) {\n+\t\t\treturn;\n+\t\t} else {\n+\t\t\terror_errno(\"unable to open '%s'\", todo_path);\n+\t\t\treturn;\n+\t\t}\n+\t}\n+\t/* If there is only one line then we are done */\n+\teol = strchr(buf.buf, '\\n');\n+\tif (!eol || !eol[1])\n+\t\tsequencer_remove_state(&opts);\n+\n+\tstrbuf_release(&buf);\n+}\n+\n static int read_populate_todo(struct repository *r,\n \t\t\t      struct todo_list *todo_list,\n \t\t\t      struct replay_opts *opts)\ndiff --git a/sequencer.h b/sequencer.h\nindex 4d505b3590..43548295a1 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -144,3 +144,4 @@ int read_author_script(const char *path, char **name, char **email, char **date,\n void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\t      const char *onto, const char *orig_head);\n+void sequencer_post_commit_cleanup(void);\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 0db166152a..69e6389e69 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -156,6 +156,25 @@ test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '\n \n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n+test_expect_success 'successful final commit clears sequencer state' '\n+\tpristine_detach initial &&\n+\n+\ttest_must_fail git cherry-pick base picked-signed &&\n+\techo resolved >foo &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\tgit commit -a &&\n+\ttest_must_fail test_path_exists .git/sequencer\n+'\n+\n+test_expect_success 'reset after final pick clears sequencer state' '\n+\tpristine_detach initial &&\n+\n+\ttest_must_fail git cherry-pick base picked-signed &&\n+\techo resolved >foo &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\tgit reset &&\n+\ttest_must_fail test_path_exists .git/sequencer\n+'\n \n test_expect_success 'failed cherry-pick produces dirty index' '\n \tpristine_detach initial &&\n-- \n2.21.0\n\n"},{"id":"372715","messageId":"20190329163009.493-3-phillip.wood123@gmail.com","threadId":"50839","inReplyTo":"20190329163009.493-1-phillip.wood123@gmail.com","subject":"[PATCH 2/2] fix cherry-pick/revert status after commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-03-29T16:30:09Z","receivedAt":"2019-03-29T16:32:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf the user commits a conflict resolution using 'git commit' during a\nsequence of picks then 'git status' missed the fact that a\ncherry-pick/revert is still in progress.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n\nNotes:\n    I'm not user about printing the oid of the commit that was just picked,\n    maybe we should just say that a cherry-pick/revert is in progress. That\n    will mean auditing all the consumers of wt_status_get_state() to ensure\n    they can cope with a null oid.\n\nsequencer.c            | 37 +++++++++++++++++++++++++++++++++++++\n sequencer.h            |  2 ++\n t/t7512-status-help.sh | 19 +++++++++++++++++++\n wt-status.c            | 12 +++++++++++-\n 4 files changed, 69 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 028699209f..5595cc786f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2142,6 +2142,43 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \treturn !item->commit;\n }\n \n+int sequencer_get_last_command(struct repository *r, enum replay_action *action,\n+\t\t\t       struct object_id *oid)\n+{\n+\tstruct todo_item item;\n+\tchar *eol;\n+\tconst char *todo_file;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint ret = -1;\n+\n+\ttodo_file = git_path_todo_file();\n+\tif (strbuf_read_file(&buf, todo_file, 0) < 0) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn -1;\n+\t\telse\n+\t\t\treturn error_errno(\"unable to open '%s'\", todo_file);\n+\t}\n+\teol = strchrnul(buf.buf, '\\n');\n+\tif (buf.buf != eol && eol[-1] == '\\r')\n+\t\teol--; /* strip Carriage Return */\n+\tif (parse_insn_line(r, &item, buf.buf, eol))\n+\t\tgoto fail;\n+\tif (item.command == TODO_PICK)\n+\t\t*action = REPLAY_PICK;\n+\telse if (item.command == TODO_REVERT)\n+\t\t*action = REPLAY_REVERT;\n+\telse\n+\t\tgoto fail;\n+\n+\toidcpy(oid, &item.commit->object.oid);\n+\tret = 0;\n+\n+ fail:\n+\tstrbuf_release(&buf);\n+\n+\treturn ret;\n+}\n+\n static int parse_insn_buffer(struct repository *r, char *buf,\n \t\t\t     struct todo_list *todo_list)\n {\ndiff --git a/sequencer.h b/sequencer.h\nindex 43548295a1..815c68c4a3 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -145,3 +145,5 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\t      const char *onto, const char *orig_head);\n void sequencer_post_commit_cleanup(void);\n+int sequencer_get_last_command(struct repository* r, enum replay_action *action,\n+\t\t\t       struct object_id *oid);\ndiff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\nindex 458608cc1e..71d28a50f7 100755\n--- a/t/t7512-status-help.sh\n+++ b/t/t7512-status-help.sh\n@@ -780,6 +780,25 @@ EOF\n \ttest_i18ncmp expected actual\n '\n \n+test_expect_success 'status when cherry-picking after committing conflict resolution' '\n+\tgit reset --hard cherry_branch &&\n+\ttest_when_finished \"git cherry-pick --abort\" &&\n+\ttest_must_fail git cherry-pick cherry_branch_second one_cherry &&\n+\tTO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&\n+\techo end >main.txt &&\n+\tgit commit -a &&\n+\tcat >expected <<EOF &&\n+On branch cherry_branch\n+You are currently cherry-picking commit $TO_CHERRY_PICK.\n+  (all conflicts fixed: run \"git cherry-pick --continue\")\n+  (use \"git cherry-pick --abort\" to cancel the cherry-pick operation)\n+\n+nothing to commit (use -u to show untracked files)\n+EOF\n+\tgit status --untracked-files=no >actual &&\n+\ttest_i18ncmp expected actual\n+'\n+\n test_expect_success 'status showing detached at and from a tag' '\n \ttest_commit atag tagging &&\n \tgit checkout atag &&\ndiff --git a/wt-status.c b/wt-status.c\nindex 1f564b12d2..355e4cd03d 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -17,6 +17,7 @@\n #include \"utf8.h\"\n #include \"worktree.h\"\n #include \"lockfile.h\"\n+#include \"sequencer.h\"\n \n static const char cut_line[] =\n \"------------------------ >8 ------------------------\\n\";\n@@ -1563,6 +1564,7 @@ void wt_status_get_state(struct repository *r,\n {\n \tstruct stat st;\n \tstruct object_id oid;\n+\tenum replay_action action;\n \n \tif (!stat(git_path_merge_head(r), &st)) {\n \t\twt_status_check_rebase(NULL, state);\n@@ -1580,7 +1582,15 @@ void wt_status_get_state(struct repository *r,\n \t\tstate->revert_in_progress = 1;\n \t\toidcpy(&state->revert_head_oid, &oid);\n \t}\n-\n+\tif (!sequencer_get_last_command(r, &action, &oid)) {\n+\t\tif (action == REPLAY_PICK) {\n+\t\t\tstate->cherry_pick_in_progress = 1;\n+\t\t\toidcpy(&state->cherry_pick_head_oid, &oid);\n+\t\t} else {\n+\t\t\tstate->revert_in_progress = 1;\n+\t\t\toidcpy(&state->revert_head_oid, &oid);\n+\t\t}\n+\t}\n \tif (get_detached_from)\n \t\twt_status_get_detached_from(r, state);\n }\n-- \n2.21.0\n\n"},{"id":"372856","messageId":"xmqq36n2yvr8.fsf@gitster-ct.c.googlers.com","threadId":"50839","inReplyTo":"20190329163009.493-2-phillip.wood123@gmail.com","subject":"Re: [PATCH 1/2] commit/reset: try to clean up sequencer state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-01T08:28:43Z","receivedAt":"2019-04-01T08:28:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> When cherry-picking or reverting a sequence of commits and if the final\n> pick/revert has conflicts and the user uses `git commit` to commit the\n> conflict resolution and does not run `git cherry-pick --continue` then\n> the sequencer state is left behind. This can cause problems later. In my\n> case I cherry-picked a sequence of commits the last one of which I\n> committed with `git commit` after resolving some conflicts, then a while\n> later, on a different branch I aborted a revert which rewound my HEAD to\n> the end of the cherry-pick sequence on the previous branch.\n\nI've certainly seen this myself.  Do you use command line prompt\nsupport to remind you of the operation in progress?  I do, and I\nhave a suspicion that it did not help me in this situation by\nceasing to tell me that I have leftover state files after a manual\ncommit of the final step that conflicted and gave control back to\nme.\n\nAnd detecting that we are finishing the last step and making sure\nthat the state files are removed does sound like the right approach\nto fix it.\n\n> diff --git a/branch.c b/branch.c\n> index 28b81a7e02..9ed60081c1 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -5,6 +5,7 @@\n>  #include \"refs.h\"\n>  #include \"refspec.h\"\n>  #include \"remote.h\"\n> +#include \"sequencer.h\"\n>  #include \"commit.h\"\n>  #include \"worktree.h\"\n>  \n> @@ -339,8 +340,10 @@ void create_branch(struct repository *r,\n>  \n>  void remove_branch_state(struct repository *r)\n>  {\n> -\tunlink(git_path_cherry_pick_head(r));\n> -\tunlink(git_path_revert_head(r));\n> +\tif (!unlink(git_path_cherry_pick_head(r)))\n> +\t\tsequencer_post_commit_cleanup();\n> +\tif (!unlink(git_path_revert_head(r)))\n> +\t\tsequencer_post_commit_cleanup();\n\nThis and the same one in builtin/commit.c feels a bit iffy.  If we\nhad CHERRY_PICK_HEAD or REVERT_HEAD and attempted to remove one or\nthe other, whether the removal succeeds or fails (perhaps a virus\nscanner on Windows had the file open while we tried to unlink it,\ncausing the unlink() to fail), don't we want the clean-up to happen?\n\n> @@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \tif (amend && !no_post_rewrite) {\n>  \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n>  \t}\n> +\n\nThis is an unrelated change.\n\n>  \tif (!quiet) {\n>  \t\tunsigned int flags = 0;\n>  \n> diff --git a/sequencer.c b/sequencer.c\n> index 0db410d590..028699209f 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)\n>  \treturn len;\n>  }\n>  \n> +void sequencer_post_commit_cleanup(void)\n> +{\n> +\tstruct replay_opts opts = REPLAY_OPTS_INIT;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tconst char *eol;\n> +\tconst char *todo_path = git_path_todo_file();\n> +\n> +\tif (strbuf_read_file(&buf, todo_path, 0) < 0) {\n> +\t\tif (errno == ENOENT) {\n> +\t\t\treturn;\n> +\t\t} else {\n> +\t\t\terror_errno(\"unable to open '%s'\", todo_path);\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\t/* If there is only one line then we are done */\n> +\teol = strchr(buf.buf, '\\n');\n> +\tif (!eol || !eol[1])\n> +\t\tsequencer_remove_state(&opts);\n> +\n> +\tstrbuf_release(&buf);\n> +}\n\nI find this helper doing a bit too much and a bit too little at the\nsame time.  To reduce the iffiness I mentioned earlier, the callers\nwould behefit to have a helper that\n\n - notices the need to remove CHERRY_PICK_HEAD or REVERT_HEAD, and\n   returns without doing anything if there is no need;\n\n - remove the *_HEAD file.\n\n - detect if we have dealt with the last step, and returns without\n   doing any more thing if there are more to do;\n\n - remove the state files.\n\nIOW, replace the existing series of two unlink() calls with a single\ncall to the helper.\n\nOn the other hand, the bulk of hand-rolled logic to determine if we\nhave processed the last step is better done in another helper\nfunction that helps this helper, i.e.\n\n\tvoid sequencer_post_commit_cleanup(struct repo *r)\n\t{\n\t\tint need_cleanup = 0;\n\n\t\tif (file_exists(git_path_cherry_pick_head(r)) {\n\t\t\tunlink(git_path_cherry_pick_head(r));\n\t\t\tneed_cleanup = 1;\n\t\t}\n\t\tif (file_exists(git_path_revert_head(r)) {\n\t\t\tunlink(git_path_revert_head(r));\n\t\t\tneed_cleanup = 1;\n\t\t}\n\t\tif (!need_cleanup)\n\t\t\treturn;\n\t\tif (!have_finished_the_last_pick())\n\t\t\treturn;\n\t\tsequencer_remove_state(&opts);\n\t}\n\nas that makes it easier to follow the logic of what is going on at\nthis level, while at the same time making the logic in the\nhave_finished_the_last_pick() helper easier to read by giving it a\nmeaningful name.\n"},{"id":"372857","messageId":"xmqqv9zyxgwk.fsf@gitster-ct.c.googlers.com","threadId":"50839","inReplyTo":"20190329163009.493-3-phillip.wood123@gmail.com","subject":"Re: [PATCH 2/2] fix cherry-pick/revert status after commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-01T08:34:51Z","receivedAt":"2019-04-01T08:34:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> If the user commits a conflict resolution using 'git commit' during a\n> sequence of picks then 'git status' missed the fact that a\n> cherry-pick/revert is still in progress.\n\nHow well would this play with the previous step?  Didn't the change\nto builtin/commit.c made in [1/2] mean that after 'git commit' that\nconcludes the last step, there is nothing 'git status' to notice?\n\n\n"},{"id":"372865","messageId":"CACsJy8D3tH0K8wNLighuNtjUtv3K3TGNMGgx3T5j5sCxok8hbQ@mail.gmail.com","threadId":"50839","inReplyTo":"20190329163009.493-2-phillip.wood123@gmail.com","subject":"Re: [PATCH 1/2] commit/reset: try to clean up sequencer state","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-01T10:09:21Z","receivedAt":"2019-04-01T10:09:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 29, 2019 at 11:32 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> When cherry-picking or reverting a sequence of commits and if the final\n> pick/revert has conflicts and the user uses `git commit` to commit the\n> conflict resolution and does not run `git cherry-pick --continue` then\n> the sequencer state is left behind. This can cause problems later. In my\n> case I cherry-picked a sequence of commits the last one of which I\n> committed with `git commit` after resolving some conflicts, then a while\n> later, on a different branch I aborted a revert which rewound my HEAD to\n> the end of the cherry-pick sequence on the previous branch. Avoid this\n> potential problem by removing the sequencer state if we're committing or\n> resetting the final pick in a sequence.\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  branch.c                        |  7 +++++--\n>  builtin/commit.c                |  7 +++++--\n>  sequencer.c                     | 23 +++++++++++++++++++++++\n>  sequencer.h                     |  1 +\n>  t/t3507-cherry-pick-conflict.sh | 19 +++++++++++++++++++\n>  5 files changed, 53 insertions(+), 4 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index 28b81a7e02..9ed60081c1 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -5,6 +5,7 @@\n>  #include \"refs.h\"\n>  #include \"refspec.h\"\n>  #include \"remote.h\"\n> +#include \"sequencer.h\"\n>  #include \"commit.h\"\n>  #include \"worktree.h\"\n>\n> @@ -339,8 +340,10 @@ void create_branch(struct repository *r,\n>\n>  void remove_branch_state(struct repository *r)\n\nThis function is also called in git-am, git-rebase and git-checkout.\nWhile the first two should not be affected, git-checkout can be\nexecuted while we're in the middle of a cherry-pick or revert. I guess\nthat's ok because git-checkout is basically the same as git-reset in\nthis case?\n\n>  {\n> -       unlink(git_path_cherry_pick_head(r));\n> -       unlink(git_path_revert_head(r));\n> +       if (!unlink(git_path_cherry_pick_head(r)))\n> +               sequencer_post_commit_cleanup();\n> +       if (!unlink(git_path_revert_head(r)))\n> +               sequencer_post_commit_cleanup();\n>         unlink(git_path_merge_head(r));\n>         unlink(git_path_merge_rr(r));\n>         unlink(git_path_merge_msg(r));\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 2986553d5f..422b7d62a5 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1657,8 +1657,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>                 die(\"%s\", err.buf);\n>         }\n>\n> -       unlink(git_path_cherry_pick_head(the_repository));\n> -       unlink(git_path_revert_head(the_repository));\n> +       if (!unlink(git_path_cherry_pick_head(the_repository)))\n> +               sequencer_post_commit_cleanup();\n> +       if (!unlink(git_path_revert_head(the_repository)))\n> +               sequencer_post_commit_cleanup();\n>         unlink(git_path_merge_head(the_repository));\n>         unlink(git_path_merge_msg(the_repository));\n>         unlink(git_path_merge_mode(the_repository));\n> @@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>         if (amend && !no_post_rewrite) {\n>                 commit_post_rewrite(the_repository, current_head, &oid);\n>         }\n> +\n>         if (!quiet) {\n>                 unsigned int flags = 0;\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 0db410d590..028699209f 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)\n>         return len;\n>  }\n>\n> +void sequencer_post_commit_cleanup(void)\n> +{\n> +       struct replay_opts opts = REPLAY_OPTS_INIT;\n> +       struct strbuf buf = STRBUF_INIT;\n> +       const char *eol;\n> +       const char *todo_path = git_path_todo_file();\n> +\n> +       if (strbuf_read_file(&buf, todo_path, 0) < 0) {\n> +               if (errno == ENOENT) {\n> +                       return;\n> +               } else {\n> +                       error_errno(\"unable to open '%s'\", todo_path);\n\n_() the string to make it translatable.\n\n> +                       return;\n> +               }\n> +       }\n> +       /* If there is only one line then we are done */\n> +       eol = strchr(buf.buf, '\\n');\n> +       if (!eol || !eol[1])\n> +               sequencer_remove_state(&opts);\n\nShould we say something to let the user know cherry-pick/revert is\nfinished? (unless --quiet is specified)\n\n> +\n> +       strbuf_release(&buf);\n> +}\n> +\n>  static int read_populate_todo(struct repository *r,\n>                               struct todo_list *todo_list,\n>                               struct replay_opts *opts)\n-- \nDuy\n"},{"id":"373421","messageId":"1a3cf9ed-69ff-d961-e62b-a38b45913411@gmail.com","threadId":"50839","inReplyTo":"xmqq36n2yvr8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] commit/reset: try to clean up sequencer state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-08T14:15:23Z","receivedAt":"2019-04-08T14:15:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 01/04/2019 09:28, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> When cherry-picking or reverting a sequence of commits and if the final\n>> pick/revert has conflicts and the user uses `git commit` to commit the\n>> conflict resolution and does not run `git cherry-pick --continue` then\n>> the sequencer state is left behind. This can cause problems later. In my\n>> case I cherry-picked a sequence of commits the last one of which I\n>> committed with `git commit` after resolving some conflicts, then a while\n>> later, on a different branch I aborted a revert which rewound my HEAD to\n>> the end of the cherry-pick sequence on the previous branch.\n> \n> I've certainly seen this myself.  Do you use command line prompt\n> support to remind you of the operation in progress?  I do, and I\n> have a suspicion that it did not help me in this situation by\n> ceasing to tell me that I have leftover state files after a manual\n> commit of the final step that conflicted and gave control back to\n> me.\n\nSame here, the prompt I use just checks for the presence of \nCHERRY_PICK_HEAD which disappears after committing.\n\n> And detecting that we are finishing the last step and making sure\n> that the state files are removed does sound like the right approach\n> to fix it.\n> \n>> diff --git a/branch.c b/branch.c\n>> index 28b81a7e02..9ed60081c1 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n>> @@ -5,6 +5,7 @@\n>>   #include \"refs.h\"\n>>   #include \"refspec.h\"\n>>   #include \"remote.h\"\n>> +#include \"sequencer.h\"\n>>   #include \"commit.h\"\n>>   #include \"worktree.h\"\n>>   \n>> @@ -339,8 +340,10 @@ void create_branch(struct repository *r,\n>>   \n>>   void remove_branch_state(struct repository *r)\n>>   {\n>> -\tunlink(git_path_cherry_pick_head(r));\n>> -\tunlink(git_path_revert_head(r));\n>> +\tif (!unlink(git_path_cherry_pick_head(r)))\n>> +\t\tsequencer_post_commit_cleanup();\n>> +\tif (!unlink(git_path_revert_head(r)))\n>> +\t\tsequencer_post_commit_cleanup();\n> \n> This and the same one in builtin/commit.c feels a bit iffy.  If we\n> had CHERRY_PICK_HEAD or REVERT_HEAD and attempted to remove one or\n> the other, whether the removal succeeds or fails (perhaps a virus\n> scanner on Windows had the file open while we tried to unlink it,\n> causing the unlink() to fail), don't we want the clean-up to happen?\n\nGood point, I could add '|| errno == ENOENT' to the if or just use \nfile_exists() as you suggest below\n\n>> @@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>   \tif (amend && !no_post_rewrite) {\n>>   \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n>>   \t}\n>> +\n> \n> This is an unrelated change.\n\nOops I'm not sure where that came from\n\n>>   \tif (!quiet) {\n>>   \t\tunsigned int flags = 0;\n>>   \n>> diff --git a/sequencer.c b/sequencer.c\n>> index 0db410d590..028699209f 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)\n>>   \treturn len;\n>>   }\n>>   \n>> +void sequencer_post_commit_cleanup(void)\n>> +{\n>> +\tstruct replay_opts opts = REPLAY_OPTS_INIT;\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> +\tconst char *eol;\n>> +\tconst char *todo_path = git_path_todo_file();\n>> +\n>> +\tif (strbuf_read_file(&buf, todo_path, 0) < 0) {\n>> +\t\tif (errno == ENOENT) {\n>> +\t\t\treturn;\n>> +\t\t} else {\n>> +\t\t\terror_errno(\"unable to open '%s'\", todo_path);\n>> +\t\t\treturn;\n>> +\t\t}\n>> +\t}\n>> +\t/* If there is only one line then we are done */\n>> +\teol = strchr(buf.buf, '\\n');\n>> +\tif (!eol || !eol[1])\n>> +\t\tsequencer_remove_state(&opts);\n>> +\n>> +\tstrbuf_release(&buf);\n>> +}\n> \n> I find this helper doing a bit too much and a bit too little at the\n> same time.  To reduce the iffiness I mentioned earlier, the callers\n> would behefit to have a helper that\n> \n>   - notices the need to remove CHERRY_PICK_HEAD or REVERT_HEAD, and\n>     returns without doing anything if there is no need;\n> \n>   - remove the *_HEAD file.\n> \n>   - detect if we have dealt with the last step, and returns without\n>     doing any more thing if there are more to do;\n> \n>   - remove the state files.\n> \n> IOW, replace the existing series of two unlink() calls with a single\n> call to the helper.\n> \n> On the other hand, the bulk of hand-rolled logic to determine if we\n> have processed the last step is better done in another helper\n> function that helps this helper, i.e.\n> \n> \tvoid sequencer_post_commit_cleanup(struct repo *r)\n> \t{\n> \t\tint need_cleanup = 0;\n> \n> \t\tif (file_exists(git_path_cherry_pick_head(r)) {\n> \t\t\tunlink(git_path_cherry_pick_head(r));\n> \t\t\tneed_cleanup = 1;\n> \t\t}\n> \t\tif (file_exists(git_path_revert_head(r)) {\n> \t\t\tunlink(git_path_revert_head(r));\n> \t\t\tneed_cleanup = 1;\n> \t\t}\n> \t\tif (!need_cleanup)\n> \t\t\treturn;\n> \t\tif (!have_finished_the_last_pick())\n> \t\t\treturn;\n> \t\tsequencer_remove_state(&opts);\n> \t}\n> \n> as that makes it easier to follow the logic of what is going on at\n> this level, while at the same time making the logic in the\n> have_finished_the_last_pick() helper easier to read by giving it a\n> meaningful name.\n\nThanks that definitely improves things, I'll send a re-roll\n\nBest Wishes\n\nPhillip\n"},{"id":"373422","messageId":"7f4fe847-8cef-efbf-7b9b-44b2df9fba36@gmail.com","threadId":"50839","inReplyTo":"xmqqv9zyxgwk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] fix cherry-pick/revert status after commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-08T14:17:49Z","receivedAt":"2019-04-08T14:17:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 01/04/2019 09:34, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> If the user commits a conflict resolution using 'git commit' during a\n>> sequence of picks then 'git status' missed the fact that a\n>> cherry-pick/revert is still in progress.\n> \n> How well would this play with the previous step?  Didn't the change\n> to builtin/commit.c made in [1/2] mean that after 'git commit' that\n> concludes the last step, there is nothing 'git status' to notice?\n\nYou're right about the final pick in a sequence but the idea here is \nthat if the user runs 'git commit' in the middle of a sequence of \ncherry-picks then we want 'git status' to report that a cherry-pick is \nin progress.\n\nBest Wishes\n\nPhillip\n"},{"id":"373426","messageId":"9f3ec8b3-f2c4-63b8-3350-a37d42bebd47@gmail.com","threadId":"50839","inReplyTo":"CACsJy8D3tH0K8wNLighuNtjUtv3K3TGNMGgx3T5j5sCxok8hbQ@mail.gmail.com","subject":"Re: [PATCH 1/2] commit/reset: try to clean up sequencer state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-08T15:47:48Z","receivedAt":"2019-04-08T15:47:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 01/04/2019 11:09, Duy Nguyen wrote:\n> On Fri, Mar 29, 2019 at 11:32 PM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>>\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> When cherry-picking or reverting a sequence of commits and if the final\n>> pick/revert has conflicts and the user uses `git commit` to commit the\n>> conflict resolution and does not run `git cherry-pick --continue` then\n>> the sequencer state is left behind. This can cause problems later. In my\n>> case I cherry-picked a sequence of commits the last one of which I\n>> committed with `git commit` after resolving some conflicts, then a while\n>> later, on a different branch I aborted a revert which rewound my HEAD to\n>> the end of the cherry-pick sequence on the previous branch. Avoid this\n>> potential problem by removing the sequencer state if we're committing or\n>> resetting the final pick in a sequence.\n>>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>   branch.c                        |  7 +++++--\n>>   builtin/commit.c                |  7 +++++--\n>>   sequencer.c                     | 23 +++++++++++++++++++++++\n>>   sequencer.h                     |  1 +\n>>   t/t3507-cherry-pick-conflict.sh | 19 +++++++++++++++++++\n>>   5 files changed, 53 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/branch.c b/branch.c\n>> index 28b81a7e02..9ed60081c1 100644\n>> --- a/branch.c\n>> +++ b/branch.c\n>> @@ -5,6 +5,7 @@\n>>   #include \"refs.h\"\n>>   #include \"refspec.h\"\n>>   #include \"remote.h\"\n>> +#include \"sequencer.h\"\n>>   #include \"commit.h\"\n>>   #include \"worktree.h\"\n>>\n>> @@ -339,8 +340,10 @@ void create_branch(struct repository *r,\n>>\n>>   void remove_branch_state(struct repository *r)\n> \n> This function is also called in git-am, git-rebase and git-checkout.\n> While the first two should not be affected, git-checkout can be\n> executed while we're in the middle of a cherry-pick or revert. I guess\n> that's ok because git-checkout is basically the same as git-reset in\n> this case?\n\nYes that's right\n\n>>   {\n>> -       unlink(git_path_cherry_pick_head(r));\n>> -       unlink(git_path_revert_head(r));\n>> +       if (!unlink(git_path_cherry_pick_head(r)))\n>> +               sequencer_post_commit_cleanup();\n>> +       if (!unlink(git_path_revert_head(r)))\n>> +               sequencer_post_commit_cleanup();\n>>          unlink(git_path_merge_head(r));\n>>          unlink(git_path_merge_rr(r));\n>>          unlink(git_path_merge_msg(r));\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 2986553d5f..422b7d62a5 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -1657,8 +1657,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>                  die(\"%s\", err.buf);\n>>          }\n>>\n>> -       unlink(git_path_cherry_pick_head(the_repository));\n>> -       unlink(git_path_revert_head(the_repository));\n>> +       if (!unlink(git_path_cherry_pick_head(the_repository)))\n>> +               sequencer_post_commit_cleanup();\n>> +       if (!unlink(git_path_revert_head(the_repository)))\n>> +               sequencer_post_commit_cleanup();\n>>          unlink(git_path_merge_head(the_repository));\n>>          unlink(git_path_merge_msg(the_repository));\n>>          unlink(git_path_merge_mode(the_repository));\n>> @@ -1678,6 +1680,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>>          if (amend && !no_post_rewrite) {\n>>                  commit_post_rewrite(the_repository, current_head, &oid);\n>>          }\n>> +\n>>          if (!quiet) {\n>>                  unsigned int flags = 0;\n>>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 0db410d590..028699209f 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -2220,6 +2220,29 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)\n>>          return len;\n>>   }\n>>\n>> +void sequencer_post_commit_cleanup(void)\n>> +{\n>> +       struct replay_opts opts = REPLAY_OPTS_INIT;\n>> +       struct strbuf buf = STRBUF_INIT;\n>> +       const char *eol;\n>> +       const char *todo_path = git_path_todo_file();\n>> +\n>> +       if (strbuf_read_file(&buf, todo_path, 0) < 0) {\n>> +               if (errno == ENOENT) {\n>> +                       return;\n>> +               } else {\n>> +                       error_errno(\"unable to open '%s'\", todo_path);\n> \n> _() the string to make it translatable.\n\nWell spotted, thanks\n\n>> +                       return;\n>> +               }\n>> +       }\n>> +       /* If there is only one line then we are done */\n>> +       eol = strchr(buf.buf, '\\n');\n>> +       if (!eol || !eol[1])\n>> +               sequencer_remove_state(&opts);\n> \n> Should we say something to let the user know cherry-pick/revert is\n> finished? (unless --quiet is specified)\n\nI'd not thought of that, at the moment we don't say anything about \nremoving CHERRY_PICK_HEAD etc when they are removed by reset or \ncheckout, I'm not sure this is much different to those cases - but maybe \nthey should be printing some feedback as well.\n\nBest Wishes\n\nPhillip\n\n>> +\n>> +       strbuf_release(&buf);\n>> +}\n>> +\n>>   static int read_populate_todo(struct repository *r,\n>>                                struct todo_list *todo_list,\n>>                                struct replay_opts *opts)\n"},{"id":"373967","messageId":"20190416101842.16556-1-phillip.wood123@gmail.com","threadId":"50839","inReplyTo":"20190329163009.493-1-phillip.wood123@gmail.com","subject":"[PATCH v2 0/2] A couple of cherry-pick related fixes","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-16T10:18:40Z","receivedAt":"2019-04-16T10:18:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nI've updated the first patch as per Junio's suggestions and added\ntests for revert as well as cherry-pick. I've also changed the second\npatch to avoid printing the oid of the origin of the commit that's\njust been created and added tests for revert as well as\ncherry-pick. In the end I didn't add any messages when we remove\n.git/sequencer as we don't say anything when we remove\nCHERRY_PICK_HEAD at the moment. Duy mentioned this, I could add this\nif there is a strong demand for it.\n\nPhillip Wood (2):\n  commit/reset: try to clean up sequencer state\n  fix cherry-pick/revert status after commit\n\n branch.c                        |  4 +-\n builtin/commit.c                |  3 +-\n sequencer.c                     | 86 +++++++++++++++++++++++++++++++++\n sequencer.h                     |  3 ++\n t/t3507-cherry-pick-conflict.sh | 39 +++++++++++++++\n t/t7512-status-help.sh          | 36 ++++++++++++++\n wt-status.c                     | 39 +++++++++++++--\n 7 files changed, 201 insertions(+), 9 deletions(-)\n\nRange-diff against v1:\n1:  9b310618ed ! 1:  e963e02628 commit/reset: try to clean up sequencer state\n    @@ -32,10 +32,7 @@\n      {\n     -\tunlink(git_path_cherry_pick_head(r));\n     -\tunlink(git_path_revert_head(r));\n    -+\tif (!unlink(git_path_cherry_pick_head(r)))\n    -+\t\tsequencer_post_commit_cleanup();\n    -+\tif (!unlink(git_path_revert_head(r)))\n    -+\t\tsequencer_post_commit_cleanup();\n    ++\tsequencer_post_commit_cleanup(r);\n      \tunlink(git_path_merge_head(r));\n      \tunlink(git_path_merge_rr(r));\n      \tunlink(git_path_merge_msg(r));\n    @@ -49,21 +46,10 @@\n      \n     -\tunlink(git_path_cherry_pick_head(the_repository));\n     -\tunlink(git_path_revert_head(the_repository));\n    -+\tif (!unlink(git_path_cherry_pick_head(the_repository)))\n    -+\t\tsequencer_post_commit_cleanup();\n    -+\tif (!unlink(git_path_revert_head(the_repository)))\n    -+\t\tsequencer_post_commit_cleanup();\n    ++\tsequencer_post_commit_cleanup(the_repository);\n      \tunlink(git_path_merge_head(the_repository));\n      \tunlink(git_path_merge_msg(the_repository));\n      \tunlink(git_path_merge_mode(the_repository));\n    -@@\n    - \tif (amend && !no_post_rewrite) {\n    - \t\tcommit_post_rewrite(the_repository, current_head, &oid);\n    - \t}\n    -+\n    - \tif (!quiet) {\n    - \t\tunsigned int flags = 0;\n    - \n     \n      diff --git a/sequencer.c b/sequencer.c\n      --- a/sequencer.c\n    @@ -72,27 +58,55 @@\n      \treturn len;\n      }\n      \n    -+void sequencer_post_commit_cleanup(void)\n    ++static int have_finished_the_last_pick(void)\n     +{\n    -+\tstruct replay_opts opts = REPLAY_OPTS_INIT;\n     +\tstruct strbuf buf = STRBUF_INIT;\n     +\tconst char *eol;\n     +\tconst char *todo_path = git_path_todo_file();\n    ++\tint ret = 0;\n     +\n     +\tif (strbuf_read_file(&buf, todo_path, 0) < 0) {\n     +\t\tif (errno == ENOENT) {\n    -+\t\t\treturn;\n    ++\t\t\treturn 0;\n     +\t\t} else {\n     +\t\t\terror_errno(\"unable to open '%s'\", todo_path);\n    -+\t\t\treturn;\n    ++\t\t\treturn 0;\n     +\t\t}\n     +\t}\n     +\t/* If there is only one line then we are done */\n     +\teol = strchr(buf.buf, '\\n');\n     +\tif (!eol || !eol[1])\n    -+\t\tsequencer_remove_state(&opts);\n    ++\t\tret = 1;\n     +\n     +\tstrbuf_release(&buf);\n    ++\n    ++\treturn ret;\n    ++}\n    ++\n    ++void sequencer_post_commit_cleanup(struct repository *r)\n    ++{\n    ++\tstruct replay_opts opts = REPLAY_OPTS_INIT;\n    ++\tint need_cleanup = 0;\n    ++\n    ++\tif (file_exists(git_path_cherry_pick_head(r))) {\n    ++\t\tunlink(git_path_cherry_pick_head(r));\n    ++\t\topts.action = REPLAY_PICK;\n    ++\t\tneed_cleanup = 1;\n    ++\t}\n    ++\n    ++\tif (file_exists(git_path_revert_head(r))) {\n    ++\t\tunlink(git_path_revert_head(r));\n    ++\t\topts.action = REPLAY_REVERT;\n    ++\t\tneed_cleanup = 1;\n    ++\t}\n    ++\n    ++\tif (!need_cleanup)\n    ++\t\treturn;\n    ++\n    ++\tif (!have_finished_the_last_pick())\n    ++\t\treturn;\n    ++\n    ++\tsequencer_remove_state(&opts);\n     +}\n     +\n      static int read_populate_todo(struct repository *r,\n    @@ -106,7 +120,7 @@\n      void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n      int write_basic_state(struct replay_opts *opts, const char *head_name,\n      \t\t      const char *onto, const char *orig_head);\n    -+void sequencer_post_commit_cleanup(void);\n    ++void sequencer_post_commit_cleanup(struct repository *r);\n     \n      diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n      --- a/t/t3507-cherry-pick-conflict.sh\n    @@ -115,7 +129,7 @@\n      \n      \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n      '\n    -+test_expect_success 'successful final commit clears sequencer state' '\n    ++test_expect_success 'successful final commit clears cherry-pick state' '\n     +\tpristine_detach initial &&\n     +\n     +\ttest_must_fail git cherry-pick base picked-signed &&\n    @@ -125,7 +139,7 @@\n     +\ttest_must_fail test_path_exists .git/sequencer\n     +'\n     +\n    -+test_expect_success 'reset after final pick clears sequencer state' '\n    ++test_expect_success 'reset after final pick clears cherry-pick state' '\n     +\tpristine_detach initial &&\n     +\n     +\ttest_must_fail git cherry-pick base picked-signed &&\n    @@ -137,3 +151,30 @@\n      \n      test_expect_success 'failed cherry-pick produces dirty index' '\n      \tpristine_detach initial &&\n    +@@\n    + \ttest_cmp_rev picked REVERT_HEAD\n    + '\n    + \n    ++test_expect_success 'successful final commit clears revert state' '\n    ++       pristine_detach picked-signed &&\n    ++\n    ++       test_must_fail git revert picked-signed base &&\n    ++       echo resolved >foo &&\n    ++       test_path_is_file .git/sequencer/todo &&\n    ++       git commit -a &&\n    ++       test_must_fail test_path_exists .git/sequencer\n    ++'\n    ++\n    ++test_expect_success 'reset after final pick clears revert state' '\n    ++       pristine_detach picked-signed &&\n    ++\n    ++       test_must_fail git revert picked-signed base &&\n    ++       echo resolved >foo &&\n    ++       test_path_is_file .git/sequencer/todo &&\n    ++       git reset &&\n    ++       test_must_fail test_path_exists .git/sequencer\n    ++'\n    ++\n    + test_expect_success 'revert conflict, diff3 -m style' '\n    + \tpristine_detach initial &&\n    + \tgit config merge.conflictstyle diff3 &&\n2:  edb1bfb3d6 ! 2:  ee7db0cfe0 fix cherry-pick/revert status after commit\n    @@ -2,9 +2,9 @@\n     \n         fix cherry-pick/revert status after commit\n     \n    -    If the user commits a conflict resolution using 'git commit' during a\n    -    sequence of picks then 'git status' missed the fact that a\n    -    cherry-pick/revert is still in progress.\n    +    If the user commits a conflict resolution using `git commit` in the\n    +    middle of a sequence of cherry-picks/reverts then `git status` missed\n    +    the fact that a cherry-pick/revert is still in progress.\n     \n         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n     \n    @@ -15,8 +15,7 @@\n      \treturn !item->commit;\n      }\n      \n    -+int sequencer_get_last_command(struct repository *r, enum replay_action *action,\n    -+\t\t\t       struct object_id *oid)\n    ++int sequencer_get_last_command(struct repository *r, enum replay_action *action)\n     +{\n     +\tstruct todo_item item;\n     +\tchar *eol;\n    @@ -43,7 +42,6 @@\n     +\telse\n     +\t\tgoto fail;\n     +\n    -+\toidcpy(oid, &item.commit->object.oid);\n     +\tret = 0;\n     +\n     + fail:\n    @@ -62,9 +60,9 @@\n     @@\n      int write_basic_state(struct replay_opts *opts, const char *head_name,\n      \t\t      const char *onto, const char *orig_head);\n    - void sequencer_post_commit_cleanup(void);\n    -+int sequencer_get_last_command(struct repository* r, enum replay_action *action,\n    -+\t\t\t       struct object_id *oid);\n    + void sequencer_post_commit_cleanup(struct repository *r);\n    ++int sequencer_get_last_command(struct repository* r,\n    ++\t\t\t       enum replay_action *action);\n     \n      diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\n      --- a/t/t7512-status-help.sh\n    @@ -77,13 +75,12 @@\n     +\tgit reset --hard cherry_branch &&\n     +\ttest_when_finished \"git cherry-pick --abort\" &&\n     +\ttest_must_fail git cherry-pick cherry_branch_second one_cherry &&\n    -+\tTO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&\n     +\techo end >main.txt &&\n     +\tgit commit -a &&\n     +\tcat >expected <<EOF &&\n     +On branch cherry_branch\n    -+You are currently cherry-picking commit $TO_CHERRY_PICK.\n    -+  (all conflicts fixed: run \"git cherry-pick --continue\")\n    ++Cherry-pick currently in progress.\n    ++  (run \"git cherry-pick --continue\" to continue)\n     +  (use \"git cherry-pick --abort\" to cancel the cherry-pick operation)\n     +\n     +nothing to commit (use -u to show untracked files)\n    @@ -95,6 +92,31 @@\n      test_expect_success 'status showing detached at and from a tag' '\n      \ttest_commit atag tagging &&\n      \tgit checkout atag &&\n    +@@\n    + \ttest_i18ncmp expected actual\n    + '\n    + \n    ++test_expect_success 'status while reverting after committing conflict resolution' '\n    ++\ttest_when_finished \"git revert --abort\" &&\n    ++\tgit reset --hard new &&\n    ++\ttest_must_fail git revert old new &&\n    ++\techo reverted >to-revert.txt &&\n    ++\tgit commit -a &&\n    ++\tcat >expected <<EOF &&\n    ++On branch master\n    ++Revert currently in progress.\n    ++  (run \"git revert --continue\" to continue)\n    ++  (use \"git revert --abort\" to cancel the revert operation)\n    ++\n    ++nothing to commit (use -u to show untracked files)\n    ++EOF\n    ++\tgit status --untracked-files=no >actual &&\n    ++\ttest_i18ncmp expected actual\n    ++'\n    ++\n    + test_expect_success 'prepare for different number of commits rebased' '\n    + \tgit reset --hard master &&\n    + \tgit checkout -b several_commits &&\n     \n      diff --git a/wt-status.c b/wt-status.c\n      --- a/wt-status.c\n    @@ -107,6 +129,55 @@\n      \n      static const char cut_line[] =\n      \"------------------------ >8 ------------------------\\n\";\n    +@@\n    + static void show_cherry_pick_in_progress(struct wt_status *s,\n    + \t\t\t\t\t const char *color)\n    + {\n    +-\tstatus_printf_ln(s, color, _(\"You are currently cherry-picking commit %s.\"),\n    +-\t\t\tfind_unique_abbrev(&s->state.cherry_pick_head_oid, DEFAULT_ABBREV));\n    ++\tif (is_null_oid(&s->state.cherry_pick_head_oid))\n    ++\t\tstatus_printf_ln(s, color,\n    ++\t\t\t_(\"Cherry-pick currently in progress.\"));\n    ++\telse\n    ++\t\tstatus_printf_ln(s, color,\n    ++\t\t\t_(\"You are currently cherry-picking commit %s.\"),\n    ++\t\t\tfind_unique_abbrev(&s->state.cherry_pick_head_oid,\n    ++\t\t\t\t\t   DEFAULT_ABBREV));\n    ++\n    + \tif (s->hints) {\n    + \t\tif (has_unmerged(s))\n    + \t\t\tstatus_printf_ln(s, color,\n    + \t\t\t\t_(\"  (fix conflicts and run \\\"git cherry-pick --continue\\\")\"));\n    ++\t\telse if (is_null_oid(&s->state.cherry_pick_head_oid))\n    ++\t\t\tstatus_printf_ln(s, color,\n    ++\t\t\t\t_(\"  (run \\\"git cherry-pick --continue\\\" to continue)\"));\n    + \t\telse\n    + \t\t\tstatus_printf_ln(s, color,\n    + \t\t\t\t_(\"  (all conflicts fixed: run \\\"git cherry-pick --continue\\\")\"));\n    +@@\n    + static void show_revert_in_progress(struct wt_status *s,\n    + \t\t\t\t    const char *color)\n    + {\n    +-\tstatus_printf_ln(s, color, _(\"You are currently reverting commit %s.\"),\n    +-\t\t\t find_unique_abbrev(&s->state.revert_head_oid, DEFAULT_ABBREV));\n    ++\tif (is_null_oid(&s->state.revert_head_oid))\n    ++\t\tstatus_printf_ln(s, color,\n    ++\t\t\t_(\"Revert currently in progress.\"));\n    ++\telse\n    ++\t\tstatus_printf_ln(s, color,\n    ++\t\t\t_(\"You are currently reverting commit %s.\"),\n    ++\t\t\tfind_unique_abbrev(&s->state.revert_head_oid,\n    ++\t\t\t\t\t   DEFAULT_ABBREV));\n    + \tif (s->hints) {\n    + \t\tif (has_unmerged(s))\n    + \t\t\tstatus_printf_ln(s, color,\n    + \t\t\t\t_(\"  (fix conflicts and run \\\"git revert --continue\\\")\"));\n    ++\t\telse if (is_null_oid(&s->state.revert_head_oid))\n    ++\t\t\tstatus_printf_ln(s, color,\n    ++\t\t\t\t_(\"  (run \\\"git revert --continue\\\" to continue)\"));\n    + \t\telse\n    + \t\t\tstatus_printf_ln(s, color,\n    + \t\t\t\t_(\"  (all conflicts fixed: run \\\"git revert --continue\\\")\"));\n     @@\n      {\n      \tstruct stat st;\n    @@ -120,13 +191,13 @@\n      \t\toidcpy(&state->revert_head_oid, &oid);\n      \t}\n     -\n    -+\tif (!sequencer_get_last_command(r, &action, &oid)) {\n    ++\tif (!sequencer_get_last_command(r, &action)) {\n     +\t\tif (action == REPLAY_PICK) {\n     +\t\t\tstate->cherry_pick_in_progress = 1;\n    -+\t\t\toidcpy(&state->cherry_pick_head_oid, &oid);\n    ++\t\t\toidcpy(&state->cherry_pick_head_oid, &null_oid);\n     +\t\t} else {\n     +\t\t\tstate->revert_in_progress = 1;\n    -+\t\t\toidcpy(&state->revert_head_oid, &oid);\n    ++\t\t\toidcpy(&state->revert_head_oid, &null_oid);\n     +\t\t}\n     +\t}\n      \tif (get_detached_from)\n--\n2.21.0\n\n"},{"id":"373969","messageId":"20190416101842.16556-3-phillip.wood123@gmail.com","threadId":"50839","inReplyTo":"20190416101842.16556-1-phillip.wood123@gmail.com","subject":"[PATCH v2 2/2] fix cherry-pick/revert status after commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-16T10:18:42Z","receivedAt":"2019-04-16T10:18:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf the user commits a conflict resolution using `git commit` in the\nmiddle of a sequence of cherry-picks/reverts then `git status` missed\nthe fact that a cherry-pick/revert is still in progress.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n sequencer.c            | 35 +++++++++++++++++++++++++++++++++++\n sequencer.h            |  2 ++\n t/t7512-status-help.sh | 36 ++++++++++++++++++++++++++++++++++++\n wt-status.c            | 39 ++++++++++++++++++++++++++++++++++-----\n 4 files changed, 107 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7c7b8a07c4..c6a9a35422 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2142,6 +2142,41 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,\n \treturn !item->commit;\n }\n \n+int sequencer_get_last_command(struct repository *r, enum replay_action *action)\n+{\n+\tstruct todo_item item;\n+\tchar *eol;\n+\tconst char *todo_file;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint ret = -1;\n+\n+\ttodo_file = git_path_todo_file();\n+\tif (strbuf_read_file(&buf, todo_file, 0) < 0) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn -1;\n+\t\telse\n+\t\t\treturn error_errno(\"unable to open '%s'\", todo_file);\n+\t}\n+\teol = strchrnul(buf.buf, '\\n');\n+\tif (buf.buf != eol && eol[-1] == '\\r')\n+\t\teol--; /* strip Carriage Return */\n+\tif (parse_insn_line(r, &item, buf.buf, eol))\n+\t\tgoto fail;\n+\tif (item.command == TODO_PICK)\n+\t\t*action = REPLAY_PICK;\n+\telse if (item.command == TODO_REVERT)\n+\t\t*action = REPLAY_REVERT;\n+\telse\n+\t\tgoto fail;\n+\n+\tret = 0;\n+\n+ fail:\n+\tstrbuf_release(&buf);\n+\n+\treturn ret;\n+}\n+\n static int parse_insn_buffer(struct repository *r, char *buf,\n \t\t\t     struct todo_list *todo_list)\n {\ndiff --git a/sequencer.h b/sequencer.h\nindex 6c7cf8d72f..c4b79165d3 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -145,3 +145,5 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\t      const char *onto, const char *orig_head);\n void sequencer_post_commit_cleanup(struct repository *r);\n+int sequencer_get_last_command(struct repository* r,\n+\t\t\t       enum replay_action *action);\ndiff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh\nindex 458608cc1e..c1eb72555d 100755\n--- a/t/t7512-status-help.sh\n+++ b/t/t7512-status-help.sh\n@@ -780,6 +780,24 @@ EOF\n \ttest_i18ncmp expected actual\n '\n \n+test_expect_success 'status when cherry-picking after committing conflict resolution' '\n+\tgit reset --hard cherry_branch &&\n+\ttest_when_finished \"git cherry-pick --abort\" &&\n+\ttest_must_fail git cherry-pick cherry_branch_second one_cherry &&\n+\techo end >main.txt &&\n+\tgit commit -a &&\n+\tcat >expected <<EOF &&\n+On branch cherry_branch\n+Cherry-pick currently in progress.\n+  (run \"git cherry-pick --continue\" to continue)\n+  (use \"git cherry-pick --abort\" to cancel the cherry-pick operation)\n+\n+nothing to commit (use -u to show untracked files)\n+EOF\n+\tgit status --untracked-files=no >actual &&\n+\ttest_i18ncmp expected actual\n+'\n+\n test_expect_success 'status showing detached at and from a tag' '\n \ttest_commit atag tagging &&\n \tgit checkout atag &&\n@@ -857,6 +875,24 @@ EOF\n \ttest_i18ncmp expected actual\n '\n \n+test_expect_success 'status while reverting after committing conflict resolution' '\n+\ttest_when_finished \"git revert --abort\" &&\n+\tgit reset --hard new &&\n+\ttest_must_fail git revert old new &&\n+\techo reverted >to-revert.txt &&\n+\tgit commit -a &&\n+\tcat >expected <<EOF &&\n+On branch master\n+Revert currently in progress.\n+  (run \"git revert --continue\" to continue)\n+  (use \"git revert --abort\" to cancel the revert operation)\n+\n+nothing to commit (use -u to show untracked files)\n+EOF\n+\tgit status --untracked-files=no >actual &&\n+\ttest_i18ncmp expected actual\n+'\n+\n test_expect_success 'prepare for different number of commits rebased' '\n \tgit reset --hard master &&\n \tgit checkout -b several_commits &&\ndiff --git a/wt-status.c b/wt-status.c\nindex 1f564b12d2..1dbb4d949c 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -17,6 +17,7 @@\n #include \"utf8.h\"\n #include \"worktree.h\"\n #include \"lockfile.h\"\n+#include \"sequencer.h\"\n \n static const char cut_line[] =\n \"------------------------ >8 ------------------------\\n\";\n@@ -1369,12 +1370,22 @@ static void show_rebase_in_progress(struct wt_status *s,\n static void show_cherry_pick_in_progress(struct wt_status *s,\n \t\t\t\t\t const char *color)\n {\n-\tstatus_printf_ln(s, color, _(\"You are currently cherry-picking commit %s.\"),\n-\t\t\tfind_unique_abbrev(&s->state.cherry_pick_head_oid, DEFAULT_ABBREV));\n+\tif (is_null_oid(&s->state.cherry_pick_head_oid))\n+\t\tstatus_printf_ln(s, color,\n+\t\t\t_(\"Cherry-pick currently in progress.\"));\n+\telse\n+\t\tstatus_printf_ln(s, color,\n+\t\t\t_(\"You are currently cherry-picking commit %s.\"),\n+\t\t\tfind_unique_abbrev(&s->state.cherry_pick_head_oid,\n+\t\t\t\t\t   DEFAULT_ABBREV));\n+\n \tif (s->hints) {\n \t\tif (has_unmerged(s))\n \t\t\tstatus_printf_ln(s, color,\n \t\t\t\t_(\"  (fix conflicts and run \\\"git cherry-pick --continue\\\")\"));\n+\t\telse if (is_null_oid(&s->state.cherry_pick_head_oid))\n+\t\t\tstatus_printf_ln(s, color,\n+\t\t\t\t_(\"  (run \\\"git cherry-pick --continue\\\" to continue)\"));\n \t\telse\n \t\t\tstatus_printf_ln(s, color,\n \t\t\t\t_(\"  (all conflicts fixed: run \\\"git cherry-pick --continue\\\")\"));\n@@ -1387,12 +1398,21 @@ static void show_cherry_pick_in_progress(struct wt_status *s,\n static void show_revert_in_progress(struct wt_status *s,\n \t\t\t\t    const char *color)\n {\n-\tstatus_printf_ln(s, color, _(\"You are currently reverting commit %s.\"),\n-\t\t\t find_unique_abbrev(&s->state.revert_head_oid, DEFAULT_ABBREV));\n+\tif (is_null_oid(&s->state.revert_head_oid))\n+\t\tstatus_printf_ln(s, color,\n+\t\t\t_(\"Revert currently in progress.\"));\n+\telse\n+\t\tstatus_printf_ln(s, color,\n+\t\t\t_(\"You are currently reverting commit %s.\"),\n+\t\t\tfind_unique_abbrev(&s->state.revert_head_oid,\n+\t\t\t\t\t   DEFAULT_ABBREV));\n \tif (s->hints) {\n \t\tif (has_unmerged(s))\n \t\t\tstatus_printf_ln(s, color,\n \t\t\t\t_(\"  (fix conflicts and run \\\"git revert --continue\\\")\"));\n+\t\telse if (is_null_oid(&s->state.revert_head_oid))\n+\t\t\tstatus_printf_ln(s, color,\n+\t\t\t\t_(\"  (run \\\"git revert --continue\\\" to continue)\"));\n \t\telse\n \t\t\tstatus_printf_ln(s, color,\n \t\t\t\t_(\"  (all conflicts fixed: run \\\"git revert --continue\\\")\"));\n@@ -1563,6 +1583,7 @@ void wt_status_get_state(struct repository *r,\n {\n \tstruct stat st;\n \tstruct object_id oid;\n+\tenum replay_action action;\n \n \tif (!stat(git_path_merge_head(r), &st)) {\n \t\twt_status_check_rebase(NULL, state);\n@@ -1580,7 +1601,15 @@ void wt_status_get_state(struct repository *r,\n \t\tstate->revert_in_progress = 1;\n \t\toidcpy(&state->revert_head_oid, &oid);\n \t}\n-\n+\tif (!sequencer_get_last_command(r, &action)) {\n+\t\tif (action == REPLAY_PICK) {\n+\t\t\tstate->cherry_pick_in_progress = 1;\n+\t\t\toidcpy(&state->cherry_pick_head_oid, &null_oid);\n+\t\t} else {\n+\t\t\tstate->revert_in_progress = 1;\n+\t\t\toidcpy(&state->revert_head_oid, &null_oid);\n+\t\t}\n+\t}\n \tif (get_detached_from)\n \t\twt_status_get_detached_from(r, state);\n }\n-- \n2.21.0\n\n"},{"id":"373968","messageId":"20190416101842.16556-2-phillip.wood123@gmail.com","threadId":"50839","inReplyTo":"20190416101842.16556-1-phillip.wood123@gmail.com","subject":"[PATCH v2 1/2] commit/reset: try to clean up sequencer state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-16T10:18:41Z","receivedAt":"2019-04-16T10:18:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nWhen cherry-picking or reverting a sequence of commits and if the final\npick/revert has conflicts and the user uses `git commit` to commit the\nconflict resolution and does not run `git cherry-pick --continue` then\nthe sequencer state is left behind. This can cause problems later. In my\ncase I cherry-picked a sequence of commits the last one of which I\ncommitted with `git commit` after resolving some conflicts, then a while\nlater, on a different branch I aborted a revert which rewound my HEAD to\nthe end of the cherry-pick sequence on the previous branch. Avoid this\npotential problem by removing the sequencer state if we're committing or\nresetting the final pick in a sequence.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n branch.c                        |  4 +--\n builtin/commit.c                |  3 +-\n sequencer.c                     | 51 +++++++++++++++++++++++++++++++++\n sequencer.h                     |  1 +\n t/t3507-cherry-pick-conflict.sh | 39 +++++++++++++++++++++++++\n 5 files changed, 94 insertions(+), 4 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 28b81a7e02..643694542a 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -5,6 +5,7 @@\n #include \"refs.h\"\n #include \"refspec.h\"\n #include \"remote.h\"\n+#include \"sequencer.h\"\n #include \"commit.h\"\n #include \"worktree.h\"\n \n@@ -339,8 +340,7 @@ void create_branch(struct repository *r,\n \n void remove_branch_state(struct repository *r)\n {\n-\tunlink(git_path_cherry_pick_head(r));\n-\tunlink(git_path_revert_head(r));\n+\tsequencer_post_commit_cleanup(r);\n \tunlink(git_path_merge_head(r));\n \tunlink(git_path_merge_rr(r));\n \tunlink(git_path_merge_msg(r));\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2986553d5f..9df3414d80 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1657,8 +1657,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tdie(\"%s\", err.buf);\n \t}\n \n-\tunlink(git_path_cherry_pick_head(the_repository));\n-\tunlink(git_path_revert_head(the_repository));\n+\tsequencer_post_commit_cleanup(the_repository);\n \tunlink(git_path_merge_head(the_repository));\n \tunlink(git_path_merge_msg(the_repository));\n \tunlink(git_path_merge_mode(the_repository));\ndiff --git a/sequencer.c b/sequencer.c\nindex 0db410d590..7c7b8a07c4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2220,6 +2220,57 @@ static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)\n \treturn len;\n }\n \n+static int have_finished_the_last_pick(void)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *eol;\n+\tconst char *todo_path = git_path_todo_file();\n+\tint ret = 0;\n+\n+\tif (strbuf_read_file(&buf, todo_path, 0) < 0) {\n+\t\tif (errno == ENOENT) {\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\terror_errno(\"unable to open '%s'\", todo_path);\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\t/* If there is only one line then we are done */\n+\teol = strchr(buf.buf, '\\n');\n+\tif (!eol || !eol[1])\n+\t\tret = 1;\n+\n+\tstrbuf_release(&buf);\n+\n+\treturn ret;\n+}\n+\n+void sequencer_post_commit_cleanup(struct repository *r)\n+{\n+\tstruct replay_opts opts = REPLAY_OPTS_INIT;\n+\tint need_cleanup = 0;\n+\n+\tif (file_exists(git_path_cherry_pick_head(r))) {\n+\t\tunlink(git_path_cherry_pick_head(r));\n+\t\topts.action = REPLAY_PICK;\n+\t\tneed_cleanup = 1;\n+\t}\n+\n+\tif (file_exists(git_path_revert_head(r))) {\n+\t\tunlink(git_path_revert_head(r));\n+\t\topts.action = REPLAY_REVERT;\n+\t\tneed_cleanup = 1;\n+\t}\n+\n+\tif (!need_cleanup)\n+\t\treturn;\n+\n+\tif (!have_finished_the_last_pick())\n+\t\treturn;\n+\n+\tsequencer_remove_state(&opts);\n+}\n+\n static int read_populate_todo(struct repository *r,\n \t\t\t      struct todo_list *todo_list,\n \t\t\t      struct replay_opts *opts)\ndiff --git a/sequencer.h b/sequencer.h\nindex 4d505b3590..6c7cf8d72f 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -144,3 +144,4 @@ int read_author_script(const char *path, char **name, char **email, char **date,\n void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);\n int write_basic_state(struct replay_opts *opts, const char *head_name,\n \t\t      const char *onto, const char *orig_head);\n+void sequencer_post_commit_cleanup(struct repository *r);\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 0db166152a..cebf91dce2 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -156,6 +156,25 @@ test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '\n \n \ttest_must_fail git rev-parse --verify CHERRY_PICK_HEAD\n '\n+test_expect_success 'successful final commit clears cherry-pick state' '\n+\tpristine_detach initial &&\n+\n+\ttest_must_fail git cherry-pick base picked-signed &&\n+\techo resolved >foo &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\tgit commit -a &&\n+\ttest_must_fail test_path_exists .git/sequencer\n+'\n+\n+test_expect_success 'reset after final pick clears cherry-pick state' '\n+\tpristine_detach initial &&\n+\n+\ttest_must_fail git cherry-pick base picked-signed &&\n+\techo resolved >foo &&\n+\ttest_path_is_file .git/sequencer/todo &&\n+\tgit reset &&\n+\ttest_must_fail test_path_exists .git/sequencer\n+'\n \n test_expect_success 'failed cherry-pick produces dirty index' '\n \tpristine_detach initial &&\n@@ -316,6 +335,26 @@ test_expect_success 'failed commit does not clear REVERT_HEAD' '\n \ttest_cmp_rev picked REVERT_HEAD\n '\n \n+test_expect_success 'successful final commit clears revert state' '\n+       pristine_detach picked-signed &&\n+\n+       test_must_fail git revert picked-signed base &&\n+       echo resolved >foo &&\n+       test_path_is_file .git/sequencer/todo &&\n+       git commit -a &&\n+       test_must_fail test_path_exists .git/sequencer\n+'\n+\n+test_expect_success 'reset after final pick clears revert state' '\n+       pristine_detach picked-signed &&\n+\n+       test_must_fail git revert picked-signed base &&\n+       echo resolved >foo &&\n+       test_path_is_file .git/sequencer/todo &&\n+       git reset &&\n+       test_must_fail test_path_exists .git/sequencer\n+'\n+\n test_expect_success 'revert conflict, diff3 -m style' '\n \tpristine_detach initial &&\n \tgit config merge.conflictstyle diff3 &&\n-- \n2.21.0\n\n"},{"id":"374002","messageId":"xmqqr2a1jenm.fsf@gitster-ct.c.googlers.com","threadId":"50839","inReplyTo":"20190416101842.16556-2-phillip.wood123@gmail.com","subject":"Re: [PATCH v2 1/2] commit/reset: try to clean up sequencer state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-17T07:04:45Z","receivedAt":"2019-04-17T07:04:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> Avoid this\n> potential problem by removing the sequencer state if we're committing or\n> resetting the final pick in a sequence.\n\nThe use-case story before this conclusion only mentioned \"commit\"\nthat concluded the multi-step cherry-pick/revert, and never talked\nabout \"reset\", which made my eyebrows to rise.\n\nAs a part of \"reset\", we have already been removing CHERRY_PICK_HEAD\nand REVERT_HEAD, so \"git reset\" during a conflicted \"cherry-pick\"\nfor example is already destructive and the user can no longer get\nback to continuing the cherry-pick anyway after running it, even\nwithout this patch.  So from that point of view, it does make sense\nto remove the other sequencer states at the same time.\n\nThanks.\n"},{"id":"374032","messageId":"nycvar.QRO.7.76.6.1904171423370.46@tvgsbejvaqbjf.bet","threadId":"50839","inReplyTo":"xmqqr2a1jenm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] commit/reset: try to clean up sequencer state","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-17T12:26:22Z","receivedAt":"2019-04-17T12:26:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 17 Apr 2019, Junio C Hamano wrote:\n\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n>\n> > Avoid this potential problem by removing the sequencer state if we're\n> > committing or resetting the final pick in a sequence.\n>\n> The use-case story before this conclusion only mentioned \"commit\"\n> that concluded the multi-step cherry-pick/revert, and never talked\n> about \"reset\", which made my eyebrows to rise.\n>\n> As a part of \"reset\", we have already been removing CHERRY_PICK_HEAD\n> and REVERT_HEAD, so \"git reset\" during a conflicted \"cherry-pick\"\n> for example is already destructive and the user can no longer get\n> back to continuing the cherry-pick anyway after running it, even\n> without this patch.  So from that point of view, it does make sense\n> to remove the other sequencer states at the same time.\n\nDo you mean to say that a `git reset` during `git cherry-pick <range>`\naborts it?\n\nIn my experience, this is not the case. The advice printed out after a\nconflict even recommends to run `git reset` (followed by `git cherry-pick\n--continue`, in lieu of the `git cherry-pick --skip` we have yet to\nimplement).\n\nSo I don't think it is correct to say that `git reset` does not let the\nuser get back to continuing a cherry-pick...\n\nCiao,\nDscho\n"},{"id":"374051","messageId":"f1371b89-8f8b-d0b9-7bda-23f8cb5d0541@gmail.com","threadId":"50839","inReplyTo":"nycvar.QRO.7.76.6.1904171423370.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/2] commit/reset: try to clean up sequencer state","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-17T15:04:19Z","receivedAt":"2019-04-17T15:04:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 17/04/2019 13:26, Johannes Schindelin wrote:\n> Hi,\n> \n> On Wed, 17 Apr 2019, Junio C Hamano wrote:\n> \n>> Phillip Wood <phillip.wood123@gmail.com> writes:\n>>\n>>> Avoid this potential problem by removing the sequencer state if we're\n>>> committing or resetting the final pick in a sequence.\n>>\n>> The use-case story before this conclusion only mentioned \"commit\"\n>> that concluded the multi-step cherry-pick/revert, and never talked\n>> about \"reset\", which made my eyebrows to rise.\n>>\n>> As a part of \"reset\", we have already been removing CHERRY_PICK_HEAD\n>> and REVERT_HEAD, so \"git reset\" during a conflicted \"cherry-pick\"\n>> for example is already destructive and the user can no longer get\n>> back to continuing the cherry-pick anyway after running it, even\n>> without this patch.  So from that point of view, it does make sense\n>> to remove the other sequencer states at the same time.\n> \n> Do you mean to say that a `git reset` during `git cherry-pick <range>`\n> aborts it?\n\nNo I mean it removes CHERRY_PICK_HEAD/REVERT_HEAD and so cancels the \nconflicting pick/revert, it does not abort the operation as a whole. If \nthe conflicting pick/revert was the last in a range then we want it to \nremove .git/sequencer as well as the ..._HEAD file as it is easy to \nforget to run --continue in that case.\n\nBest Wishes\n\nPhillip\n\n> In my experience, this is not the case. The advice printed out after a\n> conflict even recommends to run `git reset` (followed by `git cherry-pick\n> --continue`, in lieu of the `git cherry-pick --skip` we have yet to\n> implement).\n> \n> So I don't think it is correct to say that `git reset` does not let the\n> user get back to continuing a cherry-pick...\n> \n> Ciao,\n> Dscho\n> \n"}]}