cherry-pick/revert error messages

Hi Ram,

Today I encountered the following error message:

	$ git cherry-pick foo..bar
	error: .git/sequencer already exists.
	error: A cherry-pick or revert is in progress.
	hint: Use --continue to continue the operation
	hint: or --reset to forget about it
	fatal: cherry-pick failed
	$

The double "error" seemed a little weird. Also, the capital letters and periods feel out of place, when compared with other messages emited by git with an "error:" prefix. The final "fatal: cherry-pick failed" seems redundant, too. I guess I would have expected something like the following instead:

	$ git cherry-pick foo..bar
	fatal: a cherry-pick or revert is already in progress
	hint: try "git cherry-pick (--continue | --quit)"
	$

What do you think?

Hi Jonathan,

Looks much better! Yes, a series pretty'fying error messages would be really nice. I'm busy with exams this week, and "New sequencer workflow! v2" has been pending for some time -- would you like to handle this bit?

Thanks.

-- Ram

Good to hear. Here's a rough one.

I realize patch 1/3 might not be conservative enough, even though there hasn't been much time for people to get used to --reset yet. So I might be sending a second version that treats --reset as a synonym. But please forget I said that when judging this version (i.e., if it looks like a problem, I do want to know).

Thoughts?

Jonathan Nieder (3):
revert: rename --reset option to --quit
revert: restructure pick_revisions() for clarity
revert: improve error message for cherry-pick during cherry-pick

 Documentation/git-cherry-pick.txt |    2 +-
 Documentation/git-revert.txt      |    2 +-
 Documentation/sequencer.txt       |   10 +++---
 builtin/revert.c                  |   71 ++++++++++++++++++-------------------
 sequencer.h                       |    2 +-
 t/t3510-cherry-pick-sequence.sh   |   10 +++---
 t/t7106-reset-sequence.sh         |    2 +-
 7 files changed, 49 insertions(+), 50 deletions(-)

The option to "git cherry-pick" and "git revert" to discard the sequencer state introduced by v1.7.8-rc0~141^2~6 (revert: Introduce --reset to remove sequencer state, 2011-08-04) has a confusing name. Change it now, while we still have the time.

Mechanics:

This commit removes the "git cherry-pick --reset" option. Hopefully nobody was using it. If somebody was, we can add it back again as a synonym.

The new name for "cherry-pick, please get out of my way, since I've long forgotten about the sequence of commits I was cherry-picking when you wrote that old .git/sequencer directory" is --quit. Mnemonic:
this is analagous to quiting a program the user is no longer using --- we just want to get out of the multiple-command cherry-pick procedure and not to reset HEAD or restore any other old state.

Adjust documentation and tests to match. While at it, let's clarify the short descriptions of these operations in "-h" output.

Before:

--reset forget the current operation
--continue continue the current operation

After:

--quit end revert or cherry-pick sequence
--continue resume revert or cherry-pick sequence

---
 Documentation/git-cherry-pick.txt |    2 +-
 Documentation/git-revert.txt      |    2 +-
 Documentation/sequencer.txt       |   10 +++++-----
 builtin/revert.c                  |   22 +++++++++++-----------
 sequencer.h                       |    2 +-
 t/t3510-cherry-pick-sequence.sh   |   10 +++++-----
 t/t7106-reset-sequence.sh         |    2 +-
 7 files changed, 25 insertions(+), 25 deletions(-)

index 2660a842..21998b82 100644
@@ -9,8 +9,8 @@ SYNOPSIS
 --------
 [verse]
 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
-'git cherry-pick' --reset
 'git cherry-pick' --continue
+'git cherry-pick' --quit
 
 DESCRIPTION
 -----------

index f3519413..b0fcabc8 100644
@@ -9,8 +9,8 @@ SYNOPSIS
 --------
 [verse]
 'git revert' [--edit | --no-edit] [-n] [-m parent-number] [-s] <commit>...
-'git revert' --reset
 'git revert' --continue
+'git revert' --quit
 
 DESCRIPTION
 -----------

index 3e6df338..75f8e869 100644
@@ -1,9 +1,9 @@
---reset::
-	Forget about the current operation in progress.  Can be used
-	to clear the sequencer state after a failed cherry-pick or
-	revert.
-
 --continue::
 	Continue the operation in progress using the information in
 	'.git/sequencer'.  Can be used to continue after resolving
 	conflicts in a failed cherry-pick or revert.
+
+--quit::
+	Forget about the current operation in progress.  Can be used
+	to clear the sequencer state after a failed cherry-pick or
+	revert.

index 544e8c30..b59dd68c 100644
@@ -40,7 +40,7 @@ static const char * const cherry_pick_usage[] = {
 };
 
 enum replay_action { REVERT, CHERRY_PICK };
-enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
+enum replay_subcommand { REPLAY_NONE, REPLAY_REMOVE_STATE, REPLAY_CONTINUE };
 
 struct replay_opts {
 	enum replay_action action;

@@ -133,11 +133,11 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	const char *me = action_name(opts);
-	int reset = 0;
+	int remove_state = 0;
 	int contin = 0;
 	struct option options[] = {
-		OPT_BOOLEAN(0, "reset", &reset, "forget the current operation"),
-		OPT_BOOLEAN(0, "continue", &contin, "continue the current operation"),
+		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('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),

@@ -168,13 +168,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 
 	/* Check for incompatible subcommands */
 	verify_opt_mutually_compatible(me,
-				"--reset", reset,
+				"--quit", remove_state,
 				"--continue", contin,
 				NULL);
 
 	/* Set the subcommand */
-	if (reset)
-		opts->subcommand = REPLAY_RESET;
+	if (remove_state)
+		opts->subcommand = REPLAY_REMOVE_STATE;
 	else if (contin)
 		opts->subcommand = REPLAY_CONTINUE;
 	else

@@ -183,8 +183,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	/* Check for incompatible command line arguments */
 	if (opts->subcommand != REPLAY_NONE) {
 		char *this_operation;
-		if (opts->subcommand == REPLAY_RESET)
-			this_operation = "--reset";
+		if (opts->subcommand == REPLAY_REMOVE_STATE)
+			this_operation = "--quit";
 		else
 			this_operation = "--continue";
 

@@ -965,7 +965,7 @@ static int pick_revisions(struct replay_opts *opts)
 	 * cherry-pick should be handled differently from an existing
 	 * one that is being continued
 	 */
-	if (opts->subcommand == REPLAY_RESET) {
+	if (opts->subcommand == REPLAY_REMOVE_STATE) {
 		remove_sequencer_state(1);
 		return 0;
 	} else if (opts->subcommand == REPLAY_CONTINUE) {

@@ -988,7 +988,7 @@ static int pick_revisions(struct replay_opts *opts)
 		if (create_seq_dir() < 0) {
 			error(_("A cherry-pick or revert is in progress."));
 			advise(_("Use --continue to continue the operation"));
-			advise(_("or --reset to forget about it"));
+			advise(_("or --quit to forget about it"));
 			return -1;
 		}
 		if (get_sha1("HEAD", sha1)) {

index 905d2950..f435fdb4 100644
@@ -13,7 +13,7 @@
  *
  * With the aggressive flag, it additionally removes SEQ_OLD_DIR,
  * ignoring any errors.  Inteded to be used by the sequencer's
- * '--reset' subcommand.
+ * '--quit' subcommand.
  */
 void remove_sequencer_state(int aggressive);
 

index 3bca2b3d..913435e2 100755
@@ -13,7 +13,7 @@ test_description='Test cherry-pick continuation features
 . ./test-lib.sh
 
 pristine_detach () {
-	git cherry-pick --reset &&
+	git cherry-pick --quit &&
 	git checkout -f "$1^0" &&
 	git read-tree -u --reset HEAD &&
 	git clean -d -f -f -q -x

@@ -70,15 +70,15 @@ test_expect_success 'cherry-pick cleans up sequencer state upon success' '
 	test_path_is_missing .git/sequencer
 '
 
-test_expect_success '--reset does not complain when no cherry-pick is in progress' '
+test_expect_success '--quit does not complain when no cherry-pick is in progress' '
 	pristine_detach initial &&
-	git cherry-pick --reset
+	git cherry-pick --quit
 '
 
-test_expect_success '--reset cleans up sequencer state' '
+test_expect_success '--quit cleans up sequencer state' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..picked &&
-	git cherry-pick --reset &&
+	git cherry-pick --quit &&
 	test_path_is_missing .git/sequencer
 '
 

index 4956caaf..3f86e8c5 100755
@@ -12,7 +12,7 @@ test_description='Test interaction of reset --hard with sequencer
 . ./test-lib.sh
 
 pristine_detach () {
-	git cherry-pick --reset &&
+	git cherry-pick --quit &&
 	git checkout -f "$1^0" &&
 	git read-tree -u --reset HEAD &&
 	git clean -d -f -f -q -x

1.7.8.rc3

Wouldn't it match other commands better if we called this --abort instead of --quit?

Other than that I think I agree with the reasoning (and I think I too had encountered the irritation with the "sequencer state").

Actually from what I understand --reset / --quit has to have different meaning than --abort. While for multi-commit operation --abort goes back to the state before last operation, --reset / --quit just clears sequencer state, but does not change working area, nor index, not HEAD. This is to be used when encounering stale old rebase / am / cherry-pick / revert.

BTW. I think that '--clear' (or '--clear-state') would be a better name for this option.

Jakub Narębski

Right. I'll send a strawman series that introduces a "cherry-pick --abort" option soon to clarify this.

[...]

Could you explain why? When I run "git cherry-pick --clear", I'd expect that it removes some files. Problems:

(1) It's not clear to the novice _which_ files it will remove. Maybe ".rej" files or something?

(2) That the sequencer state is stored in files is an implementation detail.

Thanks for the quick feedback, both of you.

Deal completely with "cherry-pick --quit" and --continue at the beginning of pick_revisions(), leaving the rest of the function for the more interesting "git cherry-pick <commits>" case.

No functional change intended. The impact is just to unindent the code a little.

---
This also paves the way to factoring out the REPLAY_CONTINUE case into a separate function.

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

index b59dd68c..dd072ce6 100644
@@ -968,40 +968,40 @@ static int pick_revisions(struct replay_opts *opts)
 	if (opts->subcommand == REPLAY_REMOVE_STATE) {
 		remove_sequencer_state(1);
 		return 0;
-	} else if (opts->subcommand == REPLAY_CONTINUE) {
+	}
+	if (opts->subcommand == REPLAY_CONTINUE) {
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
-			goto error;
+			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;
-	} else {
-		/*
-		 * Start a new cherry-pick/ revert sequence; but
-		 * first, make sure that an existing one isn't in
-		 * progress
-		 */
+		return pick_commits(todo_list, opts);
+	}
+
+	/*
+	 * Start a new cherry-pick/ revert sequence; but
+	 * first, make sure that an existing one isn't in
+	 * progress
+	 */
 
-		walk_revs_populate_todo(&todo_list, opts);
-		if (create_seq_dir() < 0) {
-			error(_("A cherry-pick or revert is in progress."));
-			advise(_("Use --continue to continue the operation"));
-			advise(_("or --quit to forget about it"));
-			return -1;
-		}
-		if (get_sha1("HEAD", sha1)) {
-			if (opts->action == REVERT)
-				return error(_("Can't revert as initial commit"));
-			return error(_("Can't cherry-pick into empty head"));
-		}
-		save_head(sha1_to_hex(sha1));
-		save_opts(opts);
+	walk_revs_populate_todo(&todo_list, opts);
+	if (create_seq_dir() < 0) {
+		error(_("A cherry-pick or revert is in progress."));
+		advise(_("Use --continue to continue the operation"));
+		advise(_("or --quit to forget about it"));
+		return -1;
+	}
+	if (get_sha1("HEAD", sha1)) {
+		if (opts->action == REVERT)
+			return error(_("Can't revert as initial commit"));
+		return error(_("Can't cherry-pick into empty head"));
 	}
+	save_head(sha1_to_hex(sha1));
+	save_opts(opts);
 	return pick_commits(todo_list, opts);
-error:
-	return error(_("No %s in progress"), action_name(opts));
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)

1.7.8.rc3

In the spirit of v1.6.3.3~3^2 (refuse to merge during a merge, 2009-07-01), "git cherry-pick" refuses to start a new cherry-pick when in the middle of an existing conflicted cherry-pick in the following sequence:

1. git cherry-pick HEAD..origin
2. resolve conflicts
3. git cherry-pick HEAD..origin (instead of "git cherry-pick --continue", by mistake)

Good. However, the error message on attempting step 3 is more convoluted than necessary:

  $ git cherry-pick HEAD..origin
  error: .git/sequencer already exists.
  error: A cherry-pick or revert is in progress.
  hint: Use --continue to continue the operation
  hint: or --quit to forget about it
  fatal: cherry-pick failed

Clarify by removing the double redundant first "error:" message, simplifying the advice, and using lower-case and no full stops to be consistent with other commands that prefix their messages with "error:", so it becomes

error: a cherry-pick or revert is already in progress
hint: try "git cherry-pick (--continue | --quit)"
fatal: cherry-pick failed

The "fatal: cherry-pick failed" line seems unnecessary, too, but that can be fixed some other day.

---
That's the end of the series. There are more error messages, but let's start with this one.

Thanks for reading.

 builtin/revert.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

index dd072ce6..9db4c1e4 100644
@@ -843,8 +843,11 @@ static int create_seq_dir(void)
 {
 	const char *seq_dir = git_path(SEQ_DIR);
 
-	if (file_exists(seq_dir))
-		return error(_("%s already exists."), seq_dir);
+	if (file_exists(seq_dir)) {
+		error(_("a cherry-pick or revert is already in progress"));
+		advise(_("try \"git cherry-pick (--continue | --quit)\""));
+		return -1;
+	}
 	else if (mkdir(seq_dir, 0777) < 0)
 		die_errno(_("Could not create sequencer directory %s"), seq_dir);
 	return 0;

@@ -988,12 +991,8 @@ static int pick_revisions(struct replay_opts *opts)
 	 */
 
 	walk_revs_populate_todo(&todo_list, opts);
-	if (create_seq_dir() < 0) {
-		error(_("A cherry-pick or revert is in progress."));
-		advise(_("Use --continue to continue the operation"));
-		advise(_("or --quit to forget about it"));
+	if (create_seq_dir() < 0)
 		return -1;
-	}
 	if (get_sha1("HEAD", sha1)) {
 		if (opts->action == REVERT)
 			return error(_("Can't revert as initial commit"));

1.7.8.rc3

Hi again,

Here's a less rough version. Same scope.

Patch 1 renames --reset to --quit, keeping --reset as a synonym for backward compatibility this time. I don't suspect anyone was using --reset, but splitting out the backward-incompatible changes seems like a useful habit to follow nonetheless.

Patch 2 is a small code cleanup in preparation for patch 3, unchanged from v1.

The change description for patch 3 said 'double redundant first "error:" message', which was kind of amusing. This time it says 'redundant first "error:" message'.

More notably, this version has some bonus patches:

Patch 4 introduces the REVERT_HEAD pseudoref, which is analagous to CHERRY_PICK_HEAD except it is populated by "git revert" and does not influence the behavior of "git commit". This could allow tools like __git_ps1 to detect when we are in the middle of a conflicted revert, though the series does not include a patch to do that. I'm kind of fond of it.

Patch 5 introduces a "git cherry-pick --abort" command that rolls back the series of commits made so far in a conflicted cherry-pick. Compare --quit, which tells cherry-pick to leave HEAD alone and get out of the way.

Patch 6 removes the --reset compatibility option. There might not be much reason to do that, aside from removing clutter and saving a few bytes. The alternate motivation of removing the option to prevent people from relying on it seems less compelling after patch 1 made it harder to stumble upon the option.

May the series can provide some amusement.

Thanks to Junio and Jakub for the useful feedback so far.

Jonathan Nieder (3):
revert: rename --reset option to --quit
revert: rearrange pick_revisions() for clarity
revert: improve error message for cherry-pick during cherry-pick

 Documentation/git-cherry-pick.txt |    2 +-
 Documentation/git-revert.txt      |    2 +-
 Documentation/sequencer.txt       |   10 ++--
 builtin/revert.c                  |   74 +++++++++++++++++++------------------
 sequencer.h                       |    2 +-
 t/t3510-cherry-pick-sequence.sh   |   31 +++++++++++++--
 t/t7106-reset-sequence.sh         |    2 +-
 7 files changed, 73 insertions(+), 50 deletions(-)

The option to "git cherry-pick" and "git revert" to discard the sequencer state introduced by v1.7.8-rc0~141^2~6 (revert: Introduce --reset to remove sequencer state, 2011-08-04) has a confusing name. Change it now, while we still have the time.

The new name for "cherry-pick, please get out of my way, since I've long forgotten about the sequence of commits I was cherry-picking when you wrote that old .git/sequencer directory" is --quit. Mnemonic:
this is analagous to quiting a program the user is no longer using --- we just want to get out of the multiple-command cherry-pick procedure and not to reset HEAD or rewind any other old state.

The "--reset" option is kept as a synonym to minimize the impact. We might consider dropping it for simplicity in a separate patch, though.

Adjust documentation and tests to use the newly preferred name (--quit) instead of --reset. While at it, let's clarify the short descriptions of these operations in "-h" output.

Before:

--reset forget the current operation
--continue continue the current operation

After:

--quit end revert or cherry-pick sequence
--continue resume revert or cherry-pick sequence

---
 Documentation/git-cherry-pick.txt |    2 +-
 Documentation/git-revert.txt      |    2 +-
 Documentation/sequencer.txt       |   10 +++++-----
 builtin/revert.c                  |   25 ++++++++++++++-----------
 sequencer.h                       |    2 +-
 t/t3510-cherry-pick-sequence.sh   |   31 ++++++++++++++++++++++++++-----
 t/t7106-reset-sequence.sh         |    2 +-
 7 files changed, 49 insertions(+), 25 deletions(-)

index 2660a842..21998b82 100644
@@ -9,8 +9,8 @@ SYNOPSIS
 --------
 [verse]
 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
-'git cherry-pick' --reset
 'git cherry-pick' --continue
+'git cherry-pick' --quit
 
 DESCRIPTION
 -----------

index f3519413..b0fcabc8 100644
@@ -9,8 +9,8 @@ SYNOPSIS
 --------
 [verse]
 'git revert' [--edit | --no-edit] [-n] [-m parent-number] [-s] <commit>...
-'git revert' --reset
 'git revert' --continue
+'git revert' --quit
 
 DESCRIPTION
 -----------

index 3e6df338..75f8e869 100644
@@ -1,9 +1,9 @@
---reset::
-	Forget about the current operation in progress.  Can be used
-	to clear the sequencer state after a failed cherry-pick or
-	revert.
-
 --continue::
 	Continue the operation in progress using the information in
 	'.git/sequencer'.  Can be used to continue after resolving
 	conflicts in a failed cherry-pick or revert.
+
+--quit::
+	Forget about the current operation in progress.  Can be used
+	to clear the sequencer state after a failed cherry-pick or
+	revert.

index 544e8c30..e109fb11 100644
@@ -40,7 +40,7 @@ static const char * const cherry_pick_usage[] = {
 };
 
 enum replay_action { REVERT, CHERRY_PICK };
-enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
+enum replay_subcommand { REPLAY_NONE, REPLAY_REMOVE_STATE, REPLAY_CONTINUE };
 
 struct replay_opts {
 	enum replay_action action;

@@ -133,11 +133,11 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	const char *me = action_name(opts);
-	int reset = 0;
+	int remove_state = 0;
 	int contin = 0;
 	struct option options[] = {
-		OPT_BOOLEAN(0, "reset", &reset, "forget the current operation"),
-		OPT_BOOLEAN(0, "continue", &contin, "continue the current operation"),
+		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('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),

@@ -147,6 +147,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_STRING(0, "strategy", &opts->strategy, "strategy", "merge strategy"),
 		OPT_CALLBACK('X', "strategy-option", &opts, "option",
 			"option for merge strategy", option_parse_x),
+		{ OPTION_BOOLEAN, 0, "reset", &remove_state, NULL,
+			"alias for --quit (deprecated)",
+			PARSE_OPT_HIDDEN | PARSE_OPT_NOARG },
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),

@@ -168,13 +171,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 
 	/* Check for incompatible subcommands */
 	verify_opt_mutually_compatible(me,
-				"--reset", reset,
+				"--quit", remove_state,
 				"--continue", contin,
 				NULL);
 
 	/* Set the subcommand */
-	if (reset)
-		opts->subcommand = REPLAY_RESET;
+	if (remove_state)
+		opts->subcommand = REPLAY_REMOVE_STATE;
 	else if (contin)
 		opts->subcommand = REPLAY_CONTINUE;
 	else

@@ -183,8 +186,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	/* Check for incompatible command line arguments */
 	if (opts->subcommand != REPLAY_NONE) {
 		char *this_operation;
-		if (opts->subcommand == REPLAY_RESET)
-			this_operation = "--reset";
+		if (opts->subcommand == REPLAY_REMOVE_STATE)
+			this_operation = "--quit";
 		else
 			this_operation = "--continue";
 

@@ -965,7 +968,7 @@ static int pick_revisions(struct replay_opts *opts)
 	 * cherry-pick should be handled differently from an existing
 	 * one that is being continued
 	 */
-	if (opts->subcommand == REPLAY_RESET) {
+	if (opts->subcommand == REPLAY_REMOVE_STATE) {
 		remove_sequencer_state(1);
 		return 0;
 	} else if (opts->subcommand == REPLAY_CONTINUE) {

@@ -988,7 +991,7 @@ static int pick_revisions(struct replay_opts *opts)
 		if (create_seq_dir() < 0) {
 			error(_("A cherry-pick or revert is in progress."));
 			advise(_("Use --continue to continue the operation"));
-			advise(_("or --reset to forget about it"));
+			advise(_("or --quit to forget about it"));
 			return -1;
 		}
 		if (get_sha1("HEAD", sha1)) {

index 905d2950..f435fdb4 100644
@@ -13,7 +13,7 @@
  *
  * With the aggressive flag, it additionally removes SEQ_OLD_DIR,
  * ignoring any errors.  Inteded to be used by the sequencer's
- * '--reset' subcommand.
+ * '--quit' subcommand.
  */
 void remove_sequencer_state(int aggressive);
 

index 3bca2b3d..bb67cdcb 100755
@@ -13,7 +13,7 @@ test_description='Test cherry-pick continuation features
 . ./test-lib.sh
 
 pristine_detach () {
-	git cherry-pick --reset &&
+	git cherry-pick --quit &&
 	git checkout -f "$1^0" &&
 	git read-tree -u --reset HEAD &&
 	git clean -d -f -f -q -x

@@ -70,18 +70,39 @@ test_expect_success 'cherry-pick cleans up sequencer state upon success' '
 	test_path_is_missing .git/sequencer
 '
 
-test_expect_success '--reset does not complain when no cherry-pick is in progress' '
+test_expect_success '--quit does not complain when no cherry-pick is in progress' '
 	pristine_detach initial &&
-	git cherry-pick --reset
+	git cherry-pick --quit
 '
 
-test_expect_success '--reset cleans up sequencer state' '
+test_expect_success '--quit cleans up sequencer state' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..picked &&
-	git cherry-pick --reset &&
+	git cherry-pick --quit &&
 	test_path_is_missing .git/sequencer
 '
 
+test_expect_success 'cherry-pick --reset (another name for --quit)' '
+	pristine_detach initial &&
+	cat >expect <<-\EOF &&
+	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..picked &&
+	git cherry-pick --reset &&
+	test_path_is_missing .git/sequencer &&
+	test_must_fail git update-index --refresh &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	test_cmp expect actual
+'
+
 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 &&

index 4956caaf..3f86e8c5 100755
@@ -12,7 +12,7 @@ test_description='Test interaction of reset --hard with sequencer
 . ./test-lib.sh
 
 pristine_detach () {
-	git cherry-pick --reset &&
+	git cherry-pick --quit &&
 	git checkout -f "$1^0" &&
 	git read-tree -u --reset HEAD &&
 	git clean -d -f -f -q -x

1.7.8.rc3

Deal completely with "cherry-pick --quit" and --continue at the beginning of pick_revisions(), leaving the rest of the function for the more interesting "git cherry-pick <commits>" case.

No functional change intended. The impact is just to unindent the code a little.

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

index e109fb11..2346cafb 100644
@@ -971,40 +971,40 @@ static int pick_revisions(struct replay_opts *opts)
 	if (opts->subcommand == REPLAY_REMOVE_STATE) {
 		remove_sequencer_state(1);
 		return 0;
-	} else if (opts->subcommand == REPLAY_CONTINUE) {
+	}
+	if (opts->subcommand == REPLAY_CONTINUE) {
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
-			goto error;
+			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;
-	} else {
-		/*
-		 * Start a new cherry-pick/ revert sequence; but
-		 * first, make sure that an existing one isn't in
-		 * progress
-		 */
+		return pick_commits(todo_list, opts);
+	}
+
+	/*
+	 * Start a new cherry-pick/ revert sequence; but
+	 * first, make sure that an existing one isn't in
+	 * progress
+	 */
 
-		walk_revs_populate_todo(&todo_list, opts);
-		if (create_seq_dir() < 0) {
-			error(_("A cherry-pick or revert is in progress."));
-			advise(_("Use --continue to continue the operation"));
-			advise(_("or --quit to forget about it"));
-			return -1;
-		}
-		if (get_sha1("HEAD", sha1)) {
-			if (opts->action == REVERT)
-				return error(_("Can't revert as initial commit"));
-			return error(_("Can't cherry-pick into empty head"));
-		}
-		save_head(sha1_to_hex(sha1));
-		save_opts(opts);
+	walk_revs_populate_todo(&todo_list, opts);
+	if (create_seq_dir() < 0) {
+		error(_("A cherry-pick or revert is in progress."));
+		advise(_("Use --continue to continue the operation"));
+		advise(_("or --quit to forget about it"));
+		return -1;
+	}
+	if (get_sha1("HEAD", sha1)) {
+		if (opts->action == REVERT)
+			return error(_("Can't revert as initial commit"));
+		return error(_("Can't cherry-pick into empty head"));
 	}
+	save_head(sha1_to_hex(sha1));
+	save_opts(opts);
 	return pick_commits(todo_list, opts);
-error:
-	return error(_("No %s in progress"), action_name(opts));
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)

1.7.8.rc3

In the spirit of v1.6.3.3~3^2 (refuse to merge during a merge, 2009-07-01), "git cherry-pick" refuses to start a new cherry-pick when in the middle of an existing conflicted cherry-pick in the following sequence:

1. git cherry-pick HEAD..origin
2. resolve conflicts
3. git cherry-pick HEAD..origin (instead of "git cherry-pick --continue", by mistake)

Good. However, the error message on attempting step 3 is more convoluted than necessary:

  $ git cherry-pick HEAD..origin
  error: .git/sequencer already exists.
  error: A cherry-pick or revert is in progress.
  hint: Use --continue to continue the operation
  hint: or --quit to forget about it
  fatal: cherry-pick failed

Clarify by removing the redundant first "error:" message, simplifying the advice, and using lower-case and no full stops to be consistent with other commands that prefix their messages with "error:", so it becomes

error: a cherry-pick or revert is already in progress
hint: try "git cherry-pick (--continue | --quit)"
fatal: cherry-pick failed

The "fatal: cherry-pick failed" line seems unnecessary, too, but that can be fixed some other day.

---
 builtin/revert.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

index 2346cafb..1d112e4c 100644
@@ -846,8 +846,11 @@ static int create_seq_dir(void)
 {
 	const char *seq_dir = git_path(SEQ_DIR);
 
-	if (file_exists(seq_dir))
-		return error(_("%s already exists."), seq_dir);
+	if (file_exists(seq_dir)) {
+		error(_("a cherry-pick or revert is already in progress"));
+		advise(_("try \"git cherry-pick (--continue | --quit)\""));
+		return -1;
+	}
 	else if (mkdir(seq_dir, 0777) < 0)
 		die_errno(_("Could not create sequencer directory %s"), seq_dir);
 	return 0;

@@ -991,12 +994,8 @@ static int pick_revisions(struct replay_opts *opts)
 	 */
 
 	walk_revs_populate_todo(&todo_list, opts);
-	if (create_seq_dir() < 0) {
-		error(_("A cherry-pick or revert is in progress."));
-		advise(_("Use --continue to continue the operation"));
-		advise(_("or --quit to forget about it"));
+	if (create_seq_dir() < 0)
 		return -1;
-	}
 	if (get_sha1("HEAD", sha1)) {
 		if (opts->action == REVERT)
 			return error(_("Can't revert as initial commit"));

1.7.8.rc3

When conflicts are encountered while reverting a commit, it can be handy to have the name of that commit easily available. For example, to produce a copy of the patch to refer to while resolving conflicts:

	$ git revert 2eceb2a8
	error: could not revert 2eceb2a8... awesome, buggy feature
	$ git show -R REVERT_HEAD >the-patch
	$ edit $(git diff --name-only)

Set a REVERT_HEAD pseudoref when "git revert" does not make a commit, for cases like this. This also makes it possible for scripts to distinguish between a revert that encountered conflicts and other sources of an unmerged index.

After successfully committing, resetting with "git reset", or moving to another commit with "git checkout" or "git reset", the pseudoref is no longer useful, so remove it.

---
 branch.c                        |    1 +
 builtin/commit.c                |    1 +
 builtin/revert.c                |    8 +++--
 t/t3507-cherry-pick-conflict.sh |   54 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 61 insertions(+), 3 deletions(-)

index d8098762..025a97be 100644
@@ -241,6 +241,7 @@ void create_branch(const char *head,
 void remove_branch_state(void)
 {
 	unlink(git_path("CHERRY_PICK_HEAD"));
+	unlink(git_path("REVERT_HEAD"));
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_RR"));
 	unlink(git_path("MERGE_MSG"));

index c46f2d18..8f2bebec 100644
@@ -1514,6 +1514,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	}
 
 	unlink(git_path("CHERRY_PICK_HEAD"));
+	unlink(git_path("REVERT_HEAD"));
 	unlink(git_path("MERGE_HEAD"));
 	unlink(git_path("MERGE_MSG"));
 	unlink(git_path("MERGE_MODE"));

index 1d112e4c..f5ba67a5 100644
@@ -289,7 +289,7 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-static void write_cherry_pick_head(struct commit *commit)
+static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
 {
 	const char *filename;
 	int fd;

@@ -297,7 +297,7 @@ static void write_cherry_pick_head(struct commit *commit)
 
 	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
 
-	filename = git_path("CHERRY_PICK_HEAD");
+	filename = git_path(pseudoref);
 	fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"), filename);

@@ -597,7 +597,9 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * write it at all.
 	 */
 	if (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1))
-		write_cherry_pick_head(commit);
+		write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
+	if (opts->action == REVERT && ((opts->no_commit && res == 0) || res == 1))
+		write_cherry_pick_head(commit, "REVERT_HEAD");
 
 	if (res) {
 		error(opts->action == REVERT

1.7.8.rc3

There seems to be something wrong with the subject. I says [4/3], instead of [4/6]?

Hi,

Sounds a lot like CHERRY_PICK_HEAD counterpart. Let's read ahead and see if there are any unexpected differences.

Interesting. This additional symmetry will probably make life much easier for my pending "New Sequencer Workflow! v2" series.

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.

---
 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   |  102 ++++++++++++++++++++++++++++++++++++-
 5 files changed, 189 insertions(+), 5 deletions(-)

index 21998b82..fed5097e 100644
@@ -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
 -----------

index b0fcabc8..b699a345 100644
@@ -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
 -----------

index 75f8e869..5747f442 100644
@@ -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.

1.7.8.rc3

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.

---
 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(-)

index 21998b82..fed5097e 100644
@@ -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
 -----------

index b0fcabc8..b699a345 100644
@@ -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
 -----------

index 75f8e869..5747f442 100644
@@ -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.

1.7.8.rc3

From: Johannes Sixt <[email protected]>

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.

---
I guess it's too late to squash this in. ;)

-- Hannes

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

index 0c61668..5dedb51 100644
@@ -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

Nice catch.

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.

---
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(-)

index 5dedb51c..818b4abb 100644
@@ -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:

index 3f86e8c5..83f7ea59 100755
@@ -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:

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

(-cc: Phil)

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.

---
 builtin/revert.c |   28 +++++++++++++++++-----------
 1 files changed, 17 insertions(+), 11 deletions(-)

index 1ea525c1..9f6c85c1 100644
@@ -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

Hi Jonathan,

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

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

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(-)

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

index 9f6c85c1..a43b4d85 100644
@@ -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);
 }
 

1.7.8.rc3

Hi again,

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

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

Oh, good -- lots of new tests :)

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.

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.

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?

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

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

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

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

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.

[...]

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.

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.

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

[...]

Yep, works fine.

[...]

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.

[...]

[...]

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

[...]

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.

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

[...]

[...]

Likewise.

[...]

[...]

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.

[...]

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.

[...]

[...]

[...]

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

[...]

Thanks for your attention to detail.

Sincerely,
Jonathan

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.

---
 builtin/revert.c                |   53 +++++++++++++++++++++-----------------
 t/t3510-cherry-pick-sequence.sh |    5 +++
 2 files changed, 34 insertions(+), 24 deletions(-)

index a43b4d85..71570357 100644
@@ -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);
 
 	/*

index 4d1883b7..56c95ec1 100755
@@ -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

Hey,

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

-- Ram

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.

---
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(-)

index 71570357..dcb69904 100644
@@ -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

index 56c95ec1..98a27a23 100755
@@ -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

Hi,

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.

Could you link to the corresponding thread with Johannes?

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

(out of order for convenience)

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?

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

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).

Why? The purposes do not overlap much.

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

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 <[email protected]>
    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.

?

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

index a37846a5..736ccd57 100644
@@ -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);
 	}

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.

---
 builtin/revert.c                |   12 +-----------
 t/t3510-cherry-pick-sequence.sh |    6 +++---
 2 files changed, 4 insertions(+), 14 deletions(-)

index dcb69904..28deb85b 100644
@@ -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;
-		}
 	}
 
 	/*

index 98a27a23..851b147f 100755
@@ -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

Hey,

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

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.

---
 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

index 025a97be..a6b6722e 100644
@@ -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);
 }

index 851b147f..e80050e1 100755
@@ -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 &&
 

1.7.8.rc3

Hi again,

Looks good.

Thanks.

-- Ram

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.

---
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(-)

index 28deb85b..444df413 100644
@@ -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)

index bc2c046a..d1f28a69 100644
@@ -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);
 }

index f435fdb4..2d4528f2 100644
@@ -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

Hi,

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

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

Thanks for the enjoyable read.

-- Ram

[... 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

Thanks for catching this. Let's review this quickly and queue it for maintenance track as necessary.

Overall, I think this series is a more correct and faithful implementation of the design we discussed earlier in the thread

http://thread.gmane.org/gmane.comp.version-control.git/179304/focus=179625

I saw a few minor nits in the log messages but otherwise nothing objectionable jumped at me from my initial reading of the series.

Thanks.

I was tempted to send a reroll with slightly better log messages once I could see your corrections, but it looks like the series has been merged to "next" and your corrections already leave me happy enough.

Thanks for looking it over.

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 :)

This is confusing; if you have many commits in the range, and a handful of them replayed without conflicts and then you hit a conflict, where should (I am not asking "where does ... with your patch") abort take us? The state after the random commit that happened to have replayed successfully? The state before the entire cherry-pick sequence started? "back on solid ground" does not tell us which one you meant.

I am assuming that it is the latter.

Remove the "git cherry-pick --reset" option, which has a different preferred spelling nowadays ("--quit"). Luckily the old --reset name was not around long enough for anyone to get used to it.

---
Thanks for reading.

 builtin/revert.c                |    3 ---
 t/t3510-cherry-pick-sequence.sh |    4 ++--
 2 files changed, 2 insertions(+), 5 deletions(-)

index 70a5fbb6..0c61668b 100644
@@ -154,9 +154,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_STRING(0, "strategy", &opts->strategy, "strategy", "merge strategy"),
 		OPT_CALLBACK('X', "strategy-option", &opts, "option",
 			"option for merge strategy", option_parse_x),
-		{ OPTION_BOOLEAN, 0, "reset", &remove_state, NULL,
-			"alias for --quit (deprecated)",
-			PARSE_OPT_HIDDEN | PARSE_OPT_NOARG },
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),

index 63297ad6..f28a4e8a 100755
@@ -94,7 +94,7 @@ test_expect_success '--quit cleans up sequencer state' '
 	test_path_is_missing .git/sequencer
 '
 
-test_expect_success 'cherry-pick --reset (another name for --quit)' '
+test_expect_success '--quit keeps HEAD and conflicted index intact' '
 	pristine_detach initial &&
 	cat >expect <<-\EOF &&
 	OBJID

@@ -105,7 +105,7 @@ test_expect_success 'cherry-pick --reset (another name for --quit)' '
 	:000000 100644 OBJID OBJID A	unrelated
 	EOF
 	test_must_fail git cherry-pick base..picked &&
-	git cherry-pick --reset &&
+	git cherry-pick --quit &&
 	test_path_is_missing .git/sequencer &&
 	test_must_fail git update-index --refresh &&
 	{

1.7.8.rc3

If we want to reach this point eventually, given that --reset was not in any released version, and also given that we are deep in pre-release phase, we probably should either just apply the first one (and perhaps this one) from the series immediately before 1.7.8 final, or delay the final and swallow the entire series.

I am not quite convinced that quit is *that* superiour over reset though. The description in 1/3 "has a confusing name" does not say why it is confusing, either, to help readers agree with the conclusion.

Makes sense.

Ok, a few words about this.

I should say that I am not deeply attached to --quit. If some other word for "abandon" (or some other concept entirely) is more obvious, I'd be happy to see it used. From a couple of days of practice, I can say that --quit feels fine in a way that --reset didn't, but not much more.

--reset is a cognate to "git reset", the command to make some combination of the HEAD, index, and worktree just so. In other contexts --- rebooting a machine, resetting an alarm clock --- to reset something also means to return it to a known state. By contrast, the "git cherry-pick --abandon" command is about leaving the state alone as much as possible while escaping from the context of a cherry-pick sequence.

How does that do harm, though? My main fear is that people would not notice that --reset is the option they want to use and would opt to "rm -r .git/sequencer" instead, which is a kind of error-prone wizardry that should not be required.

(I assume cherry-pick --reset was named by analogy with "git bisect reset", the command to abandon a bisection and return to the branch you were on before bisection, or a branch or commit named on the command line. Which is not analagous to cherry-pick --abandon at all. The command people often run, "git bisect reset HEAD", is a closer analog if you squint right.)

Ok, then let's do the latter, and make tomorrow's pushout a 1.7.8-rc4; I read the series over and they seem to make sense.

Errata:

"Maybe".

[...]

I took these from pu before tweaking them, so in a sense it's appropriate that Junio's sign-off is there, but I do not mean to claim he looked at and acked the tweaked form. I should have just left my sign-off.

Sorry for the noise.