cherry-pick/revert error messages

cherry-pick/revert error messages

[Jonathan Nieder]

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?

Re: cherry-pick/revert error messages

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder wrote:
> Today I encountered the following error message:
> [...]

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

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

[RFC/PATCH 0/3] Re: cherry-pick/revert error messages

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Looks much better!  Yes, a series pretty'fying error messages would be
> really nice.

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

[PATCH 1/3] revert: rename --reset option to --quit

[Jonathan Nieder]

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

Noticed-by: Phil Hord <phil.hord@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 2660a842..21998b82 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -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
 -----------
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index f3519413..b0fcabc8 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -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
 -----------
diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
index 3e6df338..75f8e869 100644
--- a/Documentation/sequencer.txt
+++ b/Documentation/sequencer.txt
@@ -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.
diff --git a/builtin/revert.c b/builtin/revert.c
index 544e8c30..b59dd68c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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)) {
diff --git a/sequencer.h b/sequencer.h
index 905d2950..f435fdb4 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -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);
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 3bca2b3d..913435e2 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -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
 '
 
diff --git a/t/t7106-reset-sequence.sh b/t/t7106-reset-sequence.sh
index 4956caaf..3f86e8c5 100755
--- a/t/t7106-reset-sequence.sh
+++ b/t/t7106-reset-sequence.sh
@@ -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

[PATCH 2/3] revert: rearrange pick_revisions() for clarity

[Jonathan Nieder]

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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index b59dd68c..dd072ce6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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

[PATCH 3/3] revert: improve error message for cherry-pick during cherry-pick

[Jonathan Nieder]

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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index dd072ce6..9db4c1e4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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

Re: [PATCH 1/3] revert: rename --reset option to --quit

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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

Re: [PATCH 1/3] revert: rename --reset option to --quit

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:
> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > 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.  
> 
> Wouldn't it match other commands better if we called this --abort instead
> of --quit?

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.
 
> Other than that I think I agree with the reasoning (and I think I too had
> encountered the irritation with the "sequencer state").

-- 
Jakub Narębski

Re: [PATCH 1/3] revert: rename --reset option to --quit

[Jonathan Nieder]

Jakub Narebski wrote:

> Actually from what I understand --reset / --quit has to have different
> meaning than --abort.

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

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

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.

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

Thanks for the quick feedback, both of you.

[PATCH v2 0/3] Re: cherry-pick/revert error messages

[Jonathan Nieder]

Hi again,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:

>> Looks much better!  Yes, a series pretty'fying error messages would be
>> really nice.
>
> Good to hear.  Here's a rough one.

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

[PATCH 1/3] revert: rename --reset option to --quit

[Jonathan Nieder]

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

Noticed-by: Phil Hord <phil.hord@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 2660a842..21998b82 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -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
 -----------
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index f3519413..b0fcabc8 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -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
 -----------
diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
index 3e6df338..75f8e869 100644
--- a/Documentation/sequencer.txt
+++ b/Documentation/sequencer.txt
@@ -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.
diff --git a/builtin/revert.c b/builtin/revert.c
index 544e8c30..e109fb11 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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)) {
diff --git a/sequencer.h b/sequencer.h
index 905d2950..f435fdb4 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -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);
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 3bca2b3d..bb67cdcb 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -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 &&
diff --git a/t/t7106-reset-sequence.sh b/t/t7106-reset-sequence.sh
index 4956caaf..3f86e8c5 100755
--- a/t/t7106-reset-sequence.sh
+++ b/t/t7106-reset-sequence.sh
@@ -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

[PATCH 2/3] revert: rearrange pick_revisions() for clarity

[Jonathan Nieder]

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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c |   48 ++++++++++++++++++++++++------------------------
 1 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index e109fb11..2346cafb 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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

[PATCH 3/3] revert: improve error message for cherry-pick during cherry-pick

[Jonathan Nieder]

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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/revert.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 2346cafb..1d112e4c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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

[PATCH 4/3] revert: write REVERT_HEAD pseudoref during conflicted revert

[Jonathan Nieder]

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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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(-)

diff --git a/branch.c b/branch.c
index d8098762..025a97be 100644
--- a/branch.c
+++ b/branch.c
@@ -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"));
diff --git a/builtin/commit.c b/builtin/commit.c
index c46f2d18..8f2bebec 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -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"));
diff --git a/builtin/revert.c b/builtin/revert.c
index 1d112e4c..f5ba67a5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index cb45574a..ee1659c1 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -253,6 +253,60 @@ test_expect_success 'revert also handles conflicts sanely' '
 	test_cmp expected actual
 '
 
+test_expect_success 'failed revert sets REVERT_HEAD' '
+	pristine_detach initial &&
+	test_must_fail git revert picked &&
+	test_cmp_rev picked REVERT_HEAD
+'
+
+test_expect_success 'successful revert does not set REVERT_HEAD' '
+	pristine_detach base &&
+	git revert base &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
+	test_must_fail git rev-parse --verify REVERT_HEAD
+'
+
+test_expect_success 'revert --no-commit sets REVERT_HEAD' '
+	pristine_detach base &&
+	git revert --no-commit base &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
+	test_cmp_rev base REVERT_HEAD
+'
+
+test_expect_success 'revert w/dirty tree does not set REVERT_HEAD' '
+	pristine_detach base &&
+	echo foo > foo &&
+	test_must_fail git revert base &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
+	test_must_fail git rev-parse --verify REVERT_HEAD
+'
+
+test_expect_success 'GIT_CHERRY_PICK_HELP does not suppress REVERT_HEAD' '
+	pristine_detach initial &&
+	(
+		GIT_CHERRY_PICK_HELP="and then do something else" &&
+		GIT_REVERT_HELP="and then do something else, again" &&
+		export GIT_CHERRY_PICK_HELP GIT_REVERT_HELP &&
+		test_must_fail git revert picked
+	) &&
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
+	test_cmp_rev picked REVERT_HEAD
+'
+
+test_expect_success 'git reset clears REVERT_HEAD' '
+	pristine_detach initial &&
+	test_must_fail git revert picked &&
+	git reset &&
+	test_must_fail git rev-parse --verify REVERT_HEAD
+'
+
+test_expect_success 'failed commit does not clear REVERT_HEAD' '
+	pristine_detach initial &&
+	test_must_fail git revert picked &&
+	test_must_fail git commit &&
+	test_cmp_rev picked REVERT_HEAD
+'
+
 test_expect_success 'revert conflict, diff3 -m style' '
 	pristine_detach initial &&
 	git config merge.conflictstyle diff3 &&
-- 
1.7.8.rc3

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

[Jonathan Nieder]

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 <jrnieder@gmail.com>
---
 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(-)

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..63297ad6 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -6,7 +6,7 @@ test_description='Test cherry-pick continuation features
   + picked: rewrites foo to c
   + unrelatedpick: rewrites unrelated to reallyunrelated
   + base: rewrites foo to b
-  + initial: writes foo as a, unrelated as unrelated
+  + initial: writes foo as a, unrelated as unrelated, bar as bar
 
 '
 
@@ -19,9 +19,16 @@ 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 &&
+	echo bar >bar &&
+	git add unrelated bar &&
 	test_commit initial foo a &&
 	test_commit base foo b &&
 	test_commit unrelatedpick unrelated reallyunrelated &&
@@ -75,6 +82,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 &&
@@ -88,6 +100,7 @@ test_expect_success 'cherry-pick --reset (another name for --quit)' '
 	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
@@ -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 initial &&
+	echo changed >expect &&
+	test_must_fail git cherry-pick base..anotherpick &&
+	echo changed >bar &&
+	git cherry-pick --abort &&
+	test_cmp expect bar
+'
+
+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 &&
@@ -121,12 +207,23 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le
 	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
 	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 &&
@@ -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
-- 
1.7.8.rc3

[PATCH 6/3] revert: remove --reset compatibility option

[Jonathan Nieder]

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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for reading.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 70a5fbb6..0c61668b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -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(),
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 63297ad6..f28a4e8a 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -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

Re: [PATCH v2 0/3] Re: cherry-pick/revert error messages

[Jonathan Nieder]

Errata:

Jonathan Nieder wrote:

> May the series can provide some amusement.

"Maybe".

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

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.

Re: [PATCH 4/3] revert: write REVERT_HEAD pseudoref during conflicted revert

[Thiago Farina]

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

On Tue, Nov 22, 2011 at 9:17 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> 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.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  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(-)
>
> diff --git a/branch.c b/branch.c
> index d8098762..025a97be 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -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"));
> diff --git a/builtin/commit.c b/builtin/commit.c
> index c46f2d18..8f2bebec 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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"));
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 1d112e4c..f5ba67a5 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -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
> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index cb45574a..ee1659c1 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -253,6 +253,60 @@ test_expect_success 'revert also handles conflicts sanely' '
>        test_cmp expected actual
>  '
>
> +test_expect_success 'failed revert sets REVERT_HEAD' '
> +       pristine_detach initial &&
> +       test_must_fail git revert picked &&
> +       test_cmp_rev picked REVERT_HEAD
> +'
> +
> +test_expect_success 'successful revert does not set REVERT_HEAD' '
> +       pristine_detach base &&
> +       git revert base &&
> +       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
> +       test_must_fail git rev-parse --verify REVERT_HEAD
> +'
> +
> +test_expect_success 'revert --no-commit sets REVERT_HEAD' '
> +       pristine_detach base &&
> +       git revert --no-commit base &&
> +       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
> +       test_cmp_rev base REVERT_HEAD
> +'
> +
> +test_expect_success 'revert w/dirty tree does not set REVERT_HEAD' '
> +       pristine_detach base &&
> +       echo foo > foo &&
> +       test_must_fail git revert base &&
> +       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
> +       test_must_fail git rev-parse --verify REVERT_HEAD
> +'
> +
> +test_expect_success 'GIT_CHERRY_PICK_HELP does not suppress REVERT_HEAD' '
> +       pristine_detach initial &&
> +       (
> +               GIT_CHERRY_PICK_HELP="and then do something else" &&
> +               GIT_REVERT_HELP="and then do something else, again" &&
> +               export GIT_CHERRY_PICK_HELP GIT_REVERT_HELP &&
> +               test_must_fail git revert picked
> +       ) &&
> +       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
> +       test_cmp_rev picked REVERT_HEAD
> +'
> +
> +test_expect_success 'git reset clears REVERT_HEAD' '
> +       pristine_detach initial &&
> +       test_must_fail git revert picked &&
> +       git reset &&
> +       test_must_fail git rev-parse --verify REVERT_HEAD
> +'
> +
> +test_expect_success 'failed commit does not clear REVERT_HEAD' '
> +       pristine_detach initial &&
> +       test_must_fail git revert picked &&
> +       test_must_fail git commit &&
> +       test_cmp_rev picked REVERT_HEAD
> +'
> +
>  test_expect_success 'revert conflict, diff3 -m style' '
>        pristine_detach initial &&
>        git config merge.conflictstyle diff3 &&
> --
> 1.7.8.rc3
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 6/3] revert: remove --reset compatibility option

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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.

Re: [PATCH 6/3] revert: remove --reset compatibility option

[Jonathan Nieder]

Junio C Hamano wrote:

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

Makes sense.

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

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

Re: [PATCH 6/3] revert: remove --reset compatibility option

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> 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.
>
> Makes sense.

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.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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 <jrnieder@gmail.com>
> ---
> ...
> @@ -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?  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.

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> ...
>> @@ -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 <jrnieder@gmail.com>
---
 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

[PATCH] Fix revert --abort on Windows

[Johannes Sixt]

From: Johannes Sixt <j6t@kdbg.org>

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 <j6t@kdbg.org>
---
 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

Re: [PATCH] Fix revert --abort on Windows

[Jonathan Nieder]

Johannes Sixt wrote:

> From: Johannes Sixt <j6t@kdbg.org>
>
> 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 <jrnieder@gmail.com>

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 <jrnieder@gmail.com>
---
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

Re: [PATCH] Fix revert --abort on Windows

[Johannes Sixt]

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

Re: [PATCH] Fix revert --abort on Windows

[Alex Riesen]

On Wed, Nov 23, 2011 at 09:49, Johannes Sixt <j.sixt@viscovery.net> wrote:
> From: Johannes Sixt <j6t@kdbg.org>
>
> 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 <j6t@kdbg.org>
> ---
>  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 :)

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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.

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

Ok, it is the latter.

> +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);
> +}

So you give the value of the HEAD before the sequence started to this
function and all should go well. Where do you read that value from?

> +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"));

Ok, this is for single-pick so HEAD is where we came from. Good.

> +	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;
> +	}

And when we are in multi-pick, SEQ_HEAD_FILE has it.

Looks good from a cursory review. Thanks.

Re: [PATCH 4/3] revert: write REVERT_HEAD pseudoref during conflicted revert

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> [...]
> 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.

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

> diff --git a/branch.c b/branch.c
> index d8098762..025a97be 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -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"));
> diff --git a/builtin/commit.c b/builtin/commit.c
> index c46f2d18..8f2bebec 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -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"));

So far so good.  Behaves exactly like CHERRY_PICK_HEAD.

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 1d112e4c..f5ba67a5 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -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

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

> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index cb45574a..ee1659c1 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> [...]

And some tests.  Nice.

Thanks.

-- Ram

[RFC/PATCH 0/7] some sequencer loose ends (Re: Fix revert --abort on Windows)

[Jonathan Nieder]

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

[PATCH 1/7] revert: give --continue handling its own function

[Jonathan Nieder]

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

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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

[PATCH 2/7] revert: allow cherry-pick --continue to commit before resuming

[Jonathan Nieder]

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 <jrnieder@gmail.com>
---
 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

[PATCH 3/7] revert: pass around rev-list args in already-parsed form

[Jonathan Nieder]

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 <jrnieder@gmail.com>
Improved-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 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

[PATCH 4/7] revert: allow single-pick in the middle of cherry-pick sequence

[Jonathan Nieder]

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 <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
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

[PATCH 5/7] revert: do not remove state until sequence is finished

[Jonathan Nieder]

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 <jrnieder@gmail.com>
---
 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

[PATCH 6/7] Revert "reset: Make reset remove the sequencer state"

[Jonathan Nieder]

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 <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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

[PATCH 7/7] revert: stop creating and removing sequencer-old directory

[Jonathan Nieder]

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 <jrnieder@gmail.com>
---
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

Re: [RFC/PATCH 0/7] some sequencer loose ends (Re: Fix revert --abort on Windows)

[Jonathan Nieder]

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

Re: [RFC/PATCH 0/7] some sequencer loose ends (Re: Fix revert --abort on Windows)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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

Re: [RFC/PATCH 0/7] some sequencer loose ends (Re: Fix revert --abort on Windows)

[Junio C Hamano]

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.

Re: [RFC/PATCH 0/7] some sequencer loose ends (Re: Fix revert --abort on Windows)

[Jonathan Nieder]

Junio C Hamano wrote:

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

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.

Re: [PATCH 1/7] revert: give --continue handling its own function

[Ramkumar Ramachandra]

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.

> 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)
> [...]
> +static int sequencer_continue(struct replay_opts *opts)
> +{
> +       struct commit_list *todo_list = NULL;
> [...]

>  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

Re: [PATCH 2/7] revert: allow cherry-pick --continue to commit before resuming

[Ramkumar Ramachandra]

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

> 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);
> +}

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.

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

> @@ -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.  This is unrelated
to your patch and should be a separate commit though.

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

> @@ -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
> +'

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?

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

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?

> @@ -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
> +'

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

Re: [PATCH 3/7] revert: pass around rev-list args in already-parsed form

[Ramkumar Ramachandra]

Hey,

Jonathan Nieder wrote:
> [...]
> Original patch by Jonathan, tweaks and test from Ram.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Improved-by: Ramkumar Ramachandra <artagnon@gmail.com>

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

-- Ram

Re: [PATCH 4/7] revert: allow single-pick in the middle of cherry-pick sequence

[Ramkumar Ramachandra]

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 <j6t@kdbg.org>

Could you link to the corresponding thread with Johannes?

> 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);
> +}

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

>  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.
> +        */

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.

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

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

Re: [PATCH 5/7] revert: do not remove state until sequence is finished

[Ramkumar Ramachandra]

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

Re: [PATCH 6/7] Revert "reset: Make reset remove the sequencer state"

[Ramkumar Ramachandra]

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

Re: [PATCH 7/7] revert: stop creating and removing sequencer-old directory

[Ramkumar Ramachandra]

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

Re: [PATCH 4/7] revert: allow single-pick in the middle of cherry-pick sequence

[Jonathan Nieder]

(out of order for convenience)
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:

>> Suggested-by: Johannes Sixt <j6t@kdbg.org>
>
> 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).

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

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

Re: [PATCH 2/7] revert: allow cherry-pick --continue to commit before resuming

[Jonathan Nieder]

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:

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

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

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

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

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

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

[...]
>> +test_expect_failure 'follow advice and skip nil patch' '
[...]
> Again, what does this test have to do with the rest of your patch?

Likewise.

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

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

[...]
>> @@ -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.
[...]
>> +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

Re: [PATCH 4/7] revert: allow single-pick in the middle of cherry-pick sequence

[Ævar Arnfjörð Bjarmason]

On Sat, Dec 10, 2011 at 13:59, Jonathan Nieder <jrnieder@gmail.com> 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 <avarab@gmail.com>
    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 <tgc@statsbiblioteket.dk>
        Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
        Signed-off-by: Junio C Hamano <gitster@pobox.com>

?

Re: [PATCH 4/7] revert: allow single-pick in the middle of cherry-pick sequence

[Jonathan Nieder]

Æ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);
 	}