threads / patch / 27453

v3, 10 partsSequencer Foundations

Subject: [PATCH v3 00/10] Sequencer Foundations

## tl;dr

37 messages between May 25, 2011 and Jun 8, 2011. Diffs are folded; open one to read it.

replies: 36people: 4as markdown or json

Ramkumar Ramachandra· May 25, 2011, 14:16 UTC · lore
Hi,

Error handling is a very complex topic, and trying to build it up with the Sequencer series had disasterous consequences, as seen in the previous iteration of the series. In this iteration, I've nearly re-written everything from scratch, and tried to get the basics right, without introducing anything new. Time is short, and I'll have to work very hard if I want to target "master",

I hope there are no major issues with this iteration.
Thanks for reading.
-- Ram
Ramkumar Ramachandra (10):
  advice: Introduce error_resolve_conflict
  revert: Propogate errors upwards from do_pick_commit
  revert: Eliminate global "commit" variable
  revert: Rename no_replay to record_origin
  revert: Introduce struct to keep command-line options
  revert: Separate cmdline parsing from functional code
  revert: Catch incompatible command-line options early
  revert: Introduce HEAD, TODO files to persist state, plan
  revert: Implement parsing --continue, --abort and --skip
  revert: Implement --abort processing
 advice.c         |   17 ++-
 advice.h         |    1 +
 builtin/revert.c |  484 +++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 361 insertions(+), 141 deletions(-)
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 25, 2011, 14:16 UTC · re: Ramkumar Ramachandra · lore

[PATCH 01/10] advice: Introduce error_resolve_conflict

Introduce error_resolve_conflict corresponding to die_resolve_conflict, and implement the latter function in terms of the former.

Based-on-patch-by: Christian Couder <chistian.couder@gmail.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 advice.c |   17 +++++++++++------
 advice.h |    1 +
 2 files changed, 12 insertions(+), 6 deletions(-)
Show changes to 2 files +12 −6

advice.c, advice.h

diff --git a/advice.c b/advice.c
index 0be4b5f..80aaa14 100644
--- a/advice.c
+++ b/advice.c
@@ -34,16 +34,21 @@ 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)
 		/*
 		 * 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);
+		return error("'%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);
+	return error("'%s' is not possible because you have unmerged files.", me);
+}
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	error_resolve_conflict(me);
+	exit(128);
 }
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
Ramkumar Ramachandra· May 25, 2011, 14:16 UTC · re: Ramkumar Ramachandra · lore

[PATCH 02/10] revert: Propogate errors upwards from do_pick_commit

Introduce error_dirty_index, based on die_dirty_index which prints some hints, and returns an error to be handled by its caller, do_pick_commit. do_pick_commit, in turn, propogates the error upwards, and the error is finally handled by cmd_cherry_pick and cmd_revert. Set a simple convention for error-handling as follows: positive return values from do_pick_commit indicate conflicts, while negative ones indicate other errors.

Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   72 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 38 insertions(+), 34 deletions(-)
Show changes to builtin/revert.c +38 −34
diff --git a/builtin/revert.c b/builtin/revert.c
index f697e66..523d41a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -266,25 +266,15 @@ 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);
+
+	int ret = error(_("Your local changes would be overwritten by %s.\n"), me);
+	if (advice_commit_before_merge)
+		advise(_("Please, commit your changes or stash them to proceed."));
+	return ret;
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from)
@@ -398,18 +388,18 @@ 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();
 
 	if (!commit->parents) {
 		if (action == REVERT)
-			die (_("Cannot revert a root commit"));
+			return error(_("Cannot revert a root commit"));
 		parent = NULL;
 	}
 	else if (commit->parents->next) {
@@ -418,20 +408,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;
 
@@ -441,12 +431,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
@@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
+	int res = 0;
 	if (isatty(0))
 		edit = 1;
 	action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res > 0)
+		/* Exit status from conflict */
+		return res;
+	if (res < 0)
+		/* Other error */
+		exit(128);
+	return 0;
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
+	int res = 0;
 	action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv);
+	res = revert_or_cherry_pick(argc, argv);
+	if (res > 0)
+		return res;
+	if (res < 0)
+		exit(128);
+	return 0;
 }
-- 
1.7.5.GIT
Junio C Hamano· May 25, 2011, 22:44 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 02/10] revert: Propogate errors upwards from do_pick_commit

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 10 quoted lines
> +static int error_dirty_index(const char *me)
>  {
> +	if (read_cache_unmerged())
> +		return error_resolve_conflict(me);
> +
> +	int ret = error(_("Your local changes would be overwritten by %s.\n"), me);
> +	if (advice_commit_before_merge)
> +		advise(_("Please, commit your changes or stash them to proceed."));
> +	return ret;
>  }

I like this rewrite whose result is short-and-sweet, but you do not even need the "ret" variable. error() always yields -1, no?

Show 31 quoted lines
> @@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  
>  int cmd_revert(int argc, const char **argv, const char *prefix)
>  {
> +	int res = 0;
>  	if (isatty(0))
>  		edit = 1;
>  	action = REVERT;
> -	return revert_or_cherry_pick(argc, argv);
> +	res = revert_or_cherry_pick(argc, argv);
> +	if (res > 0)
> +		/* Exit status from conflict */
> +		return res;
> +	if (res < 0)
> +		/* Other error */
> +		exit(128);
> +	return 0;
>  }
>  
>  int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>  {
> +	int res = 0;
>  	action = CHERRY_PICK;
> -	return revert_or_cherry_pick(argc, argv);
> +	res = revert_or_cherry_pick(argc, argv);
> +	if (res > 0)
> +		return res;
> +	if (res < 0)
> +		exit(128);
> +	return 0;
>  }
This hunk is dubious.
 - Why initialize res to zero if it always is assigned the return value of
   revert_or_cherry_pick() before it is used?
 - The called function seems to return errors from various places but as
   far as I see they are all return value of error(), so it would be
   equivalent to
	if (r_o_c_p(...))
		exit(128);
	return 0;

If you are going to introduce different return values from r-o-c-p() in a later patch, these functions should be updated in that patch, I think.

Ramkumar Ramachandra· May 26, 2011, 09:34 UTC · re: Junio C Hamano · lore

Re: [PATCH 02/10] revert: Propogate errors upwards from do_pick_commit

Hi Junio,
Junio C Hamano writes:
Show 14 quoted lines
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>> +static int error_dirty_index(const char *me)
>>  {
>> +     if (read_cache_unmerged())
>> +             return error_resolve_conflict(me);
>> +
>> +     int ret = error(_("Your local changes would be overwritten by %s.\n"), me);
>> +     if (advice_commit_before_merge)
>> +             advise(_("Please, commit your changes or stash them to proceed."));
>> +     return ret;
>>  }
>
> I like this rewrite whose result is short-and-sweet, but you do not even
> need the "ret" variable. error() always yields -1, no?

Okay; I didn't do this in the first place because I thought it would be inelegant to hardcode '-1'. Fixed anyway.

Show 36 quoted lines
>> @@ -594,14 +584,28 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>>
>>  int cmd_revert(int argc, const char **argv, const char *prefix)
>>  {
>> +     int res = 0;
>>       if (isatty(0))
>>               edit = 1;
>>       action = REVERT;
>> -     return revert_or_cherry_pick(argc, argv);
>> +     res = revert_or_cherry_pick(argc, argv);
>> +     if (res > 0)
>> +             /* Exit status from conflict */
>> +             return res;
>> +     if (res < 0)
>> +             /* Other error */
>> +             exit(128);
>> +     return 0;
>>  }
>>
>>  int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>>  {
>> +     int res = 0;
>>       action = CHERRY_PICK;
>> -     return revert_or_cherry_pick(argc, argv);
>> +     res = revert_or_cherry_pick(argc, argv);
>> +     if (res > 0)
>> +             return res;
>> +     if (res < 0)
>> +             exit(128);
>> +     return 0;
>>  }
>
> This hunk is dubious.
>
>  - Why initialize res to zero if it always is assigned the return value of
>   revert_or_cherry_pick() before it is used?
Okay. Fixed.
Show 10 quoted lines
>  - The called function seems to return errors from various places but as
>   far as I see they are all return value of error(), so it would be
>   equivalent to
>
>        if (r_o_c_p(...))
>                exit(128);
>        return 0;
>
> If you are going to introduce different return values from r-o-c-p() in a
> later patch, these functions should be updated in that patch, I think.

revert_or_cherry_pick *does* return different values in this patch! As I've pointed out in the comment, positive exit status indicates a conflict, while a negative one indicates an error. To prove to myself that this is case, I applied this diff temporarily and ran all tests -- and viola, t3505-cherry-pick-empty.sh broke. Is there something I'm not understanding correctly?

Thanks for the review.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Show changes to builtin/revert.c +4 −10
diff --git a/builtin/revert.c b/builtin/revert.c
index 523d41a..9c7921b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -584,28 +584,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 = 0;
+	int res;
 	if (isatty(0))
 		edit = 1;
 	action = REVERT;
 	res = revert_or_cherry_pick(argc, argv);
-	if (res > 0)
-		/* Exit status from conflict */
-		return res;
-	if (res < 0)
-		/* Other error */
+	if (res)
 		exit(128);
 	return 0;
 }

 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
-	int res = 0;
+	int res;
 	action = CHERRY_PICK;
 	res = revert_or_cherry_pick(argc, argv);
-	if (res > 0)
-		return res;
-	if (res < 0)
+	if (res)
 		exit(128);
 	return 0;
 }
Ramkumar Ramachandra· May 25, 2011, 14:16 UTC · re: Ramkumar Ramachandra · lore

[PATCH 03/10] revert: Eliminate global "commit" variable

Since we want to develop the functionality to either pick/ revert individual commits atomically later in the series, make "commit" a local variable. Modify the get_message and add_message_to_msg API to take an additional parameter: the pre-computed SHA1 character string to use. When reporting a program bug in get_encoding, don't include the commit in the message.

Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   40 ++++++++++++++++++++--------------------
 1 files changed, 20 insertions(+), 20 deletions(-)
Show changes to builtin/revert.c +20 −20
diff --git a/builtin/revert.c b/builtin/revert.c
index 523d41a..6c485f6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
 
 static int edit, no_replay, 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,11 +115,12 @@ struct commit_message {
 	const char *message;
 };
 
-static int get_message(const char *raw_message, struct commit_message *out)
+static int get_message(const char *sha1_abbrev, const char *raw_message,
+		struct commit_message *out)
 {
 	const char *encoding;
-	const char *abbrev, *subject;
-	int abbrev_len, subject_len;
+	const char *subject;
+	int subject_len;
 	char *q;
 
 	if (!raw_message)
@@ -139,17 +139,14 @@ static int get_message(const char *raw_message, struct commit_message *out)
 	if (out->reencoded_message)
 		out->message = out->reencoded_message;
 
-	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	abbrev_len = strlen(abbrev);
-
 	subject_len = find_commit_subject(out->message, &subject);
 
-	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
+	out->parent_label = xmalloc(strlen("parent of ") + DEFAULT_ABBREV +
 			      strlen("... ") + subject_len + 1);
 	q = out->parent_label;
 	q = mempcpy(q, "parent of ", strlen("parent of "));
 	out->label = q;
-	q = mempcpy(q, abbrev, abbrev_len);
+	q = mempcpy(q, sha1_abbrev, DEFAULT_ABBREV);
 	q = mempcpy(q, "... ", strlen("... "));
 	out->subject = q;
 	q = mempcpy(q, subject, subject_len);
@@ -168,8 +165,7 @@ 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));
+		die (_("BUG: get_encoding() called with NULL"));
 	while (*p && *p != '\n') {
 		for (eol = p + 1; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
@@ -185,25 +181,26 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
+static void add_message_to_msg(const char *fallback, 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));
+		strbuf_addstr(msgbuf, fallback);
 
 	p += 2;
 	strbuf_addstr(msgbuf, p);
 }
 
-static void write_cherry_pick_head(void)
+static void write_cherry_pick_head(const char *commit_sha1_hex)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
 
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
+	strbuf_addf(&buf, "%s\n", commit_sha1_hex);
 
 	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
@@ -370,7 +367,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;
@@ -378,6 +375,7 @@ static int do_pick_commit(void)
 	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
+	const char *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
 	int res;
 
 	if (no_commit) {
@@ -434,7 +432,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(sha1_abbrev, commit->buffer, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
 			sha1_to_hex(commit->object.sha1));
 
@@ -463,18 +461,19 @@ static int do_pick_commit(void)
 		}
 		strbuf_addstr(&msgbuf, ".\n");
 	} else {
+		const char *commit_sha1_hex = sha1_to_hex(commit->object.sha1);
 		base = parent;
 		base_label = msg.parent_label;
 		next = commit;
 		next_label = msg.label;
-		add_message_to_msg(&msgbuf, msg.message);
+		add_message_to_msg(commit_sha1_hex, &msgbuf, msg.message);
 		if (no_replay) {
 			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();
+			write_cherry_pick_head(commit_sha1_hex);
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -552,6 +551,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";
@@ -574,7 +574,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
Junio C Hamano· May 25, 2011, 23:10 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 03/10] revert: Eliminate global "commit" variable

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 9 quoted lines
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 523d41a..6c485f6 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
>  
>  static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
>  static enum { REVERT, CHERRY_PICK } action;
> -static struct commit *commit;
I agree that the stated goal of this change is a worthy one.
Show 8 quoted lines
> @@ -116,11 +115,12 @@ struct commit_message {
>  	const char *message;
>  };
>  
> -static int get_message(const char *raw_message, struct commit_message *out)
> +static int get_message(const char *sha1_abbrev, const char *raw_message,
> +		struct commit_message *out)
>  {

This somehow feels dubious and also goes against the "let's not rely on global variable commit", no?

This function relied on the global variable and also got information passed from the caller that this function could have derived from that global commit object (i.e. raw_message came from commit->buffer).

A logical thing to fix that situation would be to make its signature:
    static int get_message(struct commit *commit, struct commit_message *out)
no?
Especially dubious is this part:
Show 12 quoted lines
> @@ -139,17 +139,14 @@ static int get_message(const char *raw_message, struct commit_message *out)
>  	if (out->reencoded_message)
>  		out->message = out->reencoded_message;
>  
> -	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
> -	abbrev_len = strlen(abbrev);
> -
>  	subject_len = find_commit_subject(out->message, &subject);
>  
> -	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
> +	out->parent_label = xmalloc(strlen("parent of ") + DEFAULT_ABBREV +
>  			      strlen("... ") + subject_len + 1);

You moved the call to f-u-a() to the caller, and that is why you have to pass its return value as an extra parameter. If you passed commit you do not have to.

Worse yet, the above xmalloc() and memcpy below are wrong, as you are no longer taking the actual length of the abbreviated object name into account, like the old code did. f-u-a() is _about_ uniqueness, and you can get a string longer than the minimum length you gave to it.

Show 8 quoted lines
>  	q = out->parent_label;
>  	q = mempcpy(q, "parent of ", strlen("parent of "));
>  	out->label = q;
> -	q = mempcpy(q, abbrev, abbrev_len);
> +	q = mempcpy(q, sha1_abbrev, DEFAULT_ABBREV);
>  	q = mempcpy(q, "... ", strlen("... "));
>  	out->subject = q;
>  	q = mempcpy(q, subject, subject_len);
Show 7 quoted lines
> @@ -168,8 +165,7 @@ 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));
> +		die (_("BUG: get_encoding() called with NULL"));
Hmm, do we really want to translate this error message?

As the function is file-scope static, I suspect that calling it with NULL is a programmer error, not data error (i.e. it is not like we might get an object with NULL here in some repository created by a different version or implementation of git). Once you got confident (and I hope you will be before this series hits our tree) that no caller calls this function with NULL, we might want to demote this to assert(p) or even remove that if statement altogether.

Show 15 quoted lines
> @@ -185,25 +181,26 @@ static char *get_encoding(const char *message)
>  	return NULL;
>  }
>  
> -static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
> +static void add_message_to_msg(const char *fallback, 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));
> +		strbuf_addstr(msgbuf, fallback);

Can you explain what the above loop and if statements are doing for me? It seems to be doing something a lot more than what the name of the function suggests, which is a bad sign that the helper is not designed properly with clear semantics in mind. I would probably prefer to see this function removed, and do this kind of thing in the only caller of this confused helper function. You could make an even smaller helper function that implements the logic to determine if there are two consecutive LFs in a string (seen above), but I suspect that would be just the matter of strstr(message, "\n\n")

Show 25 quoted lines
> -static void write_cherry_pick_head(void)
> +static void write_cherry_pick_head(const char *commit_sha1_hex)
>  {
>  	int fd;
>  	struct strbuf buf = STRBUF_INIT;
>  
> -	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
> +	strbuf_addf(&buf, "%s\n", commit_sha1_hex);
>  
>  	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
>  	if (fd < 0)
> @@ -370,7 +367,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;
> @@ -378,6 +375,7 @@ static int do_pick_commit(void)
>  	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
>  	char *defmsg = NULL;
>  	struct strbuf msgbuf = STRBUF_INIT;
> +	const char *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);

Given the lifetime rule of f-u-a return value, I think it is a bad design to call it and pass it as an extra parameter to get_message() much later.

Ramkumar Ramachandra· May 25, 2011, 14:16 UTC · re: Ramkumar Ramachandra · lore

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

Give the variable "no_replay" a new name "record_origin", so that there is no confusion when a "replay" structure is introduced later in the series.

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(-)
Show changes to builtin/revert.c +4 −4
diff --git a/builtin/revert.c b/builtin/revert.c
index 6c485f6..c48a7df 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 int commit_argc;
 static const char **commit_argv;
@@ -90,7 +90,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(),
 		};
@@ -467,7 +467,7 @@ static int do_pick_commit(struct commit *commit)
 		next = commit;
 		next_label = msg.label;
 		add_message_to_msg(commit_sha1_hex, &msgbuf, msg.message);
-		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");
@@ -563,7 +563,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· May 25, 2011, 14:17 UTC · re: Ramkumar Ramachandra · lore

[PATCH 05/10] revert: Introduce struct to keep command-line options

The current code uses a set of file-scope static variables to tell the cherry-pick/ revert machinery how to replay the changes, and initializes them by parsing the command-line arguments. 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.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |  174 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 97 insertions(+), 77 deletions(-)
Show changes to builtin/revert.c +97 −77
diff --git a/builtin/revert.c b/builtin/revert.c
index c48a7df..0d6a5a9 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,76 +35,88 @@ 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;
+struct replay_opts {
+	enum { REVERT, CHERRY_PICK } 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 = opt->value;
+
 	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 {
@@ -287,7 +299,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;
@@ -308,7 +321,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,
@@ -348,7 +361,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];
@@ -356,9 +369,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;
 	}
@@ -367,7 +380,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;
@@ -378,7 +391,7 @@ static int do_pick_commit(struct commit *commit)
 	const char *sha1_abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
 	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
@@ -396,7 +409,7 @@ static int do_pick_commit(struct commit *commit)
 	discard_cache();
 
 	if (!commit->parents) {
-		if (action == REVERT)
+		if (opts->action == REVERT)
 			return error(_("Cannot revert a root commit"));
 		parent = NULL;
 	}
@@ -405,25 +418,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)
@@ -445,7 +458,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;
@@ -467,18 +480,18 @@ static int do_pick_commit(struct commit *commit)
 		next = commit;
 		next_label = msg.label;
 		add_message_to_msg(commit_sha1_hex, &msgbuf, msg.message);
-		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)
+		if (!opts->no_commit)
 			write_cherry_pick_head(commit_sha1_hex);
 	}
 
-	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;
@@ -488,23 +501,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);
@@ -513,18 +526,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"));
@@ -533,7 +546,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);
@@ -548,33 +561,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;
 	}
@@ -585,10 +599,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 = 0;
+	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)
 		/* Exit status from conflict */
 		return res;
@@ -601,8 +618,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 = 0;
-	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)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 25, 2011, 14:17 UTC · re: Ramkumar Ramachandra · lore

[PATCH 06/10] revert: Separate cmdline parsing from functional code

Make cmd_cherry_pick and cmd_revert call parse_args and parse all the command-line arguments into an opts structure before sending it off to a simplified pick_commits function. pick_commits, in turn will set up the revision walker and call do_pick_commit in a loop- later in the series, it will serve as the starting point for continuation.

Based-on-patch-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(-)
Show changes to builtin/revert.c +9 −8
diff --git a/builtin/revert.c b/builtin/revert.c
index 0d6a5a9..9a612c6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -561,17 +561,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"));
@@ -605,7 +600,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)
 		/* Exit status from conflict */
 		return res;
@@ -622,7 +620,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)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 25, 2011, 14:17 UTC · re: Ramkumar Ramachandra · lore

[PATCH 07/10] revert: Catch incompatible command-line options early

Earlier, incompatible command-line options used to be caught in pick_commits after parse_args has parsed the options and populated the options structure; a lot of unncessary work has already been done, and significant amount of cleanup is required to die at this stage. Instead, hand over this responsibility to parse_args so that the program can die early. Also write a die_opt_incompabile function to handle incompatible options in a general manner; it will be used more extensively as more command-line options are introduced later in the series.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)
Show changes to builtin/revert.c +23 −11
diff --git a/builtin/revert.c b/builtin/revert.c
index 9a612c6..d21af96 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -80,6 +80,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);
@@ -116,6 +132,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;
 }
 
@@ -567,17 +590,6 @@ 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"));
-	}
-
 	read_and_refresh_cache(me, opts);
 
 	prepare_revs(&revs, opts);
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 25, 2011, 14:17 UTC · re: Ramkumar Ramachandra · lore

[PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

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 allow "git cherry-pick --abort" to cancel and "git cherry-pick --continue" to resume the entire command, we will need to store some information about the state and the plan at the beginning. Introduce a HEAD file to store this state, and TODO file to store the plan.

Helped-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 73 insertions(+), 3 deletions(-)
Show changes to builtin/revert.c +73 −3
diff --git a/builtin/revert.c b/builtin/revert.c
index d21af96..4741252 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,12 @@
  * Copyright (c) 2005 Junio C Hamano
  */
 
+#define SEQ_DIR "sequencer"
+
+#define SEQ_PATH	git_path(SEQ_DIR)
+#define HEAD_FILE	git_path(SEQ_DIR "/head")
+#define TODO_FILE	git_path(SEQ_DIR "/todo")
+
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
 	NULL
@@ -584,22 +591,85 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
+static void format_todo(struct strbuf *buf, struct commit_list *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 = list; cur; cur = cur->next) {
+		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
+		if (get_message(sha1_abbrev, cur->item->buffer, &msg))
+			die(_("Cannot get commit message for %s"), sha1_abbrev);
+		strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);
+	}
+}
+
+static void persist_head(unsigned char *head)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct lock_file head_lock;
+	int fd;
+
+	if (file_exists(SEQ_PATH)) {
+		if (!is_directory(SEQ_PATH) && remove_path(SEQ_PATH) < 0) {
+			strbuf_release(&buf);
+			die(_("Could not remove %s"), SEQ_PATH);
+		}
+	}
+	if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not create sequencer directory '%s'."), SEQ_PATH);
+	}
+
+	fd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s", head);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), HEAD_FILE);
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s"), HEAD_FILE);
+}
+
+static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct lock_file todo_lock;
+	int fd;
+
+	fd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);
+	format_todo(&buf, todo_list, opts);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), TODO_FILE);
+	if (commit_lock_file(&todo_lock) < 0)
+		die(_("Error wrapping up %s"), TODO_FILE);
+}
+
 static int pick_commits(struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
+	unsigned char head[20];
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	read_and_refresh_cache(me, opts);
 
+	if (get_sha1("HEAD", head))
+		return error(_("You do not have a valid HEAD"));
+
+	read_and_refresh_cache(me, opts);
 	prepare_revs(&revs, opts);
+	persist_head(head);
 
 	while ((commit = get_revision(&revs))) {
 		int res = do_pick_commit(commit, opts);
-		if (res)
+		if (res) {
+			commit_list_insert(commit, &revs.commits);
+			persist_todo(revs.commits, opts);
 			return res;
+		}
 	}
-
 	return 0;
 }
 
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 25, 2011, 14:17 UTC · re: Ramkumar Ramachandra · lore

[PATCH 09/10] revert: Implement parsing --continue, --abort and --skip

Introduce three new command-line options: --continue, --abort, and --skip resembling the correspoding options in "rebase -i". For now, just parse the options into the replay_opts structure, making sure that two of them are not specified together. They will actually be implemented later in the series.

Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 70 insertions(+), 1 deletions(-)
Show changes to builtin/revert.c +70 −1
diff --git a/builtin/revert.c b/builtin/revert.c
index 4741252..4607676 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -47,6 +47,11 @@ static const char *me;
 struct replay_opts {
 	enum { REVERT, CHERRY_PICK } action;
 
+	/* --abort, --skip, and --continue */
+	int abort_oper;
+	int skip_oper;
+	int continue_oper;
+
 	/* Boolean options */
 	int edit;
 	int record_origin;
@@ -103,11 +108,36 @@ 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) return;
+	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);
+	}
+	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, "abort", &opts->abort_oper, "abort the current operation"),
+		OPT_BOOLEAN(0, "skip", &opts->skip_oper, "skip the current commit"),
+		OPT_BOOLEAN(0, "continue", &opts->continue_oper, "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)",
@@ -136,7 +166,37 @@ 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 mutually incompatible command line arguments */
+	verify_opt_mutually_compatible(me,
+				"--abort", opts->abort_oper,
+				"--skip", opts->skip_oper,
+				"--continue", opts->continue_oper,
+				NULL);
+
+	/* Check for incompatible command line arguments */
+	if (opts->abort_oper || opts->skip_oper || opts->continue_oper) {
+		char *this_oper;
+		if (opts->abort_oper)
+			this_oper = "--abort";
+		else if (opts->skip_oper)
+			this_oper = "--skip";
+		else
+			this_oper = "--continue";
+
+		verify_opt_compatible(me, this_oper,
+				"--no-commit", opts->no_commit,
+				"--edit", opts->edit,
+				"--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)
@@ -146,6 +206,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"-x", opts->record_origin,
 				"--edit", opts->edit,
 				NULL);
+
+	/* Remove these when the options are actually implemented */
+	if (opts->abort_oper)
+		die("--abort is not implemented yet");
+	if (opts->skip_oper)
+		die("--skip is not implemented yet");
+	if (opts->continue_oper)
+		die("--continue is not implemented yet");
+
 	opts->commit_argv = argv;
 }
 
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 25, 2011, 14:17 UTC · re: Ramkumar Ramachandra · lore

[PATCH 10/10] revert: Implement --abort processing

To abort, perform a "rerere clear" and "reset --hard" to the ref specified by the HEAD file introduced earlier in the series.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   46 ++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 42 insertions(+), 4 deletions(-)
Show changes to builtin/revert.c +42 −4
diff --git a/builtin/revert.c b/builtin/revert.c
index 4607676..6a93175 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -208,8 +208,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				NULL);
 
 	/* Remove these when the options are actually implemented */
-	if (opts->abort_oper)
-		die("--abort is not implemented yet");
 	if (opts->skip_oper)
 		die("--skip is not implemented yet");
 	if (opts->continue_oper)
@@ -742,6 +740,46 @@ static int pick_commits(struct replay_opts *opts)
 	return 0;
 }
 
+static int process_continuation(struct replay_opts *opts)
+{
+	if (opts->abort_oper) {
+		char head[DEFAULT_ABBREV];
+		unsigned char sha1[20];
+		int fd;
+		rerere_clear(0);
+
+		if (!file_exists(HEAD_FILE))
+			goto error;
+		fd = open(HEAD_FILE, O_RDONLY, 0666);
+		if (fd < 0)
+			return error(_("Could not open '%s' for reading: %s"),
+				HEAD_FILE, strerror(errno));
+		if (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV) {
+			close(fd);
+			return error(_("Corrupt '%s': %s"), HEAD_FILE, strerror(errno));
+		}
+		close(fd);
+
+		if (get_sha1(head, sha1))
+			return error(_("Failed to resolve '%s' as a valid ref."), head);
+		update_ref(NULL, "HEAD", sha1, NULL, 0, MSG_ON_ERR);
+	}
+	else if (opts->skip_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+	else if (opts->continue_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+
+	return pick_commits(opts);
+error:
+	return error(_("No %s in progress"), me);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res = 0;
@@ -754,7 +792,7 @@ 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);
+	res = process_continuation(&opts);
 	if (res > 0)
 		/* Exit status from conflict */
 		return res;
@@ -774,7 +812,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)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT
Junio C Hamano· May 25, 2011, 23:15 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH v3 00/10] Sequencer Foundations

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> I hope there are no major issues with this iteration.

I've scanned up to 07/10; except for the internal API issues in 03/10 overall you seem to be going in the right direction.

Ramkumar Ramachandra· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH v4 00/10] Sequencer Foundations

Hi,

This is a quick re-roll after Junio's review of the previous iteration; the previous iteration was posted less than 24 hours ago. I'm in a hurry, because I want to make sure that this is ready (or nearly ready) for merge before I actually implement more features.

Thanks for reading.
Ramkumar Ramachandra (10):
  advice: Introduce error_resolve_conflict
  revert: Propogate errors upwards from do_pick_commit
  revert: Eliminate global "commit" variable
  revert: Rename no_replay to record_origin
  revert: Introduce struct to keep command-line options
  revert: Separate cmdline parsing from functional code
  revert: Catch incompatible command-line options early
  revert: Introduce HEAD, TODO files to persist state, plan
  revert: Implement parsing --continue, --abort and --skip
  revert: Implement --abort processing
 advice.c         |   17 ++-
 advice.h         |    1 +
 builtin/revert.c |  492 ++++++++++++++++++++++++++++++++++++++----------------
 3 files changed, 360 insertions(+), 150 deletions(-)
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 01/10] advice: Introduce error_resolve_conflict

Introduce error_resolve_conflict corresponding to die_resolve_conflict, and implement the latter function in terms of the former.

Based-on-patch-by: Christian Couder <chistian.couder@gmail.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 advice.c |   17 +++++++++++------
 advice.h |    1 +
 2 files changed, 12 insertions(+), 6 deletions(-)
Show changes to 2 files +12 −6

advice.c, advice.h

diff --git a/advice.c b/advice.c
index 0be4b5f..80aaa14 100644
--- a/advice.c
+++ b/advice.c
@@ -34,16 +34,21 @@ 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)
 		/*
 		 * 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);
+		return error("'%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);
+	return error("'%s' is not possible because you have unmerged files.", me);
+}
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+	error_resolve_conflict(me);
+	exit(128);
 }
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
Ramkumar Ramachandra· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 02/10] revert: Propogate errors upwards from do_pick_commit

Introduce error_dirty_index, based on die_dirty_index which prints some hints, and returns an error to be handled by its caller, do_pick_commit. do_pick_commit, in turn, propogates the error upwards, and the error is finally handled by cmd_cherry_pick and cmd_revert. Set a simple convention for error-handling as follows: positive return values from do_pick_commit indicate conflicts, while negative ones indicate other errors.

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 |   72 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 38 insertions(+), 34 deletions(-)
Show changes to builtin/revert.c +38 −34
diff --git a/builtin/revert.c b/builtin/revert.c
index f697e66..ff26856 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -266,25 +266,15 @@ 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);
+
+	error(_("Your local changes would be overwritten by %s.\n"), 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)
@@ -398,18 +388,18 @@ 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();
 
 	if (!commit->parents) {
 		if (action == REVERT)
-			die (_("Cannot revert a root commit"));
+			return error(_("Cannot revert a root commit"));
 		parent = NULL;
 	}
 	else if (commit->parents->next) {
@@ -418,20 +408,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;
 
@@ -441,12 +431,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
@@ -594,14 +584,28 @@ 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)
+		/* Exit status from conflict */
+		return res;
+	if (res < 0)
+		/* Other error */
+		exit(128);
+	return 0;
 }
 
 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)
+		return res;
+	if (res < 0)
+		exit(128);
+	return 0;
 }
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 03/10] revert: Eliminate global "commit" variable

Since we want to develop the functionality to either pick or revert individual commits atomically later in the series, make "commit" a local variable. Modify the get_message API to take "commit" as an argument, factor out add_message_to_msg, and don't check the sole argument in get_encoding.

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 |   47 ++++++++++++++++++-----------------------------
 1 files changed, 18 insertions(+), 29 deletions(-)
Show changes to builtin/revert.c +18 −29
diff --git a/builtin/revert.c b/builtin/revert.c
index ff26856..745295d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
 
 static int edit, no_replay, 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;
@@ -167,9 +166,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 */
@@ -185,25 +181,12 @@ 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)
+static void write_cherry_pick_head(const char *commit_sha1_hex)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
 
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
+	strbuf_addf(&buf, "%s\n", commit_sha1_hex);
 
 	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
@@ -370,7 +353,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;
@@ -434,7 +417,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));
 
@@ -467,14 +450,19 @@ static int do_pick_commit(void)
 		base_label = msg.parent_label;
 		next = commit;
 		next_label = msg.label;
-		add_message_to_msg(&msgbuf, msg.message);
+
+		/* Add msg.message to msgbuf */
+		const char *p = strstr(msg.message, "\n\n");
+		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));
 			strbuf_addstr(&msgbuf, ")\n");
 		}
 		if (!no_commit)
-			write_cherry_pick_head();
+			write_cherry_pick_head(sha1_to_hex(commit->object.sha1));
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -552,6 +540,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";
@@ -574,7 +563,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
Ramkumar Ramachandra· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

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

Give the variable "no_replay" a new name "record_origin", so that there is no confusion when a "replay" structure is introduced later in the series.

Mentored-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(-)
Show changes to builtin/revert.c +4 −4
diff --git a/builtin/revert.c b/builtin/revert.c
index 745295d..e9b8533 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 int commit_argc;
 static const char **commit_argv;
@@ -90,7 +90,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(),
 		};
@@ -456,7 +456,7 @@ static int do_pick_commit(struct commit *commit)
 		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");
@@ -552,7 +552,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· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 05/10] revert: Introduce struct to keep command-line options

The current code uses a set of file-scope static variables to tell the cherry-pick/ revert machinery how to replay the changes, and initializes them by parsing the command-line arguments. 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.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |  174 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 97 insertions(+), 77 deletions(-)
Show changes to builtin/revert.c +97 −77
diff --git a/builtin/revert.c b/builtin/revert.c
index e9b8533..cc2fa73 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,76 +35,88 @@ 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;
+struct replay_opts {
+	enum { REVERT, CHERRY_PICK } 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 = opt->value;
+
 	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 {
@@ -273,7 +285,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;
@@ -294,7 +307,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,
@@ -334,7 +347,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];
@@ -342,9 +355,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;
 	}
@@ -353,7 +366,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;
@@ -363,7 +376,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 +394,7 @@ static int do_pick_commit(struct commit *commit)
 	discard_cache();
 
 	if (!commit->parents) {
-		if (action == REVERT)
+		if (opts->action == REVERT)
 			return error(_("Cannot revert a root commit"));
 		parent = NULL;
 	}
@@ -390,25 +403,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)
@@ -430,7 +443,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;
@@ -456,18 +469,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)
+		if (!opts->no_commit)
 			write_cherry_pick_head(sha1_to_hex(commit->object.sha1));
 	}
 
-	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;
@@ -477,23 +490,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);
@@ -502,18 +515,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"));
@@ -522,7 +535,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);
@@ -537,33 +550,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;
 	}
@@ -574,10 +588,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)
 		/* Exit status from conflict */
 		return res;
@@ -590,8 +607,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)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 06/10] revert: Separate cmdline parsing from functional code

Make cmd_cherry_pick and cmd_revert call parse_args and parse all the command-line arguments into an opts structure before sending it off to a simplified pick_commits function. pick_commits, in turn will set up the revision walker and call do_pick_commit in a loop- later in the series, it will serve as the starting point for continuation.

Based-on-patch-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(-)
Show changes to builtin/revert.c +9 −8
diff --git a/builtin/revert.c b/builtin/revert.c
index cc2fa73..2e5f260 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -550,17 +550,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"));
@@ -594,7 +589,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)
 		/* Exit status from conflict */
 		return res;
@@ -611,7 +609,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)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 07/10] revert: Catch incompatible command-line options early

Earlier, incompatible command-line options used to be caught in pick_commits after parse_args has parsed the options and populated the options structure; a lot of unncessary work has already been done, and significant amount of cleanup is required to die at this stage. Instead, hand over this responsibility to parse_args so that the program can die early. Also write a die_opt_incompabile function to handle incompatible options in a general manner; it will be used more extensively as more command-line options are introduced later in the series.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   34 +++++++++++++++++++++++-----------
 1 files changed, 23 insertions(+), 11 deletions(-)
Show changes to builtin/revert.c +23 −11
diff --git a/builtin/revert.c b/builtin/revert.c
index 2e5f260..1c6c102 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -80,6 +80,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);
@@ -116,6 +132,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;
 }
 
@@ -556,17 +579,6 @@ 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"));
-	}
-
 	read_and_refresh_cache(me, opts);
 
 	prepare_revs(&revs, opts);
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

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 allow "git cherry-pick --abort" to cancel and "git cherry-pick --continue" to resume the entire command, we will need to store some information about the state and the plan at the beginning. Introduce a HEAD file to store this state, and TODO file to store the plan.

Helped-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 74 insertions(+), 3 deletions(-)
Show changes to builtin/revert.c +74 −3
diff --git a/builtin/revert.c b/builtin/revert.c
index 1c6c102..306f5b0 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,12 @@
  * Copyright (c) 2005 Junio C Hamano
  */
 
+#define SEQ_DIR "sequencer"
+
+#define SEQ_PATH	git_path(SEQ_DIR)
+#define HEAD_FILE	git_path(SEQ_DIR "/head")
+#define TODO_FILE	git_path(SEQ_DIR "/todo")
+
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
 	NULL
@@ -573,22 +580,86 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
 	rollback_lock_file(&index_lock);
 }
 
+static void format_todo(struct strbuf *buf, struct commit_list *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 = 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 persist_head(unsigned char *head)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct lock_file head_lock;
+	int fd;
+
+	if (file_exists(SEQ_PATH)) {
+		if (!is_directory(SEQ_PATH) && remove_path(SEQ_PATH) < 0) {
+			strbuf_release(&buf);
+			die(_("Could not remove %s"), SEQ_PATH);
+		}
+	}
+	if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not create sequencer directory '%s'."), SEQ_PATH);
+	}
+
+	fd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s", head);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), HEAD_FILE);
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s"), HEAD_FILE);
+}
+
+static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct lock_file todo_lock;
+	int fd;
+
+	fd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);
+	format_todo(&buf, todo_list, opts);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), TODO_FILE);
+	if (commit_lock_file(&todo_lock) < 0)
+		die(_("Error wrapping up %s"), TODO_FILE);
+}
+
 static int pick_commits(struct replay_opts *opts)
 {
 	struct rev_info revs;
 	struct commit *commit;
+	unsigned char head[20];
 
 	setenv(GIT_REFLOG_ACTION, me, 0);
-	read_and_refresh_cache(me, opts);
 
+	if (get_sha1("HEAD", head))
+		return error(_("You do not have a valid HEAD"));
+
+	read_and_refresh_cache(me, opts);
 	prepare_revs(&revs, opts);
+	persist_head(head);
+	persist_todo(revs.commits, opts);
 
 	while ((commit = get_revision(&revs))) {
 		int res = do_pick_commit(commit, opts);
-		if (res)
+		if (res) {
+			commit_list_insert(commit, &revs.commits);
+			persist_todo(revs.commits, opts);
 			return res;
+		}
 	}
-
 	return 0;
 }
 
-- 
1.7.5.GIT
Jonathan Nieder· May 26, 2011, 16:11 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

Ramkumar Ramachandra wrote:
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -573,22 +580,86 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
[...]
Show 13 quoted lines
>  	prepare_revs(&revs, opts);
> +	persist_head(head);
> +	persist_todo(revs.commits, opts);
>  
>  	while ((commit = get_revision(&revs))) {
>  		int res = do_pick_commit(commit, opts);
> -		if (res)
> +		if (res) {
> +			commit_list_insert(commit, &revs.commits);
> +			persist_todo(revs.commits, opts);
>  			return res;
> +		}
>  	}

Almost there. To comfort overly-worried people like me that think we have not finished converted all die() calls yet, wouldn't this need to look like

	persist_head(head);
	while ((commit = ...)) {
		int status_or_error;
		/*
		 * Checkpoint.  If do_pick_commit exits, make sure the user
		 * can still use "git cherry-pick --continue" to recover.
		 */
		persist_todo(revs.commits, opts);
		status_or_error = do_pick_commit(...);
		if (status_or_error)
			return status_or_error;
	}
	/* Success! */
	remove_todo(opts);
	remove_head();
	return 0;

And with that, this would no longer depend on the (valuable enough on their own terms) patches 1 and 2 so they could be treated as a separate series, no?

Ramkumar Ramachandra· May 26, 2011, 16:26 UTC · re: Jonathan Nieder · lore

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

Hi Jonathan,
Jonathan Nieder writes:
Show 23 quoted lines
> Almost there.  To comfort overly-worried people like me that think we
> have not finished converted all die() calls yet, wouldn't this need to
> look like
>
>        persist_head(head);
>        while ((commit = ...)) {
>                int status_or_error;
>
>                /*
>                 * Checkpoint.  If do_pick_commit exits, make sure the user
>                 * can still use "git cherry-pick --continue" to recover.
>                 */
>                persist_todo(revs.commits, opts);
>
>                status_or_error = do_pick_commit(...);
>                if (status_or_error)
>                        return status_or_error;
>        }
>
>        /* Success! */
>        remove_todo(opts);
>        remove_head();
>        return 0;

You told me about this on IRC last night, but I forgot to squash the relevant patch into this series. Thanks :)

> And with that, this would no longer depend on the (valuable enough on
> their own terms) patches 1 and 2 so they could be treated as a
> separate series, no?

Yes, but I'd like to keep 1 and 2 in this series, if that's alright. Another series will have to take care of error handling more extensively. With a few tests, would it be useful to get this series merged in? It implements '--abort', which is already useful.

-- Ram
Ramkumar Ramachandra· May 26, 2011, 17:05 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

Hi Junio,
Ramkumar Ramachandra writes:
Show 10 quoted lines
> Jonathan Nieder writes:
>> And with that, this would no longer depend on the (valuable enough on
>> their own terms) patches 1 and 2 so they could be treated as a
>> separate series, no?
>
> Yes, but I'd like to keep 1 and 2 in this series, if that's alright.
> Another series will have to take care of error handling more
> extensively.
> With a few tests, would it be useful to get this series merged in? It
> implements '--abort', which is already useful.
There's a good way to create a separation between patches 1-2, and the
rest- thanks to Jonathan's suggestion on IRC.
Junio: Would you like to merge just the first two patches? I'll
continue working on the rest, and try to come up with something useful
to merge in another week or two.
-- Ram
Junio C Hamano· May 26, 2011, 21:03 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 5 quoted lines
> There's a good way to create a separation between patches 1-2, and the
> rest- thanks to Jonathan's suggestion on IRC.
> Junio: Would you like to merge just the first two patches? I'll
> continue working on the rest, and try to come up with something useful
> to merge in another week or two.

If you can split out a smaller series that is obviously correct and justifiable with or without the rest of the series, that is very good in general.

I think [1/10] almost falls into that category, modulo that we used to say it is "fatal:" when you have unmerged files and cannot cherry-pick, but you now say "error:", which I think is an unnecessary change that you may want to avoid if you can.

I am not yet convinced [2/10] falls into that category.
Ramkumar Ramachandra· Jun 1, 2011, 15:28 UTC · re: Jonathan Nieder · lore

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

Hi Jonathan and others,
Jonathan Nieder writes:
Show 24 quoted lines
> Ramkumar Ramachandra wrote:
> Almost there.  To comfort overly-worried people like me that think we
> have not finished converted all die() calls yet, wouldn't this need to
> look like
>
>        persist_head(head);
>        while ((commit = ...)) {
>                int status_or_error;
>
>                /*
>                 * Checkpoint.  If do_pick_commit exits, make sure the user
>                 * can still use "git cherry-pick --continue" to recover.
>                 */
>                persist_todo(revs.commits, opts);
>
>                status_or_error = do_pick_commit(...);
>                if (status_or_error)
>                        return status_or_error;
>        }
>
>        /* Success! */
>        remove_todo(opts);
>        remove_head();
>        return 0;

It's a little embarrassing, but I'm not able to figure out how to persist the TODO early. prepare_revs() sets up the revision walker and populates the rev_info struct: it contains a commit_list * in its commits member, but I apparently can't access those directly -- I went through some literature about revision walking and inspected how get_revision must be used to iterate over all the individual commits. Now, to persist the TODO, I need a commit_list to use: is there some API I can use to avoid iterating over all the commits twice? Once to populate the commit_list to persist the TODO, and the second time to actually pick them?

Thanks.
-- Ram
Jonathan Nieder· Jun 1, 2011, 19:31 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

Ramkumar Ramachandra wrote:
> is there some
> API I can use to avoid iterating over all the commits twice? Once to
> populate the commit_list to persist the TODO, and the second time to
> actually pick them?

Good catch. (If I understand correctly this was a potential problem with the previous iterations, too.)

So, before thinking about what APIs git provides, what is the desired behavior? It is possible to walk a revision list incrementally (like "git log" does); should cherry-pick take advantage of that?

. If yes, the "todo list" would have to be in some form that can be
  updated incrementally (something like the original revision range
  with the commits already cherry-picked negated).  This sounds
  complicated to me.
. If no, the "todo list" needs to be fully resolved to start out, and
  then the cherry-pick loop can just walk through it.  In git API
  terms, that means first populating the commit_list (for example
  using a loop that gets revisions and inserts them at the end of a
  commit_list, or by preparing the revision walk with "limited" flag
  set), then walking through it with commit_list APIs.
Ramkumar Ramachandra· Jun 2, 2011, 12:53 UTC · re: Jonathan Nieder · lore

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

Hi Jonathan,
Jonathan Nieder writes:
Show 6 quoted lines
> . If no, the "todo list" needs to be fully resolved to start out, and
>  then the cherry-pick loop can just walk through it.  In git API
>  terms, that means first populating the commit_list (for example
>  using a loop that gets revisions and inserts them at the end of a
>  commit_list, or by preparing the revision walk with "limited" flag
>  set), then walking through it with commit_list APIs.
Thanks. I picked this approach :)
On a slightly unrelated note, I just found out that the persist_todo
can't be called twice due to a lockfile API limitation* -- the
atexit(3) cleanup handler is supposed to have a linked list of
lockfiles to clean up. When one lockfile is used twice, it becomes a
circularly linked list, and the entire program hangs at exit time
since traversing a circularly linked list is never-ending. Couple of
comments:
1. By reading the lockfile implementation, I understand why this
happens -- can't it be implemented differently to remove this
limitation? If not, shouldn't this limitation be documented somewhere?
2. What can I do? It'll be inelegant to use the lockfile API while
persisting the first time, but not the second time -- is there any way
out of this?
Thanks again.
* You brought this up during one of our IRC conversations, but I
didn't understand it then. I do now.
-- Ram
Jeff King· Jun 2, 2011, 14:29 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

On Thu, Jun 02, 2011 at 06:23:03PM +0530, Ramkumar Ramachandra wrote:
Show 10 quoted lines
> On a slightly unrelated note, I just found out that the persist_todo
> can't be called twice due to a lockfile API limitation* -- the
> atexit(3) cleanup handler is supposed to have a linked list of
> lockfiles to clean up. When one lockfile is used twice, it becomes a
> circularly linked list, and the entire program hangs at exit time
> since traversing a circularly linked list is never-ending. Couple of
> comments:
> 1. By reading the lockfile implementation, I understand why this
> happens -- can't it be implemented differently to remove this
> limitation? If not, shouldn't this limitation be documented somewhere?
Doesn't lock_file handle multiple locking already via the on_list parameter?
    $ grep -A2 on_list lockfile.c
            if (!lk->on_list) {
                    lk->next = lock_file_list;
                    lock_file_list = lk;
                    lk->on_list = 1;
            }

However, I think you have a bigger problem, which is that you are allocating the lock_file on the stack in persist_todo and persist_head. So by the time the atexit() handler is called, this storage has gone away and you are just reading random data (not to mention that it also should be zero-initialized before being passed to lock_file).

-Peff
Ramkumar Ramachandra· Jun 8, 2011, 13:14 UTC · re: Jeff King · lore

Re: [PATCH 08/10] revert: Introduce HEAD, TODO files to persist state, plan

Hi Jeff,
Jeff King writes:
Show 14 quoted lines
> Doesn't lock_file handle multiple locking already via the on_list parameter?
>
>    $ grep -A2 on_list lockfile.c
>            if (!lk->on_list) {
>                    lk->next = lock_file_list;
>                    lock_file_list = lk;
>                    lk->on_list = 1;
>            }
>
> However, I think you have a bigger problem, which is that you are
> allocating the lock_file on the stack in persist_todo and persist_head.
> So by the time the atexit() handler is called, this storage has gone
> away and you are just reading random data (not to mention that it also
> should be zero-initialized before being passed to lock_file).

Right. Thanks for the excellent pointer. I was struggling to see what was wrong on GDB; little did I realize that I was viewing nonsense. I solved the problem by making the lock_file variables static.

Thanks again, and sorry for the nonsense.
-- Ram
Ramkumar Ramachandra· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 09/10] revert: Implement parsing --continue, --abort and --skip

Introduce three new command-line options: --continue, --abort, and --skip resembling the correspoding options in "rebase -i". For now, just parse the options into the replay_opts structure, making sure that two of them are not specified together. They will actually be implemented later in the series.

Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 70 insertions(+), 1 deletions(-)
Show changes to builtin/revert.c +70 −1
diff --git a/builtin/revert.c b/builtin/revert.c
index 306f5b0..f33d40a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -47,6 +47,11 @@ static const char *me;
 struct replay_opts {
 	enum { REVERT, CHERRY_PICK } action;
 
+	/* --abort, --skip, and --continue */
+	int abort_oper;
+	int skip_oper;
+	int continue_oper;
+
 	/* Boolean options */
 	int edit;
 	int record_origin;
@@ -103,11 +108,36 @@ 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) return;
+	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);
+	}
+	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, "abort", &opts->abort_oper, "abort the current operation"),
+		OPT_BOOLEAN(0, "skip", &opts->skip_oper, "skip the current commit"),
+		OPT_BOOLEAN(0, "continue", &opts->continue_oper, "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)",
@@ -136,7 +166,37 @@ 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 mutually incompatible command line arguments */
+	verify_opt_mutually_compatible(me,
+				"--abort", opts->abort_oper,
+				"--skip", opts->skip_oper,
+				"--continue", opts->continue_oper,
+				NULL);
+
+	/* Check for incompatible command line arguments */
+	if (opts->abort_oper || opts->skip_oper || opts->continue_oper) {
+		char *this_oper;
+		if (opts->abort_oper)
+			this_oper = "--abort";
+		else if (opts->skip_oper)
+			this_oper = "--skip";
+		else
+			this_oper = "--continue";
+
+		verify_opt_compatible(me, this_oper,
+				"--no-commit", opts->no_commit,
+				"--edit", opts->edit,
+				"--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)
@@ -146,6 +206,15 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"-x", opts->record_origin,
 				"--edit", opts->edit,
 				NULL);
+
+	/* Remove these when the options are actually implemented */
+	if (opts->abort_oper)
+		die("--abort is not implemented yet");
+	if (opts->skip_oper)
+		die("--skip is not implemented yet");
+	if (opts->continue_oper)
+		die("--continue is not implemented yet");
+
 	opts->commit_argv = argv;
 }
 
-- 
1.7.5.GIT
Ramkumar Ramachandra· May 26, 2011, 15:53 UTC · re: Ramkumar Ramachandra · lore

[PATCH 10/10] revert: Implement --abort processing

To abort, perform a "rerere clear" and "reset --hard" to the ref specified by the HEAD file introduced earlier in the series.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   46 ++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 42 insertions(+), 4 deletions(-)
Show changes to builtin/revert.c +42 −4
diff --git a/builtin/revert.c b/builtin/revert.c
index f33d40a..be63aee 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -208,8 +208,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				NULL);
 
 	/* Remove these when the options are actually implemented */
-	if (opts->abort_oper)
-		die("--abort is not implemented yet");
 	if (opts->skip_oper)
 		die("--skip is not implemented yet");
 	if (opts->continue_oper)
@@ -732,6 +730,46 @@ static int pick_commits(struct replay_opts *opts)
 	return 0;
 }
 
+static int process_continuation(struct replay_opts *opts)
+{
+	if (opts->abort_oper) {
+		char head[DEFAULT_ABBREV];
+		unsigned char sha1[20];
+		int fd;
+		rerere_clear(0);
+
+		if (!file_exists(HEAD_FILE))
+			goto error;
+		fd = open(HEAD_FILE, O_RDONLY, 0666);
+		if (fd < 0)
+			return error(_("Could not open '%s' for reading: %s"),
+				HEAD_FILE, strerror(errno));
+		if (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV) {
+			close(fd);
+			return error(_("Corrupt '%s': %s"), HEAD_FILE, strerror(errno));
+		}
+		close(fd);
+
+		if (get_sha1(head, sha1))
+			return error(_("Failed to resolve '%s' as a valid ref."), head);
+		update_ref(NULL, "HEAD", sha1, NULL, 0, MSG_ON_ERR);
+	}
+	else if (opts->skip_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+	else if (opts->continue_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+
+	return pick_commits(opts);
+error:
+	return error(_("No %s in progress"), me);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	int res;
@@ -744,7 +782,7 @@ 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);
+	res = process_continuation(&opts);
 	if (res > 0)
 		/* Exit status from conflict */
 		return res;
@@ -764,7 +802,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)
 		return res;
 	if (res < 0)
-- 
1.7.5.GIT
Junio C Hamano· May 26, 2011, 19:13 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH v4 00/10] Sequencer Foundations

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> This is a quick re-roll after Junio's review of the previous
> iteration; the previous iteration was posted less than 24 hours ago.
> I'm in a hurry,...

Open source is not just about pushing new code through other people's throats, but also about the skill to make other people around you to work with you. They may be busy spending time on helping other topics moving forward. Or they may simply be bone-headed ;-).

If you are in a hurry, try to spend _more_ time making sure it is easier to review for other people. What were updated, what were kept as they were and for what reason? The purpose of adding the cover letter to the series is to do just that.

Without spending that effort and saying "I am in a hurry" give a wrong impression to others: "I cannot tell if this round is better cooked than the previous round without spending nontrivial time reading on it, but chances are it hasn't been much improved during the 24 hours. It may be better use of my time to skip it and work on other topics first. After finishing everything else I can come back---by that time Ram may have sent out another round and reviewing this round right now may become wasted time".

Also the series seems to be based on a rather old codebase where we didn't allow reverting the root commit. I tried to apply a first few to understand where these "positive" error status could be coming from, and was stopped by patch conflicts. In [2/10] I think you are propagating errors from run_command() coming back through try_merge_command(), and the change is good for that particular codepath, but I haven't followed all the other codepaths from do_cherry_pick() to convince myself that the exit status from the merge strategy backend are _the only_ positive returns you can possibly get. Have you?

Ramkumar Ramachandra· May 27, 2011, 07:22 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 00/10] Sequencer Foundations

Hi Junio,
Junio C Hamano writes:
Show 8 quoted lines
> Without spending that effort and saying "I am in a hurry" give a wrong
> impression to others: "I cannot tell if this round is better cooked than
> the previous round without spending nontrivial time reading on it, but
> chances are it hasn't been much improved during the 24 hours.  It may be
> better use of my time to skip it and work on other topics first.  After
> finishing everything else I can come back---by that time Ram may have sent
> out another round and reviewing this round right now may become wasted
> time".

You're right; this iteration was ill-thought-out. I'll try to put myself in my reviewer's shoes before submitting code next time, so that I don't waste effort re-rolling unnecessarily.

Show 9 quoted lines
> Also the series seems to be based on a rather old codebase where we didn't
> allow reverting the root commit. I tried to apply a first few to
> understand where these "positive" error status could be coming from, and
> was stopped by patch conflicts. In [2/10] I think you are propagating
> errors from run_command() coming back through try_merge_command(), and the
> change is good for that particular codepath, but I haven't followed all
> the other codepaths from do_cherry_pick() to convince myself that the exit
> status from the merge strategy backend are _the only_ positive returns you
> can possibly get. Have you?

Oops, I'll rebase. I'll trace all the code paths carefully and include a summary of my learnings in the next iteration.

-- Ram

← back to recent threads