[PATCH 0/8] Sequencer Foundations

[PATCH 0/8] Sequencer Foundations

[Ramkumar Ramachandra]

Hi,

I've not attempted to add anything new in this series -- It merely
fixes all the mistakes in the previous iteration.  I've tried to
integrate the improvements suggested by all the previous reviews.

The format of the instruction sheet hasn't changed yet, but will soon
change to the one suggested by Chistian.  There are some nits I'm not
happy with in certain patches -- I've sprinkled those comments into
the individual patches.  All tests pass in all patches, and I hope no
stray lines have travelled b/w the patches during the rebase.

Thanks for reading.

Ramkumar Ramachandra (8):
  revert: Improve error handling by cascading errors upwards
  revert: Make "commit" and "me" local variables
  revert: Introduce a struct to parse command-line options into
  revert: Separate cmdline argument handling from the functional code
  revert: Catch incompatible command-line options early
  revert: Introduce head, todo, done files to persist state
  revert: Implement parsing --continue, --abort and --skip
  revert: Implement --abort processing

 advice.c         |   14 ++
 advice.h         |    1 +
 builtin/revert.c |  578 +++++++++++++++++++++++++++++++++++++++---------------
 3 files changed, 437 insertions(+), 156 deletions(-)

-- 
1.7.5.GIT

[PATCH 1/8] revert: Improve error handling by cascading errors upwards

[Ramkumar Ramachandra]

As a prelude to libification, we would like to propogate errors
upwards through the callchain, so as to correctly handle errors and
clean up afterwards.  Also introduce a new function
"error_resolve_conflict" in advice.c in an attempt to unify the way
conflicts are reported by the various components of Git.  As a general
guideline for libification, "die" should be only be called in two
cases: by toplevel callers like command-line argument parsing
routines, or when an irrecoverable situation is encountered.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Has the commit message justified this change fully?

 How do I trap and handle the exit status from write_message in
 do_pick_commit correctly?  There's already a call to
 do_recursive_merge whose exit status is being trapped -- what happens
 when do_recursive_merge succeeds and write_message fails, or
 viceversa?

 Junio has suggested dropping error_errno, and simply using error and
 returning -errno by hand in one email.  Considering the number of
 times I've used that tecnique, and I think we should get something
 like an error_errno atleast for the sake of terseness.

 advice.c         |   14 +++++
 advice.h         |    1 +
 builtin/revert.c |  158 ++++++++++++++++++++++++++++++-----------------------
 3 files changed, 104 insertions(+), 69 deletions(-)

diff --git a/advice.c b/advice.c
index 0be4b5f..3c3c187 100644
--- a/advice.c
+++ b/advice.c
@@ -47,3 +47,17 @@ void NORETURN die_resolve_conflict(const char *me)
 	else
 		die("'%s' is not possible because you have unmerged files.", 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.
+		 */
+		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);
+	else
+		return error("'%s' is not possible because you have unmerged files.", me);
+}
diff --git a/advice.h b/advice.h
index 3244ebb..7b7cea5 100644
--- a/advice.h
+++ b/advice.h
@@ -13,5 +13,6 @@ extern int advice_detached_head;
 int git_default_advice_config(const char *var, const char *value);
 
 extern void NORETURN die_resolve_conflict(const char *me);
+extern int error_resolve_conflict(const char *me);
 
 #endif /* ADVICE_H */
diff --git a/builtin/revert.c b/builtin/revert.c
index f697e66..fefb18b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -167,9 +167,11 @@ 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));
+	if (!p) {
+		error(_("Could not read commit message of %s"),
+			sha1_to_hex(commit->object.sha1));
+		return NULL;
+	}
 	while (*p && *p != '\n') {
 		for (eol = p + 1; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
@@ -198,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
 	strbuf_addstr(msgbuf, p);
 }
 
-static void write_cherry_pick_head(void)
+static int write_cherry_pick_head(void)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
@@ -206,12 +208,22 @@ static void write_cherry_pick_head(void)
 	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
 
 	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"),
-			  git_path("CHERRY_PICK_HEAD"));
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
-		die_errno(_("Could not write to '%s'"), git_path("CHERRY_PICK_HEAD"));
+	if (fd < 0) {
+		int err = errno;
+		strbuf_release(&buf);
+		error(_("Could not open '%s' for writing: %s"),
+			git_path("CHERRY_PICK_HEAD"), strerror(err));
+		return -err;
+	}
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd)) {
+		int err = errno;
+		strbuf_release(&buf);
+		error(_("Could not write to '%s': %s"),
+			git_path("CHERRY_PICK_HEAD"), strerror(err));
+		return -err;
+	}
 	strbuf_release(&buf);
+	return 0;
 }
 
 static void advise(const char *advice, ...)
@@ -243,17 +255,22 @@ static void print_advice(void)
 	advise("and commit the result with 'git commit'");
 }
 
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(struct strbuf *msgbuf, const char *filename)
 {
 	static struct lock_file msg_file;
 
 	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
 					       LOCK_DIE_ON_ERROR);
-	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-		die_errno(_("Could not write to %s."), filename);
+	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) {
+		int err = errno;
+		strbuf_release(msgbuf);
+		error(_("Could not write to %s: %s"), filename, strerror(err));
+		return -err;
+	}
 	strbuf_release(msgbuf);
 	if (commit_lock_file(&msg_file) < 0)
-		die(_("Error wrapping up %s"), filename);
+		return error(_("Error wrapping up %s"), filename);
+	return 0;
 }
 
 static struct tree *empty_tree(void)
@@ -266,25 +283,20 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static NORETURN void die_dirty_index(const char *me)
+static int verify_resolution(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 0;
+
+	return error_resolve_conflict(me);
+}
+
+static int error_dirty_worktree(const char *me)
+{
+	if (advice_commit_before_merge)
+		return error(_("Your local changes would be overwritten by %s.\n"
+				"Please, commit your changes or stash them to proceed."), me);
+	return error(_("Your local changes would be overwritten by %s.\n"), me);
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from)
@@ -329,10 +341,12 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 			    next_tree, base_tree, &result);
 
 	if (active_cache_changed &&
-	    (write_cache(index_fd, active_cache, active_nr) ||
-	     commit_locked_index(&index_lock)))
+		(write_cache(index_fd, active_cache, active_nr) ||
+			commit_locked_index(&index_lock))) {
+		rollback_lock_file(&index_lock);
 		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		die(_("%s: Unable to write new index file"), me);
+		return error(_("%s: Unable to write new index file"), me);
+	}
 	rollback_lock_file(&index_lock);
 
 	if (!clean) {
@@ -397,19 +411,21 @@ static int do_pick_commit(void)
 		 * that represents the "current" state for merge-recursive
 		 * to work on.
 		 */
-		if (write_cache_as_tree(head, 0, NULL))
-			die (_("Your index file is unmerged."));
+		if (write_cache_as_tree(head, 0, NULL)) {
+			discard_cache();
+			return error(_("Your index file is unmerged."));
+		}
 	} else {
 		if (get_sha1("HEAD", head))
-			die (_("You do not have a valid HEAD"));
-		if (index_differs_from("HEAD", 0))
-			die_dirty_index(me);
+			return error(_("You do not have a valid HEAD"));
+		if (index_differs_from("HEAD", 0) && !verify_resolution(me))
+			return error_dirty_worktree(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,19 +434,19 @@ 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"),
+			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."),
+	} else if (mainline > 0)
+		return error(_("Mainline was specified but commit %s is not a merge."),
 		    sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;
@@ -441,11 +457,11 @@ 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"),
+		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"),
+		return error(_("Cannot get commit message for %s"),
 				sha1_to_hex(commit->object.sha1));
 
 	/*
@@ -484,7 +500,11 @@ static int do_pick_commit(void)
 			strbuf_addstr(&msgbuf, ")\n");
 		}
 		if (!no_commit)
-			write_cherry_pick_head();
+			if ((res = write_cherry_pick_head())) {
+				free_message(&msg);
+				free(defmsg);
+				return res;
+			}
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -524,44 +544,46 @@ static int do_pick_commit(void)
 	return res;
 }
 
-static void prepare_revs(struct rev_info *revs)
+static int prepare_revs(struct rev_info *revs)
 {
-	int argc;
-
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
 	if (action != REVERT)
 		revs->reverse = 1;
 
-	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
-	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage());
+	if (setup_revisions(commit_argc, commit_argv, revs, NULL) > 1)
+		return error(_("usage: %s"), *revert_or_cherry_pick_usage());
 
 	if (prepare_revision_walk(revs))
-		die(_("revision walk setup failed"));
+		return error(_("revision walk setup failed"));
 
 	if (!revs->commits)
-		die(_("empty commit set passed"));
+		return error(_("empty commit set passed"));
+	return 0;
 }
 
-static void read_and_refresh_cache(const char *me)
+static int read_and_refresh_cache(const char *me)
 {
 	static struct lock_file index_lock;
 	int index_fd = hold_locked_index(&index_lock, 0);
 	if (read_index_preload(&the_index, NULL) < 0)
-		die(_("git %s: failed to read the index"), me);
+		return error(_("%s: failed to read the index"), me);
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
 	if (the_index.cache_changed) {
 		if (write_index(&the_index, index_fd) ||
-		    commit_locked_index(&index_lock))
-			die(_("git %s: failed to refresh the index"), me);
+			commit_locked_index(&index_lock)) {
+			rollback_lock_file(&index_lock);
+			return error(_("%s: failed to refresh the index"), me);
+		}
 	}
 	rollback_lock_file(&index_lock);
+	return 0;
 }
 
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	struct rev_info revs;
+	int res;
 
 	git_config(git_default_config, NULL);
 	me = action == REVERT ? "revert" : "cherry-pick";
@@ -579,17 +601,15 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
-	read_and_refresh_cache(me);
+	if ((res = read_and_refresh_cache(me)) ||
+		(res = prepare_revs(&revs)))
+		return res;
 
-	prepare_revs(&revs);
+	while ((commit = get_revision(&revs)) &&
+		!(res = do_pick_commit()))
+		;
 
-	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit();
-		if (res)
-			return res;
-	}
-
-	return 0;
+	return res;
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
-- 
1.7.5.GIT

Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards

[Jonathan Nieder]

Hi,

Ramkumar Ramachandra wrote:
[reordered for convenience]

> --- a/advice.c
> +++ b/advice.c
> @@ -47,3 +47,17 @@ void NORETURN die_resolve_conflict(const char *me)
>  	else
>  		die("'%s' is not possible because you have unmerged files.", 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.
> +		 */
> +		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);
> +	else
> +		return error("'%s' is not possible because you have unmerged files.", me);
> +}

Would it make sense to do

 void NORETURN die_resolve_conflict(const char *me)
 {
	error_resolve_conflict(me);
	exit(128);
 }

or is the s/fatal/error/ in output too much?  I would suspect
saying "error:" instead of "fatal:" should be okay if it means having
less code to deal with, but if not, maybe there is some other way to
achieve the appropriate effect (e.g. --- please don’t use this; it’s
just an example --- when a global in usage.c about_to_die is true,
changing "error:" to "fatal:" or something like that).

> As a general
> guideline for libification, "die" should be only be called in two
> cases: by toplevel callers like command-line argument parsing
> routines, or when an irrecoverable situation is encountered.

The above hints at to a downside to this principle: currently the
message printed by "die" is clearly labelled as git’s cause of death.
Still, I think the principle is right and that it’s worth it, and that
generally speaking a library function should not have to know whether
its errors would be an appropriate time to exit or this is a GUI that
will want to stay alive.

>  Has the commit message justified this change fully?

It didn’t mention "GUI", so I guess no. :)

Could you give a reminder about how this fits in the sequencer series?
Is it that you want to give advice to the user about how to recover
when exiting (and if so, could an atexit handler work instead for
that)?

I'm reviewing the rest because I like the aforementioned "GUI" use
case, but if you get tired of the exercise, please remember that it is
not self-evidently necessary to continue along this track for the sake
of the sequencer alone.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -167,9 +167,11 @@ 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));
> +	if (!p) {
> +		error(_("Could not read commit message of %s"),
> +			sha1_to_hex(commit->object.sha1));
> +		return NULL;

The only caller to get_encoding is get_message, which already returns
early when not passed a usable message.  So maybe this should say

	assert(p);

or

	if (!p)
		die("BUG: get_message caller forgot to give a commit message");

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -198,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
>  	strbuf_addstr(msgbuf, p);
>  }
>  
> -static void write_cherry_pick_head(void)
> +static int write_cherry_pick_head(void)

write_cherry_pick_head gets called by do_pick_commit to help a person
pick up where "git cherry-pick" left off if it has to exit when
resolving conflicts.  So it’s a good example of how to avoid having
to clean up after code when it fails. :)

Anyway, the question here is, what should happen if
write_cherry_pick_head fails (for example due to EPERM or ENOSPC)?  If
this is a GUI or another program that has to stay alive, I suppose one
would want to report the error and back out the cherry-pick of the
current commit as far as it has proceeded (meaning freeing some
memory).  From the signature above I would expect that it does

	return error(...);

on failure (which would bring up an explanation of the failure under
the tab bar in my GUI).

>  {
>  	int fd;
>  	struct strbuf buf = STRBUF_INIT;
> @@ -206,12 +208,22 @@ static void write_cherry_pick_head(void)
>  	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
>  
>  	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
> -	if (fd < 0)
> -		die_errno(_("Could not open '%s' for writing"),
> -			  git_path("CHERRY_PICK_HEAD"));
> +	if (fd < 0) {
> +		int err = errno;
> +		strbuf_release(&buf);
> +		error(_("Could not open '%s' for writing: %s"),
> +			git_path("CHERRY_PICK_HEAD"), strerror(err));
> +		return -err;
> +	}

Why does the caller care about errno (i.e., why "return -errno"
instead of "return error(...)")?

> -	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
> -		die_errno(_("Could not write to '%s'"), git_path("CHERRY_PICK_HEAD"));
> +	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd)) {
> +		int err = errno;
> +		strbuf_release(&buf);
> +		error(_("Could not write to '%s': %s"),
> +			git_path("CHERRY_PICK_HEAD"), strerror(err));
> +		return -err;
> +	}

In the write_in_full case, missing "close(fd)".

In the close(fd) case, not missing "close(fd)", unless we want to
check for errno == EINTR and handle that.  (If we do, though, it would
probably be in a separate patch, by introducing an xclose function
analagous to xwrite.)  So I suppose this should look something like

	if (write_in_full(...)) {
		ret = error(_("Could not write...
		goto done3;
	}
	if (close(fd)) {
		ret = error(_(...
		goto done2;
	}
	goto done;

 done3:
	close(fd);
 done2:
	unlink_or_warn(git_path("CHERRY_PICK_HEAD"));
 done:
	strbuf_release(&buf);
	return ret;
 }

> @@ -243,17 +255,22 @@ static void print_advice(void)
>  	advise("and commit the result with 'git commit'");
>  }
>  
> -static void write_message(struct strbuf *msgbuf, const char *filename)
> +static int write_message(struct strbuf *msgbuf, const char *filename)

write_message writes the MERGE_MSG file.  Like get_cherry_pick_head,
its purpose is to allow recovery if the cherry-pick runs into
conflicts, and presumably the appropriate way to recover would be to
back out the cherry-pick (meaning to free buffers used so far and
remove the CHERRY_PICK_HEAD file).

>  {
>  	static struct lock_file msg_file;
>  
>  	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
>  					       LOCK_DIE_ON_ERROR);
> -	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
> -		die_errno(_("Could not write to %s."), filename);
> +	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0) {
> +		int err = errno;
> +		strbuf_release(msgbuf);
> +		error(_("Could not write to %s: %s"), filename, strerror(err));
> +		return -err;
> +	}

Same question as before: why does the caller care about the nature of
the error?

hold_lock_file will die on error unless we declare we want to handle
that ourselves through its flags word.  If we are not about to exit,
we will also want to roll back the lockfile on errors, too.

>  	strbuf_release(msgbuf);
>  	if (commit_lock_file(&msg_file) < 0)
> -		die(_("Error wrapping up %s"), filename);
> +		return error(_("Error wrapping up %s"), filename);

If we use the "return -errno" convention, that would mean "return
-EPERM" on traditional Unixen.

[...]
> +static int error_dirty_worktree(const char *me)
> +{
> +	if (advice_commit_before_merge)
> +		return error(_("Your local changes would be overwritten by %s.\n"
> +				"Please, commit your changes or stash them to proceed."), me);
> +	return error(_("Your local changes would be overwritten by %s.\n"), me);
> +}

Side note: git still doesn’t have a good API for dealing with advice.
I think I’d prefer

	error(_("Your local changes would be overwritten by %s.\n"), me);
	if (advice_commit_before_merge)
		advise(_("Please, commit your...

or perhaps that last part would be

	advise(advice_commit_before_merge,
	       _("Please,...

which would write

 fatal: your local changes would be overwritten by the cherry-pick
 hint: please, commit your changes or stash them to proceed

> -static NORETURN void die_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."));
[...]
> +static int verify_resolution(const char *me)
> +{
> +	if (!read_cache_unmerged())
> +		return 0;
> +
> +	return error_resolve_conflict(me);
> +}
> +

How is this function meant to be used?  die_dirty_index would
presumably become something like

	if (verify_resolution(me))
		return -1;
	return error_dirty_worktree(me);

but that is not what your caller for this does.  Will say more when I
get to it.

[...]
> @@ -329,10 +341,12 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  			    next_tree, base_tree, &result);
>  
>  	if (active_cache_changed &&
> -	    (write_cache(index_fd, active_cache, active_nr) ||
> -	     commit_locked_index(&index_lock)))
> +		(write_cache(index_fd, active_cache, active_nr) ||
> +			commit_locked_index(&index_lock))) {

Whitespace changes seem to have snuck in.

> +		rollback_lock_file(&index_lock);

Makes sense.

>  		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
> -		die(_("%s: Unable to write new index file"), me);
> +		return error(_("%s: Unable to write new index file"), me);
> +	}
>  	rollback_lock_file(&index_lock);

merge_recursive can die, too (e.g., "error building trees").

[...]
> @@ -397,19 +411,21 @@ static int do_pick_commit(void)

do_pick_commit is the main worker of "git cherry-pick foo..bar".  When
it returned an error, traditionally that meant it had encountered a
conflict, and the return value would what exit status to use to
indicate that (typically 1, one hopes, but perhaps 139 if the merge
strategy segfaulted, etc).

To recover, I suppose I can imagine a caller wanting to "try harder".
At any rate the CHERRY_PICK_HEAD, MERGE_HEAD, on-disk dirty index, and
so on are good state that should be kept.

>  		 * that represents the "current" state for merge-recursive
>  		 * to work on.
>  		 */
> -		if (write_cache_as_tree(head, 0, NULL))
> -			die (_("Your index file is unmerged."));
> +		if (write_cache_as_tree(head, 0, NULL)) {
> +			discard_cache();

I suppose this is cleaning up after the read_cache call in
write_cache_as_tree?  But the above does not back out the index lock
at the same time, so I don’t find it so clear.  I would prefer to
see write_cache_as_tree taught to clean up after itself on error.

[...]
> @@ -418,19 +434,19 @@ 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));

This is not a conflict but an error in usage.  Stepping back for a
second, what is the best way to handle that?  The aforementioned
hypothetical caller considering "try an alternate strategy" would make
a wrong move, so we need to use a return value that would dissuade it.

I’m not sure what the most appropriate thing to do is.  Maybe

	/*
	 * Positive numbers are exit status from conflicts; negative
	 * numbers are other errors.
	 */
	enum pick_commit_error {
		PICK_COMMIT_USAGE_ERROR = -1
	};

but it’s hard to think clearly about it since it seems too
hypothetical to me.  If all callers are going to exit, then

	error(...);
	return 129;

will work, but in that case why not exit for them?

>  
>  		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"),
> +			return error(_("Commit %s does not have parent %d"),
>  			    sha1_to_hex(commit->object.sha1), mainline);

Likewise.

>  		parent = p->item;
> -	} else if (0 < mainline)
> -		die(_("Mainline was specified but commit %s is not a merge."),
> +	} else if (mainline > 0)
> +		return error(_("Mainline was specified but commit %s is not a merge."),
>  		    sha1_to_hex(commit->object.sha1));

Likewise.

>  	else
>  		parent = commit->parents->item;
> @@ -441,11 +457,11 @@ 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"),
> +		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"),
> +		return error(_("Cannot get commit message for %s"),
>  				sha1_to_hex(commit->object.sha1));
>  
>  	/*
> @@ -484,7 +500,11 @@ static int do_pick_commit(void)
>  			strbuf_addstr(&msgbuf, ")\n");
>  		}
>  		if (!no_commit)
> -			write_cherry_pick_head();
> +			if ((res = write_cherry_pick_head())) {
> +				free_message(&msg);
> +				free(defmsg);
> +				return res;
> +			}


These need to eventually exit with status 128.

> @@ -524,44 +544,46 @@ static int do_pick_commit(void)
>  	return res;
>  }
>  
> -static void prepare_revs(struct rev_info *revs)
> +static int prepare_revs(struct rev_info *revs)

prepare_revs is used by the cherry-pick porcelain after parsing
arguments to initialize the revision walker.  If it fails, we can
save some trouble and cancel the whole operation.

>  {
> -	int argc;
> -
>  	init_revisions(revs, NULL);
>  	revs->no_walk = 1;
>  	if (action != REVERT)
>  		revs->reverse = 1;
>  
> -	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
> -	if (argc > 1)
> -		usage(*revert_or_cherry_pick_usage());
> +	if (setup_revisions(commit_argc, commit_argv, revs, NULL) > 1)
> +		return error(_("usage: %s"), *revert_or_cherry_pick_usage());

The exit status for errors in usage ought to be 129, and the message

	error: usage: ...

is not so pretty. :)

I think a library version of this would want to take a struct rev_info
that has been prepopulated.

[...]
> -static void read_and_refresh_cache(const char *me)
> +static int read_and_refresh_cache(const char *me)
>  {
>  	static struct lock_file index_lock;
>  	int index_fd = hold_locked_index(&index_lock, 0);
>  	if (read_index_preload(&the_index, NULL) < 0)
> -		die(_("git %s: failed to read the index"), me);
> +		return error(_("%s: failed to read the index"), me);

What is the caller expecting to happen when this fails?  Do we
want to roll back the lockfile and discard the index?

>  	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
>  	if (the_index.cache_changed) {
>  		if (write_index(&the_index, index_fd) ||
> -		    commit_locked_index(&index_lock))
> +			commit_locked_index(&index_lock)) {

Whitespace change snuck in.

> -			die(_("git %s: failed to refresh the index"), me);
> +			rollback_lock_file(&index_lock);
> +			return error(_("%s: failed to refresh the index"), me);

Likewise.  (Should this discard the index?  If so, why?  If not, why
not?)

> +		}
>  	}
>  	rollback_lock_file(&index_lock);
> +	return 0;
>  }
>  
>  static int revert_or_cherry_pick(int argc, const char **argv)

The porcelain itself, but the sequencer wants to call it.

>  {
>  	struct rev_info revs;
> +	int res;
>  
>  	git_config(git_default_config, NULL);
>  	me = action == REVERT ? "revert" : "cherry-pick";
> @@ -579,17 +601,15 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  			die(_("cherry-pick --ff cannot be used with --edit"));
>  	}
>  
> -	read_and_refresh_cache(me);
> -	prepare_revs(&revs);
> -
> +	if ((res = read_and_refresh_cache(me)) ||
> +		(res = prepare_revs(&revs)))
> +		return res;

Clearer to write:

	if (read_and_refresh_cache(me) ||
	    prepare_revs(&revs))
		return -1;

and it has the nice side-effect of making it clear to callers that
they don’t have to worry about return value conventions from those
functions.

> -	while ((commit = get_revision(&revs))) {
> -		int res = do_pick_commit();
> -		if (res)
> -			return res;
> -	}
> -
> -	return 0;
> +	while ((commit = get_revision(&revs)) &&
> +		!(res = do_pick_commit()))
> +		;
> +
> +	return res;

I think the original is clearer here.

>  }
>
[...]
>  How do I trap and handle the exit status from write_message in
>  do_pick_commit correctly?  There's already a call to
>  do_recursive_merge whose exit status is being trapped -- what happens
>  when do_recursive_merge succeeds and write_message fails, or
>  viceversa?

Good question.  See my confused "enum" ramble above.

>  Junio has suggested dropping error_errno, and simply using error and
>  returning -errno by hand in one email.  Considering the number of
>  times I've used that tecnique, and I think we should get something
>  like an error_errno atleast for the sake of terseness.

I do think an error_errno that unconditionally returns -1 (analagous
to die_errno) would be a nice thing to have, even though it wouldn’t
make anything much shorter. :)

*whew*

Thanks and hope that helps,
Jonathan

Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards

[Ramkumar Ramachandra]

Hi,

As you've pointed out, this patch is a complete disaster.  Error
handling is very non-trivial, and my earlier attempts have failed.
I'll try to redo this, and then respond to your review.

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> [...]
> *whew*

-- Ram

Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards

[Ramkumar Ramachandra]

Hi,

I'm preparing a large series dedicated to solving error-handling
issues before getting to the sequencer series.  I plan to post some
quick-and-dirty diffs of various things and ask for feedback.

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> > @@ -418,19 +434,19 @@ 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));
>
> This is not a conflict but an error in usage.  Stepping back for a
> second, what is the best way to handle that?  The aforementioned
> hypothetical caller considering "try an alternate strategy" would make
> a wrong move, so we need to use a return value that would dissuade it.
>
> I’m not sure what the most appropriate thing to do is.  Maybe
>
>       /*
>        * Positive numbers are exit status from conflicts; negative
>        * numbers are other errors.
>        */
>       enum pick_commit_error {
>               PICK_COMMIT_USAGE_ERROR = -1
>       };
>
> but it’s hard to think clearly about it since it seems too
> hypothetical to me.  If all callers are going to exit, then
>
>       error(...);
>       return 129;
>
> will work, but in that case why not exit for them?

For this part, I think the correct way to handle the usage error is to
print a message like this:

   usage: cherry-pick: Commit b8bf32 is a merge but no -m option was given.

And exit with status 129. Is this acceptable?

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

@@ -418,20 +424,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));
+                       usage(_("%s: Commit %s is a merge but no -m option was given."),
+                               me, 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);
+                       usage(_("%s: Commit %s does not have parent %d"),
+                               me, 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));
+       } else if (mainline > 0)
+               usage(_("%s: Mainline was specified but commit %s is not a merge."),
+                       me, sha1_to_hex(commit->object.sha1));

Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> I'm preparing a large series dedicated to solving error-handling
> issues before getting to the sequencer series.  I plan to post some
> quick-and-dirty diffs of various things and ask for feedback.

Sounds good.  I wonder if there's a good way to test this (maybe I'll
try making an XS module so Git.pm can take advantage of similar
changes).

> Jonathan Nieder writes:
>> Ramkumar Ramachandra wrote:

>>> @@ -418,19 +434,19 @@ 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));
>>
>> This is not a conflict but an error in usage.
[...]
> For this part, I think the correct way to handle the usage error is to
> print a message like this:
>
>     usage: cherry-pick: Commit b8bf32 is a merge but no -m option was given.
>
> And exit with status 129. Is this acceptable?

On second thought, status 128 seems appropriate --- the caller made a
mistake, but it's more analagous to merging with a dirty index than
(i.e., the command was not able to be fulfilled as desired) than to
misspelling a command-line option (i.e., broken script).  I suppose
treating it as an error (as in your "return error") would work, and
the caller can transform -1 to exit(128).

Sorry for the thinko.

Re: [PATCH 1/8] revert: Improve error handling by cascading errors upwards

[Ramkumar Ramachandra]

Hi Jonathan,

I'm a little confused, so I'm also including a commit message
justifying the change.
Have I understood the issue correctly? Does this diff look alright?

Note: I've removed the die from get_message in another unrelated
patch; essentially, do_pick_commit never calls die.

    Since do_pick_commit is only delegated the job of picking a single
    commit in an entire cherry-pick or revert operation, don't die in this
    function.  Instead, return an error to be handled by the caller
    appropriately.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

diff --git a/builtin/revert.c b/builtin/revert.c
index 0cc3b6b..138485f 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -415,20 +415,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(_("%s: Commit %s is a merge but no -m option was given."),
+				me, 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(_("%s: Commit %s does not have parent %d"),
+				me, 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(_("%s: Mainline was specified but commit %s is not a merge."),
+			me, sha1_to_hex(commit->object.sha1));
 	else
 		parent = commit->parents->item;

@@ -438,8 +438,8 @@ 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));

 	/*
 	 * "commit" is an existing commit.  We would want to apply
@@ -578,8 +578,8 @@ static int revert_or_cherry_pick(int argc, const
char **argv)

 	while ((commit = get_revision(&revs))) {
 		int res = do_pick_commit();
-		if (res)
-			return res;
+		if (res < 0)
+			exit(128);
 	}

 	return 0;

[PATCH 2/8] revert: Make "commit" and "me" local variables

[Ramkumar Ramachandra]

Currently, "commit" and "me" are global static variables. Since we
want to develop the functionality to either pick/ revert individual
commits atomically later in the series, make them local variables.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 The variable "me" is nowhere as fundamental as "commit" -- it's
 simply a string derived from a more fundamental "action".  Yet, the
 commit message seems to indicate that both "me" and "commit" are
 equally important -- how should it be reworded?

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

diff --git a/builtin/revert.c b/builtin/revert.c
index fefb18b..e5c3c6c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -37,12 +37,10 @@ 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;
 
-static const char *me;
 
 /* Merge strategy. */
 static const char *strategy;
@@ -51,7 +49,7 @@ static size_t xopts_nr, xopts_alloc;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
-static char *get_encoding(const char *message);
+static char *get_encoding(struct commit *commit, const char *message);
 
 static const char * const *revert_or_cherry_pick_usage(void)
 {
@@ -116,7 +114,8 @@ 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, const char *raw_message,
+		struct commit_message *out)
 {
 	const char *encoding;
 	const char *abbrev, *subject;
@@ -125,7 +124,7 @@ static int get_message(const char *raw_message, struct commit_message *out)
 
 	if (!raw_message)
 		return -1;
-	encoding = get_encoding(raw_message);
+	encoding = get_encoding(commit, raw_message);
 	if (!encoding)
 		encoding = "UTF-8";
 	if (!git_commit_encoding)
@@ -163,7 +162,7 @@ static void free_message(struct commit_message *msg)
 	free(msg->reencoded_message);
 }
 
-static char *get_encoding(const char *message)
+static char *get_encoding(struct commit *commit, const char *message)
 {
 	const char *p = message, *eol;
 
@@ -187,7 +186,8 @@ 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(struct commit *commit, struct strbuf *msgbuf,
+			const char *message)
 {
 	const char *p = message;
 	while (*p && (*p != '\n' || p[1] != '\n'))
@@ -200,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
 	strbuf_addstr(msgbuf, p);
 }
 
-static int write_cherry_pick_head(void)
+static int write_cherry_pick_head(struct commit *commit)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
@@ -319,6 +319,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	int clean, index_fd;
 	const char **xopt;
 	static struct lock_file index_lock;
+	const char *me = (action == REVERT ? "revert" : "cherry-pick");
 
 	index_fd = hold_locked_index(&index_lock, 1);
 
@@ -394,7 +395,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;
@@ -402,6 +403,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 *me = (action == REVERT ? "revert" : "cherry-pick");
 	int res;
 
 	if (no_commit) {
@@ -458,9 +460,10 @@ static int do_pick_commit(void)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
 		return error(_("%s: cannot parse parent commit %s"),
-		    me, sha1_to_hex(parent->object.sha1));
+			action == REVERT ? "revert" : "cherry-pick",
+			sha1_to_hex(parent->object.sha1));
 
-	if (get_message(commit->buffer, &msg) != 0)
+	if (get_message(commit, commit->buffer, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
 				sha1_to_hex(commit->object.sha1));
 
@@ -493,14 +496,14 @@ 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_message_to_msg(commit, &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)
-			if ((res = write_cherry_pick_head())) {
+			if ((res = write_cherry_pick_head(commit))) {
 				free_message(&msg);
 				free(defmsg);
 				return res;
@@ -562,10 +565,13 @@ static int prepare_revs(struct rev_info *revs)
 	return 0;
 }
 
-static int read_and_refresh_cache(const char *me)
+static int read_and_refresh_cache(void)
 {
 	static struct lock_file index_lock;
 	int index_fd = hold_locked_index(&index_lock, 0);
+	const char *me;
+
+	me = (action == REVERT ? "revert" : "cherry-pick");
 	if (read_index_preload(&the_index, NULL) < 0)
 		return error(_("%s: failed to read the index"), me);
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
@@ -583,10 +589,12 @@ static int 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;
+	const char *me;
 	int res;
 
 	git_config(git_default_config, NULL);
-	me = action == REVERT ? "revert" : "cherry-pick";
+	me = (action == REVERT ? "revert" : "cherry-pick");
 	setenv(GIT_REFLOG_ACTION, me, 0);
 	parse_args(argc, argv);
 
@@ -601,12 +609,12 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
-	if ((res = read_and_refresh_cache(me)) ||
+	if ((res = read_and_refresh_cache()) ||
 		(res = prepare_revs(&revs)))
 		return res;
 
 	while ((commit = get_revision(&revs)) &&
-		!(res = do_pick_commit()))
+		!(res = do_pick_commit(commit)))
 		;
 
 	return res;
-- 
1.7.5.GIT

Re: [PATCH 2/8] revert: Make "commit" and "me" local variables

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Currently, "commit" and "me" are global static variables. Since we
> want to develop the functionality to either pick/ revert individual
> commits atomically later in the series, make them local variables.

I suppose the idea is that the current commit and whether we are
cherry-picking or reverting is not global state and should be allowed
to differ between threads, or that for easier debugging we would like
to narrow their scope.

How does this relate to the sequencer series?  Maybe the idea is that
they are explicit parameters in the functions that will be exposed
rather than that they are local variables?

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  The variable "me" is nowhere as fundamental as "commit" -- it's
>  simply a string derived from a more fundamental "action".

That suggests to me that "action" should probably be made local at the
same time.  On second thought, it looks like this commit is doing two
unrelated things ---

 - simplifying the state that has to be kept by computing "me"
   from "action" on the fly

 - narrowing the scope of "commit" and passing it around explicitly

and would be clearer as two separate commits.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -51,7 +49,7 @@ static size_t xopts_nr, xopts_alloc;
>  
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>  
> -static char *get_encoding(const char *message);
> +static char *get_encoding(struct commit *commit, const char *message);

If the die is converted to an assert or die("BUG: ...") without
specifying which commit then this first parameter is not needed.

>  static const char * const *revert_or_cherry_pick_usage(void)
>  {
> @@ -116,7 +114,8 @@ 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, const char *raw_message,
> +		struct commit_message *out)

Likewise.

[...]
> @@ -187,7 +186,8 @@ 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(struct commit *commit, struct strbuf *msgbuf,
> +			const char *message)

Perhaps the new parameter could be "const char *fallback" and the
caller call sha1_to_hex unconditionally?  (Yes, it sounds like wasted
computation, but it might be worth the clarity.)

> @@ -200,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
>  	strbuf_addstr(msgbuf, p);
>  }
>  
> -static int write_cherry_pick_head(void)
> +static int write_cherry_pick_head(struct commit *commit)

Ah, it might not be wasted computation.  This could take
commit_sha1_hex as parameter so it only needs to be computed once.

> @@ -319,6 +319,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  	int clean, index_fd;
>  	const char **xopt;
>  	static struct lock_file index_lock;
> +	const char *me = (action == REVERT ? "revert" : "cherry-pick");

Style: I find this clearer without the parentheses (but feel free to
ignore).

[...]
> @@ -402,6 +403,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 *me = (action == REVERT ? "revert" : "cherry-pick");
>  	int res;
>  
>  	if (no_commit) {
> @@ -458,9 +460,10 @@ static int do_pick_commit(void)
>  		/* TRANSLATORS: The first %s will be "revert" or
>  		   "cherry-pick", the second %s a SHA1 */
>  		return error(_("%s: cannot parse parent commit %s"),
> -		    me, sha1_to_hex(parent->object.sha1));
> +			action == REVERT ? "revert" : "cherry-pick",
> +			sha1_to_hex(parent->object.sha1));

I think one of the computations of "me" is left over.

> @@ -562,10 +565,13 @@ static int prepare_revs(struct rev_info *revs)
>  	return 0;
>  }
>  
> -static int read_and_refresh_cache(const char *me)
> +static int read_and_refresh_cache(void)

Since you seem to be moving towards having fewer statics and more
explicit parameters, I think this part is a step backwards.  Maybe it
should take "action" as a parameter instead.

> @@ -583,10 +589,12 @@ static int 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;
> +	const char *me;
>  	int res;
>  
>  	git_config(git_default_config, NULL);
> -	me = action == REVERT ? "revert" : "cherry-pick";
> +	me = (action == REVERT ? "revert" : "cherry-pick");

Why?

>  	setenv(GIT_REFLOG_ACTION, me, 0);
>  	parse_args(argc, argv);
>  

Sorry, mostly nitpicks.  Still, hope that helps.

Regards,
Jonathan

Re: [PATCH 2/8] revert: Make "commit" and "me" local variables

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> > Currently, "commit" and "me" are global static variables. Since we
> > want to develop the functionality to either pick/ revert individual
> > commits atomically later in the series, make them local variables.
> 
> I suppose the idea is that the current commit and whether we are
> cherry-picking or reverting is not global state and should be allowed
> to differ between threads, or that for easier debugging we would like
> to narrow their scope.
> 
> How does this relate to the sequencer series?  Maybe the idea is that
> they are explicit parameters in the functions that will be exposed
> rather than that they are local variables?

Right.  I'll attempt to reword this in the next iteration.

> >
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> > ---
> >  The variable "me" is nowhere as fundamental as "commit" -- it's
> >  simply a string derived from a more fundamental "action".
> 
> That suggests to me that "action" should probably be made local at the
> same time.  On second thought, it looks like this commit is doing two
> unrelated things ---
> 
>  - simplifying the state that has to be kept by computing "me"
>    from "action" on the fly
> 
>  - narrowing the scope of "commit" and passing it around explicitly
> 
> and would be clearer as two separate commits.

Good idea -- I'll split this up into two distinct commits in the next
iteration.

> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> [...]
> > @@ -51,7 +49,7 @@ static size_t xopts_nr, xopts_alloc;
> >  
> >  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
> >  
> > -static char *get_encoding(const char *message);
> > +static char *get_encoding(struct commit *commit, const char *message);
> 
> If the die is converted to an assert or die("BUG: ...") without
> specifying which commit then this first parameter is not needed.

Agreed.  It should probably be an assertion failure, since the caller
should use the get_encoding calling API responsibly.

> > @@ -187,7 +186,8 @@ 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(struct commit *commit, struct strbuf *msgbuf,
> > +			const char *message)
> 
> Perhaps the new parameter could be "const char *fallback" and the
> caller call sha1_to_hex unconditionally?  (Yes, it sounds like wasted
> computation, but it might be worth the clarity.)

and

> > @@ -200,7 +200,7 @@ static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
> >  	strbuf_addstr(msgbuf, p);
> >  }
> >  
> > -static int write_cherry_pick_head(void)
> > +static int write_cherry_pick_head(struct commit *commit)
> 
> Ah, it might not be wasted computation.  This could take
> commit_sha1_hex as parameter so it only needs to be computed once.

Okay.

> > @@ -319,6 +319,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> >  	int clean, index_fd;
> >  	const char **xopt;
> >  	static struct lock_file index_lock;
> > +	const char *me = (action == REVERT ? "revert" : "cherry-pick");
> 
> Style: I find this clearer without the parentheses (but feel free to
> ignore).
> 
> [...]
> > @@ -402,6 +403,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 *me = (action == REVERT ? "revert" : "cherry-pick");
> >  	int res;
> >  
> >  	if (no_commit) {
> > @@ -458,9 +460,10 @@ static int do_pick_commit(void)
> >  		/* TRANSLATORS: The first %s will be "revert" or
> >  		   "cherry-pick", the second %s a SHA1 */
> >  		return error(_("%s: cannot parse parent commit %s"),
> > -		    me, sha1_to_hex(parent->object.sha1));
> > +			action == REVERT ? "revert" : "cherry-pick",
> > +			sha1_to_hex(parent->object.sha1));
> 
> I think one of the computations of "me" is left over.

Right; leaked into another patch -- rebase fail :|

> > @@ -562,10 +565,13 @@ static int prepare_revs(struct rev_info *revs)
> >  	return 0;
> >  }
> >  
> > -static int read_and_refresh_cache(const char *me)
> > +static int read_and_refresh_cache(void)
> 
> Since you seem to be moving towards having fewer statics and more
> explicit parameters, I think this part is a step backwards.  Maybe it
> should take "action" as a parameter instead.

I'll think about this.

> > @@ -583,10 +589,12 @@ static int 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;
> > +	const char *me;
> >  	int res;
> >  
> >  	git_config(git_default_config, NULL);
> > -	me = action == REVERT ? "revert" : "cherry-pick";
> > +	me = (action == REVERT ? "revert" : "cherry-pick");
> 
> Why?

Consistency, mainly.  I can't remember operator precedence, and there
are three operators in that line.  Either way, I'll lose the
paranthesis if it's clear enough otherwise.

> >  	setenv(GIT_REFLOG_ACTION, me, 0);
> >  	parse_args(argc, argv);
> >  
> 
> Sorry, mostly nitpicks.  Still, hope that helps.

Yes.  Thanks.

-- Ram

Re: [PATCH 2/8] revert: Make "commit" and "me" local variables

[Daniel Barkalow]

On Wed, 11 May 2011, Ramkumar Ramachandra wrote:

> @@ -583,10 +589,12 @@ static int 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;
> +	const char *me;
>  	int res;
>  
>  	git_config(git_default_config, NULL);
> -	me = action == REVERT ? "revert" : "cherry-pick";
> +	me = (action == REVERT ? "revert" : "cherry-pick");
>  	setenv(GIT_REFLOG_ACTION, me, 0);
>  	parse_args(argc, argv);

Later in the series, you remove everything related to "me" that you add 
here, having modified it once in the middle. Just go to

	setenv(GIT_REFLOG_ACTION, action == REVERT ? "revert" : "cherry-pick");

and remove "me" from this function in this patch. For that matter, 
squashing together the "opts" patch and this one would probably make them 
make more sense: there's not much benefit to getting rid of some globals 
when other globals remain that matter for the same reason. It also fixes 
the annoyance that each place you introduce "me" in this patch needs to be 
changed later in the series.

	-Daniel
*This .sig left intentionally blank*

[PATCH 3/8] revert: Introduce a struct to parse command-line options into

[Ramkumar Ramachandra]

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>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 I get the following warning from GCC: warning: useless storage class
 specifier in empty declaration (at the line where I've declared the
 replay_opts struct).  What is the correct way to fix this?

 Also, I'm not happy with the way I'm parsing xopts-related options
 from option_parse_x into global static variables, before actually
 putting it into the structure.  Is there a better way to do this?

 builtin/revert.c |  198 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 111 insertions(+), 87 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index e5c3c6c..8550927 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,29 +35,42 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_replay, 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;
-
-
-/* Merge strategy. */
-static const char *strategy;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+static struct replay_opts {
+	enum { REVERT, CHERRY_PICK } action;
+
+	/* Boolean options */
+	int edit;
+	int no_replay;
+	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(struct commit *commit, 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;
 }
 
+/* For option_parse_x */
+static const char **xopts;
+static size_t xopts_nr, xopts_alloc;
+
 static int option_parse_x(const struct option *opt,
-			  const char *arg, int unset)
+			const char *arg, int unset)
 {
 	if (unset)
 		return 0;
@@ -67,19 +80,18 @@ static int option_parse_x(const struct option *opt,
 	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"),
-		{ 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_BOOLEAN('n', "no-commit", &(opts->no_commit), "don't automatically commit"),
+		OPT_BOOLEAN('e', "edit", &(opts->edit), "edit the commit message"),
+		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
+		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", &xopts, "option",
 			"option for merge strategy", option_parse_x),
 		OPT_END(),
@@ -87,23 +99,29 @@ static void parse_args(int argc, const char **argv)
 		OPT_END(),
 	};
 
-	if (action == CHERRY_PICK) {
+	if (opts->action == CHERRY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOLEAN('x', NULL, &no_replay, "append commit name"),
-			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
+			OPT_BOOLEAN('x', NULL, &(opts->no_replay), "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,
+	opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
 				    PARSE_OPT_KEEP_ARGV0 |
 				    PARSE_OPT_KEEP_UNKNOWN);
-	if (commit_argc < 2)
+
+	/* Fill in the opts struct from values set by option_parse_x */
+	opts->xopts = xopts;
+	opts->xopts_nr = xopts_nr;
+	opts->xopts_alloc = xopts_alloc;
+
+	if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
-	commit_argv = argv;
+	opts->commit_argv = argv;
 }
 
 struct commit_message {
@@ -311,15 +329,15 @@ 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)
+			const char *base_label, const char *next_label,
+			unsigned char *head, struct strbuf *msgbuf,
+			struct replay_opts *opts)
 {
 	struct merge_options o;
 	struct tree *result, *next_tree, *base_tree, *head_tree;
 	int clean, index_fd;
 	const char **xopt;
 	static struct lock_file index_lock;
-	const char *me = (action == REVERT ? "revert" : "cherry-pick");
 
 	index_fd = hold_locked_index(&index_lock, 1);
 
@@ -334,7 +352,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,
@@ -346,7 +364,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 			commit_locked_index(&index_lock))) {
 		rollback_lock_file(&index_lock);
 		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		return error(_("%s: Unable to write new index file"), me);
+		return error(_("%s: Unable to write new index file"),
+			opts->action == REVERT ? "revert" : "cherry-pick");
 	}
 	rollback_lock_file(&index_lock);
 
@@ -376,7 +395,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];
@@ -384,9 +403,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;
 	}
@@ -395,7 +414,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;
@@ -403,10 +422,10 @@ static int do_pick_commit(struct commit *commit)
 	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
-	const char *me = (action == REVERT ? "revert" : "cherry-pick");
+	const char *me = (opts->action == REVERT ? "revert" : "cherry-pick");
 	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
@@ -426,7 +445,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;
 	}
@@ -435,32 +454,31 @@ 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 (mainline > 0)
+	} else if (opts->mainline > 0)
 		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)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
-		return error(_("%s: cannot parse parent commit %s"),
-			action == REVERT ? "revert" : "cherry-pick",
+		return error(_("%s: cannot parse parent commit %s"), me,
 			sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit, commit->buffer, &msg) != 0)
@@ -476,7 +494,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;
@@ -497,12 +515,12 @@ static int do_pick_commit(struct commit *commit)
 		next = commit;
 		next_label = msg.label;
 		add_message_to_msg(commit, &msgbuf, msg.message);
-		if (no_replay) {
+		if (opts->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)
+		if (!opts->no_commit)
 			if ((res = write_cherry_pick_head(commit))) {
 				free_message(&msg);
 				free(defmsg);
@@ -510,9 +528,9 @@ static int do_pick_commit(struct commit *commit)
 			}
 	}
 
-	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
-					 head, &msgbuf);
+					head, &msgbuf, opts);
 		write_message(&msgbuf, defmsg);
 	} else {
 		struct commit_list *common = NULL;
@@ -522,23 +540,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,
+		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
-		      ? _("could not revert %s... %s")
-		      : _("could not apply %s... %s"),
-		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
-		      msg.subject);
+		error(_("could not %s %s... %s"),
+			opts->action == REVERT ? "revert" : "apply",
+			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);
@@ -547,15 +565,15 @@ static int do_pick_commit(struct commit *commit)
 	return res;
 }
 
-static int prepare_revs(struct rev_info *revs)
+static int prepare_revs(struct rev_info *revs, struct replay_opts *opts)
 {
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (action != REVERT)
+	if (opts->action != REVERT)
 		revs->reverse = 1;
 
-	if (setup_revisions(commit_argc, commit_argv, revs, NULL) > 1)
-		return error(_("usage: %s"), *revert_or_cherry_pick_usage());
+	if (setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL) > 1)
+		return error(_("usage: %s"), *revert_or_cherry_pick_usage(opts));
 
 	if (prepare_revision_walk(revs))
 		return error(_("revision walk setup failed"));
@@ -565,13 +583,12 @@ static int prepare_revs(struct rev_info *revs)
 	return 0;
 }
 
-static int read_and_refresh_cache(void)
+static int read_and_refresh_cache(struct replay_opts *opts)
 {
 	static struct lock_file index_lock;
 	int index_fd = hold_locked_index(&index_lock, 0);
-	const char *me;
+	const char *me = (opts->action == REVERT ? "revert" : "cherry-pick");
 
-	me = (action == REVERT ? "revert" : "cherry-pick");
 	if (read_index_preload(&the_index, NULL) < 0)
 		return error(_("%s: failed to read the index"), me);
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
@@ -586,7 +603,8 @@ static int read_and_refresh_cache(void)
 	return 0;
 }
 
-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;
@@ -594,27 +612,27 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	int res;
 
 	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 (no_replay)
+		if (opts->no_replay)
 			die(_("cherry-pick --ff cannot be used with -x"));
-		if (edit)
+		if (opts->edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
-	if ((res = read_and_refresh_cache()) ||
-		(res = prepare_revs(&revs)))
+	if ((res = read_and_refresh_cache(opts)) ||
+		(res = prepare_revs(&revs, opts)))
 		return res;
 
 	while ((commit = get_revision(&revs)) &&
-		!(res = do_pick_commit(commit)))
+		!(res = do_pick_commit(commit, opts)))
 		;
 
 	return res;
@@ -622,14 +640,20 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
+	struct replay_opts opts;
+
+	memset(&opts, 0, sizeof(struct replay_opts));
 	if (isatty(0))
-		edit = 1;
-	action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+		opts.edit = 1;
+	opts.action = REVERT;
+	return revert_or_cherry_pick(argc, argv, &opts);
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
-	action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv);
+	struct replay_opts opts;
+
+	memset(&opts, 0, sizeof(struct replay_opts));
+	opts.action = CHERRY_PICK;
+	return revert_or_cherry_pick(argc, argv, &opts);
 }
-- 
1.7.5.GIT

Re: [PATCH 3/8] revert: Introduce a struct to parse command-line options into

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

>  I get the following warning from GCC: warning: useless storage class
>  specifier in empty declaration (at the line where I've declared the
>  replay_opts struct).  What is the correct way to fix this?

Remove the useless storage class specifier ("static"). :)

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -35,29 +35,42 @@ static const char * const cherry_pick_usage[] = {
[...]
> +static struct replay_opts {
> +	enum { REVERT, CHERRY_PICK } action;
> +
> +	/* Boolean options */
> +	int edit;
> +	int no_replay;

replay but no replay?

I think originally git-revert.sh had a "replay" variable meaning "This
is not a revert (which undoes a commit) but a cherry-pick (which
re-does it)."  Later the purpose changed to "We are not cherry-picking
and referring to the original with cherry-pick -x but replaying a
commit and treating it as new".

Now with struct replay_opts you are proposing to make the term mean
"we are using git revert machinery, or in other words replaying the
change an old commit made (forwards or backwards)", which makes sense.
In this case there should probably be a patch right before which
renames no_replay to i_really_want_to_expose_my_private_commit_object_name
(um, I mean to record_origin or something similar).

> +	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;
> +};
[...]
>  
> -static const char * const *revert_or_cherry_pick_usage(void)
> +static const char *const *revert_or_cherry_pick_usage(struct replay_opts *opts)

Line is getting long.  Whitespace change snuck in?

I suppose if I ran the world the argument would be of type "enum
replay_action", so it would be used as

	usage(revert_or_cherry_pick_usage(o->action));

> +/* For option_parse_x */
> +static const char **xopts;
> +static size_t xopts_nr, xopts_alloc;
> +

Hm.  In C89, struct initializers are not allowed to include addresses
that are not known until run-time, and we used to follow that and now
violate it all over the place.  I'm not sure if it's worth it or not.
(I'm tempted to say, let it deteriorate further and people with the
ability to test on such platforms can fix it, but commits like
v1.7.2-rc0~32^2~18, Rewrite dynamic structure initializations to
runtime assignment, 2010-05-14, suggest that some people have cared in
the recent future.)

So.

If you want to use parse_options and support such compilers, it is
indeed simplest to use static variables.  You can give them scope
local to a particular function to at least avoid namespace polution.

To avoid such static variables at the expense of support for old
compilers, one can pass a pointer to a struct to option_parse_x
instead of the dummy &xopts.  Within option_parse_x, what you pass
will be accessible as opt->value.  It's all explained in
Documentation/technical/api-parse-options.txt, or one can grep around
for OPT_CALLBACK for examples.  A simple variant on this will work
with the old compilers, too.

[...]
>  static int option_parse_x(const struct option *opt,
> -			  const char *arg, int unset)
> +			const char *arg, int unset)

Whitespace change snuck in.

[...]
> @@ -67,19 +80,18 @@ static int option_parse_x(const struct option *opt,
>  	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('n', "no-commit", &(opts->no_commit), "don't automatically commit"),

The parentheses are not needed (and not idiomatic fwiw).  The line is
getting long so I'd suggest splitting it, though that's more a matter
of taste.

> @@ -87,23 +99,29 @@ static void parse_args(int argc, const char **argv)
[...]
> -	if (commit_argc < 2)
> +
> +	/* Fill in the opts struct from values set by option_parse_x */
> +	opts->xopts = xopts;
> +	opts->xopts_nr = xopts_nr;
> +	opts->xopts_alloc = xopts_alloc;

Yep, something like this is needed (for all the options) if we want to
follow C89's option-struct-initialization rules.

>  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)
> +			const char *base_label, const char *next_label,
> +			unsigned char *head, struct strbuf *msgbuf,
> +			struct replay_opts *opts)

I'm not going to point out whitespace changes that snuck in any more.

I think I prefer the options struct to go in front (as in the
merge-recursive and diff APIs), but this is only a matter of taste.

> @@ -311,15 +329,15 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)
>  }
>  
>  {
>  	struct merge_options o;
>  	struct tree *result, *next_tree, *base_tree, *head_tree;
>  	int clean, index_fd;
>  	const char **xopt;
>  	static struct lock_file index_lock;
> -	const char *me = (action == REVERT ? "revert" : "cherry-pick");

I think this belongs in a different patch (and likewise for its
counterpart below).

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

Stepping back, I think this is a good idea, to make the state being
passed around a little clearer and to make it easier for callers to
specify what they want to happen without making up fictitious argc and
argv.  Most of what remains for this to be cooked are minor things
(the biggest part is getting it to build with -std=c89 -pedantic if
wanted and teaching option_parse_x to use a callback parameter).

Thanks.

Re: [PATCH 3/8] revert: Introduce a struct to parse command-line options into

[Ramkumar Ramachandra]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> >  I get the following warning from GCC: warning: useless storage class
> >  specifier in empty declaration (at the line where I've declared the
> >  replay_opts struct).  What is the correct way to fix this?
> 
> Remove the useless storage class specifier ("static"). :)

Ah, thanks :)

> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -35,29 +35,42 @@ static const char * const cherry_pick_usage[] = {
> [...]
> > +static struct replay_opts {
> > +	enum { REVERT, CHERRY_PICK } action;
> > +
> > +	/* Boolean options */
> > +	int edit;
> > +	int no_replay;
> 
> replay but no replay?
> 
> I think originally git-revert.sh had a "replay" variable meaning "This
> is not a revert (which undoes a commit) but a cherry-pick (which
> re-does it)."  Later the purpose changed to "We are not cherry-picking
> and referring to the original with cherry-pick -x but replaying a
> commit and treating it as new".
> 
> Now with struct replay_opts you are proposing to make the term mean
> "we are using git revert machinery, or in other words replaying the
> change an old commit made (forwards or backwards)", which makes sense.
> In this case there should probably be a patch right before which
> renames no_replay to i_really_want_to_expose_my_private_commit_object_name
> (um, I mean to record_origin or something similar).

Great suggestion: one more patch changing "no_replay" to
"record_origin" it is.

> > +	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;
> > +};
> [...]
> >  
> > -static const char * const *revert_or_cherry_pick_usage(void)
> > +static const char *const *revert_or_cherry_pick_usage(struct replay_opts *opts)
> 
> Line is getting long.  Whitespace change snuck in?

In my defense, I thought whitespace (indentation, style) changes were
permitted as long as I'm making a functional change.  If this isn't
the case, when can I correct the style/ indentation?

> I suppose if I ran the world the argument would be of type "enum
> replay_action", so it would be used as
> 
> 	usage(revert_or_cherry_pick_usage(o->action));
> 
> > +/* For option_parse_x */
> > +static const char **xopts;
> > +static size_t xopts_nr, xopts_alloc;
> > +
> 
> Hm.  In C89, struct initializers are not allowed to include addresses
> that are not known until run-time, and we used to follow that and now
> violate it all over the place.  I'm not sure if it's worth it or not.
> (I'm tempted to say, let it deteriorate further and people with the
> ability to test on such platforms can fix it, but commits like
> v1.7.2-rc0~32^2~18, Rewrite dynamic structure initializations to
> runtime assignment, 2010-05-14, suggest that some people have cared in
> the recent future.)
> 
> So.
> 
> If you want to use parse_options and support such compilers, it is
> indeed simplest to use static variables.  You can give them scope
> local to a particular function to at least avoid namespace polution.
> 
> To avoid such static variables at the expense of support for old
> compilers, one can pass a pointer to a struct to option_parse_x
> instead of the dummy &xopts.  Within option_parse_x, what you pass
> will be accessible as opt->value.  It's all explained in
> Documentation/technical/api-parse-options.txt, or one can grep around
> for OPT_CALLBACK for examples.  A simple variant on this will work
> with the old compilers, too.

Got it.

> [...]
> >  static int option_parse_x(const struct option *opt,
> > -			  const char *arg, int unset)
> > +			const char *arg, int unset)
> 
> Whitespace change snuck in.

Intended.  It changes indentation style to linux-tabs-only, which is
the style my editor currently works with.

> [...]
> > @@ -67,19 +80,18 @@ static int option_parse_x(const struct option *opt,
> >  	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('n', "no-commit", &(opts->no_commit), "don't automatically commit"),
> 
> The parentheses are not needed (and not idiomatic fwiw).  The line is
> getting long so I'd suggest splitting it, though that's more a matter
> of taste.

Ok, I'll lose the paranthesis.

> > @@ -87,23 +99,29 @@ static void parse_args(int argc, const char **argv)
> [...]
> > -	if (commit_argc < 2)
> > +
> > +	/* Fill in the opts struct from values set by option_parse_x */
> > +	opts->xopts = xopts;
> > +	opts->xopts_nr = xopts_nr;
> > +	opts->xopts_alloc = xopts_alloc;
> 
> Yep, something like this is needed (for all the options) if we want to
> follow C89's option-struct-initialization rules.

Ouch! That's much too painful :|
I think I'll break the rule for the moment.

> >  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)
> > +			const char *base_label, const char *next_label,
> > +			unsigned char *head, struct strbuf *msgbuf,
> > +			struct replay_opts *opts)
> 
> I'm not going to point out whitespace changes that snuck in any more.
> 
> I think I prefer the options struct to go in front (as in the
> merge-recursive and diff APIs), but this is only a matter of taste.

Intended again, since I'm adding an argument to the list.

> > @@ -311,15 +329,15 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)
> >  }
> >  
> >  {
> >  	struct merge_options o;
> >  	struct tree *result, *next_tree, *base_tree, *head_tree;
> >  	int clean, index_fd;
> >  	const char **xopt;
> >  	static struct lock_file index_lock;
> > -	const char *me = (action == REVERT ? "revert" : "cherry-pick");
> 
> I think this belongs in a different patch (and likewise for its
> counterpart below).
> 
> > 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.
> 
> Stepping back, I think this is a good idea, to make the state being
> passed around a little clearer and to make it easier for callers to
> specify what they want to happen without making up fictitious argc and
> argv.  Most of what remains for this to be cooked are minor things
> (the biggest part is getting it to build with -std=c89 -pedantic if
> wanted and teaching option_parse_x to use a callback parameter).

Right, thanks.

-- Ram

Re: [PATCH 3/8] revert: Introduce a struct to parse command-line options into

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> In my defense, I thought whitespace (indentation, style) changes were
> permitted as long as I'm making a functional change.  If this isn't
> the case, when can I correct the style/ indentation?

What I generally try to do is to only correct style and indentation
when it is making my life difficult (either in reading or writing the
code), through a separate commit that explains the improvement.  That
way, a person reading the diff for a functional change doesn't have to
be distracted by irrelevant changes.

Based on the advice he gives from time to time, Junio's policy seems
to be that trivial cosmetic cleanups should either be very compelling
or go at the start of a series that makes other changes to the same
section of code.

Re: [PATCH 3/8] revert: Introduce a struct to parse command-line options into

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > In my defense, I thought whitespace (indentation, style) changes were
> > permitted as long as I'm making a functional change.  If this isn't
> > the case, when can I correct the style/ indentation?
> 
> What I generally try to do is to only correct style and indentation
> when it is making my life difficult (either in reading or writing the
> code), through a separate commit that explains the improvement.  That
> way, a person reading the diff for a functional change doesn't have to
> be distracted by irrelevant changes.
> 
> Based on the advice he gives from time to time, Junio's policy seems
> to be that trivial cosmetic cleanups should either be very compelling
> or go at the start of a series that makes other changes to the same
> section of code.

Cool.  Start of the series then.

Thanks.

-- Ram

[PATCH 4/8] revert: Separate cmdline argument handling from the functional code

[Ramkumar Ramachandra]

Reading the Git configuration, setting environment variables, parsing
command-line arguments, and populating the options structure should be
done in cmd_cherry_pick/ cmd_revert.  The job pick_commits of
simplified into setting up the revision walker and calling
do_pick_commit in a loop- later in the series, it will handle
failures, and serve as the starting point for continuation.

Based-on-patch-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 This patch is fairly straightforward.

 builtin/revert.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 8550927..288c898 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 	return 0;
 }
 
-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;
-	const char *me;
 	int res;
 
-	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"));
@@ -642,18 +635,27 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
 
+	git_config(git_default_config, NULL);
 	memset(&opts, 0, sizeof(struct replay_opts));
 	if (isatty(0))
 		opts.edit = 1;
+
 	opts.action = REVERT;
-	return revert_or_cherry_pick(argc, argv, &opts);
+	setenv(GIT_REFLOG_ACTION, "revert", 0);
+	parse_args(argc, argv, &opts);
+
+	return pick_commits(&opts);
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
 
+	git_config(git_default_config, NULL);
 	memset(&opts, 0, sizeof(struct replay_opts));
 	opts.action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv, &opts);
+	setenv(GIT_REFLOG_ACTION, "cherry-pick", 0);
+	parse_args(argc, argv, &opts);
+
+	return pick_commits(&opts);
 }
-- 
1.7.5.GIT

Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Reading the Git configuration, setting environment variables, parsing
> command-line arguments, and populating the options structure should be
> done in cmd_cherry_pick/ cmd_revert.

Yes, but why? :)

> The job pick_commits of
> simplified into setting up the revision walker and calling
> do_pick_commit in a loop- later in the series, it will handle
> failures, and serve as the starting point for continuation.

ENOPARSE.  I assume the idea is that callers will want to decide what
they want pick_commits to do and specify it by filling a struct
instead of argc and argv.  Is that it?

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>  	return 0;
>  }
>  
> -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;
> -	const char *me;
>  	int res;
>  
> -	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) {

I don't see why the caller sets up GIT_REFLOG_ACTION, since the caller
is not making the commits.  Is there an example where it would use
something other than "cherry-pick" or "revert"?

Aside from clarifying that detail, this one looks good.

Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > Reading the Git configuration, setting environment variables, parsing
> > command-line arguments, and populating the options structure should be
> > done in cmd_cherry_pick/ cmd_revert.
> 
> Yes, but why? :)

Haven't I explained this sufficiently well in the next sentence?

> > The job pick_commits of
> > simplified into setting up the revision walker and calling
> > do_pick_commit in a loop- later in the series, it will handle
> > failures, and serve as the starting point for continuation.
> 
> ENOPARSE.  I assume the idea is that callers will want to decide what
> they want pick_commits to do and specify it by filling a struct
> instead of argc and argv.  Is that it?

Sorry about the ENOPARSE.  Yes, exactly.

> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> >  	return 0;
> >  }
> >  
> > -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;
> > -	const char *me;
> >  	int res;
> >  
> > -	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) {
> 
> I don't see why the caller sets up GIT_REFLOG_ACTION, since the caller
> is not making the commits.  Is there an example where it would use
> something other than "cherry-pick" or "revert"?

Nice catch! Yes, GIT_REFLOG_ACTION should be in pick_commits.

> Aside from clarifying that detail, this one looks good.

Thanks for the review.  This patch is probably the one with the least
number of mistakes :)

-- Ram

Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code

[Ramkumar Ramachandra]

Hi again,

Ramkumar Ramachandra writes:
> Jonathan Nieder writes:
> > Ramkumar Ramachandra wrote:
> > > --- a/builtin/revert.c
> > > +++ b/builtin/revert.c
> > > @@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> > >  	return 0;
> > >  }
> > >  
> > > -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;
> > > -	const char *me;
> > >  	int res;
> > >  
> > > -	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) {
> > 
> > I don't see why the caller sets up GIT_REFLOG_ACTION, since the caller
> > is not making the commits.  Is there an example where it would use
> > something other than "cherry-pick" or "revert"?
> 
> Nice catch! Yes, GIT_REFLOG_ACTION should be in pick_commits.

Er, I mean in do_pick_commit.  Right?

-- Ram

Re: [PATCH 4/8] revert: Separate cmdline argument handling from the functional code

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Ramkumar Ramachandra writes:
>>> Ramkumar Ramachandra wrote:

>>>> +++ b/builtin/revert.c
>>>> @@ -603,19 +603,12 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> -static int revert_or_cherry_pick(int argc, const char **argv,
>>>> -				struct replay_opts *opts)
>>>> +static int pick_commits(struct replay_opts *opts)
>>>>  {
[...]
>>>> -	setenv(GIT_REFLOG_ACTION, me, 0);
>>>> -	parse_args(argc, argv, opts);
>>>> -
>>>>  	if (opts->allow_ff) {
[...]
>> Nice catch! Yes, GIT_REFLOG_ACTION should be in pick_commits.
>
> Er, I mean in do_pick_commit.  Right?

It seems somehow cleaner to set the envvar once in pick_commits,
assuming do_pick_commit is a private function that won't be exported.
But either way sounds fine to me.

[PATCH 5/8] revert: Catch incompatible command-line options early

[Ramkumar Ramachandra]

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>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 I think we _should_ die when an error in command-line parsing occurs,
 since it should always be the toplevel caller.  Thanks to Junio for
 redesigning die_opt_incompatible in a sane manner.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 288c898..0fe87e8 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -80,10 +80,29 @@ static int option_parse_x(const struct option *opt,
 	return 0;
 }
 
+static void die_opt_incompatible(const char *me, const char *base_opt, ...)
+{
+	const char *this_opt;
+	int this_opt_set;
+	va_list ap;
+
+	va_start(ap, base_opt);
+	while (1) {
+		if (!(this_opt = va_arg(ap, const char *)))
+			break;
+		if ((this_opt_set = va_arg(ap, int)))
+			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);
+	const char *me = (opts->action == REVERT ? "revert" : "cherry-pick");
 	int noop;
+
 	struct option options[] = {
 		OPT_BOOLEAN('n', "no-commit", &(opts->no_commit), "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &(opts->edit), "edit the commit message"),
@@ -121,6 +140,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)
+		die_opt_incompatible(me, "--ff",
+				"--signoff", opts->signoff,
+				"--no-commit", opts->no_commit,
+				"-x", opts->no_replay,
+				"--edit", opts->edit,
+				NULL);
 	opts->commit_argv = argv;
 }
 
@@ -609,17 +635,6 @@ static int pick_commits(struct replay_opts *opts)
 	struct commit *commit;
 	int res;
 
-	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->no_replay)
-			die(_("cherry-pick --ff cannot be used with -x"));
-		if (opts->edit)
-			die(_("cherry-pick --ff cannot be used with --edit"));
-	}
-
 	if ((res = read_and_refresh_cache(opts)) ||
 		(res = prepare_revs(&revs, opts)))
 		return res;
-- 
1.7.5.GIT

Re: [PATCH 5/8] revert: Catch incompatible command-line options early

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

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

Looking at the patch, this seems like a bugfix (error messages
currently say "cherry-pick: " when they should sometimes say
"revert: ") and cleanup (dealing with options incompatible with "--ff"
in a loop instead of one by one) in addition to the "check and die
early" improvement you explain above.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -80,10 +80,29 @@ static int option_parse_x(const struct option *opt,
>  	return 0;
>  }
>  
> +static void die_opt_incompatible(const char *me, const char *base_opt, ...)
> +{
> +	const char *this_opt;
> +	int this_opt_set;
> +	va_list ap;
> +
> +	va_start(ap, base_opt);
> +	while (1) {
> +		if (!(this_opt = va_arg(ap, const char *)))
> +			break;
> +		if ((this_opt_set = va_arg(ap, int)))
> +			die(_("%s: %s cannot be used with %s"),
> +				me, this_opt, base_opt);
> +	}
> +	va_end(ap);
> +}

Wait a second --- this doesn't always die!  Why is it called
die_opt_incompatible rather than verify_opt_compatible_or_die or
something?

I think I would have written the loop something like

	va_start(ap, opt1);
	while ((opt2 = va_arg(ap, const char *))) {
		int set = va_arg(ap, int);
		if (set)
			die(opt1 cannot be used with opt2);
	}
	va_end(ap);

Thanks.  The refactoring into a loop is nice.

Re: [PATCH 5/8] revert: Catch incompatible command-line options early

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > 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.
> 
> Looking at the patch, this seems like a bugfix (error messages
> currently say "cherry-pick: " when they should sometimes say
> "revert: ") and cleanup (dealing with options incompatible with "--ff"
> in a loop instead of one by one) in addition to the "check and die
> early" improvement you explain above.

Ok, I'll reword.

> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -80,10 +80,29 @@ static int option_parse_x(const struct option *opt,
> >  	return 0;
> >  }
> >  
> > +static void die_opt_incompatible(const char *me, const char *base_opt, ...)
> > +{
> > +	const char *this_opt;
> > +	int this_opt_set;
> > +	va_list ap;
> > +
> > +	va_start(ap, base_opt);
> > +	while (1) {
> > +		if (!(this_opt = va_arg(ap, const char *)))
> > +			break;
> > +		if ((this_opt_set = va_arg(ap, int)))
> > +			die(_("%s: %s cannot be used with %s"),
> > +				me, this_opt, base_opt);
> > +	}
> > +	va_end(ap);
> > +}
> 
> Wait a second --- this doesn't always die!  Why is it called
> die_opt_incompatible rather than verify_opt_compatible_or_die or
> something?
> 
> I think I would have written the loop something like
> 
> 	va_start(ap, opt1);
> 	while ((opt2 = va_arg(ap, const char *))) {
> 		int set = va_arg(ap, int);
> 		if (set)
> 			die(opt1 cannot be used with opt2);
> 	}
> 	va_end(ap);
> 
> Thanks.  The refactoring into a loop is nice.

Thanks.  Looks like this patch is mostly right too :)

-- Ram

[PATCH 6/8] revert: Introduce head, todo, done files to persist state

[Ramkumar Ramachandra]

A cherry-pick/ revert operation consists of several smaller steps.
Later in the series, we would like to be able to resume a failed
operation.  As a prelude, we first need to persist the current state
of operation.  Introduce a "head" file to make note of the HEAD when
the operation stated (so that the operation can be aborted), a "todo"
file to keep the list of the steps to be performed, and a "done" file
to keep a list of steps that have completed successfully.  The format
of these files is similar to the one used by the "rebase -i" process.

Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Note how I've used the words "operation" and "step" to differentiate
 between a picking/ reverting a single commit versus the entire
 operation in the commit message.  In one discussion involving
 Jonathan and Daniel, it was discussed that multiple meta-picking/
 reverting levels is a goal that is tangential to this project.  I
 only intend to support two levels: the overall "operation", and the
 composite "steps".

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 0fe87e8..13569c2 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,13 @@
  * 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")
+#define DONE_FILE	git_path(SEQ_DIR "/done")
+
 static const char * const revert_usage[] = {
 	"git revert [options] <commit-ish>",
 	NULL
@@ -629,21 +637,118 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 	return 0;
 }
 
+static int 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 = NULL;
+	const char *action;
+
+	action = (opts->action == REVERT ? "revert" : "pick");
+	for (cur = list; cur; cur = cur->next) {
+		sha1 = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
+		if (get_message(cur->item, cur->item->buffer, &msg))
+			return error(_("Cannot get commit message for %s"), sha1);
+		strbuf_addf(buf, "%s %s %s\n", action, sha1, msg.subject);
+	}
+	return 0;
+}
+
+static int persist_initialize(unsigned char *head)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777)) {
+		int err = errno;
+		strbuf_release(&buf);
+		error(_("Could not create sequencer directory '%s': %s"),
+			SEQ_PATH, strerror(err));
+		return -err;
+	}
+
+	if ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {
+		int err = errno;
+		strbuf_release(&buf);
+		error(_("Could not open '%s' for writing: %s"),
+			HEAD_FILE, strerror(err));
+		return -err;
+	}
+
+	strbuf_addf(&buf, "%s", find_unique_abbrev(head, DEFAULT_ABBREV));
+	write_or_whine(fd, buf.buf, buf.len, HEAD_FILE);
+	close(fd);
+	strbuf_release(&buf);
+	return 0;
+}
+
+static int persist_todo_done(int res, struct commit_list *todo_list,
+			struct commit_list *done_list, struct replay_opts *opts)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int fd, res2;
+
+	if (!res)
+		return 0;
+
+	/* TODO file */
+	if ((fd = open(TODO_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {
+		int err = errno;
+		strbuf_release(&buf);
+		error(_("Could not open '%s' for writing: %s"),
+			TODO_FILE, strerror(err));
+		return -err;
+	}
+
+	if ((res2 = format_todo(&buf, todo_list, opts)))
+		return res2;
+	write_or_whine(fd, buf.buf, buf.len, TODO_FILE);
+	close(fd);
+
+	/* DONE file */
+	strbuf_reset(&buf);
+	if ((fd = open(DONE_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {
+		int err = errno;
+		strbuf_release(&buf);
+		error(_("Could not open '%s' for writing: %s"),
+			DONE_FILE, strerror(err));
+		return -err;
+	}
+
+	if ((res2 = format_todo(&buf, done_list, opts)))
+		return res2;
+	write_or_whine(fd, buf.buf, buf.len, DONE_FILE);
+	close(fd);
+	strbuf_release(&buf);
+	return res;
+}
+
 static int pick_commits(struct replay_opts *opts)
 {
+	struct commit_list *done_list = NULL;
 	struct rev_info revs;
 	struct commit *commit;
+	unsigned char head[20];
 	int res;
 
+	if (get_sha1("HEAD", head))
+		return error(_("You do not have a valid HEAD"));
+
 	if ((res = read_and_refresh_cache(opts)) ||
-		(res = prepare_revs(&revs, opts)))
+		(res = prepare_revs(&revs, opts)) ||
+		(res = persist_initialize(head)))
 		return res;
 
-	while ((commit = get_revision(&revs)) &&
-		!(res = do_pick_commit(commit, opts)))
-		;
-
-	return res;
+	while ((commit = get_revision(&revs))) {
+		if (!(res = do_pick_commit(commit, opts)))
+			commit_list_insert(commit, &done_list);
+		else {
+			commit_list_insert(commit, &revs.commits);
+			break;
+		}
+	}
+	return persist_todo_done(res, revs.commits, done_list, opts);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
-- 
1.7.5.GIT

Re: [PATCH 6/8] revert: Introduce head, todo, done files to persist state

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> A cherry-pick/ revert operation consists of several smaller steps.
> Later in the series, we would like to be able to resume a failed
> operation.

When introducing jargon, it is hard to make the intent perfectly
clear.  I suppose what this means is:

 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 make note of the HEAD when
> the operation stated (so that the operation can be aborted), a "todo"
> file to keep the list of the steps to be performed, and a "done" file
> to keep a list of steps that have completed successfully.  The format
> of these files is similar to the one used by the "rebase -i" process.

s/stated/started/ :)  Makes some sense, aside from that.

It would be more conventional to use all-caps symref-like names, like
MULTIPLE_CHERRY_PICK_ORIG_HEAD, CHERRY_PICK_TODO, and
CHERRY_PICK_DONE, or to put these files in a subdirectory (oh, they're
already in a subdirectory?  Why didn't you mention that? :)).

By the way, what is .git/sequencer/done used for?

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -25,6 +26,13 @@
>   * 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")
> +#define DONE_FILE	git_path(SEQ_DIR "/done")

These seeming constants that call a function are kind of scary.

> @@ -629,21 +637,118 @@ static int read_and_refresh_cache(struct replay_opts *opts)
>  	return 0;
>  }
>  
> +static int 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 = NULL;
> +	const char *action;
> +
> +	action = (opts->action == REVERT ? "revert" : "pick");
> +	for (cur = list; cur; cur = cur->next) {
> +		sha1 = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
> +		if (get_message(cur->item, cur->item->buffer, &msg))
> +			return error(_("Cannot get commit message for %s"), sha1);
> +		strbuf_addf(buf, "%s %s %s\n", action, sha1, msg.subject);

Is this internal state or for the user?  If it is internal state, I'd
naïvely have expected a sequence of 40-character hexadecimal lines,
perhaps with human-readable names like "topic~3" for the sake of
error messages if git knows about them.

> +static int persist_initialize(unsigned char *head)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int fd;
> +
> +	if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777)) {

What if .git/sequencer exists and is a file?  How does this interact
with "[core] sharedrepository" configuration?  What happens if
.git/sequencer contains some stale files --- if the power fails while
git is writing new files in .git/sequencer/, will the state be
confusing?

> +		int err = errno;
> +		strbuf_release(&buf);
> +		error(_("Could not create sequencer directory '%s': %s"),
> +			SEQ_PATH, strerror(err));
> +		return -err;

Why does the caller care about which errno, and what is it going to
do with that information?

> +	}
> +
> +	if ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {

More idiomatic in the git codebase to write:

	fd = open(...);
	if (fd < 0) {

> +		int err = errno;
> +		strbuf_release(&buf);
> +		error(_("Could not open '%s' for writing: %s"),
> +			HEAD_FILE, strerror(err));
> +		return -err;

As above.  Why does the caller care about errno?  If backing out after
an error, I suppose it might make sense to rmdir .git/sequencer while
at it.

> +	}
> +
> +	strbuf_addf(&buf, "%s", find_unique_abbrev(head, DEFAULT_ABBREV));

Why abbreviate?

> +	write_or_whine(fd, buf.buf, buf.len, HEAD_FILE);

What happens and should happen on error?

[...]
> +static int persist_todo_done(int res, struct commit_list *todo_list,
> +			struct commit_list *done_list, struct replay_opts *opts)

This is about recording what has been done and what remains to
be done?  What does the res argument represent?

> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int fd, res2;
> +
> +	if (!res)
> +		return 0;
> +
> +	/* TODO file */
> +	if ((fd = open(TODO_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {

What happens if we are interrupted in the middle of writing this?

> +		int err = errno;
> +		strbuf_release(&buf);
> +		error(_("Could not open '%s' for writing: %s"),
> +			TODO_FILE, strerror(err));
> +		return -err;

I don't think the caller should care which errno. :)

[...]
>  static int pick_commits(struct replay_opts *opts)
>  {
> +	struct commit_list *done_list = NULL;
>  	struct rev_info revs;
>  	struct commit *commit;
> +	unsigned char head[20];
>  	int res;
>  
> +	if (get_sha1("HEAD", head))
> +		return error(_("You do not have a valid HEAD"));

What should happen if I try to cherry-pick onto an unborn branch?  I
haven't checked what happens.

> +
>  	if ((res = read_and_refresh_cache(opts)) ||
> -		(res = prepare_revs(&revs, opts)))
> +		(res = prepare_revs(&revs, opts)) ||
> +		(res = persist_initialize(head)))
>  		return res;
>  
> -	while ((commit = get_revision(&revs)) &&
> -		!(res = do_pick_commit(commit, opts)))
> -		;
> -
> -	return res;
> +	while ((commit = get_revision(&revs))) {
> +		if (!(res = do_pick_commit(commit, opts)))
> +			commit_list_insert(commit, &done_list);

This puts done_list in the reverse order that the commits were
cherry-picked.  Is that the intent?

> +		else {
> +			commit_list_insert(commit, &revs.commits);
> +			break;
> +		}
> +	}
> +	return persist_todo_done(res, revs.commits, done_list, opts);

A few potential trade-offs:

 - should cherry-pick record the state after every commit?  This would
   be safe against stray die() calls or segfaults but requires hitting
   the filesystem which might not be wanted if doing a run of
   cherry-picks in memory (though git is far from supporting such a
   "many cherry picks in core followed by checkout and packed
   collection of objects written to disk all at once" optimization
   anyway).

 - should we use O_TRUNC or O_APPEND to modify the state in-place or
   use separate files and rename them into place?  The latter is
   safer against sudden exit.

 - should we (perhaps optionally) fsync the state when commiting to it?
   I think no, but someone performing a rebase and running a test suite
   with the potential to crash the system between commits might appreciate
   the effort.

Re: [PATCH 6/8] revert: Introduce head, todo, done files to persist state

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > A cherry-pick/ revert operation consists of several smaller steps.
> > Later in the series, we would like to be able to resume a failed
> > operation.
> 
> When introducing jargon, it is hard to make the intent perfectly
> clear.  I suppose what this means is:
> 
>  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.

Thanks for digging that up for me.  I'll try to reword it in the next
iteration without introducing more jargon, if that's preferred.

> > Introduce a "head" file to make note of the HEAD when
> > the operation stated (so that the operation can be aborted), a "todo"
> > file to keep the list of the steps to be performed, and a "done" file
> > to keep a list of steps that have completed successfully.  The format
> > of these files is similar to the one used by the "rebase -i" process.
> 
> s/stated/started/ :)  Makes some sense, aside from that.
> 
> It would be more conventional to use all-caps symref-like names, like
> MULTIPLE_CHERRY_PICK_ORIG_HEAD, CHERRY_PICK_TODO, and
> CHERRY_PICK_DONE, or to put these files in a subdirectory (oh, they're
> already in a subdirectory?  Why didn't you mention that? :)).

I'll mention that in the commit message next time.

> By the way, what is .git/sequencer/done used for?

I don't know :p
I just added it because "rebase -i" uses it too, although I can't find
a definite usecase for it.

> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -25,6 +26,13 @@
> >   * 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")
> > +#define DONE_FILE	git_path(SEQ_DIR "/done")
> 
> These seeming constants that call a function are kind of scary.

Uh, you'd prefer seeing it literally spelt out over and over again?

> > @@ -629,21 +637,118 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> >  	return 0;
> >  }
> >  
> > +static int 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 = NULL;
> > +	const char *action;
> > +
> > +	action = (opts->action == REVERT ? "revert" : "pick");
> > +	for (cur = list; cur; cur = cur->next) {
> > +		sha1 = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
> > +		if (get_message(cur->item, cur->item->buffer, &msg))
> > +			return error(_("Cannot get commit message for %s"), sha1);
> > +		strbuf_addf(buf, "%s %s %s\n", action, sha1, msg.subject);
> 
> Is this internal state or for the user?  If it is internal state, I'd
> naïvely have expected a sequence of 40-character hexadecimal lines,
> perhaps with human-readable names like "topic~3" for the sake of
> error messages if git knows about them.

For the user.  This is the instruction sheet that the user will be
able to edit at a later stage, when we develop something like
"sequencer -i".

> > +static int persist_initialize(unsigned char *head)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +	int fd;
> > +
> > +	if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777)) {
> 
> What if .git/sequencer exists and is a file?  How does this interact
> with "[core] sharedrepository" configuration?  What happens if
> .git/sequencer contains some stale files --- if the power fails while
> git is writing new files in .git/sequencer/, will the state be
> confusing?

Ah, thanks for pointing these out -- I hadn't thought about them earlier.

> > +		int err = errno;
> > +		strbuf_release(&buf);
> > +		error(_("Could not create sequencer directory '%s': %s"),
> > +			SEQ_PATH, strerror(err));
> > +		return -err;
> 
> Why does the caller care about which errno, and what is it going to
> do with that information?

Hm, error_errno seems that consistently returns -1 seems like a good
idea now.  I'll get it back into the series next time.

> > +	}
> > +
> > +	if ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {
> 
> More idiomatic in the git codebase to write:
> 
> 	fd = open(...);
> 	if (fd < 0) {

Ok.

> > +		int err = errno;
> > +		strbuf_release(&buf);
> > +		error(_("Could not open '%s' for writing: %s"),
> > +			HEAD_FILE, strerror(err));
> > +		return -err;
> 
> As above.  Why does the caller care about errno?  If backing out after
> an error, I suppose it might make sense to rmdir .git/sequencer while
> at it.

Ok.

> > +	}
> > +
> > +	strbuf_addf(&buf, "%s", find_unique_abbrev(head, DEFAULT_ABBREV));
> 
> Why abbreviate?
> 
> > +	write_or_whine(fd, buf.buf, buf.len, HEAD_FILE);
> 
> What happens and should happen on error?



> [...]
> > +static int persist_todo_done(int res, struct commit_list *todo_list,
> > +			struct commit_list *done_list, struct replay_opts *opts)
> 
> This is about recording what has been done and what remains to
> be done?  What does the res argument represent?

Exit status? I have to think back to figure out why I chose to pass it
around like this.

> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +	int fd, res2;
> > +
> > +	if (!res)
> > +		return 0;
> > +
> > +	/* TODO file */
> > +	if ((fd = open(TODO_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {
> 
> What happens if we are interrupted in the middle of writing this?

I'll use the lockfile API if I have time before the next iteration.

> > +		int err = errno;
> > +		strbuf_release(&buf);
> > +		error(_("Could not open '%s' for writing: %s"),
> > +			TODO_FILE, strerror(err));
> > +		return -err;
> 
> I don't think the caller should care which errno. :)

Right :)

> [...]
> >  static int pick_commits(struct replay_opts *opts)
> >  {
> > +	struct commit_list *done_list = NULL;
> >  	struct rev_info revs;
> >  	struct commit *commit;
> > +	unsigned char head[20];
> >  	int res;
> >  
> > +	if (get_sha1("HEAD", head))
> > +		return error(_("You do not have a valid HEAD"));
> 
> What should happen if I try to cherry-pick onto an unborn branch?  I
> haven't checked what happens.

fatal: You do not have a valid HEAD
I just tried it :)

> > +
> >  	if ((res = read_and_refresh_cache(opts)) ||
> > -		(res = prepare_revs(&revs, opts)))
> > +		(res = prepare_revs(&revs, opts)) ||
> > +		(res = persist_initialize(head)))
> >  		return res;
> >  
> > -	while ((commit = get_revision(&revs)) &&
> > -		!(res = do_pick_commit(commit, opts)))
> > -		;
> > -
> > -	return res;
> > +	while ((commit = get_revision(&revs))) {
> > +		if (!(res = do_pick_commit(commit, opts)))
> > +			commit_list_insert(commit, &done_list);
> 
> This puts done_list in the reverse order that the commits were
> cherry-picked.  Is that the intent?

Yes, although I'm not sure what to do with the done file now (you
pointed that out earlier).

> > +		else {
> > +			commit_list_insert(commit, &revs.commits);
> > +			break;
> > +		}
> > +	}
> > +	return persist_todo_done(res, revs.commits, done_list, opts);
> 
> A few potential trade-offs:
> 
>  - should cherry-pick record the state after every commit?  This would
>    be safe against stray die() calls or segfaults but requires hitting
>    the filesystem which might not be wanted if doing a run of
>    cherry-picks in memory (though git is far from supporting such a
>    "many cherry picks in core followed by checkout and packed
>    collection of objects written to disk all at once" optimization
>    anyway).

Agreed, but I don't think this kind of optimization will be in the
scope of this series.  It's nice to think about though.

>  - should we use O_TRUNC or O_APPEND to modify the state in-place or
>    use separate files and rename them into place?  The latter is
>    safer against sudden exit.

The latter, definitely.  I don't want to have to deal with malformed
instruction sheets.

>  - should we (perhaps optionally) fsync the state when commiting to it?
>    I think no, but someone performing a rebase and running a test suite
>    with the potential to crash the system between commits might appreciate
>    the effort.

Yes, yes! I'd love this feature.

Thanks.

-- Ram

[PATCH 7/8] revert: Implement parsing --continue, --abort and --skip

[Ramkumar Ramachandra]

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.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 This patch is fairly straightforward.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 13569c2..ccfc295 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -46,6 +46,11 @@ static const char * const cherry_pick_usage[] = {
 static 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 no_replay;
@@ -112,6 +117,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *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"),
 		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
@@ -145,7 +153,47 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	opts->xopts_nr = xopts_nr;
 	opts->xopts_alloc = xopts_alloc;
 
-	if (opts->commit_argc < 2)
+	/* 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";
+			die_opt_incompatible(me, this_oper,
+					"--skip", opts->skip_oper,
+					NULL);
+			die_opt_incompatible(me, this_oper,
+					"--continue", opts->continue_oper,
+					NULL);
+		} else if (opts->skip_oper) {
+			this_oper = "--skip";
+			die_opt_incompatible(me, this_oper,
+					"--abort", opts->abort_oper,
+					NULL);
+			die_opt_incompatible(me, this_oper,
+					"--continue", opts->continue_oper,
+					NULL);
+		} else {
+			this_oper = "--continue";
+			die_opt_incompatible(me, this_oper,
+					"--abort", opts->abort_oper,
+					NULL);
+			die_opt_incompatible(me, this_oper,
+					"--skip", opts->skip_oper,
+					NULL);
+		}
+		die_opt_incompatible(me, this_oper,
+				"--no-commit", opts->no_commit,
+				"--edit", opts->edit, "-r", noop,
+				"--signoff", opts->signoff,
+				"--mainline", opts->mainline,
+				"--strategy", opts->strategy ? 1 : 0,
+				"--strategy-option", opts->xopts ? 1 : 0,
+				"-x", opts->no_replay,
+				"--ff", opts->allow_ff,
+				NULL);
+	}
+
+	else if (opts->commit_argc < 2)
 		usage_with_options(usage_str, options);
 
 	if (opts->allow_ff)
-- 
1.7.5.GIT

Re: [PATCH 7/8] revert: Implement parsing --continue, --abort and --skip

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

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

I'd suggest squashing this patch with the next one.  If a "git
cherry-pick" accepting an --abort option that does not do anything
leaked into the wild, that would not be a good outcome.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -145,7 +153,47 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  	opts->xopts_nr = xopts_nr;
>  	opts->xopts_alloc = xopts_alloc;
>  
> -	if (opts->commit_argc < 2)
> +	/* 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";
> +			die_opt_incompatible(me, this_oper,
> +					"--skip", opts->skip_oper,
> +					NULL);
> +			die_opt_incompatible(me, this_oper,
> +					"--continue", opts->continue_oper,
> +					NULL);

What happened to

			...(me, "--abort",
				"--skip", opts->skip,
				"--continue", opts->continue);

?  I also wonder if there should not be a function to deal with
mutually incompatible options:

	va_start(ap, commandname);
	while ((arg1 = va_arg(ap, const char *))) {
		int set = va_arg(ap, int);
		if (set)
			break;
	}
	while ((arg2 = va_arg(ap, const char *))) {
		int set = va_arg(ap, int);
		if (set)
			die(arg1 and arg2 are incompatible);
	}
	va_end(ap);

> +		die_opt_incompatible(me, this_oper,
> +				"--no-commit", opts->no_commit,
[...]

Seems reasonable.  A part of me would want to accept such options and
only error out if the saved state indicates that they are different
from the options supplied before, so if a person has

	alias applycommits = git cherry-pick --no-commit

then "applycommits --continue" could work without trouble, but
that's probably overegineering.

Re: [PATCH 7/8] revert: Implement parsing --continue, --abort and --skip

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > 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.
> 
> I'd suggest squashing this patch with the next one.  If a "git
> cherry-pick" accepting an --abort option that does not do anything
> leaked into the wild, that would not be a good outcome.

What about --continue and --skip? They're no-ops too here, and
there'll soon be patches adding the functionality.  Do you think it's
alright to parse and exit immediately?

> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -145,7 +153,47 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
> >  	opts->xopts_nr = xopts_nr;
> >  	opts->xopts_alloc = xopts_alloc;
> >  
> > -	if (opts->commit_argc < 2)
> > +	/* 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";
> > +			die_opt_incompatible(me, this_oper,
> > +					"--skip", opts->skip_oper,
> > +					NULL);
> > +			die_opt_incompatible(me, this_oper,
> > +					"--continue", opts->continue_oper,
> > +					NULL);
> 
> What happened to
> 
> 			...(me, "--abort",
> 				"--skip", opts->skip,
> 				"--continue", opts->continue);

Huh? Why? I've caught every possible combination of two of those
options -- that already covers all three.

> ?  I also wonder if there should not be a function to deal with
> mutually incompatible options:
> 
> 	va_start(ap, commandname);
> 	while ((arg1 = va_arg(ap, const char *))) {
> 		int set = va_arg(ap, int);
> 		if (set)
> 			break;
> 	}
> 	while ((arg2 = va_arg(ap, const char *))) {
> 		int set = va_arg(ap, int);
> 		if (set)
> 			die(arg1 and arg2 are incompatible);
> 	}
> 	va_end(ap);

I personally think having a function is cleaner: I even like the new
API suggested by Junio.  We can probably even move it to a common
place, and have others use it as well.

> > +		die_opt_incompatible(me, this_oper,
> > +				"--no-commit", opts->no_commit,
> [...]
> 
> Seems reasonable.  A part of me would want to accept such options and
> only error out if the saved state indicates that they are different
> from the options supplied before, so if a person has
> 
> 	alias applycommits = git cherry-pick --no-commit
> 
> then "applycommits --continue" could work without trouble, but
> that's probably overegineering.

Over-engineering definitely! I'm looking to get something working
first; add-on functionality like this can come as later patches.

And yes, as you pointed out in another review, the name
verify_opt_incompatible_or_die is more appropriate.

Thanks for the detailed review.

-- Ram

Re: [PATCH 7/8] revert: Implement parsing --continue, --abort and --skip

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> What about --continue and --skip? They're no-ops too here, and
> there'll soon be patches adding the functionality.  Do you think it's
> alright to parse and exit immediately?

You're right: the same considerations apply to them.  If adding these
options before the functionality is ready makes the series easier to
read, then I'd at least prefer to see

	if (opts->abort_oper)
		die("--abort is not implemented yet");

to prevent scripts and humans from being confused.  And on the other
hand I suspect adding each option at the same time as adding the
corresponding functionality would be clearer anyway.

> Jonathan Nieder writes:
>> Ramkumar Ramachandra wrote:

>>> --- a/builtin/revert.c
>>> +++ b/builtin/revert.c
>>> @@ -145,7 +153,47 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
[...]
>>> +			die_opt_incompatible(me, this_oper,
>>> +					"--skip", opts->skip_oper,
>>> +					NULL);
>>> +			die_opt_incompatible(me, this_oper,
>>> +					"--continue", opts->continue_oper,
>>> +					NULL);
>>
>> What happened to
>> 
>> 			...(me, "--abort",
>> 				"--skip", opts->skip,
>> 				"--continue", opts->continue);
>
> Huh? Why? I've caught every possible combination of two of those
> options -- that already covers all three.

Sorry, that was unclear of me.  What I meant to say is that one
function call instead of two would suffice, like the API is
supposed to make possible.

In other words, nothing actually wrong here, just a possibility
of simplification.

>> ?  I also wonder if there should not be a function to deal with
>> mutually incompatible options:
>>
>> 	va_start(ap, commandname);
>> 	while ((arg1 = va_arg(ap, const char *))) {
>> 		int set = va_arg(ap, int);
>> 		if (set)
>> 			break;
>> 	}
>> 	while ((arg2 = va_arg(ap, const char *))) {
>> 		int set = va_arg(ap, int);
>> 		if (set)
>> 			die(arg1 and arg2 are incompatible);
>> 	}
>> 	va_end(ap);
>
> I personally think having a function is cleaner

Sorry, I was unclear again.  What I meant is that there could be
two functions:

 - one to check a single option against various options it is
   incompatible with, which you've already written
 - another to check a family of mutually incompatible options

The above was a sample implementation for the second function, but it
has a bug: the second "while" loop should have been preceded by
"if (!arg1) return;".

>> Seems reasonable.  A part of me would want to accept such options and
>> only error out if the saved state indicates that they are different
[...]
> Over-engineering definitely!

Yep, sorry.  Was just thinking out loud.

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

[Ramkumar Ramachandra]

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>
---
 This code is dependent on the rerere_clear public API (which I've
 posted in another thread).  Have the essential parts of the "reset
 --hard" been copied over correctly?  Should reset get a public API as
 well?

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

diff --git a/builtin/revert.c b/builtin/revert.c
index ccfc295..50c36e9 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -799,6 +799,54 @@ static int pick_commits(struct replay_opts *opts)
 	return persist_todo_done(res, revs.commits, done_list, opts);
 }
 
+static int process_args(int argc, const char **argv, struct replay_opts *opts)
+{
+	const char *me;
+	int fd;
+
+	parse_args(argc, argv, opts);
+	me = (opts->action == REVERT ? "revert" : "cherry-pick");
+	if (opts->abort_oper) {
+		char head[DEFAULT_ABBREV];
+		unsigned char sha1[20];
+		rerere_clear(0);
+
+		if (!file_exists(HEAD_FILE))
+			goto error;
+		if ((fd = open(HEAD_FILE, O_RDONLY, 0666)) < 0) {
+			int err = errno;
+			error(_("Could not open '%s' for reading: %s"),
+				HEAD_FILE, strerror(err));
+			return -err;
+		}
+		if (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV) {
+			int err = errno;
+			close(fd);
+			error(_("Corrupt '%s': %s"), HEAD_FILE, strerror(err));
+			return -err;
+		}
+		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)
 {
 	struct replay_opts opts;
@@ -810,9 +858,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 
 	opts.action = REVERT;
 	setenv(GIT_REFLOG_ACTION, "revert", 0);
-	parse_args(argc, argv, &opts);
-
-	return pick_commits(&opts);
+	return process_args(argc, argv, &opts);
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
@@ -823,7 +869,5 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(struct replay_opts));
 	opts.action = CHERRY_PICK;
 	setenv(GIT_REFLOG_ACTION, "cherry-pick", 0);
-	parse_args(argc, argv, &opts);
-
-	return pick_commits(&opts);
+	return process_args(argc, argv, &opts);
 }
-- 
1.7.5.GIT

Re: [PATCH 0/8] Sequencer Foundations

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> I've not attempted to add anything new in this series -- It merely
> fixes all the mistakes in the previous iteration.  I've tried to
> integrate the improvements suggested by all the previous reviews.

Thanks!  This is much more readable, probably because of the commit
messages. ;-)

> All tests pass in all patches, and I hope no
> stray lines have travelled b/w the patches during the rebase.

Speaking of which, some tests and documentation would be nice as icing
on the cake.

> Ramkumar Ramachandra (8):
>   revert: Improve error handling by cascading errors upwards
>   revert: Make "commit" and "me" local variables
>   revert: Introduce a struct to parse command-line options into
>   revert: Separate cmdline argument handling from the functional code
>   revert: Catch incompatible command-line options early
>   revert: Introduce head, todo, done files to persist state
>   revert: Implement parsing --continue, --abort and --skip
>   revert: Implement --abort processing

The heart is patch 6/8.  I have not thought about this deeply yet, but
I wonder if it would be simpler if the behavior of "git cherry-pick
1..10" looked like this:

. if there is state in .git/sequencer already, error out
. lock .git/sequencer/head with the lockfile API to prevent
  concurrent access
. write current state, including remaining commits to cherry-pick
. unlock .git/sequencer/head
. cherry-pick commit #1
. lock sequencer, check state, update state, unlock
. cherry-pick commit #2
 ...

This way, even if cherry-picking causes git to segfault, the sequencer
state is in good order and we know where to pick up.  More
importantly, massive refactoring of the merge_recursive API would not
be needed to keep everything in working order.  An atexit and sigchain
handler could be added to print advice for the reader about how to
resume, but that's just an extra hint and it's okay if it sometimes
doesn't happen sometimes.

What do you think?

Ciao,
Jonathan

Re: [PATCH 0/8] Sequencer Foundations

[Christian Couder]

Hi,

On Wed, May 11, 2011 at 3:14 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ramkumar Ramachandra wrote:
>
>> Ramkumar Ramachandra (8):
>>   revert: Improve error handling by cascading errors upwards
>>   revert: Make "commit" and "me" local variables
>>   revert: Introduce a struct to parse command-line options into
>>   revert: Separate cmdline argument handling from the functional code
>>   revert: Catch incompatible command-line options early
>>   revert: Introduce head, todo, done files to persist state
>>   revert: Implement parsing --continue, --abort and --skip
>>   revert: Implement --abort processing

I had no time to look at this yet but I will try do to so in the coming days.

> The heart is patch 6/8.  I have not thought about this deeply yet, but
> I wonder if it would be simpler if the behavior of "git cherry-pick
> 1..10" looked like this:
>
> . if there is state in .git/sequencer already, error out
> . lock .git/sequencer/head with the lockfile API to prevent
>  concurrent access
> . write current state, including remaining commits to cherry-pick
> . unlock .git/sequencer/head
> . cherry-pick commit #1
> . lock sequencer, check state, update state, unlock
> . cherry-pick commit #2
>  ...
>
> This way, even if cherry-picking causes git to segfault, the sequencer
> state is in good order and we know where to pick up.  More
> importantly, massive refactoring of the merge_recursive API would not
> be needed to keep everything in working order.  An atexit and sigchain
> handler could be added to print advice for the reader about how to
> resume, but that's just an extra hint and it's okay if it sometimes
> doesn't happen sometimes.
>
> What do you think?

I think that the risk at this point might be to overengineer things
and to lose time, and then we will perhaps find out that we need to do
some refactoring of the merge_recursive API anyway.
If we have cherry-pick with --abort, --continue and --skip that just
works as well or nearly as well (because it's new) as other stuff it
will be already a very good thing. And with enough tests we will
hopefully be able to build and refactor safely after that. Maybe we
will eventually find out that what you suggest is in fact needed even
for cherry-pick with --abort, --continue and --skip, but for now I
would prefer trying to make it work with as few changes and work as
possible.

Thanks,
Christian.

Re: [PATCH 0/8] Sequencer Foundations

[Jonathan Nieder]

Hi,

Christian Couder wrote:

> I think that the risk at this point might be to overengineer things
> and to lose time, and then we will perhaps find out that we need to do
> some refactoring of the merge_recursive API anyway.

I agree with the general principle... let's see if I understand the
details of what you are saying.

> If we have cherry-pick with --abort, --continue and --skip that just
> works as well or nearly as well (because it's new) as other stuff it
> will be already a very good thing.

Does "other stuff" mean scripts like "git rebase"?  If I understand
correctly, "git rebase" updates the $dotest directory before each
cherry-pick, unlike this series which only updates $dotest after a
failed cherry-pick.

> And with enough tests we will
> hopefully be able to build and refactor safely after that. Maybe we
> will eventually find out that what you suggest is in fact needed even
> for cherry-pick with --abort, --continue and --skip, but for now I
> would prefer trying to make it work with as few changes and work as
> possible.

I suspect that refactoring everything to use error() in place of die()
requires more risky changes and work than updating .git/sequencer/
between cherry-picks.  Of course I can easily be wrong.

Hoping that is clearer,
Jonathan

Re: [PATCH 0/8] Sequencer Foundations

[Jonathan Nieder]

Jonathan Nieder wrote:
> Christian Couder wrote:

>> I think that the risk at this point might be to overengineer things
>> and to lose time, and then we will perhaps find out that we need to do
>> some refactoring of the merge_recursive API anyway.
>
> I agree with the general principle... let's see if I understand the
> details of what you are saying.

It occurs to me now that you were probably talking about the
suggestion of using the lockfile API (i.e., the write temporary/rename
trick).  In that case, I agree --- no need to overengineer it and
concurrency problems can be fixed later.  Sorry for an overcomplicated
explanation.

And thanks for looking out for these things.

Re: [PATCH 0/8] Sequencer Foundations

[Christian Couder]

Hi,

On Thu, May 12, 2011 at 1:44 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>> Christian Couder wrote:
>
>>> I think that the risk at this point might be to overengineer things
>>> and to lose time, and then we will perhaps find out that we need to do
>>> some refactoring of the merge_recursive API anyway.
>>
>> I agree with the general principle... let's see if I understand the
>> details of what you are saying.
>
> It occurs to me now that you were probably talking about the
> suggestion of using the lockfile API (i.e., the write temporary/rename
> trick).  In that case, I agree --- no need to overengineer it and
> concurrency problems can be fixed later.  Sorry for an overcomplicated
> explanation.

Yeah, it was mostly the lockfile API.

About writing files before each cherry-pick, I am not against it, if
it is really needed to be safe. I even suggested it in my patch series
back in November
(http://article.gmane.org/gmane.comp.version-control.git/162183).
But it will make cherry-pick less efficient, so it is a kind of
performance regression that we can perhaps avoid by changing some
die() into error().

So what i suggest and in fact started is to just try that. We may find
that we could indeed do it quite safely or we may find that it's too
much work to be safe enough. When I tried it, it seemed to me that it
was not a lot of work, and not very complex, though it added many
commits to the patch series. But perhaps I overlooked some problems. I
will have another look soon.

Thanks,
Christian.

Re: [PATCH 0/8] Sequencer Foundations

[Jonathan Nieder]

Christian Couder wrote:

> About writing files before each cherry-pick, I am not against it, if
> it is really needed to be safe. I even suggested it in my patch series
> back in November
> (http://article.gmane.org/gmane.comp.version-control.git/162183).
> But it will make cherry-pick less efficient, so it is a kind of
> performance regression that we can perhaps avoid by changing some
> die() into error().

Yes, that's a good point.  Maybe in the long term the extra safety
could become optional.  And I am happy about the die() elimination;
the only part I was not as thrilled about is relying on it.

Some die() calls, like the one in xmalloc, would be very difficult to
eliminate.

Re: [PATCH 0/8] Sequencer Foundations

[Christian Couder]

On Friday 13 May 2011 12:37:56 Jonathan Nieder wrote:
> Christian Couder wrote:
> > About writing files before each cherry-pick, I am not against it, if
> > it is really needed to be safe. I even suggested it in my patch series
> > back in November
> > (http://article.gmane.org/gmane.comp.version-control.git/162183).
> > But it will make cherry-pick less efficient, so it is a kind of
> > performance regression that we can perhaps avoid by changing some
> > die() into error().
> 
> Yes, that's a good point.  Maybe in the long term the extra safety
> could become optional.  And I am happy about the die() elimination;
> the only part I was not as thrilled about is relying on it.
> 
> Some die() calls, like the one in xmalloc, would be very difficult to
> eliminate.

Yeah, but to address this problem, maybe we can use a special die routine like 
you already suggested. I think if we use both error() and a special die 
routine we should be pretty safe.

Anyway I looked at the patch series and I found nothing that your very good 
review had not already spotted. My only nit is that maybe as the error 
handling patch is growing bigger, it could be splitted in 2 or 3 patchs.

Thanks to you and Ram,
Christian.