Re: [PATCH 5/3] revert: introduce --abort to cancel a failed cherry-pick

Jonathan Nieder [off-list ref] writes:

What is this hunk about?  Don't you also need another one to the test
after that one?  When merged with rr/revert-cherry-pick series, t3510 seems
to misbehave around this area.

Junio C Hamano wrote:

Jonathan Nieder [off-list ref] writes:

quoted

...

@@ -168,6 +265,7 @@ test_expect_success '--continue continues after conflicts are resolved' '
 	OBJID
 	:100644 100644 OBJID OBJID M	unrelated
 	OBJID
+	:000000 100644 OBJID OBJID A	bar
 	:000000 100644 OBJID OBJID A	foo
 	:000000 100644 OBJID OBJID A	unrelated
 	EOF

What is this hunk about?

Just laziness.  It would have been more polite for changes to the
script's setup to be made in such a way as not to require changing
unrelated tests, like this.

-- >8 --
Subject: revert: introduce --abort to cancel a failed cherry-pick

After running some ill-advised command like "git cherry-pick
HEAD..linux-next", the bewildered novice may want to return to more
familiar territory.  Introduce a "git cherry-pick --abort" command
that rolls back the entire cherry-pick sequence and places the
repository back on solid ground.

Just like "git merge --abort", this internally uses "git reset
--merge", so local changes not involved in the conflict resolution are
preserved.

Signed-off-by: Jonathan Nieder <redacted>
---
 Documentation/git-cherry-pick.txt |    1 +
 Documentation/git-revert.txt      |    1 +
 Documentation/sequencer.txt       |    3 +
 builtin/revert.c                  |   87 ++++++++++++++++++++++++++++++++-
 t/t3510-cherry-pick-sequence.sh   |   96 +++++++++++++++++++++++++++++++++++++
 5 files changed, 185 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 21998b82..fed5097e 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt

@@ -11,6 +11,7 @@ SYNOPSIS
 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
 'git cherry-pick' --continue
 'git cherry-pick' --quit
+'git cherry-pick' --abort
 
 DESCRIPTION
 -----------

diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index b0fcabc8..b699a345 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt

@@ -11,6 +11,7 @@ SYNOPSIS
 'git revert' [--edit | --no-edit] [-n] [-m parent-number] [-s] <commit>...
 'git revert' --continue
 'git revert' --quit
+'git revert' --abort
 
 DESCRIPTION
 -----------

diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
index 75f8e869..5747f442 100644
--- a/Documentation/sequencer.txt
+++ b/Documentation/sequencer.txt

@@ -7,3 +7,6 @@
 	Forget about the current operation in progress.  Can be used
 	to clear the sequencer state after a failed cherry-pick or
 	revert.
+
+--abort::
+	Cancel the operation and return to the pre-sequence state.

diff --git a/builtin/revert.c b/builtin/revert.c
index f5ba67a5..70a5fbb6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -40,7 +40,12 @@ static const char * const cherry_pick_usage[] = {
 };
 
 enum replay_action { REVERT, CHERRY_PICK };
-enum replay_subcommand { REPLAY_NONE, REPLAY_REMOVE_STATE, REPLAY_CONTINUE };
+enum replay_subcommand {
+	REPLAY_NONE,
+	REPLAY_REMOVE_STATE,
+	REPLAY_CONTINUE,
+	REPLAY_ROLLBACK
+};
 
 struct replay_opts {
 	enum replay_action action;

@@ -135,9 +140,11 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	const char *me = action_name(opts);
 	int remove_state = 0;
 	int contin = 0;
+	int rollback = 0;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "quit", &remove_state, "end revert or cherry-pick sequence"),
 		OPT_BOOLEAN(0, "continue", &contin, "resume revert or cherry-pick sequence"),
+		OPT_BOOLEAN(0, "abort", &rollback, "cancel revert or cherry-pick sequence"),
 		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		OPT_NOOP_NOARG('r', NULL),

@@ -173,6 +180,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	verify_opt_mutually_compatible(me,
 				"--quit", remove_state,
 				"--continue", contin,
+				"--abort", rollback,
 				NULL);
 
 	/* Set the subcommand */

@@ -180,6 +188,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		opts->subcommand = REPLAY_REMOVE_STATE;
 	else if (contin)
 		opts->subcommand = REPLAY_CONTINUE;
+	else if (rollback)
+		opts->subcommand = REPLAY_ROLLBACK;
 	else
 		opts->subcommand = REPLAY_NONE;
 

@@ -188,8 +198,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		char *this_operation;
 		if (opts->subcommand == REPLAY_REMOVE_STATE)
 			this_operation = "--quit";
-		else
+		else if (opts->subcommand == REPLAY_CONTINUE)
 			this_operation = "--continue";
+		else {
+			assert(opts->subcommand == REPLAY_ROLLBACK);
+			this_operation = "--abort";
+		}
 
 		verify_opt_compatible(me, this_operation,
 				"--no-commit", opts->no_commit,

@@ -850,7 +864,7 @@ static int create_seq_dir(void)
 
 	if (file_exists(seq_dir)) {
 		error(_("a cherry-pick or revert is already in progress"));
-		advise(_("try \"git cherry-pick (--continue | --quit)\""));
+		advise(_("try \"git cherry-pick (--continue | --quit | --abort)\""));
 		return -1;
 	}
 	else if (mkdir(seq_dir, 0777) < 0)

@@ -873,6 +887,71 @@ static void save_head(const char *head)
 		die(_("Error wrapping up %s."), head_file);
 }
 
+static int reset_for_rollback(const unsigned char *sha1)
+{
+	const char *argv[4];	/* reset --merge <arg> + NULL */
+	argv[0] = "reset";
+	argv[1] = "--merge";
+	argv[2] = sha1_to_hex(sha1);
+	argv[3] = NULL;
+	return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int rollback_single_pick(void)
+{
+	unsigned char head_sha1[20];
+
+	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
+	    !file_exists(git_path("REVERT_HEAD")))
+		return error(_("no cherry-pick or revert in progress"));
+	if (!resolve_ref("HEAD", head_sha1, 0, NULL))
+		return error(_("cannot resolve HEAD"));
+	if (is_null_sha1(head_sha1))
+		return error(_("cannot abort from a branch yet to be born"));
+	return reset_for_rollback(head_sha1);
+}
+
+static int sequencer_rollback(struct replay_opts *opts)
+{
+	const char *filename;
+	FILE *f;
+	unsigned char sha1[20];
+	struct strbuf buf = STRBUF_INIT;
+
+	filename = git_path(SEQ_HEAD_FILE);
+	f = fopen(filename, "r");
+	if (!f && errno == ENOENT) {
+		/*
+		 * There is no multiple-cherry-pick in progress.
+		 * If CHERRY_PICK_HEAD or REVERT_HEAD indicates
+		 * a single-cherry-pick in progress, abort that.
+		 */
+		return rollback_single_pick();
+	}
+	if (!f)
+		return error(_("cannot open %s: %s"), filename,
+						strerror(errno));
+	if (strbuf_getline(&buf, f, '\n')) {
+		error(_("cannot read %s: %s"), filename, ferror(f) ?
+			strerror(errno) : _("unexpected end of file"));
+		goto fail;
+	}
+	if (get_sha1_hex(buf.buf, sha1) || buf.buf[40] != '\0') {
+		error(_("stored pre-cherry-pick HEAD file '%s' is corrupt"),
+			filename);
+		goto fail;
+	}
+	if (reset_for_rollback(sha1))
+		goto fail;
+	strbuf_release(&buf);
+	fclose(f);
+	return 0;
+fail:
+	strbuf_release(&buf);
+	fclose(f);
+	return -1;
+}
+
 static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 {
 	const char *todo_file = git_path(SEQ_TODO_FILE);

@@ -977,6 +1056,8 @@ static int pick_revisions(struct replay_opts *opts)
 		remove_sequencer_state(1);
 		return 0;
 	}
+	if (opts->subcommand == REPLAY_ROLLBACK)
+		return sequencer_rollback(opts);
 	if (opts->subcommand == REPLAY_CONTINUE) {
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
 			return error(_("No %s in progress"), action_name(opts));

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index bb67cdcb..e97397f9 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh

@@ -2,6 +2,7 @@
 
 test_description='Test cherry-pick continuation features
 
+  + yetanotherpick: rewrites foo to e
   + anotherpick: rewrites foo to d
   + picked: rewrites foo to c
   + unrelatedpick: rewrites unrelated to reallyunrelated

@@ -19,6 +20,12 @@ pristine_detach () {
 	git clean -d -f -f -q -x
 }
 
+test_cmp_rev () {
+	git rev-parse --verify "$1" >expect.rev &&
+	git rev-parse --verify "$2" >actual.rev &&
+	test_cmp expect.rev actual.rev
+}
+
 test_expect_success setup '
 	echo unrelated >unrelated &&
 	git add unrelated &&

@@ -27,6 +34,7 @@ test_expect_success setup '
 	test_commit unrelatedpick unrelated reallyunrelated &&
 	test_commit picked foo c &&
 	test_commit anotherpick foo d &&
+	test_commit yetanotherpick foo e &&
 	git config advice.detachedhead false
 
 '

@@ -75,6 +83,11 @@ test_expect_success '--quit does not complain when no cherry-pick is in progress
 	git cherry-pick --quit
 '
 
+test_expect_success '--abort requires cherry-pick in progress' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --abort
+'
+
 test_expect_success '--quit cleans up sequencer state' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..picked &&

@@ -103,6 +116,79 @@ test_expect_success 'cherry-pick --reset (another name for --quit)' '
 	test_cmp expect actual
 '
 
+test_expect_success '--abort to cancel multiple cherry-pick' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	git cherry-pick --abort &&
+	test_path_is_missing .git/sequencer &&
+	test_cmp_rev initial HEAD &&
+	git update-index --refresh &&
+	git diff-index --exit-code HEAD
+'
+
+test_expect_success '--abort to cancel single cherry-pick' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick picked &&
+	git cherry-pick --abort &&
+	test_path_is_missing .git/sequencer &&
+	test_cmp_rev initial HEAD &&
+	git update-index --refresh &&
+	git diff-index --exit-code HEAD
+'
+
+test_expect_success 'cherry-pick --abort to cancel multiple revert' '
+	pristine_detach anotherpick &&
+	test_must_fail git revert base..picked &&
+	git cherry-pick --abort &&
+	test_path_is_missing .git/sequencer &&
+	test_cmp_rev anotherpick HEAD &&
+	git update-index --refresh &&
+	git diff-index --exit-code HEAD
+'
+
+test_expect_success 'revert --abort works, too' '
+	pristine_detach anotherpick &&
+	test_must_fail git revert base..picked &&
+	git revert --abort &&
+	test_path_is_missing .git/sequencer &&
+	test_cmp_rev anotherpick HEAD
+'
+
+test_expect_success '--abort to cancel single revert' '
+	pristine_detach anotherpick &&
+	test_must_fail git revert picked &&
+	git revert --abort &&
+	test_path_is_missing .git/sequencer &&
+	test_cmp_rev anotherpick HEAD &&
+	git update-index --refresh &&
+	git diff-index --exit-code HEAD
+'
+
+test_expect_success '--abort keeps unrelated change, easy case' '
+	pristine_detach unrelatedpick &&
+	echo changed >expect &&
+	test_must_fail git cherry-pick picked..yetanotherpick &&
+	echo changed >unrelated &&
+	git cherry-pick --abort &&
+	test_cmp expect unrelated
+'
+
+test_expect_success '--abort refuses to clobber unrelated change, harder case' '
+	pristine_detach initial &&
+	echo changed >expect &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo changed >unrelated &&
+	test_must_fail git cherry-pick --abort &&
+	test_cmp expect unrelated &&
+	git rev-list HEAD >log &&
+	test_line_count = 2 log &&
+	test_must_fail git update-index --refresh &&
+
+	git checkout unrelated &&
+	git cherry-pick --abort &&
+	test_cmp_rev initial HEAD
+'
+
 test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..picked &&

@@ -127,6 +213,16 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le
 	test_cmp expect actual
 '
 
+test_expect_failure '--abort after last commit in sequence' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..picked &&
+	git cherry-pick --abort &&
+	test_path_is_missing .git/sequencer &&
+	test_cmp_rev initial HEAD &&
+	git update-index --refresh &&
+	git diff-index --exit-code HEAD
+'
+
 test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..anotherpick &&

-- 
1.7.8.rc3

From: Johannes Sixt <redacted>

On Windows, it is not possible to rename or remove a directory that has
open files. 'revert --abort' renamed .git/sequencer when it still had
.git/sequencer/head open. Close the file as early as possible to allow
the rename operation on Windows.

Signed-off-by: Johannes Sixt <redacted>
---
 I guess it's too late to squash this in. ;)

 -- Hannes

 builtin/revert.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 0c61668..5dedb51 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -931,8 +931,10 @@ static int sequencer_rollback(struct replay_opts *opts)
 	if (strbuf_getline(&buf, f, '\n')) {
 		error(_("cannot read %s: %s"), filename, ferror(f) ?
 			strerror(errno) : _("unexpected end of file"));
+		fclose(f);
 		goto fail;
 	}
+	fclose(f);
 	if (get_sha1_hex(buf.buf, sha1) || buf.buf[40] != '\0') {
 		error(_("stored pre-cherry-pick HEAD file '%s' is corrupt"),
 			filename);

@@ -941,11 +943,9 @@ static int sequencer_rollback(struct replay_opts *opts)
 	if (reset_for_rollback(sha1))
 		goto fail;
 	strbuf_release(&buf);
-	fclose(f);
 	return 0;
 fail:
 	strbuf_release(&buf);
-	fclose(f);
 	return -1;
 }
 

-- 
1.7.8.rc3.1164.gba869.dirty

Johannes Sixt wrote:

From: Johannes Sixt <redacted>

On Windows, it is not possible to rename or remove a directory that has
open files. 'revert --abort' renamed .git/sequencer when it still had
.git/sequencer/head open. Close the file as early as possible to allow
the rename operation on Windows.

Nice catch.

Acked-by: Jonathan Nieder <redacted>

Speaking of which, it doesn't make a lot of sense for "git revert
--abort" to be leaving a .git/sequencer-old directory around.  How
about this on top?

-- >8 --
Subject: revert --abort: do not leave behind useless sequencer-old directory

The "git cherry-pick --abort" command currently renames the
.git/sequencer directory to .git/sequencer-old instead of removing it
on success due to an accident.  cherry-pick --abort is designed to
work in three steps:

 1) find which commit to roll back to
 2) call "git reset --merge <commit>" to move to that commit
 3) remove the .git/sequencer directory

But the careless author forgot step 3 entirely.  The only reason the
command worked anyway is that "git reset --merge <commit>" renames the
.git/sequencer directory as a secondary effect --- after moving to
<commit>, or so the logic goes, it is unlikely but possible that the
caller of git reset wants to continue the series of cherry-picks that
was in progress, so git renames the sequencer state to
.git/sequencer-old to be helpful while allowing the cherry-pick to be
resumed if the caller did not want to end the sequence after all.

By running "git cherry-pick --abort", the operator has clearly
indicated that she is not planning to continue cherry-picking.  Remove
the (renamed) .git/sequencer directory as intended all along.

Signed-off-by: Jonathan Nieder <redacted>
---
By the way, as the length of the second-to-last paragraph above might
have hinted, I am not convinced that allowing "git reset --hard" as an
escape route from a cherry-pick sequence was very sensible.  It
_would_ be nice to have a command to return to a known state,
discarding progress in all pending multiple-command guided workflows
(am, rebase, bisect), but git reset is not that command.

 builtin/revert.c          |    1 +
 t/t7106-reset-sequence.sh |    8 ++++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 5dedb51c..818b4abb 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -942,6 +942,7 @@ static int sequencer_rollback(struct replay_opts *opts)
 	}
 	if (reset_for_rollback(sha1))
 		goto fail;
+	remove_sequencer_state(1);
 	strbuf_release(&buf);
 	return 0;
 fail:

diff --git a/t/t7106-reset-sequence.sh b/t/t7106-reset-sequence.sh
index 3f86e8c5..83f7ea59 100755
--- a/t/t7106-reset-sequence.sh
+++ b/t/t7106-reset-sequence.sh

@@ -41,4 +41,12 @@ test_expect_success 'reset --hard cleans up sequencer state, providing one-level
 	test_path_is_missing .git/sequencer-old
 '
 
+test_expect_success 'cherry-pick --abort does not leave sequencer-old dir' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	git cherry-pick --abort &&
+	test_path_is_missing .git/sequencer &&
+	test_path_is_missing .git/sequencer-old
+'
+
 test_done

-- 
1.7.8.rc3

Am 11/23/2011 11:04, schrieb Jonathan Nieder:

... "git reset --merge <commit>" renames the
.git/sequencer directory as a secondary effect --- after moving to
<commit>, or so the logic goes, it is unlikely but possible that the
caller of git reset wants to continue the series of cherry-picks that
was in progress, so git renames the sequencer state to
.git/sequencer-old to be helpful while allowing the cherry-pick to be
resumed if the caller did not want to end the sequence after all.
...
By the way, as the length of [this paragraph] might
have hinted, I am not convinced that allowing "git reset --hard" as an
escape route from a cherry-pick sequence was very sensible.  It
_would_ be nice to have a command to return to a known state,
discarding progress in all pending multiple-command guided workflows
(am, rebase, bisect), but git reset is not that command.

IMO, it doesn't make sense that git-reset aborts a cherry-pick sequence:
When I messed up a difficult conflict in the middle of a cherry-pick
sequence, it might be useful to be able to 'git reset --hard && git
cherry-pick that-one-commit' to restart the conflict resolution.

(But does a single-commit cherry-pick during a multi-commit cherry-pick
work to begin with?)

-- Hannes

On Wed, Nov 23, 2011 at 09:49, Johannes Sixt [off-list ref] wrote:

From: Johannes Sixt <redacted>

On Windows, it is not possible to rename or remove a directory that has
open files. 'revert --abort' renamed .git/sequencer when it still had
.git/sequencer/head open. Close the file as early as possible to allow
the rename operation on Windows.

Signed-off-by: Johannes Sixt <redacted>
---
 I guess it's too late to squash this in. ;)

Just made a patch for this of my own (noticed it on Cygwin), should have
looked in the archives first. Thanks!

The minority platforms can wait, I guess :)

(-cc: Phil)
Johannes Sixt wrote:

IMO, it doesn't make sense that git-reset aborts a cherry-pick sequence:
When I messed up a difficult conflict in the middle of a cherry-pick
sequence, it might be useful to be able to 'git reset --hard && git
cherry-pick that-one-commit' to restart the conflict resolution.

(But does a single-commit cherry-pick during a multi-commit cherry-pick
work to begin with?)

It should, I think.

Here are patches to address some UI warts (such as that one) in
current cherry-pick code.

Patch 1 cleans up to prepare for patch 2, which in turn makes "git
cherry-pick --continue" act like "git rebase --continue" by commiting
a conflict resolution if it has not already been committed.  This
brings us closer to a less confusing world in which all commands that
can exit and ask the user to help to get closer to their goal provide
a --continue option as a standard interface to resume (I guess "git
merge --continue" is all that's left to do afterwards).

Patch 3 is from Ram's rr/revert-cherry-pick series.  It doesn't have
much to do with this series, but I'd rather work on a codebase with
this particular patch applied, so I applied it before working on
patch 4.

Patch 4 uses Junio's cmdline_info API to distinguish single-picks
from multi-picks.  This is the title feature.  For a while I thought
something like this was the only sane thing to do, but laziness won
out.

Patches 5-7 remove hacks that patch 4 makes superfluous.

Patch 7 has the downside that if anyone had a .git/sequencer-old
directory lying around, then git will not care after applying the
patch and it will sit just taking up its few bytes and distracting
people that run "ls .git".  I'm not sure whether that's worth fixing,
and if so how.

Anyway, I hope you enjoy the series.  Thoughts and bug reports
appreciated as usual.

Jonathan Nieder (7):
  revert: give --continue handling its own function
  revert: allow cherry-pick --continue to commit before resuming
  revert: pass around rev-list args in already-parsed form
  revert: allow single-pick in the middle of cherry-pick sequence
  revert: do not remove state until sequence is finished
  Revert "reset: Make reset remove the sequencer state"
  revert: stop creating or removing sequencer-old directory

 branch.c                        |    2 -
 builtin/revert.c                |  138 ++++++++++++++++++++++-----------
 sequencer.c                     |   10 +--
 sequencer.h                     |   12 +---
 t/t3510-cherry-pick-sequence.sh |  162 +++++++++++++++++++++++++++++++++++++--
 t/t7106-reset-sequence.sh       |   52 -------------
 6 files changed, 251 insertions(+), 125 deletions(-)
 delete mode 100755 t/t7106-reset-sequence.sh

This makes pick_revisions() a little shorter and easier to read
straight through.

Signed-off-by: Jonathan Nieder <redacted>
---
 builtin/revert.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c1..9f6c85c1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -1038,6 +1038,21 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	return 0;
 }
 
+static int sequencer_continue(struct replay_opts *opts)
+{
+	struct commit_list *todo_list = NULL;
+
+	if (!file_exists(git_path(SEQ_TODO_FILE)))
+		return error(_("No %s in progress"), action_name(opts));
+	read_populate_opts(&opts);
+	read_populate_todo(&todo_list, opts);
+
+	/* Verify that the conflict has been resolved */
+	if (!index_differs_from("HEAD", 0))
+		todo_list = todo_list->next;
+	return pick_commits(todo_list, opts);
+}
+
 static int pick_revisions(struct replay_opts *opts)
 {
 	struct commit_list *todo_list = NULL;

@@ -1056,17 +1071,8 @@ static int pick_revisions(struct replay_opts *opts)
 	}
 	if (opts->subcommand == REPLAY_ROLLBACK)
 		return sequencer_rollback(opts);
-	if (opts->subcommand == REPLAY_CONTINUE) {
-		if (!file_exists(git_path(SEQ_TODO_FILE)))
-			return error(_("No %s in progress"), action_name(opts));
-		read_populate_opts(&opts);
-		read_populate_todo(&todo_list, opts);
-
-		/* Verify that the conflict has been resolved */
-		if (!index_differs_from("HEAD", 0))
-			todo_list = todo_list->next;
-		return pick_commits(todo_list, opts);
-	}
+	if (opts->subcommand == REPLAY_CONTINUE)
+		return sequencer_continue(opts);
 
 	/*
 	 * Start a new cherry-pick/ revert sequence; but

-- 
1.7.8.rc3

When "git cherry-pick ..bar" encounters conflicts, permit the operator
to use cherry-pick --continue after resolving them as a shortcut for
"git commit && git cherry-pick --continue" to record the resolution
and carry on with the rest of the sequence.

This improves the analogy with "git rebase" (in olden days --continue
was the way to preserve authorship when a rebase encountered
conflicts) and fits well with a general UI goal of making "git cmd
--continue" save humans the trouble of deciding what to do next.

Example: after encountering a conflict from running "git cherry-pick
foo bar baz":

	CONFLICT (content): Merge conflict in main.c
	error: could not apply f78a8d98c... bar!
	hint: after resolving the conflicts, mark the corrected paths
	hint: with 'git add <paths>' or 'git rm <paths>'
	hint: and commit the result with 'git commit'

We edit main.c to resolve the conflict, mark it acceptable with "git
add main.c", and can run "cherry-pick --continue" to resume the
sequence.

	$ git cherry-pick --continue
	[editor opens to confirm commit message]
	[master 78c8a8c98] bar!
	 1 files changed, 1 insertions(+), 1 deletions(-)
	[master 87ca8798c] baz!
	 1 files changed, 1 insertions(+), 1 deletions(-)

Signed-off-by: Jonathan Nieder <redacted>
---
 builtin/revert.c                |   23 ++++++-
 t/t3510-cherry-pick-sequence.sh |  139 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 156 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 9f6c85c1..a43b4d85 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -1038,18 +1038,35 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	return 0;
 }
 
+static int continue_single_pick(void)
+{
+	const char *argv[] = { "commit", NULL };
+
+	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
+	    !file_exists(git_path("REVERT_HEAD")))
+		return error(_("no cherry-pick or revert in progress"));
+	return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
 static int sequencer_continue(struct replay_opts *opts)
 {
 	struct commit_list *todo_list = NULL;
 
 	if (!file_exists(git_path(SEQ_TODO_FILE)))
-		return error(_("No %s in progress"), action_name(opts));
+		return continue_single_pick();
 	read_populate_opts(&opts);
 	read_populate_todo(&todo_list, opts);
 
 	/* Verify that the conflict has been resolved */
-	if (!index_differs_from("HEAD", 0))
-		todo_list = todo_list->next;
+	if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
+	    file_exists(git_path("REVERT_HEAD"))) {
+		int ret = continue_single_pick();
+		if (ret)
+			return ret;
+	}
+	if (index_differs_from("HEAD", 0))
+		return error_dirty_index(opts);
+	todo_list = todo_list->next;
 	return pick_commits(todo_list, opts);
 }
 

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 2c4c1c85..4d1883b7 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh

@@ -2,6 +2,7 @@
 
 test_description='Test cherry-pick continuation features
 
+ +  conflicting: rewrites unrelated to conflicting
   + yetanotherpick: rewrites foo to e
   + anotherpick: rewrites foo to d
   + picked: rewrites foo to c

@@ -27,6 +28,7 @@ test_cmp_rev () {
 }
 
 test_expect_success setup '
+	git config advice.detachedhead false
 	echo unrelated >unrelated &&
 	git add unrelated &&
 	test_commit initial foo a &&

@@ -35,8 +37,8 @@ test_expect_success setup '
 	test_commit picked foo c &&
 	test_commit anotherpick foo d &&
 	test_commit yetanotherpick foo e &&
-	git config advice.detachedhead false
-
+	pristine_detach initial &&
+	test_commit conflicting unrelated
 '
 
 test_expect_success 'cherry-pick persists data on failure' '

@@ -243,7 +245,66 @@ test_expect_success '--continue complains when there are unresolved conflicts' '
 	test_must_fail git cherry-pick --continue
 '
 
-test_expect_success '--continue continues after conflicts are resolved' '
+test_expect_success '--continue of single cherry-pick' '
+	pristine_detach initial &&
+	echo c >expect &&
+	test_must_fail git cherry-pick picked &&
+	echo c >foo &&
+	git add foo &&
+	git cherry-pick --continue &&
+
+	test_cmp expect foo &&
+	test_cmp_rev initial HEAD^ &&
+	git diff --exit-code HEAD &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
+'
+
+test_expect_success '--continue of single revert' '
+	pristine_detach initial &&
+	echo resolved >expect &&
+	echo "Revert \"picked\"" >expect.msg &&
+	test_must_fail git revert picked &&
+	echo resolved >foo &&
+	git add foo &&
+	git cherry-pick --continue &&
+
+	git diff --exit-code HEAD &&
+	test_cmp expect foo &&
+	test_cmp_rev initial HEAD^ &&
+	git diff-tree -s --pretty=tformat:%s HEAD >msg &&
+	test_cmp expect.msg msg &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
+	test_must_fail git rev-parse --verify REVERT_HEAD
+'
+
+test_expect_success '--continue after resolving conflicts' '
+	pristine_detach initial &&
+	echo d >expect &&
+	cat >expect.log <<-\EOF &&
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	OBJID
+	:100644 100644 OBJID OBJID M	foo
+	OBJID
+	:100644 100644 OBJID OBJID M	unrelated
+	OBJID
+	:000000 100644 OBJID OBJID A	foo
+	:000000 100644 OBJID OBJID A	unrelated
+	EOF
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo c >foo &&
+	git add foo &&
+	git cherry-pick --continue &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual.log &&
+	test_cmp expect foo &&
+	test_cmp expect.log actual.log
+'
+
+test_expect_success '--continue after resolving conflicts and committing' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..anotherpick &&
 	echo "c" >foo &&

@@ -270,6 +331,29 @@ test_expect_success '--continue continues after conflicts are resolved' '
 	test_cmp expect actual
 '
 
+test_expect_success '--continue asks for help after resolving patch to nil' '
+	pristine_detach conflicting &&
+	test_must_fail git cherry-pick initial..picked &&
+
+	test_cmp_rev unrelatedpick CHERRY_PICK_HEAD &&
+	git checkout HEAD -- unrelated &&
+	test_must_fail git cherry-pick --continue 2>msg &&
+	test_i18ngrep "The previous cherry-pick is now empty" msg
+'
+
+test_expect_failure 'follow advice and skip nil patch' '
+	pristine_detach conflicting &&
+	test_must_fail git cherry-pick initial..picked &&
+
+	git checkout HEAD -- unrelated &&
+	test_must_fail git cherry-pick --continue &&
+	git reset &&
+	git cherry-pick --continue &&
+
+	git rev-list initial..HEAD >commits &&
+	test_line_count = 3 commits
+'
+
 test_expect_success '--continue respects opts' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -x base..anotherpick &&

@@ -288,6 +372,29 @@ test_expect_success '--continue respects opts' '
 	grep "cherry picked from" anotherpick_msg
 '
 
+test_expect_success '--continue of single-pick respects -x' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -x picked &&
+	echo c >foo &&
+	git add foo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	git cat-file commit HEAD >msg &&
+	grep "cherry picked from" msg
+'
+
+test_expect_success '--continue respects -x in first commit in multi-pick' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -x picked anotherpick &&
+	echo c >foo &&
+	git add foo &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	git cat-file commit HEAD^ >msg &&
+	picked=$(git rev-parse --verify picked) &&
+	grep "cherry picked from.*$picked" msg
+'
+
 test_expect_success '--signoff is not automatically propagated to resolved conflict' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick --signoff base..anotherpick &&

@@ -306,6 +413,32 @@ test_expect_success '--signoff is not automatically propagated to resolved confl
 	grep "Signed-off-by:" anotherpick_msg
 '
 
+test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -s picked anotherpick &&
+	echo c >foo &&
+	git add foo &&
+	git cherry-pick --continue &&
+
+	git diff --exit-code HEAD &&
+	test_cmp_rev initial HEAD^^ &&
+	git cat-file commit HEAD^ >msg &&
+	! grep Signed-off-by: msg
+'
+
+test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick -s picked &&
+	echo c >foo &&
+	git add foo &&
+	git cherry-pick --continue &&
+
+	git diff --exit-code HEAD &&
+	test_cmp_rev initial HEAD^ &&
+	git cat-file commit HEAD >msg &&
+	! grep Signed-off-by: msg
+'
+
 test_expect_success 'malformed instruction sheet 1' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..anotherpick &&

-- 
1.7.8.rc3

Date: Sat, 13 Aug 2011 12:06:23 -0500

Since 7e2bfd3f (revert: allow cherry-picking more than one commit,
2010-07-02), the pick/revert machinery has kept track of the set of
commits to be cherry-picked or reverted using commit_argc and
commit_argv variables, storing the corresponding command-line
parameters.

Future callers as other commands are built in (am, rebase, sequencer)
may find it easier to pass rev-list options to this machinery in
already-parsed form.  Teach cmd_cherry_pick and cmd_revert to parse
the rev-list arguments in advance and pass the commit set to
pick_revisions() as a rev_info structure.

Original patch by Jonathan, tweaks and test from Ram.

Signed-off-by: Jonathan Nieder <redacted>
Improved-by: Ramkumar Ramachandra [off-list ref]
---
 builtin/revert.c                |   53 +++++++++++++++++++++-----------------
 t/t3510-cherry-pick-sequence.sh |    5 +++
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index a43b4d85..71570357 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -60,13 +60,14 @@ struct replay_opts {
 	int allow_rerere_auto;
 
 	int mainline;
-	int commit_argc;
-	const char **commit_argv;
 
 	/* Merge strategy */
 	const char *strategy;
 	const char **xopts;
 	size_t xopts_nr, xopts_alloc;
+
+	/* Only used by REPLAY_NONE */
+	struct rev_info *revs;
 };
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"

@@ -169,9 +170,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			die(_("program error"));
 	}
 
-	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
-					PARSE_OPT_KEEP_ARGV0 |
-					PARSE_OPT_KEEP_UNKNOWN);
+	argc = parse_options(argc, argv, NULL, options, usage_str,
+			PARSE_OPT_KEEP_ARGV0 |
+			PARSE_OPT_KEEP_UNKNOWN);
 
 	/* Check for incompatible subcommands */
 	verify_opt_mutually_compatible(me,

@@ -213,9 +214,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				NULL);
 	}
 
-	else if (opts->commit_argc < 2)
-		usage_with_options(usage_str, options);
-
 	if (opts->allow_ff)
 		verify_opt_compatible(me, "--ff",
 				"--signoff", opts->signoff,

@@ -223,7 +221,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"-x", opts->record_origin,
 				"--edit", opts->edit,
 				NULL);
-	opts->commit_argv = argv;
+
+	if (opts->subcommand != REPLAY_NONE) {
+		opts->revs = NULL;
+	} else {
+		opts->revs = xmalloc(sizeof(*opts->revs));
+		init_revisions(opts->revs, NULL);
+		opts->revs->no_walk = 1;
+		if (argc < 2)
+			usage_with_options(usage_str, options);
+		argc = setup_revisions(argc, argv, opts->revs, NULL);
+	}
+
+	if (argc > 1)
+		usage_with_options(usage_str, options);
 }
 
 struct commit_message {

@@ -631,23 +642,15 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
+static void prepare_revs(struct replay_opts *opts)
 {
-	int argc;
-
-	init_revisions(revs, NULL);
-	revs->no_walk = 1;
 	if (opts->action != REVERT)
-		revs->reverse = 1;
+		opts->revs->reverse ^= 1;
 
-	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
-	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage(opts));
-
-	if (prepare_revision_walk(revs))
+	if (prepare_revision_walk(opts->revs))
 		die(_("revision walk setup failed"));
 
-	if (!revs->commits)
+	if (!opts->revs->commits)
 		die(_("empty commit set passed"));
 }
 

@@ -844,14 +847,13 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
 static void walk_revs_populate_todo(struct commit_list **todo_list,
 				struct replay_opts *opts)
 {
-	struct rev_info revs;
 	struct commit *commit;
 	struct commit_list **next;
 
-	prepare_revs(&revs, opts);
+	prepare_revs(opts);
 
 	next = todo_list;
-	while ((commit = get_revision(&revs)))
+	while ((commit = get_revision(opts->revs)))
 		next = commit_list_append(commit, next);
 }
 

@@ -1075,6 +1077,9 @@ static int pick_revisions(struct replay_opts *opts)
 	struct commit_list *todo_list = NULL;
 	unsigned char sha1[20];
 
+	if (opts->subcommand == REPLAY_NONE)
+		assert(opts->revs);
+
 	read_and_refresh_cache(opts);
 
 	/*

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 4d1883b7..56c95ec1 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh

@@ -461,4 +461,9 @@ test_expect_success 'malformed instruction sheet 2' '
 	test_must_fail git cherry-pick --continue
 '
 
+test_expect_success 'empty commit set' '
+	pristine_detach initial &&
+	test_expect_code 128 git cherry-pick base..base
+'
+
 test_done

-- 
1.7.8.rc3

When I messed up a difficult conflict in the middle of a cherry-pick
sequence, it can be useful to be able to 'git checkout HEAD . && git
cherry-pick that-one-commit' to restart the conflict resolution.

Suggested-by: Johannes Sixt <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Maybe this should come after patch 6/7, so the use case could use
"git reset --hard".

 builtin/revert.c                |   26 ++++++++++++++++++++++++++
 t/t3510-cherry-pick-sequence.sh |   12 ++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 71570357..dcb69904 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -1072,6 +1072,12 @@ static int sequencer_continue(struct replay_opts *opts)
 	return pick_commits(todo_list, opts);
 }
 
+static int single_pick(struct commit *cmit, struct replay_opts *opts)
+{
+	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
+	return do_pick_commit(cmit, opts);
+}
+
 static int pick_revisions(struct replay_opts *opts)
 {
 	struct commit_list *todo_list = NULL;

@@ -1097,6 +1103,26 @@ static int pick_revisions(struct replay_opts *opts)
 		return sequencer_continue(opts);
 
 	/*
+	 * If we were called as "git cherry-pick <commit>", just
+	 * cherry-pick/revert it, set CHERRY_PICK_HEAD /
+	 * REVERT_HEAD, and don't touch the sequencer state.
+	 * This means it is possible to cherry-pick in the middle
+	 * of a cherry-pick sequence.
+	 */
+	if (opts->revs->cmdline.nr == 1 &&
+	    opts->revs->cmdline.rev->whence == REV_CMD_REV &&
+	    opts->revs->no_walk &&
+	    !opts->revs->cmdline.rev->flags) {
+		struct commit *cmit;
+		if (prepare_revision_walk(opts->revs))
+			die(_("revision walk setup failed"));
+		cmit = get_revision(opts->revs);
+		if (!cmit || get_revision(opts->revs))
+			die("BUG: expected exactly one commit from walk");
+		return single_pick(cmit, opts);
+	}
+
+	/*
 	 * Start a new cherry-pick/ revert sequence; but
 	 * first, make sure that an existing one isn't in
 	 * progress

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 56c95ec1..98a27a23 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh

@@ -50,6 +50,18 @@ test_expect_success 'cherry-pick persists data on failure' '
 	test_path_is_file .git/sequencer/opts
 '
 
+test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	test_cmp_rev picked CHERRY_PICK_HEAD &&
+	# "oops, I forgot that these patches rely on the change from base"
+	git checkout HEAD foo &&
+	git cherry-pick base &&
+	git cherry-pick picked &&
+	git cherry-pick --continue &&
+	git diff --exit-code anotherpick
+'
+
 test_expect_success 'cherry-pick persists opts correctly' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&

-- 
1.7.8.rc3

As v1.7.8-rc0~141^2~4 (2011-08-04) explains, git cherry-pick removes
the sequencer state just before applying the final patch.  In the
single-pick case, that was a good thing, since --abort and --continue
work fine without access to such state and removing it provides a
signal that git should not complain about the need to clobber it ("a
cherry-pick or revert is already in progress") in sequences like the
following:

	git cherry-pick foo
	git read-tree -m -u HEAD; # forget that; let's try a different one
	git cherry-pick bar

After the recent patch "allow single-pick in the middle of cherry-pick
sequence" we don't need that hack any more.  In the new regime, a
traditional "git cherry-pick <commit>" command never looks at
.git/sequencer, so we do not need to cripple "git cherry-pick
<commit>..<commit>" for it any more.

So now you can run "git cherry-pick --abort" near the end of a
multi-pick sequence and it will abort the entire sequence, instead of
misbehaving and aborting just the final commit.

Signed-off-by: Jonathan Nieder <redacted>
---
 builtin/revert.c                |   12 +-----------
 t/t3510-cherry-pick-sequence.sh |    6 +++---
 2 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index dcb69904..28deb85b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -1018,18 +1018,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur, opts);
 		res = do_pick_commit(cur->item, opts);
-		if (res) {
-			if (!cur->next)
-				/*
-				 * An error was encountered while
-				 * picking the last commit; the
-				 * sequencer state is useless now --
-				 * the user simply needs to resolve
-				 * the conflict and commit
-				 */
-				remove_sequencer_state(0);
+		if (res)
 			return res;
-		}
 	}
 
 	/*

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 98a27a23..851b147f 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh

@@ -203,10 +203,10 @@ test_expect_success '--abort refuses to clobber unrelated change, harder case' '
 	test_cmp_rev initial HEAD
 '
 
-test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
+test_expect_success 'cherry-pick still writes sequencer state when one commit is left' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..picked &&
-	test_path_is_missing .git/sequencer &&
+	test_path_is_dir .git/sequencer &&
 	echo "resolved" >foo &&
 	git add foo &&
 	git commit &&

@@ -227,7 +227,7 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le
 	test_cmp expect actual
 '
 
-test_expect_failure '--abort after last commit in sequence' '
+test_expect_success '--abort after last commit in sequence' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..picked &&
 	git cherry-pick --abort &&

-- 
1.7.8.rc3

This reverts commit 95eb88d8ee588d89b4f06d2753ed4d16ab13b39f, which
was a UI experiment that did not reflect how "git reset" actually gets
used.  The reversion also fixes a test, indicated in the patch.

Encouraged-by: Johannes Sixt [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
 branch.c                        |    2 -
 t/t3510-cherry-pick-sequence.sh |    2 +-
 t/t7106-reset-sequence.sh       |   52 ---------------------------------------
 3 files changed, 1 insertions(+), 55 deletions(-)
 delete mode 100755 t/t7106-reset-sequence.sh

diff --git a/branch.c b/branch.c
index 025a97be..a6b6722e 100644
--- a/branch.c
+++ b/branch.c

@@ -3,7 +3,6 @@
 #include "refs.h"
 #include "remote.h"
 #include "commit.h"
-#include "sequencer.h"
 
 struct tracking {
 	struct refspec spec;

@@ -247,5 +246,4 @@ void remove_branch_state(void)
 	unlink(git_path("MERGE_MSG"));
 	unlink(git_path("MERGE_MODE"));
 	unlink(git_path("SQUASH_MSG"));
-	remove_sequencer_state(0);
 }

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 851b147f..e80050e1 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh

@@ -353,7 +353,7 @@ test_expect_success '--continue asks for help after resolving patch to nil' '
 	test_i18ngrep "The previous cherry-pick is now empty" msg
 '
 
-test_expect_failure 'follow advice and skip nil patch' '
+test_expect_success 'follow advice and skip nil patch' '
 	pristine_detach conflicting &&
 	test_must_fail git cherry-pick initial..picked &&
 

diff --git a/t/t7106-reset-sequence.sh b/t/t7106-reset-sequence.sh
deleted file mode 100755
index 83f7ea59..00000000
--- a/t/t7106-reset-sequence.sh
+++ /dev/null

@@ -1,52 +0,0 @@
-#!/bin/sh
-
-test_description='Test interaction of reset --hard with sequencer
-
-  + anotherpick: rewrites foo to d
-  + picked: rewrites foo to c
-  + unrelatedpick: rewrites unrelated to reallyunrelated
-  + base: rewrites foo to b
-  + initial: writes foo as a, unrelated as unrelated
-'
-
-. ./test-lib.sh
-
-pristine_detach () {
-	git cherry-pick --quit &&
-	git checkout -f "$1^0" &&
-	git read-tree -u --reset HEAD &&
-	git clean -d -f -f -q -x
-}
-
-test_expect_success setup '
-	echo unrelated >unrelated &&
-	git add unrelated &&
-	test_commit initial foo a &&
-	test_commit base foo b &&
-	test_commit unrelatedpick unrelated reallyunrelated &&
-	test_commit picked foo c &&
-	test_commit anotherpick foo d &&
-	git config advice.detachedhead false
-
-'
-
-test_expect_success 'reset --hard cleans up sequencer state, providing one-level undo' '
-	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
-	test_path_is_dir .git/sequencer &&
-	git reset --hard &&
-	test_path_is_missing .git/sequencer &&
-	test_path_is_dir .git/sequencer-old &&
-	git reset --hard &&
-	test_path_is_missing .git/sequencer-old
-'
-
-test_expect_success 'cherry-pick --abort does not leave sequencer-old dir' '
-	pristine_detach initial &&
-	test_must_fail git cherry-pick base..anotherpick &&
-	git cherry-pick --abort &&
-	test_path_is_missing .git/sequencer &&
-	test_path_is_missing .git/sequencer-old
-'
-
-test_done

-- 
1.7.8.rc3

Now that "git reset" no longer implicitly removes .git/sequencer that
the operator may or may not have wanted to keep, the logic to write a
backup copy of .git/sequencer and remove it when stale is not needed
any more.  Simplify the sequencer API and repository layout by
dropping it.

Signed-off-by: Jonathan Nieder <redacted>
---
That's the end.  I hope the patches provided some amusement, and
advice towards making them more useful would be welcome.

Good night,
Jonathan

 builtin/revert.c |    6 +++---
 sequencer.c      |   10 ++--------
 sequencer.h      |   12 ++----------
 3 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 28deb85b..444df413 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -944,7 +944,7 @@ static int sequencer_rollback(struct replay_opts *opts)
 	}
 	if (reset_for_rollback(sha1))
 		goto fail;
-	remove_sequencer_state(1);
+	remove_sequencer_state();
 	strbuf_release(&buf);
 	return 0;
 fail:

@@ -1026,7 +1026,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	 * Sequence of picks finished successfully; cleanup by
 	 * removing the .git/sequencer directory
 	 */
-	remove_sequencer_state(1);
+	remove_sequencer_state();
 	return 0;
 }
 

@@ -1084,7 +1084,7 @@ static int pick_revisions(struct replay_opts *opts)
 	 * one that is being continued
 	 */
 	if (opts->subcommand == REPLAY_REMOVE_STATE) {
-		remove_sequencer_state(1);
+		remove_sequencer_state();
 		return 0;
 	}
 	if (opts->subcommand == REPLAY_ROLLBACK)

diff --git a/sequencer.c b/sequencer.c
index bc2c046a..d1f28a69 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -3,17 +3,11 @@
 #include "strbuf.h"
 #include "dir.h"
 
-void remove_sequencer_state(int aggressive)
+void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
-	struct strbuf seq_old_dir = STRBUF_INIT;
 
 	strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
-	strbuf_addf(&seq_old_dir, "%s", git_path(SEQ_OLD_DIR));
-	remove_dir_recursively(&seq_old_dir, 0);
-	rename(git_path(SEQ_DIR), git_path(SEQ_OLD_DIR));
-	if (aggressive)
-		remove_dir_recursively(&seq_old_dir, 0);
+	remove_dir_recursively(&seq_dir, 0);
 	strbuf_release(&seq_dir);
-	strbuf_release(&seq_old_dir);
 }

diff --git a/sequencer.h b/sequencer.h
index f435fdb4..2d4528f2 100644
--- a/sequencer.h
+++ b/sequencer.h

@@ -2,19 +2,11 @@
 #define SEQUENCER_H
 
 #define SEQ_DIR		"sequencer"
-#define SEQ_OLD_DIR	"sequencer-old"
 #define SEQ_HEAD_FILE	"sequencer/head"
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
-/*
- * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
- * any errors.  Intended to be used by 'git reset'.
- *
- * With the aggressive flag, it additionally removes SEQ_OLD_DIR,
- * ignoring any errors.  Inteded to be used by the sequencer's
- * '--quit' subcommand.
- */
-void remove_sequencer_state(int aggressive);
+/* Removes SEQ_DIR. */
+extern void remove_sequencer_state(void);
 
 #endif

-- 
1.7.8.rc3

Jonathan Nieder wrote:

Here are patches to address some UI warts

[... rambling cover letter snipped ...]

Let's try that again. :)

Current git has a somewhat odd behavior when cherry-picking multiple
commits and running into a conflict in the _last_ commit of the
series.  Imagine the following sequence of operations:

 1. git cherry-pick simplething simplethingtwo complexthing
 2. CONFLICT.
 3. git cherry-pick --abort

It would be most consistent for the entire cherry-pick sequence to be
rolled back, so the user can come up with some other sequence of
commits to try.  After all, that's what happens if a conflict is
encountered applying simplethingtwo and the user asks to abort.

Instead, by the time complexthing is being applied, git has forgotten
about the multi-pick sequence entirely.  And the --abort does not even
warn about this weird state --- it just cancels complexthing and
leaves the earlier commits applied.

This is an edge case, but I think it's worth fixing.  Patch 5/7 does
so.

In the same vein, now imagine a different sequence of operations:

 1. git cherry-pick simplething complexthing morecommits...
 2. CONFLICT.
 3. git reset --merge
 4. git cherry-pick --continue

It would be sensible for this to remove the conflicted patch and go
on with the remaining ones, right?  But instead, "git reset"
automagically removes the sequencer state, so you can't even use
git cherry-pick --abort any more.  Well, you can, if you say the
magic words "mv .git/sequencer-old .git/sequencer", but nobody
actually tells you that.

How did we ever let this in?  "git reset" already has well defined
semantics that have nothing to do with this.  Patches 6/7 and 7/7
would help us forget this UI mistake (and I believe it was a mistake)
ever happened.

Patch 2 makes cherry-pick --continue behave a little more like
rebase --continue, for people who like to learn by analogy.

Patches 1 and 3 are just code style things.

Patch 4 is the basic building block that makes patch 5 possible: it
teaches "git cherry-pick" to treat picks of a single commit named on
the command line differently from the more complex multi-picks
requested with general rev-list arguments.  Single-pick is all that
git cherry-pick originally supported, and in some details it has to
differ from multi-pick (for example, "git commit" after resolving
conflicts after a conflicted single-pick needs to be enough to clear
the state).  As a side-benefit, we get the ability to do a single-pick
in the middle of a multi-pick, which is kind of cool and handy from
time to time.

I am interested in sanity checking of the patches and testing.  It
would be pleasant to find comments like "yeah, that looks good" or
"what are you thinking?!  I ran into the following bug" arriving in my
inbox.

Incidentally, I'd like to apologize about not protesting more about
these things (and even having suggested some of them) as they
happened.  Instead of exercising careful oversight over the sanity of
patches that passed through my mailbox, I had some strange idea of
using the Socratic method to help others learn to explore the design
space and sanity-check findings themselves...

Thanks for reading.

Ciao,
Jonathan

Hi Jonathan,

Jonathan Nieder wrote:

This makes pick_revisions() a little shorter and easier to read
straight through.

Ah, yes: you've asked about this earlier.  Sounds sane; let's read
ahead and see if anything jumps out.

 static int pick_revisions(struct replay_opts *opts)
 {
       struct commit_list *todo_list = NULL;
[...]

This is the only detailed that jumped out- you're filling up two
different commit_list structures, depending on whether we're
performing a fresh operation or continuing an existing one.  Okay.

Thanks.

p.s- Sorry about the delay; just returned from a short vacation.

-- Ram

Hi again,

Jonathan Nieder wrote:

When "git cherry-pick ..bar" encounters conflicts, permit the operator
to use cherry-pick --continue after resolving them as a shortcut for
"git commit && git cherry-pick --continue" to record the resolution
and carry on with the rest of the sequence.

Sounds good.  I remember my implementation being quite complicated;
let's see how you've done this.

Example: after encountering a conflict from running "git cherry-pick
foo bar baz":

       CONFLICT (content): Merge conflict in main.c
       error: could not apply f78a8d98c... bar!
       hint: after resolving the conflicts, mark the corrected paths
       hint: with 'git add <paths>' or 'git rm <paths>'
       hint: and commit the result with 'git commit'

We edit main.c to resolve the conflict, mark it acceptable with "git
add main.c", and can run "cherry-pick --continue" to resume the
sequence.

       $ git cherry-pick --continue
       [editor opens to confirm commit message]
       [master 78c8a8c98] bar!
        1 files changed, 1 insertions(+), 1 deletions(-)
       [master 87ca8798c] baz!
        1 files changed, 1 insertions(+), 1 deletions(-)

I like the presentation of this example: much clearer than my examples.

 builtin/revert.c                |   23 ++++++-
 t/t3510-cherry-pick-sequence.sh |  139 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 156 insertions(+), 6 deletions(-)

Oh, good -- lots of new tests :)

Very nice!  I can see how the introduction of REVERT_HEAD simplifies things :)
I'm totally embarrassed by the horribly convoluted logic in the "New
sequencer workflow!" I posted earlier.

Note to self: don't capitalize error() messages.

 static int sequencer_continue(struct replay_opts *opts)
 {
       struct commit_list *todo_list = NULL;

       if (!file_exists(git_path(SEQ_TODO_FILE)))
-               return error(_("No %s in progress"), action_name(opts));
+               return continue_single_pick();
       read_populate_opts(&opts);
       read_populate_todo(&todo_list, opts);

       /* Verify that the conflict has been resolved */
-       if (!index_differs_from("HEAD", 0))
-               todo_list = todo_list->next;
+       if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
+           file_exists(git_path("REVERT_HEAD"))) {
+               int ret = continue_single_pick();
+               if (ret)
+                       return ret;
+       }
+       if (index_differs_from("HEAD", 0))
+               return error_dirty_index(opts);
+       todo_list = todo_list->next;
       return pick_commits(todo_list, opts);
 }

Very nicely done.  I can see why 1/7 makes so much sense now:  it
helps think of different operations independently.

Note to self: this list of commits is becoming quite unwieldy.  We
should probably refactor these sometime.

Huh, why are you moving this line up?  Oh, right: there are
"test_commit" statements in the setup- good catch.  This is unrelated
to your patch and should be a separate commit though.

Looks fishy- I wonder why you're doing this.  Let's read ahead and find out.

Beautiful.  The tests I wrote are ugly in comparison :\

+test_expect_success '--continue of single revert' '
+       pristine_detach initial &&
+       echo resolved >expect &&
+       echo "Revert \"picked\"" >expect.msg &&
+       test_must_fail git revert picked &&
+       echo resolved >foo &&
+       git add foo &&
+       git cherry-pick --continue &&

Huh?  You're continuing a "git revert" with a a "git cherry-pick
--continue"?  The current 'master' still uses a commit_list, and
doesn't allow mixed "pick" and "revert" instructions yet.

+       git diff --exit-code HEAD &&
+       test_cmp expect foo &&
+       test_cmp_rev initial HEAD^ &&
+       git diff-tree -s --pretty=tformat:%s HEAD >msg &&
+       test_cmp expect.msg msg &&
+       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
+       test_must_fail git rev-parse --verify REVERT_HEAD
+'

A couple of notes:

1. I haven't used the "-s" flag of "git diff-tree" before, so I opened
up the documentation to find this:

        By default, 'git diff-tree --stdin' shows differences,
        either in machine-readable form (without '-p') or in patch
        form (with '-p').  This output can be suppressed.  It is
        only useful with '-v' flag.

Very misleading.  TODO: Fix this.

2. Why did you use "diff-tree" to get the commit message?  Isn't
"cat-file commit" much more straightforward?

+test_expect_success '--continue after resolving conflicts' '
+       pristine_detach initial &&
+       echo d >expect &&
+       cat >expect.log <<-\EOF &&
+       OBJID
+       :100644 100644 OBJID OBJID M    foo
+       OBJID
+       :100644 100644 OBJID OBJID M    foo
+       OBJID
+       :100644 100644 OBJID OBJID M    unrelated
+       OBJID
+       :000000 100644 OBJID OBJID A    foo
+       :000000 100644 OBJID OBJID A    unrelated
+       EOF
+       test_must_fail git cherry-pick base..anotherpick &&
+       echo c >foo &&
+       git add foo &&
+       git cherry-pick --continue &&
+       {
+               git rev-list HEAD |
+               git diff-tree --root --stdin |
+               sed "s/$_x40/OBJID/g"
+       } >actual.log &&
+       test_cmp expect foo &&
+       test_cmp expect.log actual.log
+'

Unchanged from the original: I suspect you've moved the generation of
expectation messages up to produce a clean diff.

+test_expect_success '--continue after resolving conflicts and committing' '
       pristine_detach initial &&
       test_must_fail git cherry-pick base..anotherpick &&
       echo "c" >foo &&

Okay, the diff isn't all that clean :P

+test_expect_success '--continue asks for help after resolving patch to nil' '
+       pristine_detach conflicting &&
+       test_must_fail git cherry-pick initial..picked &&
+
+       test_cmp_rev unrelatedpick CHERRY_PICK_HEAD &&
+       git checkout HEAD -- unrelated &&
+       test_must_fail git cherry-pick --continue 2>msg &&
+       test_i18ngrep "The previous cherry-pick is now empty" msg
+'

I thought it was a bad idea to grep for specific output messages,
because of their volatile nature?  Remind me what this test has to do
with the rest of your patch?

+test_expect_failure 'follow advice and skip nil patch' '
+       pristine_detach conflicting &&
+       test_must_fail git cherry-pick initial..picked &&
+
+       git checkout HEAD -- unrelated &&
+       test_must_fail git cherry-pick --continue &&
+       git reset &&
+       git cherry-pick --continue &&
+
+       git rev-list initial..HEAD >commits &&
+       test_line_count = 3 commits
+'

Again, what does this test have to do with the rest of your patch?

I'd have liked s/respects -x/respects opts/ here for symmetry with the
previous test.

+test_expect_success '--continue respects -x in first commit in multi-pick' '
+       pristine_detach initial &&
+       test_must_fail git cherry-pick -x picked anotherpick &&
+       echo c >foo &&
+       git add foo &&
+       git cherry-pick --continue &&
+       test_path_is_missing .git/sequencer &&
+       git cat-file commit HEAD^ >msg &&
+       picked=$(git rev-parse --verify picked) &&
+       grep "cherry picked from.*$picked" msg
+'

Can you explain why "first commit in a multi-pick" is a special case?

Unrelated.

+test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '
+       pristine_detach initial &&
+       test_must_fail git cherry-pick -s picked &&
+       echo c >foo &&
+       git add foo &&
+       git cherry-pick --continue &&
+
+       git diff --exit-code HEAD &&
+       test_cmp_rev initial HEAD^ &&
+       git cat-file commit HEAD >msg &&
+       ! grep Signed-off-by: msg
+'

If the previous test were a separate patch preceding this one, this'd
belong here.

Thanks for working on this.

-- Ram

Hey,

Jonathan Nieder wrote:

[...]
Original patch by Jonathan, tweaks and test from Ram.

Signed-off-by: Jonathan Nieder <redacted>
Improved-by: Ramkumar Ramachandra [off-list ref]

I've already seen this, so let's see how it fits into the rest of the series.

-- Ram

Hi,

Jonathan Nieder wrote:

When I messed up a difficult conflict in the middle of a cherry-pick
sequence, it can be useful to be able to 'git checkout HEAD . && git
cherry-pick that-one-commit' to restart the conflict resolution.

I was about to complain about the commit message until I noticed that
Junio already fixed it in `next`:

    revert: allow single-pick in the middle of cherry-pick sequence

    After messing up a difficult conflict resolution in the middle of a
    cherry-pick sequence, it can be useful to be able to

        git checkout HEAD . && git cherry-pick that-one-commit

    to restart the conflict resolution. The current code however errors out
    saying that another cherry-pick is already in progress.

Interesting concept; let's see how it's implemented.

Suggested-by: Johannes Sixt <redacted>

Could you link to the corresponding thread with Johannes?

single_pick as opposed to the continue_single_pick introduced in 2/7.

Conceptually all very good.  What I'm really interested in seeing is
how you persist opts for "cherry-pick --continue" when a single-commit
pick fails: in other words, how you manage to get " --continue of
single-pick respects -x" to pass.

+       if (opts->revs->cmdline.nr == 1 &&
+           opts->revs->cmdline.rev->whence == REV_CMD_REV &&
+           opts->revs->no_walk &&
+           !opts->revs->cmdline.rev->flags) {

Yuck, seriously.
1. I'd have expected you to check opts->revs->commits, not
opts->revs->cmdline.nr.  Okay, you're using the cmdline because the
revision walk hasn't happened yet.
2. Why are you using opts->revs->cmdline.rev->whence as opposed to
opts->action?  Why do you want to expose the underlying revision
walking mechanism?
3. When will the opts->revs->no_walk condition not be satisfied?  Only
when you explicitly set it to 0 or NULL, right -- where is this
happening in revert.c?
4. Why are you checking flags?  When is this condition not going to be
satisfied?

Since 3 and 4 indicate that you're being overly defensive, consistency
requires you to guarantee that this code will work no matter what the
rev_info struct is filled up with prior to this segment.
Is this true?

+               struct commit *cmit;
+               if (prepare_revision_walk(opts->revs))
+                       die(_("revision walk setup failed"));
+               cmit = get_revision(opts->revs);
+               if (!cmit || get_revision(opts->revs))
+                       die("BUG: expected exactly one commit from walk");
+               return single_pick(cmit, opts);
+       }

I'd have expected you to reuse prepare_revs().

+       /*
        * Start a new cherry-pick/ revert sequence; but
        * first, make sure that an existing one isn't in
        * progress

Since all your new code is a special case of "Start a new cherry-pick/
revert sequence", you don't check the sequencer state in the first
place.

Cute feature, although I don't ever recall needing it personally.  Why
does this relatively esoteric "feature" belong along with the other
"maintenance patches" in  jn/maint-sequencer-fixes?

-- Ram

Hey,

Jonathan Nieder wrote:

[...]
After the recent patch "allow single-pick in the middle of cherry-pick
sequence" we don't need that hack any more.  In the new regime, a
traditional "git cherry-pick <commit>" command never looks at
.git/sequencer, so we do not need to cripple "git cherry-pick
<commit>..<commit>" for it any more.

So this is why you needed that "relatively esoteric feature" :P
This approach competes with the approach I presented in "New sequencer
workflow!" [1], where I use a special case to side-step various
sequencer state files: this approach wins on the grounds of
simplicity, and I can't see any potential long-term issues with my
limited foresight.  Do you have any other points of comparison?

[1]: https://github.com/artagnon/git sequencer #; 8a08d09b9

-- Ram

Hi again,

Jonathan Nieder wrote:

This reverts commit 95eb88d8ee588d89b4f06d2753ed4d16ab13b39f, which
was a UI experiment that did not reflect how "git reset" actually gets
used.  The reversion also fixes a test, indicated in the patch.

Looks good.

Thanks.

-- Ram

Hi,

Jonathan Nieder wrote:

That's the end.

Apart from the few minor details, I'm happy with the series overall.

 I hope the patches provided some amusement, and
advice towards making them more useful would be welcome.

Oh, yes.  There were quite a few :facepalm: moments in there for me ;)

Thanks for the enjoyable read.

-- Ram

(out of order for convenience)
Ramkumar Ramachandra wrote:

Jonathan Nieder wrote:

quoted

Suggested-by: Johannes Sixt <redacted>

Could you link to the corresponding thread with Johannes?

No, I prefer not to.  If I did a good job, the commit message would
explain enough already, and in exceptional cases, the interested
reader can look up the mailing list message the commit comes from and
walk upthread, no?

Cute feature, although I don't ever recall needing it personally.  Why
does this relatively esoteric "feature" belong along with the other
"maintenance patches" in  jn/maint-sequencer-fixes?

Read ahead in the series, or read the cover letter. :)

                             What I'm really interested in seeing is
how you persist opts for "cherry-pick --continue" when a single-commit
pick fails: in other words, how you manage to get " --continue of
single-pick respects -x" to pass.

That's a good question.  I did the lazy thing and let the existing
"git cherry-pick" logic take care of it (it writes MERGE_MSG).

quoted

+               struct commit *cmit;
+               if (prepare_revision_walk(opts->revs))
+                       die(_("revision walk setup failed"));
+               cmit = get_revision(opts->revs);
+               if (!cmit || get_revision(opts->revs))
+                       die("BUG: expected exactly one commit from walk");
+               return single_pick(cmit, opts);
+       }

I'd have expected you to reuse prepare_revs().

Why?  The purposes do not overlap much.

quoted

+       if (opts->revs->cmdline.nr == 1 &&
+           opts->revs->cmdline.rev->whence == REV_CMD_REV &&
+           opts->revs->no_walk &&
+           !opts->revs->cmdline.rev->flags) {

Yuck, seriously.
1. I'd have expected you to check opts->revs->commits, not
opts->revs->cmdline.nr.  Okay, you're using the cmdline because the
revision walk hasn't happened yet.

It would have been easy to do a revision walk and count and I'm using
the cmdline instead deliberately --- the goal really is "anything more
complicated than a simple rev on the command line should trip the
multi-pick logic".

I admit though that I'm not too familiar with the new cmdline_info
API.  I'd welcome a simpler expression with the same effect.

Also, I probably should have included a test that does some

		git cherry-pick picked^..picked

thing and verifies that this is treated as a multi-pick.  And
documented this. :)

Thanks for pointing out the questionable bits.  I am tempted to reroll
to put this after patch 6/7, which would make it possible to use "git
reset --merge" in the commit message for a more natural explanation.

That would also provide an opportunity to reuse some text from [1],
which in hindsight seems to have explained some aspects of each patch
a little more clearly.

Thanks, and hoping that clarifies a little,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/185716/focus=186811

Ramkumar Ramachandra wrote:

Sounds good.  I remember my implementation being quite complicated;
let's see how you've done this.

I have to confess that I don't remember the implementation you are
referring to.  Maybe I could have taken inspiration from it.

The rest of this message is about tests.

[...]

Jonathan Nieder wrote:

quoted

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 2c4c1c85..4d1883b7 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh

@@ -2,6 +2,7 @@

 test_description='Test cherry-pick continuation features

+ +  conflicting: rewrites unrelated to conflicting
   + yetanotherpick: rewrites foo to e
   + anotherpick: rewrites foo to d
   + picked: rewrites foo to c

Note to self: this list of commits is becoming quite unwieldy.  We
should probably refactor these sometime.

To clarify, "conflicting" is sitting on a separate branch from the
rest of the commits.  This --help text uses "git show-branch"-style
output, which is perhaps out of fashion but compact and used by some
existing tests.

quoted

@@ -27,6 +28,7 @@ test_cmp_rev () {

 }

 test_expect_success setup '
+       git config advice.detachedhead false
       echo unrelated >unrelated &&
       git add unrelated &&
       test_commit initial foo a &&

Huh, why are you moving this line up?  Oh, right: there are
"test_commit" statements in the setup- good catch.

Nah, it's for the pristine_detach, not the test_commit.

I did miss an &&, though.  Not enough to justify rerolling on its own
but seems worth fixing if resending anyway.

quoted

@@ -35,8 +37,8 @@ test_expect_success setup '

       test_commit picked foo c &&
       test_commit anotherpick foo d &&
       test_commit yetanotherpick foo e &&
-       git config advice.detachedhead false
-
+       pristine_detach initial &&
+       test_commit conflicting unrelated
 '

Looks fishy- I wonder why you're doing this.  Let's read ahead and find out.

Do you mean that you'd prefer this "conflicting" commit not to be
part of the setup shared between tests?

[...]

quoted

+test_expect_success '--continue of single revert' '
+       pristine_detach initial &&
+       echo resolved >expect &&
+       echo "Revert \"picked\"" >expect.msg &&
+       test_must_fail git revert picked &&
+       echo resolved >foo &&
+       git add foo &&
+       git cherry-pick --continue &&

Huh?  You're continuing a "git revert" with a a "git cherry-pick
--continue"?

Yep, works fine.

[...]

1. I haven't used the "-s" flag of "git diff-tree" before, so I opened
up the documentation to find this:

Yeah, that documentation sucks.  I'll keep this message marked as a
reminder to look at it.

Just like "git show" is the porcelain command to show a commit, "git
diff-tree" is the corresponding plumbing.

[...]

quoted

+test_expect_success '--continue after resolving conflicts' '

[...]

Unchanged from the original: I suspect you've moved the generation of
expectation messages up to produce a clean diff.

It's just a new test.  If rerolling, I'll make it imitate the style of
the existing test following it better.

[...]

quoted

+test_expect_success '--continue asks for help after resolving patch to nil' '
+       pristine_detach conflicting &&
+       test_must_fail git cherry-pick initial..picked &&
+
+       test_cmp_rev unrelatedpick CHERRY_PICK_HEAD &&
+       git checkout HEAD -- unrelated &&
+       test_must_fail git cherry-pick --continue 2>msg &&
+       test_i18ngrep "The previous cherry-pick is now empty" msg
+'

I thought it was a bad idea to grep for specific output messages,
because of their volatile nature?

This test is about --continue asking for help instead of succeeding or
failing in some uncontrolled way, so it seemed useful to check that
the message actually pertains to that.

Remind me what this test has to do
with the rest of your patch?

With this change in how --continue works, I wanted to make sure the
semantics that were not supposed to be changed were still intact.

[...]

quoted

+test_expect_failure 'follow advice and skip nil patch' '

[...]

Again, what does this test have to do with the rest of your patch?

Likewise.

[...]

quoted

+test_expect_success '--continue of single-pick respects -x' '

[...]

I'd have liked s/respects -x/respects opts/ here for symmetry with the
previous test.

Maybe the previous one should say "respects -x".

I am not sure what it would mean for --continue of a single-pick to
respect --strategy, for example.

quoted

+test_expect_success '--continue respects -x in first commit in multi-pick' '

[...]

Can you explain why "first commit in a multi-pick" is a special case?

I guess you mean "how does this differ from the existing '--continue
respects opts' test?".

Good question.  In the existing "--continue respects opts" test, we
explicitly run "git commit" before "git cherry-pick --continue".  This
test checks that running "git cherry-pick --continue" without
commiting first does not cause the commit message to be clobbered.

[...]

quoted

@@ -306,6 +413,32 @@ test_expect_success '--signoff is not automatically propagated to resolved confl

       grep "Signed-off-by:" anotherpick_msg
 '

+test_expect_success '--signoff dropped for implicit commit of resolution, multi-pick case' '

[...]

Unrelated.

[...]

quoted

+test_expect_success 'sign-off needs to be reaffirmed after conflict resolution, single-pick case' '

There was no implicit commit of resolution before this patch, so how
can it be unrelated?

[...]

Thanks for working on this.

Thanks for your attention to detail.

Sincerely,
Jonathan

On Sat, Dec 10, 2011 at 13:59, Jonathan Nieder [off-list ref] wrote:

       /*
+        * If we were called as "git cherry-pick <commit>", just
+        * cherry-pick/revert it, set CHERRY_PICK_HEAD /
+        * REVERT_HEAD, and don't touch the sequencer state.
+        * This means it is possible to cherry-pick in the middle
+        * of a cherry-pick sequence.
+        */
+       if (opts->revs->cmdline.nr == 1 &&
+           opts->revs->cmdline.rev->whence == REV_CMD_REV &&
+           opts->revs->no_walk &&
+           !opts->revs->cmdline.rev->flags) {
+               struct commit *cmit;
+               if (prepare_revision_walk(opts->revs))
+                       die(_("revision walk setup failed"));
+               cmit = get_revision(opts->revs);
+               if (!cmit || get_revision(opts->revs))
+                       die("BUG: expected exactly one commit from walk");
+               return single_pick(cmit, opts);
+       }
+
+       /*
        * Start a new cherry-pick/ revert sequence; but

This might be an issue introduced later in Ramkumar's code when he
moved this around, but on git.git's e5056c0 I get this:

    $ ./git revert --author=Ævar :/i18n
    fatal: BUG: expected exactly one commit from walk

That should find and revert this, right:

    $ git --no-pager log --author=Ævar --grep="i18n" -1
    commit 5eb660e
    Author: Ævar Arnfjörð Bjarmason [off-list ref]
    Date:   Sat Mar 10 12:29:35 2012 +0000

        perl/Makefile: install Git::I18N under NO_PERL_MAKEMAKER

        When I added the i18n infrastructure in v1.7.8-rc2-1-g5e9637c I forgot
        to install Git::I18N also when NO_PERL_MAKEMAKER=YesPlease was
        set. Change the generation of the fallback perl.mak file to do that.

        Now Git/I18N.pm is installed alongside Git.pm in such a way that
        anything that uses GITPERLLIB will find it.

        Reported-by: Tom G. Christensen [off-list ref]
        Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
        Signed-off-by: Junio C Hamano [off-list ref]

?

Ævar Arnfjörð Bjarmason wrote:

This might be an issue introduced later in Ramkumar's code when he
moved this around, but on git.git's e5056c0 I get this:

    $ ./git revert --author=Ævar :/i18n
    fatal: BUG: expected exactly one commit from walk

This seems buggy on two counts:

 1. The ":/" magic should probably imply --do-walk so that

	git show --author=Ævar :/i18n

    does the right thing.

 2.

	$ git cherry-pick --author=Ævar origin/pu
	fatal: BUG: expected exactly one commit from walk

   The single-pick code does not understand that such a
   simple revision specifier can return no revisions.  A more
   appropriate error message would be

	fatal: empty commit set passed

diff --git i/sequencer.c w/sequencer.c
index a37846a5..736ccd57 100644
--- i/sequencer.c
+++ w/sequencer.c

@@ -908,7 +908,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		if (prepare_revision_walk(opts->revs))
 			die(_("revision walk setup failed"));
 		cmit = get_revision(opts->revs);
-		if (!cmit || get_revision(opts->revs))
+		if (!cmit)
+			/* e.g. "git cherry-pick --author=nobody <commit>" */
+			die(_("empty commit set passed"));
+		if (get_revision(opts->revs))
 			die("BUG: expected exactly one commit from walk");
 		return single_pick(cmit, opts);
 	}