[GSoC update] Sequencer: The insn sheet format

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · lore

Hi,

Thanks for reading.

-- Ram

Ramkumar Ramachandra (14):
  advice: Introduce error_resolve_conflict
  revert: Inline add_message_to_msg function
  revert: Don't check lone argument in get_encoding
  revert: Rename no_replay to record_origin
  revert: Propogate errors upwards from do_pick_commit
  revert: Eliminate global "commit" variable
  revert: Introduce struct to keep command-line options
  revert: Separate cmdline parsing from functional code
  revert: Don't create invalid replay_opts in parse_args
  revert: Persist data for continuation
  revert: Introduce a layer of indirection over pick_commits
  revert: Introduce --reset to cleanup sequencer data
  revert: Introduce --continue to continue the operation
  revert: Change insn sheet format

 advice.c                           |   31 ++-
 advice.h                           |    1 +
 builtin/revert.c                   |  657 ++++++++++++++++++++++++++++--------
 git-rebase--interactive.sh         |   25 ++-
 t/t3032-merge-recursive-options.sh |    2 +
 t/t3501-revert-cherry-pick.sh      |    1 +
 t/t3502-cherry-pick-merge.sh       |    9 +-
 t/t3504-cherry-pick-rerere.sh      |    2 +
 t/t3505-cherry-pick-empty.sh       |   14 +-
 t/t3506-cherry-pick-ff.sh          |    3 +
 t/t3507-cherry-pick-conflict.sh    |   24 +-
 t/t3510-cherry-pick-sequence.sh    |  119 +++++++
 t/t7502-commit.sh                  |    1 +
 13 files changed, 720 insertions(+), 169 deletions(-)
 create mode 100644 t/t3510-cherry-pick-sequence.sh

-- 
1.7.5.GIT

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 01/14] advice: Introduce error_resolve_conflict

fatal: 'commit' is not possible because you have unmerged files.
       Please, fix them up in the work tree ...
       ...

to

Inspired-by: Christian Couder <chistian.couder@gmail.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 advice.c |   31 ++++++++++++++++++++++++-------
 advice.h |    1 +
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/advice.c b/advice.c
index 0be4b5f..a031732 100644
--- a/advice.c
+++ b/advice.c
@@ -19,6 +19,15 @@ static struct {
 	{ "detachedhead", &advice_detached_head },
 };
 
+static void advise(const char *advice, ...)
+{
+	va_list params;
+
+	va_start(params, advice);
+	vreportf("hint: ", advice, params);
+	va_end(params);
+}
+
 int git_default_advice_config(const char *var, const char *value)
 {
 	const char *k = skip_prefix(var, "advice.");
@@ -34,16 +43,24 @@ int git_default_advice_config(const char *var, const char *value)
 	return 0;
 }
 
-void NORETURN die_resolve_conflict(const char *me)
+int error_resolve_conflict(const char *me)
 {
-	if (advice_resolve_conflict)
+	error("'%s' is not possible because you have unmerged files.", me);
+	if (advice_resolve_conflict) {
 		/*
 		 * Message used both when 'git commit' fails and when
 		 * other commands doing a merge do.
 		 */
-		die("'%s' is not possible because you have unmerged files.\n"
-		    "Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
-		    "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me);
-	else
-		die("'%s' is not possible because you have unmerged files.", me);
+		advise("Please, fix them up in the work tree,");
+		advise("and then use 'git add/rm <file>' as");
+		advise("appropriate to mark resolution and make a commit,");
+		advise("or use 'git commit -a'.");
+	}
+	return -1;
+}
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	error_resolve_conflict(me);
+	die("Exiting because of an unresolved conflict.");
 }
diff --git a/advice.h b/advice.h
index 3244ebb..f537366 100644
--- a/advice.h
+++ b/advice.h
@@ -12,6 +12,7 @@ extern int advice_detached_head;
 
 int git_default_advice_config(const char *var, const char *value);
 
+int error_resolve_conflict(const char *me);
 extern void NORETURN die_resolve_conflict(const char *me);
 
 #endif /* ADVICE_H */

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 08:35 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 01/14] advice: Introduce error_resolve_conflict

Ramkumar Ramachandra wrote:

> Enable future callers to report a conflict and not die immediately by
> introducing a new function called error_resolve_conflict.
> Re-implement die_resolve_conflict as a call to error_resolve_conflict
> followed by a call to die.  Consequently, the message printed by
> die_resolve_conflict changes from
>
> fatal: 'commit' is not possible because you have unmerged files.
>        Please, fix them up in the work tree ...
>        ...
>
> to
>
> error: 'commit' is not possible because you have unmerged files.
> hint: Please, fix them up in the work tree ...
> hint: ...
> fatal: Exiting because of an unresolved conflict.

> --- a/advice.c
> +++ b/advice.c
> @@ -19,6 +19,15 @@ static struct {
>  	{ "detachedhead", &advice_detached_head },
>  };
> 
> +static void advise(const char *advice, ...)
> +{
> +	va_list params;
> +
> +	va_start(params, advice);
> +	vreportf("hint: ", advice, params);
> +	va_end(params);
> +}

For what it's worth, with that change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Ramkumar Ramachandra· Jul 6, 2011, 09:28 UTC · re: Jonathan Nieder · lore

Re: [PATCH 01/14] advice: Introduce error_resolve_conflict

Hi Jonathan,

Jonathan Nieder writes:

> Ramkumar Ramachandra wrote:
>> --- a/advice.c
>> +++ b/advice.c
>> @@ -19,6 +19,15 @@ static struct {
>>       { "detachedhead", &advice_detached_head },
>>  };
>>
>> +static void advise(const char *advice, ...)
>> +{
>> +     va_list params;
>> +
>> +     va_start(params, advice);
>> +     vreportf("hint: ", advice, params);
>> +     va_end(params);
>> +}
>
> Rather than copy+pasting this code verbatim, wouldn't it make sense to
> move it and expose it through advice.h so the old call site can use
> the same code?

    It is local to revert.c for now because I am not sure this is
    the right API (we may want to take an array of advice lines or a
    boolean argument for easy suppression of unwanted advice).

So, is it still alright to expose it in advice.h?

> For what it's worth, with that change,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

-- Ram

Jonathan Nieder· Jul 6, 2011, 10:03 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 01/14] advice: Introduce error_resolve_conflict

Ramkumar Ramachandra wrote:

> Yes, but I was worried that I shouldn't expose it because your commit
> message (2a41df) says:
>
>     It is local to revert.c for now because I am not sure this is
>     the right API (we may want to take an array of advice lines or a
>     boolean argument for easy suppression of unwanted advice).
>
> So, is it still alright to expose it in advice.h?

Of course the API can still easily be changed later.

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 02/14] revert: Inline add_message_to_msg function

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   20 ++++++--------------
 1 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1f27c63..19d604c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -185,19 +185,6 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
-{
-	const char *p = message;
-	while (*p && (*p != '\n' || p[1] != '\n'))
-		p++;
-
-	if (!*p)
-		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
-
-	p += 2;
-	strbuf_addstr(msgbuf, p);
-}
-
 static void write_cherry_pick_head(void)
 {
 	int fd;
@@ -471,11 +458,16 @@ static int do_pick_commit(void)
 		}
 		strbuf_addstr(&msgbuf, ".\n");
 	} else {
+		const char *p = strstr(msg.message, "\n\n");
+
 		base = parent;
 		base_label = msg.parent_label;
 		next = commit;
 		next_label = msg.label;
-		add_message_to_msg(&msgbuf, msg.message);
+
+		p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
+		strbuf_addstr(&msgbuf, p);
+
 		if (no_replay) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));

-- 
1.7.5.GIT

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 03/14] revert: Don't check lone argument in get_encoding

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 19d604c..a6b51b1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -167,9 +167,6 @@ static char *get_encoding(const char *message)
 {
 	const char *p = message, *eol;
 
-	if (!p)
-		die (_("Could not read commit message of %s"),
-				sha1_to_hex(commit->object.sha1));
 	while (*p && *p != '\n') {
 		for (eol = p + 1; *eol && *eol != '\n'; eol++)
 			; /* do nothing */

-- 
1.7.5.GIT

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 04/14] revert: Rename no_replay to record_origin

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index a6b51b1..1612911 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,7 +35,7 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
+static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
 static struct commit *commit;
 static int commit_argc;
@@ -91,7 +91,7 @@ static void parse_args(int argc, const char **argv)
 
 	if (action == CHERRY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &no_replay, "append commit name"),
+			OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"),
 			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
 			OPT_END(),
 		};
@@ -465,7 +465,7 @@ static int do_pick_commit(void)
 		p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
 		strbuf_addstr(&msgbuf, p);
 
-		if (no_replay) {
+		if (record_origin) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
@@ -560,7 +560,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
 		if (no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (no_replay)
+		if (record_origin)
 			die(_("cherry-pick --ff cannot be used with -x"));
 		if (edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));

-- 
1.7.5.GIT

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 05/14] revert: Propogate errors upwards from do_pick_commit

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   69 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 36 insertions(+), 33 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 1612911..3de7ec4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -250,25 +250,20 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static NORETURN void die_dirty_index(const char *me)
+static int error_dirty_index(const char *me)
 {
-	if (read_cache_unmerged()) {
-		die_resolve_conflict(me);
-	} else {
-		if (advice_commit_before_merge) {
-			if (action == REVERT)
-				die(_("Your local changes would be overwritten by revert.\n"
-					  "Please, commit your changes or stash them to proceed."));
-			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"
-					  "Please, commit your changes or stash them to proceed."));
-		} else {
-			if (action == REVERT)
-				die(_("Your local changes would be overwritten by revert.\n"));
-			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"));
-		}
-	}
+	if (read_cache_unmerged())
+		return error_resolve_conflict(me);
+
+	/* Different translation strings for cherry-pick and revert */
+	if (action == CHERRY_PICK)
+		error(_("Your local changes would be overwritten by %s."), me);
+	else
+		error(_("Your local changes would be overwritten by %s."), me);
+
+	if (advice_commit_before_merge)
+		advise(_("Please, commit your changes or stash them to proceed."));
+	return -1;
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from)
@@ -382,12 +377,12 @@ static int do_pick_commit(void)
 		 * to work on.
 		 */
 		if (write_cache_as_tree(head, 0, NULL))
-			die (_("Your index file is unmerged."));
+			return error(_("Your index file is unmerged."));
 	} else {
 		if (get_sha1("HEAD", head))
-			die (_("You do not have a valid HEAD"));
+			return error(_("You do not have a valid HEAD"));
 		if (index_differs_from("HEAD", 0))
-			die_dirty_index(me);
+			return error_dirty_index(me);
 	}
 	discard_cache();
 
@@ -400,20 +395,20 @@ static int do_pick_commit(void)
 		struct commit_list *p;
 
 		if (!mainline)
-			die(_("Commit %s is a merge but no -m option was given."),
-			    sha1_to_hex(commit->object.sha1));
+			return error(_("Commit %s is a merge but no -m option was given."),
+				sha1_to_hex(commit->object.sha1));
 
 		for (cnt = 1, p = commit->parents;
 		     cnt != mainline && p;
 		     cnt++)
 			p = p->next;
 		if (cnt != mainline || !p)
-			die(_("Commit %s does not have parent %d"),
-			    sha1_to_hex(commit->object.sha1), mainline);
+			return error(_("Commit %s does not have parent %d"),
+				sha1_to_hex(commit->object.sha1), mainline);
 		parent = p->item;
 	} else if (0 < mainline)
-		die(_("Mainline was specified but commit %s is not a merge."),
-		    sha1_to_hex(commit->object.sha1));
+		return error(_("Mainline was specified but commit %s is not a merge."),
+			sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
 
@@ -423,12 +418,12 @@ static int do_pick_commit(void)
 	if (parent && parse_commit(parent) < 0)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
-		die(_("%s: cannot parse parent commit %s"),
-		    me, sha1_to_hex(parent->object.sha1));
+		return error(_("%s: cannot parse parent commit %s"),
+			me, sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit->buffer, &msg) != 0)
-		die(_("Cannot get commit message for %s"),
-				sha1_to_hex(commit->object.sha1));
+		return error(_("Cannot get commit message for %s"),
+			sha1_to_hex(commit->object.sha1));
 
 	/*
 	 * "commit" is an existing commit.  We would want to apply
@@ -581,14 +576,22 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
+	int res;
 	if (isatty(0))
 		edit = 1;
 	action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res < 0)
+		die(_("%s failed"), me);
+	return res;
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
+	int res;
 	action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res < 0)
+		die(_("%s failed"), me);
+	return res;
 }

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 08:50 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 05/14] revert: Propogate errors upwards from do_pick_commit

Hi,

Ramkumar Ramachandra wrote:

> Currently, the return value from revert_or_cherry_pick is a
> non-negative number representing the intended exit status from `git
> revert` or `git cherry-pick`.  Change this by replacing some of the
> calls to "die" with calls to "error", so that it can return negative
> values too.  Postive return values indicate conflicts, while negative

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -250,25 +250,20 @@ static struct tree *empty_tree(void)

[...]

> +	if (action == CHERRY_PICK)
> +		error(_("Your local changes would be overwritten by %s."), me);
> +	else
> +		error(_("Your local changes would be overwritten by %s."), me);

[...]

> +	if (res < 0)
> +		die(_("%s failed"), me);
> +	return res;
>  }

Likewise.

Ramkumar Ramachandra· Jul 6, 2011, 09:30 UTC · re: Jonathan Nieder · lore

Re: [PATCH 05/14] revert: Propogate errors upwards from do_pick_commit

Hi again,

Jonathan Nieder writes:

>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -250,25 +250,20 @@ static struct tree *empty_tree(void)
> [...]
>> +     if (action == CHERRY_PICK)
>> +             error(_("Your local changes would be overwritten by %s."), me);
>> +     else
>> +             error(_("Your local changes would be overwritten by %s."), me);
>
> gettext creates one msgid for these two strings, so translators have
> no choice but to give them the same translation.  Is that the intent?
>
> [...]
>> +     if (res < 0)
>> +             die(_("%s failed"), me);
>> +     return res;
>>  }
>
> Likewise.

Oops, I didn't realize that! Will fix.

-- Ram

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 06/14] revert: Eliminate global "commit" variable

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 3de7ec4..fcc2047 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
 
 static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
-static struct commit *commit;
 static int commit_argc;
 static const char **commit_argv;
 static int allow_rerere_auto;
@@ -116,25 +115,25 @@ struct commit_message {
 	const char *message;
 };
 
-static int get_message(const char *raw_message, struct commit_message *out)
+static int get_message(struct commit *commit, struct commit_message *out)
 {
 	const char *encoding;
 	const char *abbrev, *subject;
 	int abbrev_len, subject_len;
 	char *q;
 
-	if (!raw_message)
+	if (!commit->buffer)
 		return -1;
-	encoding = get_encoding(raw_message);
+	encoding = get_encoding(commit->buffer);
 	if (!encoding)
 		encoding = "UTF-8";
 	if (!git_commit_encoding)
 		git_commit_encoding = "UTF-8";
 
 	out->reencoded_message = NULL;
-	out->message = raw_message;
+	out->message = commit->buffer;
 	if (strcmp(encoding, git_commit_encoding))
-		out->reencoded_message = reencode_string(raw_message,
+		out->reencoded_message = reencode_string(commit->buffer,
 					git_commit_encoding, encoding);
 	if (out->reencoded_message)
 		out->message = out->reencoded_message;
@@ -182,7 +181,7 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-static void write_cherry_pick_head(void)
+static void write_cherry_pick_head(struct commit *commit)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
@@ -359,7 +358,7 @@ static int run_git_commit(const char *defmsg)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(void)
+static int do_pick_commit(struct commit *commit)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -421,7 +420,7 @@ static int do_pick_commit(void)
 		return error(_("%s: cannot parse parent commit %s"),
 			me, sha1_to_hex(parent->object.sha1));
 
-	if (get_message(commit->buffer, &msg) != 0)
+	if (get_message(commit, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
 			sha1_to_hex(commit->object.sha1));
 
@@ -466,7 +465,7 @@ static int do_pick_commit(void)
 			strbuf_addstr(&msgbuf, ")\n");
 		}
 		if (!no_commit)
-			write_cherry_pick_head();
+			write_cherry_pick_head(sha1_to_hex(commit));
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -544,6 +543,7 @@ static void read_and_refresh_cache(const char *me)
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	struct rev_info revs;
+	struct commit *commit;
 
 	git_config(git_default_config, NULL);
 	me = action == REVERT ? "revert" : "cherry-pick";
@@ -566,7 +566,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	prepare_revs(&revs);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit();
+		int res = do_pick_commit(commit);
 		if (res)
 			return res;
 	}

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 08:55 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 06/14] revert: Eliminate global "commit" variable

Ramkumar Ramachandra wrote:

> Since we want to develop the functionality to either pick or revert
> individual commits atomically later in the series, make "commit" a
> variable to be passed around explicitly as an argument for clarity.
> This involves changing several functions to take an additional
> argument, but no functional changes.  Additionaly, this will permit
> more than one commit to be cherry-picked at once, should we choose to
> develop this functionality in future.

The patch looks good, except for:

[...]

> -static void write_cherry_pick_head(void)
> +static void write_cherry_pick_head(struct commit *commit)

[...]

> -			write_cherry_pick_head();
> +			write_cherry_pick_head(sha1_to_hex(commit));

I don't see how this would even compile.

Ramkumar Ramachandra· Jul 6, 2011, 09:37 UTC · re: Jonathan Nieder · lore

Re: [PATCH 06/14] revert: Eliminate global "commit" variable

Hi,

Jonathan Nieder writes:

> Ramkumar Ramachandra wrote:
>> Since we want to develop the functionality to either pick or revert
>> individual commits atomically later in the series, make "commit" a
>> variable to be passed around explicitly as an argument for clarity.
>> This involves changing several functions to take an additional
>> argument, but no functional changes.  Additionaly, this will permit
>> more than one commit to be cherry-picked at once, should we choose to
>> develop this functionality in future.
>
> I don't understand the last sentence above --- doesn't "git cherry-pick
> A B" work already?

Ugh.  Removed the line.

> The patch looks good, except for:
>
> [...]
>> -static void write_cherry_pick_head(void)
>> +static void write_cherry_pick_head(struct commit *commit)
> [...]
>> -                     write_cherry_pick_head();
>> +                     write_cherry_pick_head(sha1_to_hex(commit));
>
> I don't see how this would even compile.

Rebase fail.  Thanks.

-- Ram

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 07/14] revert: Introduce struct to keep command-line options

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |  182 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 102 insertions(+), 80 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index fcc2047..f34e51f 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,76 +35,90 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
-static enum { REVERT, CHERRY_PICK } action;
-static int commit_argc;
-static const char **commit_argv;
-static int allow_rerere_auto;
-
 static const char *me;
-
-/* Merge strategy. */
-static const char *strategy;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+enum replay_action { REVERT, CHERRY_PICK };
+
+struct replay_opts {
+	enum replay_action action;
+
+	/* Boolean options */
+	int edit;
+	int record_origin;
+	int no_commit;
+	int signoff;
+	int allow_ff;
+	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;
+};
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 static char *get_encoding(const char *message);
 
-static const char * const *revert_or_cherry_pick_usage(void)
+static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
-	return action == REVERT ? revert_usage : cherry_pick_usage;
+	return opts->action == REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
 			  const char *arg, int unset)
 {
+	struct replay_opts **opts_ptr = opt->value;
+	struct replay_opts *opts = *opts_ptr;
+
 	if (unset)
 		return 0;
 
-	ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
-	xopts[xopts_nr++] = xstrdup(arg);
+	ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
+	opts->xopts[opts->xopts_nr++] = xstrdup(arg);
 	return 0;
 }
 
-static void parse_args(int argc, const char **argv)
+static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
-	const char * const * usage_str = revert_or_cherry_pick_usage();
+	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	int noop;
 	struct option options[] = {
-		OPT_BOOLEAN('n', "no-commit", &no_commit, "don't automatically commit"),
-		OPT_BOOLEAN('e', "edit", &edit, "edit the commit message"),
+		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
+		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
 		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },
-		OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
-		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
-		OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
-		OPT_STRING(0, "strategy", &strategy, "strategy", "merge strategy"),
-		OPT_CALLBACK('X', "strategy-option", &xopts, "option",
+		OPT_BOOLEAN('s', "signoff", &opts->signoff, "add Signed-off-by:"),
+		OPT_INTEGER('m', "mainline", &opts->mainline, "parent number"),
+		OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
+		OPT_STRING(0, "strategy", &opts->strategy, "strategy", "merge strategy"),
+		OPT_CALLBACK('X', "strategy-option", &opts, "option",
 			"option for merge strategy", option_parse_x),
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
 	};
 
-	if (action == CHERRY_PICK) {
+	if (opts->action == CHERRY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"),
-			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
+			OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
+			OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
 			die(_("program error"));
 	}
 
-	commit_argc = parse_options(argc, argv, NULL, options, usage_str,
-				    PARSE_OPT_KEEP_ARGV0 |
-				    PARSE_OPT_KEEP_UNKNOWN);
-	if (commit_argc < 2)
+	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
+					PARSE_OPT_KEEP_ARGV0 |
+					PARSE_OPT_KEEP_UNKNOWN);
+	if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
-	commit_argv = argv;
+	opts->commit_argv = argv;
 }
 
 struct commit_message {
@@ -249,7 +263,7 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static int error_dirty_index(const char *me)
+static int error_dirty_index(const char *me, enum replay_action action)
 {
 	if (read_cache_unmerged())
 		return error_resolve_conflict(me);
@@ -278,7 +292,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)
 
 static int do_recursive_merge(struct commit *base, struct commit *next,
 			      const char *base_label, const char *next_label,
-			      unsigned char *head, struct strbuf *msgbuf)
+			      unsigned char *head, struct strbuf *msgbuf,
+			      struct replay_opts *opts)
 {
 	struct merge_options o;
 	struct tree *result, *next_tree, *base_tree, *head_tree;
@@ -299,7 +314,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	next_tree = next ? next->tree : empty_tree();
 	base_tree = base ? base->tree : empty_tree();
 
-	for (xopt = xopts; xopt != xopts + xopts_nr; xopt++)
+	for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
 		parse_merge_opt(&o, *xopt);
 
 	clean = merge_trees(&o,
@@ -339,7 +354,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
  * If we are revert, or if our cherry-pick results in a hand merge,
  * we had better say that the current user is responsible for that.
  */
-static int run_git_commit(const char *defmsg)
+static int run_git_commit(const char *defmsg, struct replay_opts *opts)
 {
 	/* 6 is max possible length of our args array including NULL */
 	const char *args[6];
@@ -347,9 +362,9 @@ static int run_git_commit(const char *defmsg)
 
 	args[i++] = "commit";
 	args[i++] = "-n";
-	if (signoff)
+	if (opts->signoff)
 		args[i++] = "-s";
-	if (!edit) {
+	if (!opts->edit) {
 		args[i++] = "-F";
 		args[i++] = defmsg;
 	}
@@ -358,7 +373,7 @@ static int run_git_commit(const char *defmsg)
 	return run_command_v_opt(args, RUN_GIT_CMD);
 }
 
-static int do_pick_commit(struct commit *commit)
+static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -368,7 +383,7 @@ static int do_pick_commit(struct commit *commit)
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res;
 
-	if (no_commit) {
+	if (opts->no_commit) {
 		/*
 		 * We do not intend to commit immediately.  We just want to
 		 * merge the differences in, so let's compute the tree
@@ -381,7 +396,7 @@ static int do_pick_commit(struct commit *commit)
 		if (get_sha1("HEAD", head))
 			return error(_("You do not have a valid HEAD"));
 		if (index_differs_from("HEAD", 0))
-			return error_dirty_index(me);
+			return error_dirty_index(me, opts->action);
 	}
 	discard_cache();
 
@@ -393,25 +408,25 @@ static int do_pick_commit(struct commit *commit)
 		int cnt;
 		struct commit_list *p;
 
-		if (!mainline)
+		if (!opts->mainline)
 			return error(_("Commit %s is a merge but no -m option was given."),
 				sha1_to_hex(commit->object.sha1));
 
 		for (cnt = 1, p = commit->parents;
-		     cnt != mainline && p;
+		     cnt != opts->mainline && p;
 		     cnt++)
 			p = p->next;
-		if (cnt != mainline || !p)
+		if (cnt != opts->mainline || !p)
 			return error(_("Commit %s does not have parent %d"),
-				sha1_to_hex(commit->object.sha1), mainline);
+				sha1_to_hex(commit->object.sha1), opts->mainline);
 		parent = p->item;
-	} else if (0 < mainline)
+	} else if (0 < opts->mainline)
 		return error(_("Mainline was specified but commit %s is not a merge."),
 			sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
 
-	if (allow_ff && parent && !hashcmp(parent->object.sha1, head))
+	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
 		return fast_forward_to(commit->object.sha1, head);
 
 	if (parent && parse_commit(parent) < 0)
@@ -433,7 +448,7 @@ static int do_pick_commit(struct commit *commit)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (action == REVERT) {
+	if (opts->action == REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -459,18 +474,18 @@ static int do_pick_commit(struct commit *commit)
 		p = p ? p + 2 : sha1_to_hex(commit->object.sha1);
 		strbuf_addstr(&msgbuf, p);
 
-		if (record_origin) {
+		if (opts->record_origin) {
 			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
 		}
-		if (!no_commit)
-			write_cherry_pick_head(sha1_to_hex(commit));
+		if (!opts->no_commit)
+			write_cherry_pick_head(commit);
 	}
 
-	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
-					 head, &msgbuf);
+					 head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
 	} else {
 		struct commit_list *common = NULL;
@@ -480,23 +495,23 @@ static int do_pick_commit(struct commit *commit)
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-		res = try_merge_command(strategy, xopts_nr, xopts, common,
-					sha1_to_hex(head), remotes);
+		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+					common, sha1_to_hex(head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
 	}
 
 	if (res) {
-		error(action == REVERT
+		error(opts->action == REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
 		      msg.subject);
 		print_advice();
-		rerere(allow_rerere_auto);
+		rerere(opts->allow_rerere_auto);
 	} else {
-		if (!no_commit)
-			res = run_git_commit(defmsg);
+		if (!opts->no_commit)
+			res = run_git_commit(defmsg, opts);
 	}
 
 	free_message(&msg);
@@ -505,18 +520,18 @@ static int do_pick_commit(struct commit *commit)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs)
+static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
 {
 	int argc;
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (action != REVERT)
+	if (opts->action != REVERT)
 		revs->reverse = 1;
 
-	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
+	argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
 	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage());
+		usage(*revert_or_cherry_pick_usage(opts));
 
 	if (prepare_revision_walk(revs))
 		die(_("revision walk setup failed"));
@@ -525,7 +540,7 @@ static void prepare_revs(struct rev_info *revs)
 		die(_("empty commit set passed"));
 }
 
-static void read_and_refresh_cache(const char *me)
+static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 {
 	static struct lock_file index_lock;
 	int index_fd = hold_locked_index(&index_lock, 0);
@@ -540,33 +555,34 @@ static void read_and_refresh_cache(const char *me)
 	rollback_lock_file(&index_lock);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int revert_or_cherry_pick(int argc, const char **argv,
+				struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
 
 	git_config(git_default_config, NULL);
-	me = action == REVERT ? "revert" : "cherry-pick";
+	me = opts->action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv);
+	parse_args(argc, argv, opts);
 
-	if (allow_ff) {
-		if (signoff)
+	if (opts->allow_ff) {
+		if (opts->signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (no_commit)
+		if (opts->no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (record_origin)
+		if (opts->record_origin)
 			die(_("cherry-pick --ff cannot be used with -x"));
-		if (edit)
+		if (opts->edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
-	read_and_refresh_cache(me);
+	read_and_refresh_cache(me, opts);
 
-	prepare_revs(&revs);
+	prepare_revs(&revs, opts);
 
 	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit);
+		int res = do_pick_commit(commit, opts);
 		if (res)
 			return res;
 	}
@@ -577,10 +593,13 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res;
+	struct replay_opts opts;
+
+	memset(&opts, 0, sizeof(struct replay_opts));
 	if (isatty(0))
-		edit = 1;
-	action = REVERT;
-	res = revert_or_cherry_pick(argc, argv);
+		opts.edit = 1;
+	opts.action = REVERT;
+	res = revert_or_cherry_pick(argc, argv, &opts);
 	if (res < 0)
 		die(_("%s failed"), me);
 	return res;
@@ -589,8 +608,11 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	int res;
-	action = CHERRY_PICK;
-	res = revert_or_cherry_pick(argc, argv);
+	struct replay_opts opts;
+
+	memset(&opts, 0, sizeof(struct replay_opts));
+	opts.action = CHERRY_PICK;
+	res = revert_or_cherry_pick(argc, argv, &opts);
 	if (res < 0)
 		die(_("%s failed"), me);
 	return res;

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 09:09 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 07/14] revert: Introduce struct to keep command-line options

Ramkumar Ramachandra wrote:

> In later
> steps in this series, we would like to introduce an API function that
> calls into this machinery directly and have a way to tell it what to
> do.  Hence, introduce a structure to group these variables, so that
> the API can take them as a single replay_options parameter.
>
> The variable "me" is left as a file-scope static variable because it
> is not an independent option.  "me" is simply a string that needs to
> be inferred from the "action" option, and is kept global to save each
> function the trouble of determining it independently.

> Unfortunately, this patch introduces a minor regression.  Parsing
> strategy-option violates a C89 rule: Initializers cannot refer to
> variables whose address is not known at compile time.  Currently, this
> rule is violated by some other parts of Git as well, and it is
> possible to get GCC to report these instances using the "-std=c89
> -pedantic" option.

 A. Apply patch 8 and make cmd_revert, cmd_cherry_pick, and parse_args
    manipulate a static "struct replay_opts" while pick_commits et al
    pass around a pointer to it

 B. Make parse_args work like this:

	copy from argument to private static struct replay_opts
	call parse_options()
	copy private static struct replay_opts to argument

 C. Use new option types:

	OPT_BOOL_MEMBER('n', "no-commit",
		offsetof(struct replay_opts, no_commit),
		"don't automatically commit"),

    and teach parse_options to take an additional parameter like it
    takes "prefix" now, to be used as a base address for options that
    write to an offset instead of a pointer

Ramkumar Ramachandra· Jul 6, 2011, 11:20 UTC · re: Jonathan Nieder · lore

Re: [PATCH 07/14] revert: Introduce struct to keep command-line options

Hi,

Jonathan Nieder writes:

> Ramkumar Ramachandra wrote:
>> In later
>> steps in this series, we would like to introduce an API function that
>> calls into this machinery directly and have a way to tell it what to
>> do.  Hence, introduce a structure to group these variables, so that
>> the API can take them as a single replay_options parameter.
>>
>> The variable "me" is left as a file-scope static variable because it
>> is not an independent option.  "me" is simply a string that needs to
>> be inferred from the "action" option, and is kept global to save each
>> function the trouble of determining it independently.
>
> Hm, would it make sense for there to be a "private" section at the
> end of the replay_opts struct for variables like this?

>> Unfortunately, this patch introduces a minor regression.  Parsing
>> strategy-option violates a C89 rule: Initializers cannot refer to
>> variables whose address is not known at compile time.  Currently, this
>> rule is violated by some other parts of Git as well, and it is
>> possible to get GCC to report these instances using the "-std=c89
>> -pedantic" option.
>
> I would be interested in fixing that (as a patch on top, maybe).
> What do you suggest:
>
>  A. Apply patch 8 and make cmd_revert, cmd_cherry_pick, and parse_args
>    manipulate a static "struct replay_opts" while pick_commits et al
>    pass around a pointer to it
>
>  B. Make parse_args work like this:
>
>        copy from argument to private static struct replay_opts
>        call parse_options()
>        copy private static struct replay_opts to argument

This is something you suggested earlier, but I find it extremely inelegant.

>  C. Use new option types:
>
>        OPT_BOOL_MEMBER('n', "no-commit",
>                offsetof(struct replay_opts, no_commit),
>                "don't automatically commit"),
>
>    and teach parse_options to take an additional parameter like it
>    takes "prefix" now, to be used as a base address for options that
>    write to an offset instead of a pointer
>
> I'm leaning towards A but not sure if that would be wasted work in
> light of your plans for these APIs in the long run (i.e., is
> parse_args() going to be exposed and want to act on a caller-supplied
> struct)?

-- Ram

Jonathan Nieder· Jul 6, 2011, 12:06 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 07/14] revert: Introduce struct to keep command-line options

Ramkumar Ramachandra wrote:

> My justification: in later steps, we'd want to be able to mix
> "pick" and "revert" instructions in the same instruction sheet.  This
> will essentially require the parser to return a commit + a replay_opts
> struct (which will contain the action information).

Side note: is it intended to support insns like

 pick A..B

?

[...]

> Yes, I'm definitely considering exposing parse_args in the future,
> especially since I want to support command-line options in my
> instruction sheet.

Thanks for some useful clarifications.

Ramkumar Ramachandra· Jul 12, 2011, 06:14 UTC · re: Jonathan Nieder · lore

Re: [PATCH 07/14] revert: Introduce struct to keep command-line options

Hi Jonathan,

Jonathan Nieder writes:

>> My justification: in later steps, we'd want to be able to mix
>> "pick" and "revert" instructions in the same instruction sheet.  This
>> will essentially require the parser to return a commit + a replay_opts
>> struct (which will contain the action information).
>
> Side note: is it intended to support insns like
>
>  pick A..B
>
> ?

> Anyway, the above explanation about the intended use for "struct
> replay_opts" (it needs to be small, I guess?) would be a good thing
> to add to the commit message, too.  Basically, whatever information a
> person needs in order to understand the design is a useful thing to
> add.

>> Yes, I'm definitely considering exposing parse_args in the future,
>> especially since I want to support command-line options in my
>> instruction sheet.
>
> Hm, I am not sure what to think about this direction (is the git
> sequencer actually just a fast git shell?  in that case, why is pick
> spelled "pick" instead of "cherry-pick"?).  Maybe it's a good thing.

-- Ram

Junio C Hamano· Jul 6, 2011, 21:20 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 07/14] revert: Introduce struct to keep command-line options

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> The variable "me" is left as a file-scope static variable because it
> is not an independent option.  "me" is simply a string that needs to
> be inferred from the "action" option, and is kept global to save each
> function the trouble of determining it independently.

Ramkumar Ramachandra· Jul 12, 2011, 06:21 UTC · re: Junio C Hamano · lore

Re: [PATCH 07/14] revert: Introduce struct to keep command-line options

Hi Junio,

Again- intended to reply to this earlier; sorry.

Junio C Hamano writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> The variable "me" is left as a file-scope static variable because it
>> is not an independent option.  "me" is simply a string that needs to
>> be inferred from the "action" option, and is kept global to save each
>> function the trouble of determining it independently.
>
> Would it make more sense to remove the variable, pass "action" around
> where only "me" is passed around right now, and introduce a function
> "static const char *action_name()" to help places that wants textual
> command name for display purposes?

Thanks.

-- Ram

Jonathan Nieder· Jul 12, 2011, 06:33 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 07/14] revert: Introduce struct to keep command-line options

Ramkumar Ramachandra wrote:

> Since I'm not sure the function you propose will make it to
> sequencer.c, I don't want to introduce it now.

Wouldn't it be easy to remove such a function later?

 a. make "me" a member of the replay_opts struct; or
 b. compute "me" in each function that needs it by calling a helper
    function; or
 c. add some explanation to the commit message to clarify the status
    of "me" as static-but-won't-be-in-the-long-term and a reason for
    that

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 08/14] revert: Separate cmdline parsing from functional code

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index f34e51f..e3a7c9e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -555,17 +555,12 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv,
-				struct replay_opts *opts)
+static int pick_commits(struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
 
-	git_config(git_default_config, NULL);
-	me = opts->action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv, opts);
-
 	if (opts->allow_ff) {
 		if (opts->signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
@@ -599,7 +594,10 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	if (isatty(0))
 		opts.edit = 1;
 	opts.action = REVERT;
-	res = revert_or_cherry_pick(argc, argv, &opts);
+	git_config(git_default_config, NULL);
+	me = "revert";
+	parse_args(argc, argv, &opts);
+	res = pick_commits(&opts);
 	if (res < 0)
 		die(_("%s failed"), me);
 	return res;
@@ -612,7 +610,10 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 
 	memset(&opts, 0, sizeof(struct replay_opts));
 	opts.action = CHERRY_PICK;
-	res = revert_or_cherry_pick(argc, argv, &opts);
+	git_config(git_default_config, NULL);
+	me = "cherry-pick";
+	parse_args(argc, argv, &opts);
+	res = pick_commits(&opts);
 	if (res < 0)
 		die(_("%s failed"), me);
 	return res;

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 09:13 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 08/14] revert: Separate cmdline parsing from functional code

Ramkumar Ramachandra wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c

[...]

> @@ -612,7 +610,10 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>  
>  	memset(&opts, 0, sizeof(struct replay_opts));
>  	opts.action = CHERRY_PICK;
> -	res = revert_or_cherry_pick(argc, argv, &opts);
> +	git_config(git_default_config, NULL);
> +	me = "cherry-pick";
> +	parse_args(argc, argv, &opts);
> +	res = pick_commits(&opts);
>  	if (res < 0)
>  		die(_("%s failed"), me);
>  	return res;

Ramkumar Ramachandra· Jul 6, 2011, 09:34 UTC · re: Jonathan Nieder · lore

Re: [PATCH 08/14] revert: Separate cmdline parsing from functional code

Hi,

Jonathan Nieder writes:

> Ramkumar Ramachandra wrote:
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> @@ -612,7 +610,10 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>>
>>       memset(&opts, 0, sizeof(struct replay_opts));
>>       opts.action = CHERRY_PICK;
>> -     res = revert_or_cherry_pick(argc, argv, &opts);
>> +     git_config(git_default_config, NULL);
>> +     me = "cherry-pick";
>> +     parse_args(argc, argv, &opts);
>> +     res = pick_commits(&opts);
>>       if (res < 0)
>>               die(_("%s failed"), me);
>>       return res;
>
> I'd put the "me =" line right after "opts.action =" if doing it this
> way.  This means callers to pick_commits() are responsible for setting
> the "me" variable and in particular it will not make sense to export
> that function to callers outside of this file any more, right?

Exactly.  Good point.

-- Ram

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 09/14] revert: Don't create invalid replay_opts in parse_args

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   37 ++++++++++++++++++++++++++-----------
 1 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index e3a7c9e..27dc538 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -82,6 +82,22 @@ static int option_parse_x(const struct option *opt,
 	return 0;
 }
 
+static void verify_opt_compatible(const char *me, const char *base_opt, ...)
+{
+	const char *this_opt;
+	va_list ap;
+	int set;
+
+	va_start(ap, base_opt);
+	while ((this_opt = va_arg(ap, const char *))) {
+		set = va_arg(ap, int);
+		if (set)
+			die(_("%s: %s cannot be used with %s"),
+				me, this_opt, base_opt);
+	}
+	va_end(ap);
+}
+
 static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
@@ -118,6 +134,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
+	if (opts->allow_ff)
+		verify_opt_compatible(me, "--ff",
+				"--signoff", opts->signoff,
+				"--no-commit", opts->no_commit,
+				"-x", opts->record_origin,
+				"--edit", opts->edit,
+				NULL);
 	opts->commit_argv = argv;
 }
 
@@ -561,17 +584,9 @@ static int pick_commits(struct replay_opts *opts)
 	struct commit *commit;
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	if (opts->allow_ff) {
-		if (opts->signoff)
-			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (opts->no_commit)
-			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (opts->record_origin)
-			die(_("cherry-pick --ff cannot be used with -x"));
-		if (opts->edit)
-			die(_("cherry-pick --ff cannot be used with --edit"));
-	}
-
+	if (opts->allow_ff)
+		assert(!(opts->signoff || opts->no_commit ||
+				opts->record_origin || opts->edit));
 	read_and_refresh_cache(me, opts);
 
 	prepare_revs(&revs, opts);

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 09:20 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 09/14] revert: Don't create invalid replay_opts in parse_args

Ramkumar Ramachandra wrote:

> The "--ff" command-line option cannot be used with four other
> command-line options.  However, when these options are specified with
> "--ff" on the command-line, parse_args will still parse these
> incompatible options into a replay_opts structure for use by the rest
> of the program.  Although pick_commits checks the validity of the
> replay_opts strucutre before before starting its operation, this is
> inelegant design; pick_commits is currently the gatekeeper to the
> cherry-pick machinery, but this will change in future.  To futureproof
> the code and catch these errors in one place, make sure that an
> invalid replay_opts structure is not created by parse_args in the
> first place.  Also ensure that regressions in maintaining this
> invariant are caught in the future by adding an assertion in
> pick_commits.

The patch itself looks good.

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 10/14] revert: Persist data for continuation

Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c                |  122 +++++++++++++++++++++++++++++++++++++--
 t/t3510-cherry-pick-sequence.sh |   37 ++++++++++++
 2 files changed, 153 insertions(+), 6 deletions(-)
 create mode 100644 t/t3510-cherry-pick-sequence.sh

diff --git a/builtin/revert.c b/builtin/revert.c
index 27dc538..7d76f92 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -13,6 +13,7 @@
 #include "rerere.h"
 #include "merge-recursive.h"
 #include "refs.h"
+#include "dir.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -25,6 +26,10 @@
  * Copyright (c) 2005 Junio C Hamano
  */
 
+#define SEQ_DIR		"sequencer"
+#define SEQ_HEAD_FILE	"sequencer/head"
+#define SEQ_TODO_FILE	"sequencer/todo"
+
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
 	NULL
@@ -417,7 +422,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 			return error(_("Your index file is unmerged."));
 	} else {
 		if (get_sha1("HEAD", head))
-			return error(_("You do not have a valid HEAD"));
+			return error(_("Can't %s on an unborn branch"), me);
 		if (index_differs_from("HEAD", 0))
 			return error_dirty_index(me, opts->action);
 	}
@@ -578,10 +583,106 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
-static int pick_commits(struct replay_opts *opts)
+static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
+			struct replay_opts *opts)
+{
+	struct commit_list *cur = NULL;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	const char *sha1_abbrev = NULL;
+	const char *action;
+
+	action = (opts->action == REVERT ? "revert" : "pick");
+	for (cur = todo_list; cur; cur = cur->next) {
+		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
+		if (get_message(cur->item, &msg))
+			die(_("Cannot get commit message for %s"), sha1_abbrev);
+		strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);
+	}
+}
+
+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 *new;
+	struct commit_list **next;
+
+	prepare_revs(&revs, opts);
+
+	/* Insert into todo_list in the same order */
+	/* NEEDSWORK: Expose this as commit_list_append */
+	next = todo_list;
+	while ((commit = get_revision(&revs))) {
+		new = xmalloc(sizeof(struct commit_list));
+		new->item = commit;
+		*next = new;
+		next = &new->next;
+	}
+	*next = NULL;
+}
+
+static void create_seq_dir(void)
+{
+	if (file_exists(git_path(SEQ_DIR))) {
+		if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
+			die(_("Could not remove %s"), git_path(SEQ_DIR));
+	} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
+		die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
+}
+
+static void save_head(const char *head)
+{
+	static struct lock_file head_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&head_lock, git_path(SEQ_HEAD_FILE), LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s\n", head);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), git_path(SEQ_HEAD_FILE));
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s"), git_path(SEQ_HEAD_FILE));
+}
+
+static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+{
+	static struct lock_file todo_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	fd = hold_lock_file_for_update(&todo_lock, git_path(SEQ_TODO_FILE), LOCK_DIE_ON_ERROR);
+	format_todo(&buf, todo_list, opts);
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not write to %s."), git_path(SEQ_TODO_FILE));
+	}
+	if (commit_lock_file(&todo_lock) < 0) {
+		strbuf_release(&buf);
+		die(_("Error wrapping up %s"), git_path(SEQ_TODO_FILE));
+	}
+	strbuf_release(&buf);
+}
+
+static int cleanup_sequencer_data(void)
+{
+	static struct strbuf seq_dir = STRBUF_INIT;
+
+	strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
+	if (remove_dir_recursively(&seq_dir, 0) < 0) {
+		strbuf_release(&seq_dir);
+		return error(_("Unable to clean up after successful %s"), me);
+	}
+	strbuf_release(&seq_dir);
+	return 0;
+}
+
+static int pick_commits(struct replay_opts *opts)
+{
+	struct commit_list *todo_list = NULL;
+	unsigned char sha1[20];
+	struct commit_list *cur;
+	int res;
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
 	if (opts->allow_ff)
@@ -589,15 +690,24 @@ static int pick_commits(struct replay_opts *opts)
 				opts->record_origin || opts->edit));
 	read_and_refresh_cache(me, opts);
 
-	prepare_revs(&revs, opts);
+	walk_revs_populate_todo(&todo_list, opts);
+	create_seq_dir();
+	if (!get_sha1("HEAD", sha1))
+		save_head(sha1_to_hex(sha1));
+	save_todo(todo_list, opts);
 
-	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit, opts);
+	for (cur = todo_list; cur; cur = cur->next) {
+		save_todo(cur, opts);
+		res = do_pick_commit(cur->item, opts);
 		if (res)
 			return res;
 	}
 
-	return 0;
+	/*
+	 * Sequence of picks finished successfully; cleanup by
+	 * removing the .git/sequencer directory
+	 */
+	return cleanup_sequencer_data();
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
new file mode 100644
index 0000000..83e2722
--- /dev/null
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+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
+
+'
+
+. ./test-lib.sh
+
+pristine_detach () {
+	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 &&
+	git config advice.detachedhead false
+
+'
+
+test_expect_success 'cherry-pick cleans up sequencer directory upon success' '
+	pristine_detach initial &&
+	git cherry-pick initial..picked &&
+	test_path_is_missing .git/sequencer
+'
+
+test_done

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 10:01 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 10/14] revert: Persist data for continuation

Ramkumar Ramachandra wrote:

> Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than
> one commit, 2010-06-02), a single invocation of "git cherry-pick" or
> "git revert" can perform picks of several individual commits.  To
> implement features like "--continue" to continue the whole operation,
> we will need to store some information about the state and the plan at
> the beginning.  Introduce a ".git/sequencer/head" file to store this
> state, and ".git/sequencer/todo" file to store the plan.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c

[...]

> @@ -25,6 +26,10 @@
>   * Copyright (c) 2005 Junio C Hamano
>   */
>  
> +#define SEQ_DIR		"sequencer"
> +#define SEQ_HEAD_FILE	"sequencer/head"
> +#define SEQ_TODO_FILE	"sequencer/todo"

Yay. :)

> @@ -417,7 +422,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  			return error(_("Your index file is unmerged."));
>  	} else {
>  		if (get_sha1("HEAD", head))
> -			return error(_("You do not have a valid HEAD"));
> +			return error(_("Can't %s on an unborn branch"), me);

Perhaps it would make sense to do something like

	if (get_sha1("HEAD", head)) {
		if (opts->action == REVERT)
			return error(_("can't revert as initial commit"));
		return error(_("cherry-pick into empty head not supported yet"));
	}

> @@ -578,10 +583,106 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
>  	rollback_lock_file(&index_lock);
>  }
>  
> -static int pick_commits(struct replay_opts *opts)
> +static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
> +			struct replay_opts *opts)
> +{
> +	struct commit_list *cur = NULL;
> +	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
> +	const char *sha1_abbrev = NULL;
> +	const char *action;
> +
> +	action = (opts->action == REVERT ? "revert" : "pick");
> +	for (cur = todo_list; cur; cur = cur->next) {
> +		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
> +		if (get_message(cur->item, &msg))
> +			die(_("Cannot get commit message for %s"), sha1_abbrev);
> +		strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);

> +	}
> +}
> +
> +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 *new;
> +	struct commit_list **next;
> +
> +	prepare_revs(&revs, opts);
> +
> +	/* Insert into todo_list in the same order */
> +	/* NEEDSWORK: Expose this as commit_list_append */
> +	next = todo_list;
> +	while ((commit = get_revision(&revs))) {
> +		new = xmalloc(sizeof(struct commit_list));
> +		new->item = commit;
> +		*next = new;
> +		next = &new->next;
> +	}
> +	*next = NULL;

 /*
  * Example:
  *
  *	struct commit_list *list;
  *	struct commit_list **next = &list;
  *
  *	next = commit_list_append(c1, next);
  *	next = commit_list_append(c2, next);
  *	*next = NULL;
  *	assert(commit_list_count(list) == 2);
  *	return list;
  *
  * Don't forget to NULL-terminate!
  */
 struct commit_list **commit_list_append(struct commit *commit,
					struct commit_list **next)
 {
	struct commit_list *new = xmalloc(sizeof(*new_list));
	new->item = commit;
	*next = new;
	return &new->next;
 }

> +static void create_seq_dir(void)
> +{
> +	if (file_exists(git_path(SEQ_DIR))) {
> +		if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
> +			die(_("Could not remove %s"), git_path(SEQ_DIR));
> +	} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
> +		die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
> +}

> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> new file mode 100644

[...]

> +test_expect_success 'cherry-pick cleans up sequencer directory upon success' '
> +	pristine_detach initial &&
> +	git cherry-pick initial..picked &&
> +	test_path_is_missing .git/sequencer
> +'

Ramkumar Ramachandra· Jul 6, 2011, 11:55 UTC · re: Jonathan Nieder · lore

Re: [PATCH 10/14] revert: Persist data for continuation

Hi,

Jonathan Nieder writes:

> Ramkumar Ramachandra wrote:
>> Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than
>> one commit, 2010-06-02), a single invocation of "git cherry-pick" or
>> "git revert" can perform picks of several individual commits.  To
>> implement features like "--continue" to continue the whole operation,
>> we will need to store some information about the state and the plan at
>> the beginning.  Introduce a ".git/sequencer/head" file to store this
>> state, and ".git/sequencer/todo" file to store the plan.
>
> I think I remember Junio being curious about which commit is stored in
> "head"; this might be a good place to put a reminder so future readers
> don't have to be confused.

>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> @@ -25,6 +26,10 @@
>>   * Copyright (c) 2005 Junio C Hamano
>>   */
>>
>> +#define SEQ_DIR              "sequencer"
>> +#define SEQ_HEAD_FILE        "sequencer/head"
>> +#define SEQ_TODO_FILE        "sequencer/todo"
>
> Yay. :)

>> @@ -417,7 +422,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>>                       return error(_("Your index file is unmerged."));
>>       } else {
>>               if (get_sha1("HEAD", head))
>> -                     return error(_("You do not have a valid HEAD"));
>> +                     return error(_("Can't %s on an unborn branch"), me);
>
> Remember that "me" is an untranslated command name, and see also
> http://thread.gmane.org/gmane.comp.version-control.git/153026
>
> Perhaps it would make sense to do something like
>
>        if (get_sha1("HEAD", head)) {
>                if (opts->action == REVERT)
>                        return error(_("can't revert as initial commit"));
>                return error(_("cherry-pick into empty head not supported yet"));
>        }
>
> In a way they feel like different operations, anyway.  On the other
> hand, there's no reason I can think of not to allow reverting a patch
> that only removes files as the initial commit other than not having
> implemented it.

>> @@ -578,10 +583,106 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
>>       rollback_lock_file(&index_lock);
>>  }
>>
>> -static int pick_commits(struct replay_opts *opts)
>> +static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
>> +                     struct replay_opts *opts)
>> +{
>> +     struct commit_list *cur = NULL;
>> +     struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
>> +     const char *sha1_abbrev = NULL;
>> +     const char *action;
>> +
>> +     action = (opts->action == REVERT ? "revert" : "pick");
>> +     for (cur = todo_list; cur; cur = cur->next) {
>> +             sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
>> +             if (get_message(cur->item, &msg))
>> +                     die(_("Cannot get commit message for %s"), sha1_abbrev);
>> +             strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);
>
> Maybe some word like "command", "insn", or "keyword" would be more
> suggestive than "action".  It also might be worth mentioning somewhere
> (in the commit message?) that this format is inspired by
> rebase--interactive's insn sheet.

Okay, will do.

> The operation that could be exposed does not include get_revision,
> does it?
>
>  /*
>  * Example:
>  *
>  *     struct commit_list *list;
>  *     struct commit_list **next = &list;
>  *
>  *     next = commit_list_append(c1, next);
>  *     next = commit_list_append(c2, next);
>  *     *next = NULL;
>  *     assert(commit_list_count(list) == 2);
>  *     return list;
>  *
>  * Don't forget to NULL-terminate!
>  */
>  struct commit_list **commit_list_append(struct commit *commit,
>                                        struct commit_list **next)
>  {
>        struct commit_list *new = xmalloc(sizeof(*new_list));
>        new->item = commit;
>        *next = new;
>        return &new->next;
>  }

>> +static void create_seq_dir(void)
>> +{
>> +     if (file_exists(git_path(SEQ_DIR))) {
>> +             if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
>> +                     die(_("Could not remove %s"), git_path(SEQ_DIR));
>> +     } else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
>> +             die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
>> +}
>
> A local variable to cache the git_path result would make this much
> easier to read.

Okay.

>> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
>> new file mode 100644
>
> Should "chmod +x" so the test can be run directly (I forget to do that
> all the time).

Good catch.

>> +test_expect_success 'cherry-pick cleans up sequencer directory upon success' '
>> +     pristine_detach initial &&
>> +     git cherry-pick initial..picked &&
>> +     test_path_is_missing .git/sequencer
>> +'
>
> Thanks for thinking about these things.  Maybe another test
> demonstrating that the .git/sequencer directory is left behind on
> failure would help put this in context.

Good suggestion.

-- Ram

Junio C Hamano· Jul 6, 2011, 21:20 UTC · re: Jonathan Nieder · lore

Re: [PATCH 10/14] revert: Persist data for continuation

Jonathan Nieder <jrnieder@gmail.com> writes:

>  /*
>   * Example:
>   *
>   *	struct commit_list *list;
>   *	struct commit_list **next = &list;
>   *
>   *	next = commit_list_append(c1, next);
>   *	next = commit_list_append(c2, next);
>   *	*next = NULL;
>   *	assert(commit_list_count(list) == 2);
>   *	return list;
>   *
>   * Don't forget to NULL-terminate!
>   */

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 11/14] revert: Introduce a layer of indirection over pick_commits

Helped-by: Jonathan Nieder <jrnider@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   36 +++++++++++++++++++++++++-----------
 1 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 7d76f92..8cdcdb6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -677,10 +677,8 @@ static int cleanup_sequencer_data(void)
 	return 0;
 }
 
-static int pick_commits(struct replay_opts *opts)
+static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 {
-	struct commit_list *todo_list = NULL;
-	unsigned char sha1[20];
 	struct commit_list *cur;
 	int res;
 
@@ -690,12 +688,6 @@ static int pick_commits(struct replay_opts *opts)
 				opts->record_origin || opts->edit));
 	read_and_refresh_cache(me, opts);
 
-	walk_revs_populate_todo(&todo_list, opts);
-	create_seq_dir();
-	if (!get_sha1("HEAD", sha1))
-		save_head(sha1_to_hex(sha1));
-	save_todo(todo_list, opts);
-
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur, opts);
 		res = do_pick_commit(cur->item, opts);
@@ -710,6 +702,22 @@ static int pick_commits(struct replay_opts *opts)
 	return cleanup_sequencer_data();
 }
 
+static int process_continuation(struct replay_opts *opts)
+{
+	struct commit_list *todo_list = NULL;
+	unsigned char sha1[20];
+
+	read_and_refresh_cache(me, opts);
+
+	walk_revs_populate_todo(&todo_list, opts);
+	create_seq_dir();
+	if (!get_sha1("HEAD", sha1))
+		persist_head(sha1_to_hex(sha1));
+	persist_todo(todo_list, opts);
+
+	return pick_commits(todo_list, opts);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res;
@@ -722,7 +730,13 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	me = "revert";
 	parse_args(argc, argv, &opts);
-	res = pick_commits(&opts);
+
+	/*
+	 * Decide what to do depending on the arguments; a fresh
+	 * cherry-pick should be handled differently from an existing
+	 * one that is being continued
+	 */
+	res = process_continuation(&opts);
 	if (res < 0)
 		die(_("%s failed"), me);
 	return res;
@@ -738,7 +752,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	me = "cherry-pick";
 	parse_args(argc, argv, &opts);
-	res = pick_commits(&opts);
+	res = process_continuation(&opts);
 	if (res < 0)
 		die(_("%s failed"), me);
 	return res;

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 10:37 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits

Ramkumar Ramachandra wrote:

> Write a new function called process_continuation to prepare a
> todo_list to call pick_commits with; the job of pick_commits is
> simplified into performing the tasks listed in todo_list.

> +	/*
> +	 * Decide what to do depending on the arguments; a fresh
> +	 * cherry-pick should be handled differently from an existing
> +	 * one that is being continued
> +	 */
> +	res = process_continuation(&opts);

Ramkumar Ramachandra· Jul 6, 2011, 11:24 UTC · re: Jonathan Nieder · lore

Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits

Hi again,

Jonathan Nieder writes:

> Ramkumar Ramachandra wrote:
>> Write a new function called process_continuation to prepare a
>> todo_list to call pick_commits with; the job of pick_commits is
>> simplified into performing the tasks listed in todo_list.
>
> Why is it called process_continuation?  What is its responsibility?
> When would I call it?

>> +     /*
>> +      * Decide what to do depending on the arguments; a fresh
>> +      * cherry-pick should be handled differently from an existing
>> +      * one that is being continued
>> +      */
>> +     res = process_continuation(&opts);
>
> Is this the new sole entry point to the cherry-pick/revert machinery?

Yes.

> In that case, I'd be mildly tempted to call it something crazy like
> start_or_continue_replay(), and even more tempted to split it into
> separate entry points for new_replay(), continue_replay(),
> abort_replay(), and remove_replay_state() (but please don't trust me
> about the names; this is just to get the idea across).

-- Ram

Jonathan Nieder· Jul 6, 2011, 11:39 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits

Ramkumar Ramachandra wrote:

> Jonathan Nieder writes:

>> In that case, I'd be mildly tempted to call it something crazy like
>> start_or_continue_replay()

[...]

> Why? Is introducing new terminology so bad?  Should I explain what I
> mean by "continuation" in the commit message/ a comment?

Ramkumar Ramachandra· Jul 6, 2011, 11:44 UTC · re: Jonathan Nieder · lore

Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits

Hi,

Jonathan Nieder writes:

> Ramkumar Ramachandra wrote:
>> Jonathan Nieder writes:
>>> In that case, I'd be mildly tempted to call it something crazy like
>>> start_or_continue_replay()
> [...]
>> Why? Is introducing new terminology so bad?  Should I explain what I
>> mean by "continuation" in the commit message/ a comment?
>
> If "process_continuation" means "parse .git/sequencer state, which we
> are pretending is a serialized continuation object, and either (a)
> call it, (b) throw it away, or (c) modify it and then call it", then
> yes, how do you expect anyone to know what you are talking about?
>
> Less importantly, starting a cherry-pick (which is what pick_commits()
> already does) doesn't seem to fit in that picture.
>
> A simpler jargon-filled description of this model is checkpoint/
> restart.  But it is an incomplete analogy and still not a great name.
> With a goal of making future writers' lives happier and more
> productive in mind, I do not think it is often worth confusing them by
> choosing a clever presentation of ideas instead of a clear one.

Thanks.

-- Ram

Jonathan Nieder· Jul 6, 2011, 11:53 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits

Ramkumar Ramachandra wrote:

> Thanks for the elaborate explanation; I can see what's wrong with it
> now.  However, I "start_or_continue_or_stop_or_[insert more options
> here]_replay" isn't a good name.  I want something future-proof,
> because I intend to extend this with more nifty helpers like "skip
> one".  Your earlier "pick_revisions" suggestion doesn't sound like a
> bad alternative now -- let me know if you have any other suggestions.

Junio C Hamano· Jul 6, 2011, 21:00 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Write a new function called process_continuation to prepare a
> todo_list to call pick_commits with; the job of pick_commits is
> simplified into performing the tasks listed in todo_list.  This will
> be useful when continuation functionality like "--continue" is
> introduced later in the series.
>
> Helped-by: Jonathan Nieder <jrnider@gmail.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/revert.c |   36 +++++++++++++++++++++++++-----------
>  1 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 7d76f92..8cdcdb6 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -677,10 +677,8 @@ static int cleanup_sequencer_data(void)
>  	return 0;
>  }
>  
> -static int pick_commits(struct replay_opts *opts)
> +static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
>  {
> -	struct commit_list *todo_list = NULL;
> -	unsigned char sha1[20];
>  	struct commit_list *cur;
>  	int res;
>  
> @@ -690,12 +688,6 @@ static int pick_commits(struct replay_opts *opts)
>  				opts->record_origin || opts->edit));
>  	read_and_refresh_cache(me, opts);
>  
> -	walk_revs_populate_todo(&todo_list, opts);
> -	create_seq_dir();
> -	if (!get_sha1("HEAD", sha1))
> -		save_head(sha1_to_hex(sha1));
> -	save_todo(todo_list, opts);
> -
>  	for (cur = todo_list; cur; cur = cur->next) {
>  		save_todo(cur, opts);
>  		res = do_pick_commit(cur->item, opts);
> @@ -710,6 +702,22 @@ static int pick_commits(struct replay_opts *opts)
>  	return cleanup_sequencer_data();
>  }
>  
> +static int process_continuation(struct replay_opts *opts)
> +{
> +	struct commit_list *todo_list = NULL;
> +	unsigned char sha1[20];
> +
> +	read_and_refresh_cache(me, opts);
> +
> +	walk_revs_populate_todo(&todo_list, opts);
> +	create_seq_dir();
> +	if (!get_sha1("HEAD", sha1))
> +		persist_head(sha1_to_hex(sha1));
> +	persist_todo(todo_list, opts);

Don't these two need forward declarations before their use?

Ramkumar Ramachandra· Jul 7, 2011, 06:31 UTC · re: Junio C Hamano · lore

Re: [PATCH 11/14] revert: Introduce a layer of indirection over pick_commits

Hi Junio,

Junio C Hamano writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> diff --git a/builtin/revert.c b/builtin/revert.c
>> index 7d76f92..8cdcdb6 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -677,10 +677,8 @@ static int cleanup_sequencer_data(void)
>>       return 0;
>>  }
>>
>> -static int pick_commits(struct replay_opts *opts)
>> +static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
>>  {
>> -     struct commit_list *todo_list = NULL;
>> -     unsigned char sha1[20];
>>       struct commit_list *cur;
>>       int res;
>>
>> @@ -690,12 +688,6 @@ static int pick_commits(struct replay_opts *opts)
>>                               opts->record_origin || opts->edit));
>>       read_and_refresh_cache(me, opts);
>>
>> -     walk_revs_populate_todo(&todo_list, opts);
>> -     create_seq_dir();
>> -     if (!get_sha1("HEAD", sha1))
>> -             save_head(sha1_to_hex(sha1));
>> -     save_todo(todo_list, opts);
>> -
>>       for (cur = todo_list; cur; cur = cur->next) {
>>               save_todo(cur, opts);
>>               res = do_pick_commit(cur->item, opts);
>> @@ -710,6 +702,22 @@ static int pick_commits(struct replay_opts *opts)
>>       return cleanup_sequencer_data();
>>  }
>>
>> +static int process_continuation(struct replay_opts *opts)
>> +{
>> +     struct commit_list *todo_list = NULL;
>> +     unsigned char sha1[20];
>> +
>> +     read_and_refresh_cache(me, opts);
>> +
>> +     walk_revs_populate_todo(&todo_list, opts);
>> +     create_seq_dir();
>> +     if (!get_sha1("HEAD", sha1))
>> +             persist_head(sha1_to_hex(sha1));
>> +     persist_todo(todo_list, opts);
>
> Don't these two need forward declarations before their use?

Good catch.  I'm surprised the compiler didn't catch these.

-- Ram

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 This is perfect but for the fact that 'git reset --hard' doesn't blow
 away the sequencer state.  Why I haven't implemented that yet: should
 ONLY a hard reset blow away the state?

 builtin/revert.c                   |   54 ++++++++++++++++++++++++++++++-----
 git-rebase--interactive.sh         |   25 +++++++++++++---
 t/t3032-merge-recursive-options.sh |    2 +
 t/t3501-revert-cherry-pick.sh      |    1 +
 t/t3502-cherry-pick-merge.sh       |    9 ++++-
 t/t3504-cherry-pick-rerere.sh      |    2 +
 t/t3505-cherry-pick-empty.sh       |   14 ++++-----
 t/t3506-cherry-pick-ff.sh          |    3 ++
 t/t3507-cherry-pick-conflict.sh    |   24 ++++++++++++---
 t/t3510-cherry-pick-sequence.sh    |   14 +++++++++
 t/t7502-commit.sh                  |    1 +
 11 files changed, 121 insertions(+), 28 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 8cdcdb6..9e18d64 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -46,6 +46,8 @@ enum replay_action { REVERT, CHERRY_PICK };
 struct replay_opts {
 	enum replay_action action;
 
+	int reset;
+
 	/* Boolean options */
 	int edit;
 	int record_origin;
@@ -108,6 +110,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	int noop;
 	struct option options[] = {
+		OPT_BOOLEAN(0, "reset", &opts->reset, "forget the current operation"),
 		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
@@ -136,7 +139,21 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
 					PARSE_OPT_KEEP_ARGV0 |
 					PARSE_OPT_KEEP_UNKNOWN);
-	if (opts->commit_argc < 2)
+
+	/* Check for incompatible command line arguments */
+	if (opts->reset) {
+		verify_opt_compatible(me, "--reset",
+				"--no-commit", opts->no_commit,
+				"--signoff", opts->signoff,
+				"--mainline", opts->mainline,
+				"--strategy", opts->strategy ? 1 : 0,
+				"--strategy-option", opts->xopts ? 1 : 0,
+				"-x", opts->record_origin,
+				"--ff", opts->allow_ff,
+				NULL);
+	}
+
+	else if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
 	if (opts->allow_ff)
@@ -625,8 +642,11 @@ static void walk_revs_populate_todo(struct commit_list **todo_list,
 static void create_seq_dir(void)
 {
 	if (file_exists(git_path(SEQ_DIR))) {
-		if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
-			die(_("Could not remove %s"), git_path(SEQ_DIR));
+		error(_("%s already exists."), git_path(SEQ_DIR));
+		advise(_("This usually means that a %s operation is in progress."), me);
+		advise(_("Use %s --continue to continue the operation"), me);
+		advise(_("or use %s --reset to forget about it"), me);
+		die(_("%s failed"), me);
 	} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
 		die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
 }
@@ -709,13 +729,31 @@ static int process_continuation(struct replay_opts *opts)
 
 	read_and_refresh_cache(me, opts);
 
-	walk_revs_populate_todo(&todo_list, opts);
-	create_seq_dir();
-	if (!get_sha1("HEAD", sha1))
-		persist_head(sha1_to_hex(sha1));
-	persist_todo(todo_list, opts);
+	if (opts->reset) {
+		if (!file_exists(git_path(SEQ_TODO_FILE)))
+			goto error;
+		return cleanup_sequencer_data();
+	} else {
+		/*
+		 * Start a new cherry-pick/ revert sequence; but
+		 * first, make sure that an existing one isn't in
+		 * progress
+		 */
+		if (file_exists(git_path(SEQ_TODO_FILE))) {
+			error(_("A %s is already in progress"), me);
+			advise(_("Use %s --reset to forget about it"), me);
+			return -1;
+		}
 
+		walk_revs_populate_todo(&todo_list, opts);
+		create_seq_dir();
+		if (!get_sha1("HEAD", sha1))
+			save_head(sha1_to_hex(sha1));
+		save_todo(todo_list, opts);
+	}
 	return pick_commits(todo_list, opts);
+error:
+	return error(_("No %s in progress"), me);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 65690af..14c57dd 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -136,6 +136,10 @@ make_patch () {
 		get_author_ident_from_commit "$1" > "$author_script"
 }
 
+clear_cherry_pick_state () {
+	git cherry-pick --reset
+}
+
 die_with_patch () {
 	echo "$1" > "$state_dir"/stopped-sha
 	make_patch "$1"
@@ -279,8 +283,10 @@ pick_one_preserving_merges () {
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
 		*)
-			output git cherry-pick "$@" ||
+			output git cherry-pick "$@" || {
+				clear_cherry_pick_state
 				die_with_patch $sha1 "Could not pick $sha1"
+			}
 			;;
 		esac
 		;;
@@ -385,16 +391,20 @@ do_next () {
 		comment_for_reflog pick
 
 		mark_action_done
-		pick_one $sha1 ||
+		pick_one $sha1 || {
+			clear_cherry_pick_state
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		}
 		record_in_rewritten $sha1
 		;;
 	reword|r)
 		comment_for_reflog reword
 
 		mark_action_done
-		pick_one $sha1 ||
+		pick_one $sha1 || {
+			clear_cherry_pick_state
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		}
 		git commit --amend --no-post-rewrite
 		record_in_rewritten $sha1
 		;;
@@ -402,8 +412,10 @@ do_next () {
 		comment_for_reflog edit
 
 		mark_action_done
-		pick_one $sha1 ||
+		pick_one $sha1 || {
+			clear_cherry_pick_state
 			die_with_patch $sha1 "Could not apply $sha1... $rest"
+		}
 		echo "$sha1" > "$state_dir"/stopped-sha
 		make_patch $sha1
 		git rev-parse --verify HEAD > "$amend"
@@ -438,7 +450,10 @@ do_next () {
 		echo "$author_script_content" > "$author_script"
 		eval "$author_script_content"
 		output git reset --soft HEAD^
-		pick_one -n $sha1 || die_failed_squash $sha1 "$rest"
+		pick_one -n $sha1 || {
+			clear_cherry_pick_state
+			die_failed_squash $sha1 "$rest"
+		}
 		case "$(peek_next_command)" in
 		squash|s|fixup|f)
 			# This is an intermediate commit; its message will only be
diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
index 2b17311..81191ae 100755
--- a/t/t3032-merge-recursive-options.sh
+++ b/t/t3032-merge-recursive-options.sh
@@ -113,8 +113,10 @@ test_expect_success '--ignore-space-change makes merge succeed' '
 test_expect_success 'naive cherry-pick fails' '
 	git read-tree --reset -u HEAD &&
 	test_must_fail git cherry-pick --no-commit remote &&
+	git cherry-pick --reset &&
 	git read-tree --reset -u HEAD &&
 	test_must_fail git cherry-pick remote &&
+	git cherry-pick --reset &&
 	test_must_fail git update-index --refresh &&
 	grep "<<<<<<" text.txt
 '
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 595d2ff..e0c805d 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -96,6 +96,7 @@ test_expect_success 'revert forbidden on dirty working tree' '
 	echo content >extra_file &&
 	git add extra_file &&
 	test_must_fail git revert HEAD 2>errors &&
+	git revert --reset &&
 	test_i18ngrep "Your local changes would be overwritten by " errors
 
 '
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index 0ab52da..db21e47 100755
--- a/t/t3502-cherry-pick-merge.sh
+++ b/t/t3502-cherry-pick-merge.sh
@@ -36,6 +36,7 @@ test_expect_success 'cherry-pick a non-merge with -m should fail' '
 	git reset --hard &&
 	git checkout a^0 &&
 	test_must_fail git cherry-pick -m 1 b &&
+	git cherry-pick --reset &&
 	git diff --exit-code a --
 
 '
@@ -45,6 +46,7 @@ test_expect_success 'cherry pick a merge without -m should fail' '
 	git reset --hard &&
 	git checkout a^0 &&
 	test_must_fail git cherry-pick c &&
+	git cherry-pick --reset &&
 	git diff --exit-code a --
 
 '
@@ -71,8 +73,8 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
 
 	git reset --hard &&
 	git checkout b^0 &&
-	test_must_fail git cherry-pick -m 3 c
-
+	test_must_fail git cherry-pick -m 3 c &&
+	git cherry-pick --reset
 '
 
 test_expect_success 'revert a non-merge with -m should fail' '
@@ -80,6 +82,7 @@ test_expect_success 'revert a non-merge with -m should fail' '
 	git reset --hard &&
 	git checkout c^0 &&
 	test_must_fail git revert -m 1 b &&
+	git cherry-pick --reset &&
 	git diff --exit-code c
 
 '
@@ -89,6 +92,7 @@ test_expect_success 'revert a merge without -m should fail' '
 	git reset --hard &&
 	git checkout c^0 &&
 	test_must_fail git revert c &&
+	git cherry-pick --reset &&
 	git diff --exit-code c
 
 '
@@ -116,6 +120,7 @@ test_expect_success 'revert a merge relative to nonexistent parent should fail'
 	git reset --hard &&
 	git checkout c^0 &&
 	test_must_fail git revert -m 3 c &&
+	git cherry-pick --reset &&
 	git diff --exit-code c
 
 '
diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
index e6a6481..85d5fc1 100755
--- a/t/t3504-cherry-pick-rerere.sh
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -29,6 +29,7 @@ test_expect_success 'fixup' '
 
 test_expect_success 'cherry-pick conflict' '
 	test_must_fail git cherry-pick master &&
+	git cherry-pick --reset &&
 	test_cmp expect foo
 '
 
@@ -39,6 +40,7 @@ test_expect_success 'reconfigure' '
 
 test_expect_success 'cherry-pick conflict without rerere' '
 	test_must_fail git cherry-pick master &&
+	git cherry-pick --reset &&
 	test_must_fail test_cmp expect foo
 '
 
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index c10b28c..dc02227 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -23,10 +23,9 @@ test_expect_success setup '
 '
 
 test_expect_success 'cherry-pick an empty commit' '
-	git checkout master && {
-		git cherry-pick empty-branch^
-		test "$?" = 1
-	}
+	git checkout master &&
+	test_expect_code 1 git cherry-pick empty-branch^
+	git cherry-pick --reset
 '
 
 test_expect_success 'index lockfile was removed' '
@@ -36,10 +35,9 @@ test_expect_success 'index lockfile was removed' '
 '
 
 test_expect_success 'cherry-pick a commit with an empty message' '
-	git checkout master && {
-		git cherry-pick empty-branch
-		test "$?" = 1
-	}
+	git checkout master &&
+	test_expect_code 1 git cherry-pick empty-branch &&
+	git cherry-pick --reset
 '
 
 test_expect_success 'index lockfile was removed' '
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index 51ca391..abd63a5 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -67,12 +67,14 @@ test_expect_success 'merge setup' '
 test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '
 	git reset --hard A -- &&
 	test_must_fail git cherry-pick --ff -m 1 B &&
+	git cherry-pick --reset &&
 	git diff --exit-code A --
 '
 
 test_expect_success 'cherry pick a merge with --ff but without -m should fail' '
 	git reset --hard A -- &&
 	test_must_fail git cherry-pick --ff C &&
+	git cherry-pick --reset &&
 	git diff --exit-code A --
 '
 
@@ -93,6 +95,7 @@ test_expect_success 'cherry pick with --ff a merge (2)' '
 test_expect_success 'cherry pick a merge relative to nonexistent parent with --ff should fail' '
 	git reset --hard B -- &&
 	test_must_fail git cherry-pick --ff -m 3 C
+	git cherry-pick --reset
 '
 
 test_expect_success 'cherry pick a root commit with --ff' '
diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 212ec54..86f8626 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -39,6 +39,7 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '
 
 	head=$(git rev-parse HEAD) &&
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --reset &&
 	newhead=$(git rev-parse HEAD) &&
 
 	test "$head" = "$newhead"
@@ -55,6 +56,7 @@ test_expect_success 'advice from failed cherry-pick' "
 	hint: and commit the result with 'git commit'
 	EOF
 	test_must_fail git cherry-pick picked 2>actual &&
+	git cherry-pick --reset &&
 
 	test_i18ncmp expected actual
 "
@@ -62,7 +64,8 @@ test_expect_success 'advice from failed cherry-pick' "
 test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick picked &&
-	test_cmp_rev picked CHERRY_PICK_HEAD
+	test_cmp_rev picked CHERRY_PICK_HEAD &&
+	git cherry-pick --reset
 '
 
 test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' '
@@ -84,13 +87,15 @@ test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '
 		export GIT_CHERRY_PICK_HELP &&
 		test_must_fail git cherry-pick picked
 	) &&
-	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
+	git cherry-pick --reset
 '
 
 test_expect_success 'git reset clears CHERRY_PICK_HEAD' '
 	pristine_detach initial &&
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --reset &&
 	git reset &&
 
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
@@ -102,7 +107,8 @@ test_expect_success 'failed commit does not clear CHERRY_PICK_HEAD' '
 	test_must_fail git cherry-pick picked &&
 	test_must_fail git commit &&
 
-	test_cmp_rev picked CHERRY_PICK_HEAD
+	test_cmp_rev picked CHERRY_PICK_HEAD &&
+	git cherry-pick --reset
 '
 
 test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' '
@@ -119,7 +125,8 @@ test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' '
 		test_must_fail git commit
 	) &&
 
-	test_cmp_rev picked CHERRY_PICK_HEAD
+	test_cmp_rev picked CHERRY_PICK_HEAD &&
+	git cherry-pick --reset
 '
 
 test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '
@@ -130,13 +137,15 @@ test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '
 	git add foo &&
 	git commit &&
 
-	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
+	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
+	git cherry-pick --reset
 '
 
 test_expect_success 'failed cherry-pick produces dirty index' '
 	pristine_detach initial &&
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --reset &&
 
 	test_must_fail git update-index --refresh -q &&
 	test_must_fail git diff-index --exit-code HEAD
@@ -160,6 +169,7 @@ test_expect_success 'failed cherry-pick registers participants in index' '
 	git read-tree -u --reset HEAD &&
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --reset &&
 	git ls-files --stage --unmerged > actual &&
 
 	test_cmp expected actual
@@ -176,6 +186,7 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' '
 	EOF
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --reset &&
 
 	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
 	test_cmp expected actual
@@ -195,6 +206,7 @@ test_expect_success 'diff3 -m style' '
 	EOF
 
 	test_must_fail git cherry-pick picked &&
+	git cherry-pick --reset &&
 
 	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
 	test_cmp expected actual
@@ -227,6 +239,7 @@ test_expect_success 'revert also handles conflicts sanely' '
 
 	head=$(git rev-parse HEAD) &&
 	test_must_fail git revert picked &&
+	git revert --reset &&
 	newhead=$(git rev-parse HEAD) &&
 	git ls-files --stage --unmerged > actual-stages &&
 
@@ -252,6 +265,7 @@ test_expect_success 'revert conflict, diff3 -m style' '
 	EOF
 
 	test_must_fail git revert picked &&
+	git revert --reset &&
 
 	sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
 	test_cmp expected actual
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 83e2722..9e97a7b 100644
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -34,4 +34,18 @@ test_expect_success 'cherry-pick cleans up sequencer directory upon success' '
 	test_path_is_missing .git/sequencer
 '
 
+test_expect_success '--reset complains when no cherry-pick is in progress' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --reset >actual 2>&1 &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success '--reset cleans up sequencer directory' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	git cherry-pick --reset &&
+	test_path_is_missing .git/sequencer
+'
+
 test_done
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 3f3adc3..0d54027 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -291,6 +291,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' '
 	git commit -m second &&
 	# Must fail due to conflict
 	test_must_fail git cherry-pick -n master &&
+	git cherry-pick --reset &&
 	echo "editor not started" >.git/result &&
 	(
 		GIT_EDITOR="$(pwd)/.git/FAKE_EDITOR" &&

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 10:14 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

Ramkumar Ramachandra wrote:

>  This is perfect but for the fact that 'git reset --hard' doesn't blow
>  away the sequencer state.  Why I haven't implemented that yet: should
>  ONLY a hard reset blow away the state?

I don't know.  What do you think?

The constraints I see are:

 1. outside scripts that use "git cherry-pick" should continue to work

 2. as a small indication that that's vaguely possible, unrelated parts
    of the test suite should not need to be patched

 3. when a person uses commands like "git reset --hard" without
   _intending_ to blow away the sequencer state, it should be possible
   to get the sequencer state back.

Ramkumar Ramachandra· Jul 6, 2011, 10:55 UTC · re: Jonathan Nieder · lore

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

Hi,

Jonathan Nieder writes:

> Ramkumar Ramachandra wrote:
>>  This is perfect but for the fact that 'git reset --hard' doesn't blow
>>  away the sequencer state.  Why I haven't implemented that yet: should
>>  ONLY a hard reset blow away the state?
>
> I don't know.  What do you think?

> The constraints I see are:
>
>  1. outside scripts that use "git cherry-pick" should continue to work
>
>  2. as a small indication that that's vaguely possible, unrelated parts
>    of the test suite should not need to be patched

>  3. when a person uses commands like "git reset --hard" without
>   _intending_ to blow away the sequencer state, it should be possible
>   to get the sequencer state back.

> For dealing with "git rebase --interactive" and similar porcelain-ish
> scripts, the CHERRY_PICK_HEAD code-path has its own trick of falling
> back to traditional behavior when the GIT_CHERRY_PICK_HELP environment
> variable is set (see v1.5.4-rc0~106^2~1, revert/cherry-pick: Allow
> overriding the help text by the calling Porcelain, 2007-11-28).

Ah, I see.

-- Ram

Ramkumar Ramachandra· Jul 6, 2011, 14:32 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

Hi,

Ramkumar Ramachandra writes:

>  builtin/revert.c                   |   54 ++++++++++++++++++++++++++++++-----
>  git-rebase--interactive.sh         |   25 +++++++++++++---
>  t/t3032-merge-recursive-options.sh |    2 +
>  t/t3501-revert-cherry-pick.sh      |    1 +
>  t/t3502-cherry-pick-merge.sh       |    9 ++++-
>  t/t3504-cherry-pick-rerere.sh      |    2 +
>  t/t3505-cherry-pick-empty.sh       |   14 ++++-----
>  t/t3506-cherry-pick-ff.sh          |    3 ++
>  t/t3507-cherry-pick-conflict.sh    |   24 ++++++++++++---
>  t/t3510-cherry-pick-sequence.sh    |   14 +++++++++
>  t/t7502-commit.sh                  |    1 +
>  11 files changed, 121 insertions(+), 28 deletions(-)

> diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
> index 2b17311..81191ae 100755
> --- a/t/t3032-merge-recursive-options.sh
> +++ b/t/t3032-merge-recursive-options.sh
> @@ -113,8 +113,10 @@ test_expect_success '--ignore-space-change makes merge succeed' '
>  test_expect_success 'naive cherry-pick fails' '
>        git read-tree --reset -u HEAD &&
>        test_must_fail git cherry-pick --no-commit remote &&
> +       git cherry-pick --reset &&
>        git read-tree --reset -u HEAD &&
>        test_must_fail git cherry-pick remote &&
> +       git cherry-pick --reset &&
>        test_must_fail git update-index --refresh &&
>        grep "<<<<<<" text.txt
>  '
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 595d2ff..e0c805d 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -96,6 +96,7 @@ test_expect_success 'revert forbidden on dirty working tree' '
>        echo content >extra_file &&
>        git add extra_file &&
>        test_must_fail git revert HEAD 2>errors &&
> +       git revert --reset &&
>        test_i18ngrep "Your local changes would be overwritten by " errors
>
>  '

> diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
> index c10b28c..dc02227 100755
> --- a/t/t3505-cherry-pick-empty.sh
> +++ b/t/t3505-cherry-pick-empty.sh
> @@ -23,10 +23,9 @@ test_expect_success setup '
>  '
>
>  test_expect_success 'cherry-pick an empty commit' '
> -       git checkout master && {
> -               git cherry-pick empty-branch^
> -               test "$?" = 1
> -       }
> +       git checkout master &&
> +       test_expect_code 1 git cherry-pick empty-branch^
> +       git cherry-pick --reset
>  '
>
>  test_expect_success 'index lockfile was removed' '
> @@ -36,10 +35,9 @@ test_expect_success 'index lockfile was removed' '
>  '
>
>  test_expect_success 'cherry-pick a commit with an empty message' '
> -       git checkout master && {
> -               git cherry-pick empty-branch
> -               test "$?" = 1
> -       }
> +       git checkout master &&
> +       test_expect_code 1 git cherry-pick empty-branch &&
> +       git cherry-pick --reset
>  '
>
>  test_expect_success 'index lockfile was removed' '

> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
> index 212ec54..86f8626 100755
> --- a/t/t3507-cherry-pick-conflict.sh
> +++ b/t/t3507-cherry-pick-conflict.sh
> @@ -39,6 +39,7 @@ test_expect_success 'failed cherry-pick does not advance HEAD' '
>
>        head=$(git rev-parse HEAD) &&
>        test_must_fail git cherry-pick picked &&
> +       git cherry-pick --reset &&
>        newhead=$(git rev-parse HEAD) &&
>
>        test "$head" = "$newhead"
> @@ -55,6 +56,7 @@ test_expect_success 'advice from failed cherry-pick' "
>        hint: and commit the result with 'git commit'
>        EOF
>        test_must_fail git cherry-pick picked 2>actual &&
> +       git cherry-pick --reset &&
>
>        test_i18ncmp expected actual
>  "
> @@ -62,7 +64,8 @@ test_expect_success 'advice from failed cherry-pick' "
>  test_expect_success 'failed cherry-pick sets CHERRY_PICK_HEAD' '
>        pristine_detach initial &&
>        test_must_fail git cherry-pick picked &&
> -       test_cmp_rev picked CHERRY_PICK_HEAD
> +       test_cmp_rev picked CHERRY_PICK_HEAD &&
> +       git cherry-pick --reset
>  '
>
>  test_expect_success 'successful cherry-pick does not set CHERRY_PICK_HEAD' '
> @@ -84,13 +87,15 @@ test_expect_success 'GIT_CHERRY_PICK_HELP suppresses CHERRY_PICK_HEAD' '
>                export GIT_CHERRY_PICK_HELP &&
>                test_must_fail git cherry-pick picked
>        ) &&
> -       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
> +       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
> +       git cherry-pick --reset
>  '
>
>  test_expect_success 'git reset clears CHERRY_PICK_HEAD' '
>        pristine_detach initial &&
>
>        test_must_fail git cherry-pick picked &&
> +       git cherry-pick --reset &&
>        git reset &&
>
>        test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
> @@ -102,7 +107,8 @@ test_expect_success 'failed commit does not clear CHERRY_PICK_HEAD' '
>        test_must_fail git cherry-pick picked &&
>        test_must_fail git commit &&
>
> -       test_cmp_rev picked CHERRY_PICK_HEAD
> +       test_cmp_rev picked CHERRY_PICK_HEAD &&
> +       git cherry-pick --reset
>  '
>
>  test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' '
> @@ -119,7 +125,8 @@ test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' '
>                test_must_fail git commit
>        ) &&
>
> -       test_cmp_rev picked CHERRY_PICK_HEAD
> +       test_cmp_rev picked CHERRY_PICK_HEAD &&
> +       git cherry-pick --reset
>  '
>
>  test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '
> @@ -130,13 +137,15 @@ test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '
>        git add foo &&
>        git commit &&
>
> -       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
> +       test_must_fail git rev-parse --verify CHERRY_PICK_HEAD &&
> +       git cherry-pick --reset
>  '
>
>  test_expect_success 'failed cherry-pick produces dirty index' '
>        pristine_detach initial &&
>
>        test_must_fail git cherry-pick picked &&
> +       git cherry-pick --reset &&
>
>        test_must_fail git update-index --refresh -q &&
>        test_must_fail git diff-index --exit-code HEAD
> @@ -160,6 +169,7 @@ test_expect_success 'failed cherry-pick registers participants in index' '
>        git read-tree -u --reset HEAD &&
>
>        test_must_fail git cherry-pick picked &&
> +       git cherry-pick --reset &&
>        git ls-files --stage --unmerged > actual &&
>
>        test_cmp expected actual
> @@ -176,6 +186,7 @@ test_expect_success 'failed cherry-pick describes conflict in work tree' '
>        EOF
>
>        test_must_fail git cherry-pick picked &&
> +       git cherry-pick --reset &&
>
>        sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
>        test_cmp expected actual
> @@ -195,6 +206,7 @@ test_expect_success 'diff3 -m style' '
>        EOF
>
>        test_must_fail git cherry-pick picked &&
> +       git cherry-pick --reset &&
>
>        sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
>        test_cmp expected actual
> @@ -227,6 +239,7 @@ test_expect_success 'revert also handles conflicts sanely' '
>
>        head=$(git rev-parse HEAD) &&
>        test_must_fail git revert picked &&
> +       git revert --reset &&
>        newhead=$(git rev-parse HEAD) &&
>        git ls-files --stage --unmerged > actual-stages &&
>
> @@ -252,6 +265,7 @@ test_expect_success 'revert conflict, diff3 -m style' '
>        EOF
>
>        test_must_fail git revert picked &&
> +       git revert --reset &&
>
>        sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
>        test_cmp expected actual

-- Ram

Jonathan Nieder· Jul 6, 2011, 19:21 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

Ramkumar Ramachandra wrote:

>> --- a/t/t3507-cherry-pick-conflict.sh
>> +++ b/t/t3507-cherry-pick-conflict.sh

[...]

> As you can see, there is no "reset --hard" in these

>> --- a/t/t3505-cherry-pick-empty.sh
>> +++ b/t/t3505-cherry-pick-empty.sh

[...]

> There is however, one other thing I can do: if there is
> nothing left to cherry-pick after a successful conflict resolution +
> git commit, I can modify commit.c to blow away the sequencer state
> after checking appropriately.  This will also have a nice end-user
> experience side-effect:
> $ git cherry-pick moo
> fatal: Conflict in foo!
> $ echo "Resolved" > foo
> $ git add moo
> $ git commit
> $ git cherry-pick --continue # This no-op will be unnecessary

> Then again, teaching commit about the sequencer is inelegant,

> and it's
> possible to achieve this effect in another way: when a conflict is
> encountered in the sequencer && length(todo_file) == 1, throw away the
> sequencer state.

Jonathan Nieder· Jul 6, 2011, 19:56 UTC · re: Jonathan Nieder · lore

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

Jonathan Nieder wrote:

> Ramkumar Ramachandra wrote:

>> and it's
>> possible to achieve this effect in another way: when a conflict is
>> encountered in the sequencer && length(todo_file) == 1, throw away the
>> sequencer state.
>
> Yep, that seems like basically the same effect.  Are there downsides?
> (Maybe years from now when a "git cherry-pick --rewind" is introduced
> we would regret this?  But that can be figured out years from now.)

Ramkumar Ramachandra· Jul 7, 2011, 03:03 UTC · re: Jonathan Nieder · lore

Re: [PATCH 12/14] revert: Introduce --reset to cleanup sequencer data

Hi again,

Jonathan Nieder writes:

> Ramkumar Ramachandra wrote:
>
>>> --- a/t/t3507-cherry-pick-conflict.sh
>>> +++ b/t/t3507-cherry-pick-conflict.sh
> [...]
>> As you can see, there is no "reset --hard" in these
>
> In this one, there's "git checkout -f && git read-tree -u --reset HEAD
> && git clean -fdx", which is almost the same thing.

>>> --- a/t/t3505-cherry-pick-empty.sh
>>> +++ b/t/t3505-cherry-pick-empty.sh
>
> This one is not a typical script, I think --- if you knew the
> cherry-pick was going to be empty, why did you try it in the first
> place?  I think it would make sense to make it "git reset --hard" at
> the beginning of each test as a separate, preparatory patch with
> explanation.
>
> [...]
>> There is however, one other thing I can do: if there is
>> nothing left to cherry-pick after a successful conflict resolution +
>> git commit, I can modify commit.c to blow away the sequencer state
>> after checking appropriately.  This will also have a nice end-user
>> experience side-effect:
>> $ git cherry-pick moo
>> fatal: Conflict in foo!
>> $ echo "Resolved" > foo
>> $ git add moo
>> $ git commit
>> $ git cherry-pick --continue # This no-op will be unnecessary
>
> Though it's not obvious to me how this would affect the scripts above,
> it sounds like a nice enhancement to me independently, fwiw.

>> Then again, teaching commit about the sequencer is inelegant,
>
> It's possible to add some hook-like thing to do this, or to structure
> the code as if a hook was used.
>
>> and it's
>> possible to achieve this effect in another way: when a conflict is
>> encountered in the sequencer && length(todo_file) == 1, throw away the
>> sequencer state.
>
> Yep, that seems like basically the same effect.  Are there downsides?
> (Maybe years from now when a "git cherry-pick --rewind" is introduced
> we would regret this?  But that can be figured out years from now.)

> Doh, I'm not thinking straight.  Would this break "git cherry-pick
> --abort", or is there some hack layered on top to avoid that?  Making
> "git commit" remove the .git/sequencer for the last commit of the
> sequence seems a little saner.

[1]: https://github.com/artagnon/git/commit/0653bcccfa8d69687ed939f07f5b32dd14d302d3

-- Ram

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[PATCH 13/14] revert: Introduce --continue to continue the operation

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 As suggested by Miles and Junio, I've obfuscated the variable name to
 "contin".  Would it make sense to persist the entire replay_opts
 structure (so that it applies to all the commits, and no
 commit-specific command-line options are supported) somewhere?  Where
 and how?  My idea is that patch can be considered for inclusion then.
 Patch 14 onwards can then start adding features like "commit-specific
 command-line options", "mixed commands" etc.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 9e18d64..6ef56ee 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -47,6 +47,7 @@ struct replay_opts {
 	enum replay_action action;
 
 	int reset;
+	int contin;
 
 	/* Boolean options */
 	int edit;
@@ -105,12 +106,37 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 	va_end(ap);
 }
 
+static void verify_opt_mutually_compatible(const char *me, ...)
+{
+	const char *opt1, *opt2;
+	va_list ap;
+	int set;
+
+	va_start(ap, me);
+	while ((opt1 = va_arg(ap, const char *))) {
+		set = va_arg(ap, int);
+		if (set)
+			break;
+	}
+	if (!opt1)
+		goto ok;
+	while ((opt2 = va_arg(ap, const char *))) {
+		set = va_arg(ap, int);
+		if (set)
+			die(_("%s: %s cannot be used with %s"),
+				me, opt1, opt2);
+	}
+ok:
+	va_end(ap);
+}
+
 static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	int noop;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "reset", &opts->reset, "forget the current operation"),
+		OPT_BOOLEAN(0, "continue", &opts->contin, "continue the current operation"),
 		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
 		{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
@@ -140,9 +166,21 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 					PARSE_OPT_KEEP_ARGV0 |
 					PARSE_OPT_KEEP_UNKNOWN);
 
+	/* Check for mutually incompatible command line arguments */
+	verify_opt_mutually_compatible(me,
+				"--reset", opts->reset,
+				"--continue", opts->contin,
+				NULL);
+
 	/* Check for incompatible command line arguments */
-	if (opts->reset) {
-		verify_opt_compatible(me, "--reset",
+	if (opts->reset || opts->contin) {
+		char *this_operation;
+		if (opts->reset)
+			this_operation = "--reset";
+		else
+			this_operation = "--continue";
+
+		verify_opt_compatible(me, this_operation,
 				"--no-commit", opts->no_commit,
 				"--signoff", opts->signoff,
 				"--mainline", opts->mainline,
@@ -617,6 +655,83 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
 	}
 }
 
+static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+{
+	unsigned char commit_sha1[20];
+	char sha1_abbrev[40];
+	struct commit *commit;
+	enum replay_action action;
+	int insn_len = 0;
+	char *p;
+
+	p = start;
+	if (!(p = strchr(p, ' ')))
+		return NULL;
+	insn_len = p - start;
+	if (!(p = strchr(p + 1, ' ')))
+		return NULL;
+	p += 1;
+	strlcpy(sha1_abbrev, start + insn_len + 1,
+		p - (start + insn_len + 1));
+
+	if (!strncmp(start, "pick", insn_len))
+		action = CHERRY_PICK;
+	else if (!strncmp(start, "revert", insn_len))
+		action = REVERT;
+	else
+		return NULL;
+
+	/*
+	 * Verify that the action matches up with the one in
+	 * opts; we don't support arbitrary instructions
+	 */
+	if (action != opts->action)
+		return NULL;
+
+	if ((get_sha1(sha1_abbrev, commit_sha1) < 0)
+		|| !(commit = lookup_commit_reference(commit_sha1)))
+		return NULL;
+
+	return commit;
+}
+
+static void read_populate_todo(struct commit_list **todo_list,
+			struct replay_opts *opts)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct commit_list *new;
+	struct commit_list **next;
+	struct commit *commit;
+	char *p;
+	int fd;
+
+	fd = open(git_path(SEQ_TODO_FILE), O_RDONLY);
+	if (fd < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not open %s."), git_path(SEQ_TODO_FILE));
+	}
+	if (strbuf_read(&buf, fd, 0) < buf.len) {
+		close(fd);
+		strbuf_release(&buf);
+		die(_("Could not read %s."), git_path(SEQ_TODO_FILE));
+	}
+	close(fd);
+
+	next = todo_list;
+	for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
+		if (!(commit = parse_insn_line(p, opts))) {
+			strbuf_release(&buf);
+			die(_("Malformed instruction sheet: %s"), git_path(SEQ_TODO_FILE));
+		}
+		new = xmalloc(sizeof(struct commit_list));
+		new->item = commit;
+		*next = new;
+		next = &new->next;
+	}
+	*next = NULL;
+	strbuf_release(&buf);
+}
+
 static void walk_revs_populate_todo(struct commit_list **todo_list,
 				struct replay_opts *opts)
 {
@@ -733,6 +848,14 @@ static int process_continuation(struct replay_opts *opts)
 		if (!file_exists(git_path(SEQ_TODO_FILE)))
 			goto error;
 		return cleanup_sequencer_data();
+	} else if (opts->contin) {
+		if (!file_exists(git_path(SEQ_TODO_FILE)))
+			goto error;
+		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
@@ -741,7 +864,8 @@ static int process_continuation(struct replay_opts *opts)
 		 */
 		if (file_exists(git_path(SEQ_TODO_FILE))) {
 			error(_("A %s is already in progress"), me);
-			advise(_("Use %s --reset to forget about it"), me);
+			advise(_("Use %s --continue to continue the operation"), me);
+			advise(_("or use %s --reset to forget about it"), me);
 			return -1;
 		}
 
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 9e97a7b..7a57216 100644
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -48,4 +48,72 @@ test_expect_success '--reset cleans up sequencer directory' '
 	test_path_is_missing .git/sequencer
 '
 
+test_expect_success '--continue complains when no cherry-pick is in progress' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick --continue >actual 2>&1 &&
+	test_i18ngrep "error" actual
+'
+
+test_expect_success '--continue complains when there are unresolved conflicts' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	test_must_fail git cherry-pick --continue &&
+	git cherry-pick --reset
+'
+
+test_expect_success '--continue continues after conflicts are resolved' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	git cherry-pick --continue &&
+	test_path_is_missing .git/sequencer &&
+	{
+		git rev-list HEAD |
+		git diff-tree --root --stdin |
+		sed "s/[0-9a-f]\{40\}/OBJID/g"
+	} >actual &&
+	cat >expect <<-\EOF &&
+	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_cmp expect actual
+'
+
+test_expect_success 'malformed instruction sheet 1' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick /pick/" .git/sequencer/todo >new_sheet
+	cp new_sheet .git/sequencer/todo
+	test_must_fail git cherry-pick --continue >actual 2>&1 &&
+	git cherry-pick --reset &&
+	test_i18ngrep "fatal" actual
+'
+
+test_expect_success 'malformed instruction sheet 2' '
+	pristine_detach initial &&
+	head=$(git rev-parse HEAD) &&
+	test_must_fail git cherry-pick base..picked &&
+	echo "resolved" >foo &&
+	git add foo &&
+	git commit &&
+	sed "s/pick/revert/" .git/sequencer/todo >new_sheet
+	cp new_sheet .git/sequencer/todo
+	test_must_fail git cherry-pick --continue >actual 2>&1 &&
+	git cherry-pick --reset &&
+	test_i18ngrep "fatal" actual
+'
+
 test_done

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 10:25 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 13/14] revert: Introduce --continue to continue the operation

Ramkumar Ramachandra wrote:

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -47,6 +47,7 @@ struct replay_opts {
>  	enum replay_action action;
>
>  	int reset;
> +	int contin;

Maybe:

	int just_remove_state;
	int resume;
	int abort;

Or:

	enum replay_subcommand {
		REPLAY_RESET,
		REPLAY_CONTINUE,
		REPLAY_ABORT
	};

	enum replay_subcommand subcommand;

Junio C Hamano· Jul 6, 2011, 21:21 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 13/14] revert: Introduce --continue to continue the operation

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 9e18d64..6ef56ee 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -47,6 +47,7 @@ struct replay_opts {
>  	enum replay_action action;
>  
>  	int reset;
> +	int contin;

As Jonathan mentiond, these three look like all action's to me.

> +static void verify_opt_mutually_compatible(const char *me, ...)
> +{

Isn't "being compatible" by definition "mutual"?  

I.e. verify-option-compatibility perhaps?

Drew Northup· Jul 6, 2011, 21:52 UTC · re: Junio C Hamano · lore

Re: [PATCH 13/14] revert: Introduce --continue to continue the operation

On Wed, 2011-07-06 at 14:21 -0700, Junio C Hamano wrote:

> > +static void verify_opt_mutually_compatible(const char *me, ...)
> > +{
> 
> Isn't "being compatible" by definition "mutual"?  

> 
> I.e. verify-option-compatibility perhaps?

+1 sensible, if that's the intention.

-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Ramkumar Ramachandra· Jul 7, 2011, 06:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 13/14] revert: Introduce --continue to continue the operation

Hi Junio,

Junio C Hamano writes:

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> diff --git a/builtin/revert.c b/builtin/revert.c
>> index 9e18d64..6ef56ee 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -47,6 +47,7 @@ struct replay_opts {
>>       enum replay_action action;
>>
>>       int reset;
>> +     int contin;
>
> As Jonathan mentiond, these three look like all action's to me.

True.  I'll try to figure something out before the next iteration.

>> +static void verify_opt_mutually_compatible(const char *me, ...)
>> +{
>
> Isn't "being compatible" by definition "mutual"?
>
> I.e. verify-option-compatibility perhaps?

-- Ram

Ramkumar Ramachandra· Jul 6, 2011, 07:54 UTC · re: Ramkumar Ramachandra · lore

[RFC PATCH 14/14] revert: Change insn sheet format

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 I've intentionally left parse_cmdline_args unimplemented -- Although
 Stephen (and Christian subsequently) have written a parser [1], I'm
 not happy with it.  The point of this patch is to illustrate the
 problem with this instruction sheet format, and provoke some
 discussion about this.  I don't want to parse command-line arguments
 by hand handling all sorts of corner cases in quoting etc -- is there
 any other way?  Existing implementations in libraries like Glib are
 much too heavyweight.

 Note that I've used the '#' character as a separator to simplify
 parsing.  It's not the most elegant solution, but I think it should
 work.
 
 [1]: http://article.gmane.org/gmane.comp.version-control.git/162198

 builtin/revert.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 6ef56ee..a0a4377 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -638,6 +638,34 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
+static void format_args(struct strbuf *buf, struct replay_opts *opts)
+{
+	int i;
+
+	if (opts->no_commit)
+		strbuf_addstr(buf, "-n ");
+	if (opts->edit)
+		strbuf_addstr(buf, "-e ");
+	if (opts->signoff)
+		strbuf_addstr(buf, "-s ");
+	if (opts->mainline)
+		strbuf_addf(buf, "-m %d ", opts->mainline);
+	if (opts->strategy)
+		strbuf_addf(buf, "--strategy %s ", opts->strategy);
+	if (opts->xopts)
+		for (i = 0; i < opts->xopts_nr; i ++)
+			strbuf_addf(buf, "-X %s ", opts->xopts[i]);
+	if (opts->record_origin)
+		strbuf_addstr(buf, "-x ");
+	if (opts->allow_ff)
+		strbuf_addstr(buf, "--ff ");
+}
+
+/*
+ * Instruction sheet format ::
+ * pick -s 537d2e # revert: Introduce --continue to continue the operation
+ * pick 4a15c1 # revert: Introduce --reset to cleanup sequencer data
+ */
 static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
 			struct replay_opts *opts)
 {
@@ -651,29 +679,53 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
 		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
 		if (get_message(cur->item, &msg))
 			die(_("Cannot get commit message for %s"), sha1_abbrev);
-		strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);
+		strbuf_addf(buf, "%s ", action);
+		format_args(buf, opts);
+		strbuf_addf(buf, "%s # %s\n", sha1_abbrev, msg.subject);
 	}
 }
 
-static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
+static void parse_cmdline_args(struct strbuf *args_to_parse,
+			int *argc, const char ***argv)
+{
+	return;
+}
+
+static struct commit *parse_insn_line(const char *start, struct replay_opts *opts)
 {
 	unsigned char commit_sha1[20];
 	char sha1_abbrev[40];
 	struct commit *commit;
 	enum replay_action action;
 	int insn_len = 0;
-	char *p;
+	char *p, *end;
+
+	int argc = 0;
+	const char **argv = NULL;
+	struct strbuf args_to_parse = STRBUF_INIT;
 
-	p = start;
+	p = (char *) start;
 	if (!(p = strchr(p, ' ')))
 		return NULL;
 	insn_len = p - start;
-	if (!(p = strchr(p + 1, ' ')))
+	if (!(end = strchr(p + 1, '#')))
 		return NULL;
-	p += 1;
-	strlcpy(sha1_abbrev, start + insn_len + 1,
-		p - (start + insn_len + 1));
+	*(end - 1) = '\0';
+	strbuf_addstr(&args_to_parse, p + 1);
+
+	/* Parse argc, argv out of args_to_parse */
+	/* TODO: Implement parse_cmdline_args! */
+	parse_cmdline_args(&args_to_parse, &argc, &argv);
+	strbuf_release(&args_to_parse);
+	if (argc)
+		parse_args(argc, argv, opts);
+
+	/* Copy out the sha1_abbrev explicitly */
+	if (!(p = strrchr(p, ' ')))
+		return NULL;
+	strcpy(sha1_abbrev, p + 1);
 
+	/* Determine the action */
 	if (!strncmp(start, "pick", insn_len))
 		action = CHERRY_PICK;
 	else if (!strncmp(start, "revert", insn_len))
@@ -718,7 +770,7 @@ static void read_populate_todo(struct commit_list **todo_list,
 	close(fd);
 
 	next = todo_list;
-	for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
+	for (p = buf.buf; *p;) {
 		if (!(commit = parse_insn_line(p, opts))) {
 			strbuf_release(&buf);
 			die(_("Malformed instruction sheet: %s"), git_path(SEQ_TODO_FILE));
@@ -727,6 +779,11 @@ static void read_populate_todo(struct commit_list **todo_list,
 		new->item = commit;
 		*next = new;
 		next = &new->next;
+
+		if ((p = strchr(p, '\n')))
+			p += 1;
+		else
+			break;
 	}
 	*next = NULL;
 	strbuf_release(&buf);

-- 
1.7.5.GIT

Jonathan Nieder· Jul 6, 2011, 10:33 UTC · re: Ramkumar Ramachandra · lore

Re: [RFC PATCH 14/14] revert: Change insn sheet format

Ramkumar Ramachandra wrote:

>  I've intentionally left parse_cmdline_args unimplemented

[...]

>                  Existing implementations in libraries like Glib are
>  much too heavyweight.

> [1]: http://article.gmane.org/gmane.comp.version-control.git/162198

Ramkumar Ramachandra· Jul 6, 2011, 10:49 UTC · re: Jonathan Nieder · lore

Re: [RFC PATCH 14/14] revert: Change insn sheet format

Hi again,

Jonathan Nieder writes:

> Ramkumar Ramachandra wrote:
>>  I've intentionally left parse_cmdline_args unimplemented
> [...]
>>                  Existing implementations in libraries like Glib are
>>  much too heavyweight.
>
> Wait, how did glib enter the picture? :)

> The implementation of
> shell-style quoting in [1] is not very complicated; perhaps it could
> complement git's existing parsers for shell-style single-quoted
> expressions and C-style double-quoted expressions in quote.c.

Okay.

> Of course, a more basic question is whether we want to allow passing
> arbitrary command-line arguments through the insn sheet at all (a
> part of me wishes "no", at least at first).

I have an overwhelming desire to say "no", but I can't think of an alternative.

> Could you give an example to illustrate what this functionality would
> be used for?  I can understand wanting to pass "-s" and "-X" flags to
> a merge insn and "-X" to pick, but that's as far as my imagination
> goes.

-- Ram

Jonathan Nieder· Jul 6, 2011, 10:41 UTC · re: Ramkumar Ramachandra · lore

Re: [GSoC update] Sequencer: The insn sheet format

Ramkumar Ramachandra wrote:

> Thanks for reading.

Thanks for the many improvements!

	git cherry-pick something-i-shouldnt-cherry-pick
	git reset --hard; # or git reset --merge HEAD
	git cherry-pick what-i-should-have-cherry-picked-instead