[v2,1/2] commit/reset: try to clean up sequencer state

Commit Message

Phillip Wood April 16, 2019, 10:18 a.m. UTC

From: Phillip Wood <[e-mail]>

When cherry-picking or reverting a sequence of commits and if the final
pick/revert has conflicts and the user uses `git commit` to commit the
conflict resolution and does not run `git cherry-pick --continue` then
the sequencer state is left behind. This can cause problems later. In my
case I cherry-picked a sequence of commits the last one of which I
committed with `git commit` after resolving some conflicts, then a while
later, on a different branch I aborted a revert which rewound my HEAD to
the end of the cherry-pick sequence on the previous branch. Avoid this
potential problem by removing the sequencer state if we're committing or
resetting the final pick in a sequence.

Signed-off-by: Phillip Wood <[e-mail]>
---
 branch.c                        |  4 +--
 builtin/commit.c                |  3 +-
 sequencer.c                     | 51 +++++++++++++++++++++++++++++++++
 sequencer.h                     |  1 +
 t/t3507-cherry-pick-conflict.sh | 39 +++++++++++++++++++++++++
 5 files changed, 94 insertions(+), 4 deletions(-)

Comments

Junio C Hamano April 17, 2019, 7:04 a.m. UTC | #1

Phillip Wood <[e-mail]> writes:

> Avoid this
> potential problem by removing the sequencer state if we're committing or
> resetting the final pick in a sequence.

The use-case story before this conclusion only mentioned "commit"
that concluded the multi-step cherry-pick/revert, and never talked
about "reset", which made my eyebrows to rise.

As a part of "reset", we have already been removing CHERRY_PICK_HEAD
and REVERT_HEAD, so "git reset" during a conflicted "cherry-pick"
for example is already destructive and the user can no longer get
back to continuing the cherry-pick anyway after running it, even
without this patch.  So from that point of view, it does make sense
to remove the other sequencer states at the same time.

Thanks.

Johannes Schindelin April 17, 2019, 12:26 p.m. UTC | #2

Hi,

On Wed, 17 Apr 2019, Junio C Hamano wrote:

> Phillip Wood <[e-mail]> writes:
>
> > Avoid this potential problem by removing the sequencer state if we're
> > committing or resetting the final pick in a sequence.
>
> The use-case story before this conclusion only mentioned "commit"
> that concluded the multi-step cherry-pick/revert, and never talked
> about "reset", which made my eyebrows to rise.
>
> As a part of "reset", we have already been removing CHERRY_PICK_HEAD
> and REVERT_HEAD, so "git reset" during a conflicted "cherry-pick"
> for example is already destructive and the user can no longer get
> back to continuing the cherry-pick anyway after running it, even
> without this patch.  So from that point of view, it does make sense
> to remove the other sequencer states at the same time.

Do you mean to say that a `git reset` during `git cherry-pick <range>`
aborts it?

In my experience, this is not the case. The advice printed out after a
conflict even recommends to run `git reset` (followed by `git cherry-pick
--continue`, in lieu of the `git cherry-pick --skip` we have yet to
implement).

So I don't think it is correct to say that `git reset` does not let the
user get back to continuing a cherry-pick...

Ciao,
Dscho

Phillip Wood April 17, 2019, 3:04 p.m. UTC | #3

Hi Dscho

On 17/04/2019 13:26, Johannes Schindelin wrote:
> Hi,
> 
> On Wed, 17 Apr 2019, Junio C Hamano wrote:
> 
>> Phillip Wood <[e-mail]> writes:
>>
>>> Avoid this potential problem by removing the sequencer state if we're
>>> committing or resetting the final pick in a sequence.
>>
>> The use-case story before this conclusion only mentioned "commit"
>> that concluded the multi-step cherry-pick/revert, and never talked
>> about "reset", which made my eyebrows to rise.
>>
>> As a part of "reset", we have already been removing CHERRY_PICK_HEAD
>> and REVERT_HEAD, so "git reset" during a conflicted "cherry-pick"
>> for example is already destructive and the user can no longer get
>> back to continuing the cherry-pick anyway after running it, even
>> without this patch.  So from that point of view, it does make sense
>> to remove the other sequencer states at the same time.
> 
> Do you mean to say that a `git reset` during `git cherry-pick <range>`
> aborts it?

No I mean it removes CHERRY_PICK_HEAD/REVERT_HEAD and so cancels the 
conflicting pick/revert, it does not abort the operation as a whole. If 
the conflicting pick/revert was the last in a range then we want it to 
remove .git/sequencer as well as the ..._HEAD file as it is easy to 
forget to run --continue in that case.

Best Wishes

Phillip

> In my experience, this is not the case. The advice printed out after a
> conflict even recommends to run `git reset` (followed by `git cherry-pick
> --continue`, in lieu of the `git cherry-pick --skip` we have yet to
> implement).
> 
> So I don't think it is correct to say that `git reset` does not let the
> user get back to continuing a cherry-pick...
> 
> Ciao,
> Dscho
>

Patch

diff --git a/branch.c b/branch.c
index 28b81a7e02..643694542a 100644
--- a/branch.c
+++ b/branch.c
@@ -5,6 +5,7 @@ 
 #include "refs.h"
 #include "refspec.h"
 #include "remote.h"
+#include "sequencer.h"
 #include "commit.h"
 #include "worktree.h"
 
@@ -339,8 +340,7 @@  void create_branch(struct repository *r,
 
 void remove_branch_state(struct repository *r)
 {
-	unlink(git_path_cherry_pick_head(r));
-	unlink(git_path_revert_head(r));
+	sequencer_post_commit_cleanup(r);
 	unlink(git_path_merge_head(r));
 	unlink(git_path_merge_rr(r));
 	unlink(git_path_merge_msg(r));
diff --git a/builtin/commit.c b/builtin/commit.c
index 2986553d5f..9df3414d80 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1657,8 +1657,7 @@  int cmd_commit(int argc, const char **argv, const char *prefix)
 		die("%s", err.buf);
 	}
 
-	unlink(git_path_cherry_pick_head(the_repository));
-	unlink(git_path_revert_head(the_repository));
+	sequencer_post_commit_cleanup(the_repository);
 	unlink(git_path_merge_head(the_repository));
 	unlink(git_path_merge_msg(the_repository));
 	unlink(git_path_merge_mode(the_repository));
diff --git a/sequencer.c b/sequencer.c
index 0db410d590..7c7b8a07c4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2220,6 +2220,57 @@  static ssize_t strbuf_read_file_or_whine(struct strbuf *sb, const char *path)
 	return len;
 }
 
+static int have_finished_the_last_pick(void)
+{
+	struct strbuf buf = STRBUF_INIT;
+	const char *eol;
+	const char *todo_path = git_path_todo_file();
+	int ret = 0;
+
+	if (strbuf_read_file(&buf, todo_path, 0) < 0) {
+		if (errno == ENOENT) {
+			return 0;
+		} else {
+			error_errno("unable to open '%s'", todo_path);
+			return 0;
+		}
+	}
+	/* If there is only one line then we are done */
+	eol = strchr(buf.buf, '\n');
+	if (!eol || !eol[1])
+		ret = 1;
+
+	strbuf_release(&buf);
+
+	return ret;
+}
+
+void sequencer_post_commit_cleanup(struct repository *r)
+{
+	struct replay_opts opts = REPLAY_OPTS_INIT;
+	int need_cleanup = 0;
+
+	if (file_exists(git_path_cherry_pick_head(r))) {
+		unlink(git_path_cherry_pick_head(r));
+		opts.action = REPLAY_PICK;
+		need_cleanup = 1;
+	}
+
+	if (file_exists(git_path_revert_head(r))) {
+		unlink(git_path_revert_head(r));
+		opts.action = REPLAY_REVERT;
+		need_cleanup = 1;
+	}
+
+	if (!need_cleanup)
+		return;
+
+	if (!have_finished_the_last_pick())
+		return;
+
+	sequencer_remove_state(&opts);
+}
+
 static int read_populate_todo(struct repository *r,
 			      struct todo_list *todo_list,
 			      struct replay_opts *opts)
diff --git a/sequencer.h b/sequencer.h
index 4d505b3590..6c7cf8d72f 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -144,3 +144,4 @@  int read_author_script(const char *path, char **name, char **email, char **date,
 void parse_strategy_opts(struct replay_opts *opts, char *raw_opts);
 int write_basic_state(struct replay_opts *opts, const char *head_name,
 		      const char *onto, const char *orig_head);
+void sequencer_post_commit_cleanup(struct repository *r);
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 0db166152a..cebf91dce2 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -156,6 +156,25 @@  test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '
 
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
 '
+test_expect_success 'successful final commit clears cherry-pick state' '
+	pristine_detach initial &&
+
+	test_must_fail git cherry-pick base picked-signed &&
+	echo resolved >foo &&
+	test_path_is_file .git/sequencer/todo &&
+	git commit -a &&
+	test_must_fail test_path_exists .git/sequencer
+'
+
+test_expect_success 'reset after final pick clears cherry-pick state' '
+	pristine_detach initial &&
+
+	test_must_fail git cherry-pick base picked-signed &&
+	echo resolved >foo &&
+	test_path_is_file .git/sequencer/todo &&
+	git reset &&
+	test_must_fail test_path_exists .git/sequencer
+'
 
 test_expect_success 'failed cherry-pick produces dirty index' '
 	pristine_detach initial &&
@@ -316,6 +335,26 @@  test_expect_success 'failed commit does not clear REVERT_HEAD' '
 	test_cmp_rev picked REVERT_HEAD
 '
 
+test_expect_success 'successful final commit clears revert state' '
+       pristine_detach picked-signed &&
+
+       test_must_fail git revert picked-signed base &&
+       echo resolved >foo &&
+       test_path_is_file .git/sequencer/todo &&
+       git commit -a &&
+       test_must_fail test_path_exists .git/sequencer
+'
+
+test_expect_success 'reset after final pick clears revert state' '
+       pristine_detach picked-signed &&
+
+       test_must_fail git revert picked-signed base &&
+       echo resolved >foo &&
+       test_path_is_file .git/sequencer/todo &&
+       git reset &&
+       test_must_fail test_path_exists .git/sequencer
+'
+
 test_expect_success 'revert conflict, diff3 -m style' '
 	pristine_detach initial &&
 	git config merge.conflictstyle diff3 &&