threads / rfc / 27050

RFC patch, 11 partsSequencer Foundations

Subject: [RFC PATCH 00/11] Sequencer Foundations

## tl;dr

36 messages between Apr 10, 2011 and May 8, 2011. Diffs are folded; open one to read it.

replies: 35people: 5as markdown or json

Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · lore
Hi,

I've started working on building a sequencer for Git. While the outline is described in [1], I'd like some additional clarifications. A big thanks to Christian's series [2] for the valuable roadmap.

Please note that 10/11 is not related to this series, but seems to be a minor nit that's required to make all existing tests pass.

0. Is the general flow alright?
1. Is it okay to use this kind of adaptive error handling (calling
'die' in some places and returning error in other places), or should
it be more uniform?
2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
inelegant, mainly because of the command-line argument parsing
overhead.  Re-implementing it using more low-level functions doesn't
seem to be the way to go either: for example, 'reset --hard' has some
additional logic of writing HEAD and ORIG_HEAD, which I don't want to
duplicate.  Should I work on reworking parts of 'rerere.c' and
'revert.c', or is there some other way?
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format.  This part will go into the sequencer.  Then I'll use a
cherry-pick specific file to keep the command-line options.  Yes, I'm
trying to work on Daniel's idea [3] from the very start.  Is this a
good idea?
4. I have a feeling that I've broken translation strings.  Is there a
README, plus a bunch of tests I can run to make sure that I've not
broken anything?
Thanks for reading.
-- Ram

[1]: http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170908 [2]: http://thread.gmane.org/gmane.comp.version-control.git/162183 [3]: http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170834

Ramkumar Ramachandra (11):
  revert: Avoid calling die; return error instead
  revert: Lose global variables "commit" and "me"
  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: Implement parsing --continue, --abort and --skip
  revert: Handle conflict resolutions more elegantly
  usage: Introduce error_errno correspoding to die_errno
  revert: Write head, todo, done files
  revert: Give noop a default value while argument parsing
  revert: Implement --abort processing
 advice.c          |   14 ++
 advice.h          |    1 +
 builtin/revert.c  |  454 ++++++++++++++++++++++++++++++++++++++---------------
 git-compat-util.h |    2 +
 usage.c           |   34 ++++
 5 files changed, 381 insertions(+), 124 deletions(-)
-- 
1.7.4.rc1.7.g2cf08.dirty
Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

[PATCH 01/11] revert: Avoid calling die; return error instead

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   71 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 37 insertions(+), 34 deletions(-)
Show changes to builtin/revert.c +37 −34
diff --git a/builtin/revert.c b/builtin/revert.c
index 2bb13eb..017b1af 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -265,23 +265,23 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static NORETURN void die_dirty_index(const char *me)
+static int error_dirty_index(const char *me)
 {
 	if (read_cache_unmerged()) {
 		die_resolve_conflict(me);
 	} else {
 		if (advice_commit_before_merge) {
 			if (action == REVERT)
-				die(_("Your local changes would be overwritten by revert.\n"
+				return error(_("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"
+				return error(_("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"));
+				return error(_("Your local changes would be overwritten by revert.\n"));
 			else
-				die(_("Your local changes would be overwritten by cherry-pick.\n"));
+				return error(_("Your local changes would be overwritten by cherry-pick.\n"));
 		}
 	}
 }
@@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	    (write_cache(index_fd, active_cache, active_nr) ||
 	     commit_locked_index(&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,18 +397,18 @@ static int do_pick_commit(void)
 		 * to work on.
 		 */
 		if (write_cache_as_tree(head, 0, NULL))
-			die (_("Your index file is unmerged."));
+			return error(_("Your index file is unmerged."));
 	} else {
 		if (get_sha1("HEAD", head))
-			die (_("You do not have a valid HEAD"));
+			return error(_("You do not have a valid HEAD"));
 		if (index_differs_from("HEAD", 0))
-			die_dirty_index(me);
+			return error_dirty_index(me);
 	}
 	discard_cache();
 
 	if (!commit->parents) {
 		if (action == REVERT)
-			die (_("Cannot revert a root commit"));
+			return error(_("Cannot revert a root commit"));
 		parent = NULL;
 	}
 	else if (commit->parents->next) {
@@ -417,19 +417,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;
@@ -440,11 +440,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));
 
 	/*
@@ -523,7 +523,7 @@ 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;
 
@@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)
 		revs->reverse = 1;
 
 	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
-	if (argc > 1)
-		usage(*revert_or_cherry_pick_usage());
+	if (argc > 1) {
+		fprintf(stderr, "usage: %s", _(*revert_or_cherry_pick_usage()));
+		return 129;
+	}
 
 	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);
+			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";
@@ -578,17 +583,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(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.4.rc1.7.g2cf08.dirty
Jonathan Nieder· Apr 10, 2011, 19:14 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 01/11] revert: Avoid calling die; return error instead

Ramkumar Ramachandra wrote:
> [Subject: revert: Avoid calling die; return error instead]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Presumably this is because the sequencer is going to pick up after the error and clean up a little (why doesn't the change description say so?). Will it be resuming after that or just performing a little cleanup before the exit?

Might make sense, but:
Show 11 quoted lines
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -265,23 +265,23 @@ static struct tree *empty_tree(void)
>  	return tree;
>  }
>  
> -static NORETURN void die_dirty_index(const char *me)
> +static int error_dirty_index(const char *me)
>  {
>  	if (read_cache_unmerged()) {
>  		die_resolve_conflict(me);
Won't that exit?  
>  	} else {

This "else" could be removed (decreasing the indent of the rest by one tab stop) since the "if" case has already returned or exited. Not the subject of this patch, just an idea for earlier or later. ;-)

[...]
Show 7 quoted lines
> @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  	    (write_cache(index_fd, active_cache, active_nr) ||
>  	     commit_locked_index(&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);
What happens to index_lock in the error case?
[...]
Show 10 quoted lines
> @@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)
>  		revs->reverse = 1;
>  
>  	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
> -	if (argc > 1)
> -		usage(*revert_or_cherry_pick_usage());
> +	if (argc > 1) {
> +		fprintf(stderr, "usage: %s", _(*revert_or_cherry_pick_usage()));
> +		return 129;
> +	}

Yuck. Maybe the error can be returned to the caller somehow, but that seems somehow ambitious given that setup_revisions has all sorts of ways to die anyway.

So you are bending the assumptions of many existing git functions (in a good way).

I can think of at least three ways to go:
 1) Come up with a convention to give more information about the nature
    of returned errors in the functions you are touching.  For
    example, make sure errno is valid after the relevant functions, or
    use multiple negative values to express the nature of the error.
    So a caller could do:
	if (prepare_revs(...)) {
		if (errno == EINVAL)
			usage(*revert_or_cherry_pick_usage());
		die("BUG: unexpected error from prepare_revs");
	}
    Or:
 2) Use set_die_routine or sigchain_push + atexit to declare what cleanup
    has to happen before exiting.  Keep using die().
 3) Provide a new facility to register cleanup handlers that will free
    resources and otherwise return to a consistent state before
    unwinding the stack.  This way, you'd still have to audit die()
    calls to look for missing cleanup handlers, but they could stay as
    die() rather than changing to "return error" and the worried
    caller could use
	set_die_routine(longjmp_to_here);
    to keep git alive.  I don't suggest doing this.  It is a pain to
    get right and not obviously cleaner than "return error", and some
    errors really are unrecoverable (rather than just being a symptom
    of programmers to lazy to write error recovery code :)).
Okay, I'm stopping here.  Hope that helps.

Thanks, Jonathan

Ramkumar Ramachandra· May 8, 2011, 12:04 UTC · re: Jonathan Nieder · lore

Re: [PATCH 01/11] revert: Avoid calling die; return error instead

Hi Jonathan,
Jonathan Nieder writes:
Show 9 quoted lines
> Ramkumar Ramachandra wrote:
> > [Subject: revert: Avoid calling die; return error instead]
> >
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> 
> Presumably this is because the sequencer is going to pick up after the
> error and clean up a little (why doesn't the change description say
> so?).  Will it be resuming after that or just performing a little
> cleanup before the exit?

I didn't write commit messages for any of the patches in the previous round -- I just wanted to show the idea quickly. Anyway, it's fixed in the next round (coming soon).

Show 13 quoted lines
> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -265,23 +265,23 @@ static struct tree *empty_tree(void)
> >  	return tree;
> >  }
> >  
> > -static NORETURN void die_dirty_index(const char *me)
> > +static int error_dirty_index(const char *me)
> >  {
> >  	if (read_cache_unmerged()) {
> >  		die_resolve_conflict(me);
> 
> Won't that exit?  
Fixed.
Show 5 quoted lines
> >  	} else {
> 
> This "else" could be removed (decreasing the indent of the rest by
> one tab stop) since the "if" case has already returned or exited.
> Not the subject of this patch, just an idea for earlier or later. ;-)
Fixed.
Show 10 quoted lines
> [...]
> > @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> >  	    (write_cache(index_fd, active_cache, active_nr) ||
> >  	     commit_locked_index(&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);
> 
> What happens to index_lock in the error case?
Fixed.
Show 52 quoted lines
> [...]
> > @@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)
> >  		revs->reverse = 1;
> >  
> >  	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
> > -	if (argc > 1)
> > -		usage(*revert_or_cherry_pick_usage());
> > +	if (argc > 1) {
> > +		fprintf(stderr, "usage: %s", _(*revert_or_cherry_pick_usage()));
> > +		return 129;
> > +	}
> 
> Yuck.  Maybe the error can be returned to the caller somehow, but
> that seems somehow ambitious given that setup_revisions has all sorts
> of ways to die anyway.
> 
> So you are bending the assumptions of many existing git functions (in
> a good way).
> 
> I can think of at least three ways to go:
> 
>  1) Come up with a convention to give more information about the nature
>     of returned errors in the functions you are touching.  For
>     example, make sure errno is valid after the relevant functions, or
>     use multiple negative values to express the nature of the error.
> 
>     So a caller could do:
> 
> 	if (prepare_revs(...)) {
> 		if (errno == EINVAL)
> 			usage(*revert_or_cherry_pick_usage());
> 		die("BUG: unexpected error from prepare_revs");
> 	}
> 
>     Or:
> 
>  2) Use set_die_routine or sigchain_push + atexit to declare what cleanup
>     has to happen before exiting.  Keep using die().
> 
>  3) Provide a new facility to register cleanup handlers that will free
>     resources and otherwise return to a consistent state before
>     unwinding the stack.  This way, you'd still have to audit die()
>     calls to look for missing cleanup handlers, but they could stay as
>     die() rather than changing to "return error" and the worried
>     caller could use
> 
> 	set_die_routine(longjmp_to_here);
> 
>     to keep git alive.  I don't suggest doing this.  It is a pain to
>     get right and not obviously cleaner than "return error", and some
>     errors really are unrecoverable (rather than just being a symptom
>     of programmers to lazy to write error recovery code :)).

Hm, I'm a little confused about error handling now. I'll defer this part until my patches naturally establish some convention for error handling -- designing one in advance isn't as easy as I thought.

Finally, thanks for the review! I'll look forward to more reviews as I post more iterations of the series.

-- Ram
Junio C Hamano· Apr 11, 2011, 20:26 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 01/11] revert: Avoid calling die; return error instead

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

You would need to write a lot more than that to justify why this is a good change and does not regress the existing codepaths. The above Subject: implies as if all you did was to replace "die()" with "return error()", but I am sure that you would also have audited all the existing callers of the affected codepaths and either they already handled an error return correctly by dying or exiting with non-zero status, or you adjusted them to expect an error return and exit with 129 in this patch.

Also we know from the context of this post that you are planning to add new callsites to some of the functions that are converted to give an error return with this patch, but it is nevertheless a good idea to briefly mention that (just "the codepath to implement new nitfol feature will be making calls to xyzzy and frotz and it does not want these to die; rather it wants to handle error cases itself" would do).

Show 7 quoted lines
> @@ -331,7 +331,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  	    (write_cache(index_fd, active_cache, active_nr) ||
>  	     commit_locked_index(&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);

Do the callers rollback the lockfile in their error return codepaths now? Should they? If not why not?

One acceptable answer is "the only thing the current callers do in their error codepaths is to exit(129), and that will roll it back for us", but then that might mean this patch made the API more error prone to use when the next callsite you add wants to do more than just exitting.

Show 15 quoted lines
> @@ -397,18 +397,18 @@ static int do_pick_commit(void)
>  		 * to work on.
>  		 */
>  		if (write_cache_as_tree(head, 0, NULL))
> -			die (_("Your index file is unmerged."));
> +			return error(_("Your index file is unmerged."));
>  	} else {
>  		if (get_sha1("HEAD", head))
> -			die (_("You do not have a valid HEAD"));
> +			return error(_("You do not have a valid HEAD"));
>  		if (index_differs_from("HEAD", 0))
> -			die_dirty_index(me);
> +			return error_dirty_index(me);
>  	}
>  	discard_cache();

Likewise for this "discard-cache". Should it be the responsibility to the caller to discard the in-core cache when they handle an error return and possibly take an alternative action, or should this function be the one to do so for them?

Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

[PATCH 02/11] revert: Lose global variables "commit" and "me"

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   44 +++++++++++++++++++++++++-------------------
 1 files changed, 25 insertions(+), 19 deletions(-)
Show changes to builtin/revert.c +25 −19
diff --git a/builtin/revert.c b/builtin/revert.c
index 017b1af..fdbacad 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)
 {
@@ -115,7 +113,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;
@@ -124,7 +123,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)
@@ -162,7 +161,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;
 
@@ -184,7 +183,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'))
@@ -197,7 +197,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 void write_cherry_pick_head(struct commit *commit)
 {
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
@@ -265,10 +265,10 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static int error_dirty_index(const char *me)
+static int error_dirty_index()
 {
 	if (read_cache_unmerged()) {
-		die_resolve_conflict(me);
+		die_resolve_conflict(action == REVERT ? "revert" : "cherry-pick");
 	} else {
 		if (advice_commit_before_merge) {
 			if (action == REVERT)
@@ -379,7 +379,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,7 +402,7 @@ static int do_pick_commit(void)
 		if (get_sha1("HEAD", head))
 			return error(_("You do not have a valid HEAD"));
 		if (index_differs_from("HEAD", 0))
-			return error_dirty_index(me);
+			return error_dirty_index();
 	}
 	discard_cache();
 
@@ -441,9 +441,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));
 
@@ -476,14 +477,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)
-			write_cherry_pick_head();
+			write_cherry_pick_head(commit);
 	}
 
 	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -546,10 +547,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 = (cmd_opts.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);
@@ -565,10 +569,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);
 
@@ -583,7 +589,7 @@ 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;
 
-- 
1.7.4.rc1.7.g2cf08.dirty
Christian Couder· Apr 11, 2011, 03:24 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 02/11] revert: Lose global variables "commit" and "me"

On Sunday 10 April 2011 17:11:48 Ramkumar Ramachandra wrote:
Show 12 quoted lines
> @@ -546,10 +547,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 = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");

It looks like this patch will not compile as cmd_opts is introduced in patch 03/11.

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

Best regards, Christian.

Ramkumar Ramachandra· Apr 11, 2011, 08:57 UTC · re: Christian Couder · lore

Re: [PATCH 02/11] revert: Lose global variables "commit" and "me"

Hi Christian,
Christian Couder writes:
Show 16 quoted lines
> On Sunday 10 April 2011 17:11:48 Ramkumar Ramachandra wrote:
> > @@ -546,10 +547,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 = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
> 
> It looks like this patch will not compile as cmd_opts is introduced in patch 
> 03/11.

Rebase fail. Thanks for pointing it out. I'll add some features and rework the series shortly.

-- Ram
Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

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

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |  132 +++++++++++++++++++++++++++++------------------------
 1 files changed, 72 insertions(+), 60 deletions(-)
Show changes to builtin/revert.c +72 −60
diff --git a/builtin/revert.c b/builtin/revert.c
index fdbacad..2b33220 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,17 +35,27 @@ 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;
 
+static struct {
+	enum { REVERT, CHERRY_PICK } action;
 
-/* Merge strategy. */
-static const char *strategy;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+	/* 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;
+} cmd_opts;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -53,7 +63,7 @@ static char *get_encoding(struct commit *commit, const char *message);
 
 static const char * const *revert_or_cherry_pick_usage(void)
 {
-	return action == REVERT ? revert_usage : cherry_pick_usage;
+	return cmd_opts.action == REVERT ? revert_usage : cherry_pick_usage;
 }
 
 static int option_parse_x(const struct option *opt,
@@ -62,8 +72,8 @@ static int option_parse_x(const struct option *opt,
 	if (unset)
 		return 0;
 
-	ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
-	xopts[xopts_nr++] = xstrdup(arg);
+	ALLOC_GROW(cmd_opts.xopts, cmd_opts.xopts_nr + 1, cmd_opts.xopts_alloc);
+	cmd_opts.xopts[cmd_opts.xopts_nr++] = xstrdup(arg);
 	return 0;
 }
 
@@ -72,37 +82,37 @@ static void parse_args(int argc, const char **argv)
 	const char * const * usage_str = revert_or_cherry_pick_usage();
 	int noop;
 	struct option options[] = {
-		OPT_BOOLEAN('n', "no-commit", &no_commit, "don't automatically commit"),
-		OPT_BOOLEAN('e', "edit", &edit, "edit the commit message"),
+		OPT_BOOLEAN('n', "no-commit", &cmd_opts.no_commit, "don't automatically commit"),
+		OPT_BOOLEAN('e', "edit", &cmd_opts.edit, "edit the commit message"),
 		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
-		OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
-		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
-		OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
-		OPT_STRING(0, "strategy", &strategy, "strategy", "merge strategy"),
-		OPT_CALLBACK('X', "strategy-option", &xopts, "option",
+		OPT_BOOLEAN('s', "signoff", &cmd_opts.signoff, "add Signed-off-by:"),
+		OPT_INTEGER('m', "mainline", &cmd_opts.mainline, "parent number"),
+		OPT_RERERE_AUTOUPDATE(&cmd_opts.allow_rerere_auto),
+		OPT_STRING(0, "strategy", &cmd_opts.strategy, "strategy", "merge strategy"),
+		OPT_CALLBACK('X', "strategy-option", &cmd_opts.xopts, "option",
 			"option for merge strategy", option_parse_x),
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
 	};
 
-	if (action == CHERRY_PICK) {
+	if (cmd_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, &cmd_opts.no_replay, "append commit name"),
+			OPT_BOOLEAN(0, "ff", &cmd_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,
+	cmd_opts.commit_argc = parse_options(argc, argv, NULL, options, usage_str,
 				    PARSE_OPT_KEEP_ARGV0 |
 				    PARSE_OPT_KEEP_UNKNOWN);
-	if (commit_argc < 2)
+	if (cmd_opts.commit_argc < 2)
 		usage_with_options(usage_str, options);
 
-	commit_argv = argv;
+	cmd_opts.commit_argv = argv;
 }
 
 struct commit_message {
@@ -268,17 +278,17 @@ static struct tree *empty_tree(void)
 static int error_dirty_index()
 {
 	if (read_cache_unmerged()) {
-		die_resolve_conflict(action == REVERT ? "revert" : "cherry-pick");
+		die_resolve_conflict(cmd_opts.action == REVERT ? "revert" : "cherry-pick");
 	} else {
 		if (advice_commit_before_merge) {
-			if (action == REVERT)
+			if (cmd_opts.action == REVERT)
 				return error(_("Your local changes would be overwritten by revert.\n"
 					  "Please, commit your changes or stash them to proceed."));
 			else
 				return error(_("Your local changes would be overwritten by cherry-pick.\n"
 					  "Please, commit your changes or stash them to proceed."));
 		} else {
-			if (action == REVERT)
+			if (cmd_opts.action == REVERT)
 				return error(_("Your local changes would be overwritten by revert.\n"));
 			else
 				return error(_("Your local changes would be overwritten by cherry-pick.\n"));
@@ -320,7 +330,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 = cmd_opts.xopts; xopt != cmd_opts.xopts + cmd_opts.xopts_nr; xopt++)
 		parse_merge_opt(&o, *xopt);
 
 	clean = merge_trees(&o,
@@ -331,7 +341,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	    (write_cache(index_fd, active_cache, active_nr) ||
 	     commit_locked_index(&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"),
+			cmd_opts.action == REVERT ? "revert" : "cherry-pick");
 	rollback_lock_file(&index_lock);
 
 	if (!clean) {
@@ -368,9 +379,9 @@ static int run_git_commit(const char *defmsg)
 
 	args[i++] = "commit";
 	args[i++] = "-n";
-	if (signoff)
+	if (cmd_opts.signoff)
 		args[i++] = "-s";
-	if (!edit) {
+	if (!cmd_opts.edit) {
 		args[i++] = "-F";
 		args[i++] = defmsg;
 	}
@@ -389,7 +400,7 @@ static int do_pick_commit(struct commit *commit)
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res;
 
-	if (no_commit) {
+	if (cmd_opts.no_commit) {
 		/*
 		 * We do not intend to commit immediately.  We just want to
 		 * merge the differences in, so let's compute the tree
@@ -407,7 +418,7 @@ static int do_pick_commit(struct commit *commit)
 	discard_cache();
 
 	if (!commit->parents) {
-		if (action == REVERT)
+		if (cmd_opts.action == REVERT)
 			return error(_("Cannot revert a root commit"));
 		parent = NULL;
 	}
@@ -416,32 +427,32 @@ static int do_pick_commit(struct commit *commit)
 		int cnt;
 		struct commit_list *p;
 
-		if (!mainline)
+		if (!cmd_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 != cmd_opts.mainline && p;
 		     cnt++)
 			p = p->next;
-		if (cnt != mainline || !p)
+		if (cnt != cmd_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), cmd_opts.mainline);
 		parent = p->item;
-	} else if (mainline > 0)
+	} else if (cmd_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 (cmd_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",
+			cmd_opts.action == REVERT ? "revert" : "cherry-pick",
 			sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit, commit->buffer, &msg) != 0)
@@ -457,7 +468,7 @@ static int do_pick_commit(struct commit *commit)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	if (action == REVERT) {
+	if (cmd_opts.action == REVERT) {
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -478,16 +489,16 @@ 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 (cmd_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 (!cmd_opts.no_commit)
 			write_cherry_pick_head(commit);
 	}
 
-	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
+	if (!cmd_opts.strategy || !strcmp(cmd_opts.strategy, "recursive") || cmd_opts.action == REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf);
 		write_message(&msgbuf, defmsg);
@@ -499,22 +510,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(cmd_opts.strategy, cmd_opts.xopts_nr,
+					cmd_opts.xopts, common,
 					sha1_to_hex(head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
 	}
 
 	if (res) {
-		error(action == REVERT
+		error(cmd_opts.action == REVERT
 		      ? _("could not revert %s... %s")
 		      : _("could not apply %s... %s"),
 		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
 		      msg.subject);
 		print_advice();
-		rerere(allow_rerere_auto);
+		rerere(cmd_opts.allow_rerere_auto);
 	} else {
-		if (!no_commit)
+		if (!cmd_opts.no_commit)
 			res = run_git_commit(defmsg);
 	}
 
@@ -530,10 +542,10 @@ static int prepare_revs(struct rev_info *revs)
 
 	init_revisions(revs, NULL);
 	revs->no_walk = 1;
-	if (action != REVERT)
+	if (cmd_opts.action != REVERT)
 		revs->reverse = 1;
 
-	argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
+	argc = setup_revisions(cmd_opts.commit_argc, cmd_opts.commit_argv, revs, NULL);
 	if (argc > 1) {
 		fprintf(stderr, "usage: %s", _(*revert_or_cherry_pick_usage()));
 		return 129;
@@ -574,18 +586,18 @@ 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 = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
 	setenv(GIT_REFLOG_ACTION, me, 0);
 	parse_args(argc, argv);
 
-	if (allow_ff) {
-		if (signoff)
+	if (cmd_opts.allow_ff) {
+		if (cmd_opts.signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (no_commit)
+		if (cmd_opts.no_commit)
 			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (no_replay)
+		if (cmd_opts.no_replay)
 			die(_("cherry-pick --ff cannot be used with -x"));
-		if (edit)
+		if (cmd_opts.edit)
 			die(_("cherry-pick --ff cannot be used with --edit"));
 	}
 
@@ -603,13 +615,13 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	if (isatty(0))
-		edit = 1;
-	action = REVERT;
+		cmd_opts.edit = 1;
+	cmd_opts.action = REVERT;
 	return revert_or_cherry_pick(argc, argv);
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
-	action = CHERRY_PICK;
+	cmd_opts.action = CHERRY_PICK;
 	return revert_or_cherry_pick(argc, argv);
 }
-- 
1.7.4.rc1.7.g2cf08.dirty
Jonathan Nieder· Apr 10, 2011, 19:21 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
> [Subject: revert: Introduce a struct to parse command-line options into]
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

This gives a hint of another way to avoid the usage() trouble mentioned in patch 1: it might be possible to let the caller take care of parsing arguments.

Show 14 quoted lines
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -35,17 +35,27 @@ 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;
>  
> +static struct {
> +	enum { REVERT, CHERRY_PICK } action;

That would require giving this struct a name, so it can be passed around. Not a bad idea anyway imho since then a person reading top-to-bottom is not left in suspense:

 struct cherry_pick_opts {
	enum { REVERT, CHERRY_PICK } action;
	unsigned edit:1;
	unsigned no_replay:1;
	...
 };
Ramkumar Ramachandra· May 8, 2011, 12:18 UTC · re: Jonathan Nieder · lore

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

Hi Jonathan,
Jonathan Nieder writes:
Show 27 quoted lines
> Ramkumar Ramachandra wrote:
> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -35,17 +35,27 @@ 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;
> >  
> > +static struct {
> > +	enum { REVERT, CHERRY_PICK } action;
> 
> That would require giving this struct a name, so it can be passed
> around.  Not a bad idea anyway imho since then a person reading
> top-to-bottom is not left in suspense:
> 
>  struct cherry_pick_opts {
> 	enum { REVERT, CHERRY_PICK } action;
> 
> 	unsigned edit:1;
> 	unsigned no_replay:1;
> 	...
>  };
Thanks.  A couple of nits:
1. You can't take the address of a bitfield, so 'edit' and 'replay'
   can't be passed to the command line argument parsing framework
   directly.
2. GCC throws a "warning: useless storage class specifier in empty
   declaration" for this kind of declaration.
-- Ram
Junio C Hamano· Apr 11, 2011, 21:41 UTC · re: Ramkumar Ramachandra · lore

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

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

Again, "In later steps, a new API that takes a single commit and replays it in forward (cherry-pick) or backward (revert) direction will be introduced, and will take this structure as a parameter to tell it what to do" is missing from the above description.

More importantly, the primary purpose of these variables is _not_ "to parse command line options into". It is to actively affect what happens in the code, and "parse command line" is merely a way to assign the initial values to them. So I'd rather see this patch described perhaps like this:

    cherry-pick/revert: introduce "struct replay_options"
    The current code uses a set of file-scope static variables to instruct
    the cherry-pick/revert machinery how to replay the changes, and
    initialises 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.
    Introduce a structure to group these variables, so that the API can
    take them as a single "replay_options" parameter.

I strongly prefer to see this patch also update the callchain to pass a pointer to the options struct as parameter. I can guess without reading the rest the series that at some later step you would do that, but I think it makes more sense to do the conversion at this step, as you will be touching lines that use the global variables in this patch anyway, like this:

- return action == REVERT ? revert_usage : cherry_pick_usage; + return cmd_opts.action == REVERT ? revert_usage : cherry_pick_usage;

which should eventually become something like:
	return opts->usage_message;

at the very last step when this codepath is _fully_ libified (I don't know if the final result of your series still is a "choose only from these two canned messages" API, or gives the ability to the caller to specify the usage message---I am just showing how it should look like at the end of a full libification). So it would make sense to see:

	return opts->action == REVERT ? revert_usage : cherry_pick_usage;
in this patch.
> @@ -268,17 +278,17 @@ static struct tree *empty_tree(void)
>  static int error_dirty_index()

It is probably a remnant of the earlier patches in this series, but this should start with:

	static int error_dirty_index(void)

Of course, you will actually be passing the options structure, so it would become:

	static int error_dirty_index(struct replay_options *opts)
        {
        	...
                if (opts->action == REVERT)
                	...
	}
Ramkumar Ramachandra· May 8, 2011, 12:09 UTC · re: Junio C Hamano · lore

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

Hi Junio,
Junio C Hamano writes:
Show 25 quoted lines
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> 
> Again, "In later steps, a new API that takes a single commit and replays
> it in forward (cherry-pick) or backward (revert) direction will be
> introduced, and will take this structure as a parameter to tell it what to
> do" is missing from the above description.
> 
> More importantly, the primary purpose of these variables is _not_ "to
> parse command line options into".  It is to actively affect what happens
> in the code, and "parse command line" is merely a way to assign the
> initial values to them.  So I'd rather see this patch described perhaps
> like this:
> 
>     cherry-pick/revert: introduce "struct replay_options"
> 
>     The current code uses a set of file-scope static variables to instruct
>     the cherry-pick/revert machinery how to replay the changes, and
>     initialises 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.
> 
>     Introduce a structure to group these variables, so that the API can
>     take them as a single "replay_options" parameter.
Right.  Thanks for the nice commit message :)
Show 6 quoted lines
> I strongly prefer to see this patch also update the callchain to pass a
> pointer to the options struct as parameter.  I can guess without reading
> the rest the series that at some later step you would do that, but I think
> it makes more sense to do the conversion at this step, as you will be
> touching lines that use the global variables in this patch anyway, like
> this:
Good idea.  I've passed the opts pointer around in the new series.
Show 17 quoted lines
> > @@ -268,17 +278,17 @@ static struct tree *empty_tree(void)
> >  static int error_dirty_index()
> 
> It is probably a remnant of the earlier patches in this series, but this
> should start with:
> 
> 	static int error_dirty_index(void)
> 
> Of course, you will actually be passing the options structure, so it would
> become:
> 
> 	static int error_dirty_index(struct replay_options *opts)
>         {
>         	...
>                 if (opts->action == REVERT)
>                 	...
> 	}

The very first patch removes this function (and puts the functionality elsewhere) in the new series, so this comment doesn't apply.

Thanks for the review!
-- Ram
Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

[PATCH 04/11] revert: Separate cmdline argument handling from the functional code

Based-on-patch-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)
Show changes to builtin/revert.c +14 −9
diff --git a/builtin/revert.c b/builtin/revert.c
index 2b33220..9381541 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -578,18 +578,12 @@ static int read_and_refresh_cache(void)
 	return 0;
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int pick_commits(void)
 {
 	struct rev_info revs;
 	struct commit *commit;
-	const char *me;
 	int res;
 
-	git_config(git_default_config, NULL);
-	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
-	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv);
-
 	if (cmd_opts.allow_ff) {
 		if (cmd_opts.signoff)
 			die(_("cherry-pick --ff cannot be used with --signoff"));
@@ -616,12 +610,23 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	if (isatty(0))
 		cmd_opts.edit = 1;
+
+	git_config(git_default_config, NULL);
+	memset(&cmd_opts, 0, sizeof(cmd_opts));
 	cmd_opts.action = REVERT;
-	return revert_or_cherry_pick(argc, argv);
+	setenv(GIT_REFLOG_ACTION, "revert", 0);
+	parse_args(argc, argv);
+
+	return pick_commits();
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
+	git_config(git_default_config, NULL);
+	memset(&cmd_opts, 0, sizeof(cmd_opts));
 	cmd_opts.action = CHERRY_PICK;
-	return revert_or_cherry_pick(argc, argv);
+	setenv(GIT_REFLOG_ACTION, "cherry-pick", 0);
+	parse_args(argc, argv);
+
+	return pick_commits();
 }
-- 
1.7.4.rc1.7.g2cf08.dirty
Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

[PATCH 05/11] revert: Catch incompatible command-line options early

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   36 +++++++++++++++++++++++++-----------
 1 files changed, 25 insertions(+), 11 deletions(-)
Show changes to builtin/revert.c +25 −11
diff --git a/builtin/revert.c b/builtin/revert.c
index 9381541..facae24 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -77,6 +77,24 @@ static int option_parse_x(const struct option *opt,
 	return 0;
 }
 
+static void die_opt_incompatible(const char *me, const char *base_opt,
+				int nargs, int opt_bitarray[], ...)
+{
+	const char *this_opt = NULL;
+	va_list ap;
+	int i;
+
+	va_start(ap, opt_bitarray);
+	for (i = 0; i < nargs; i ++) {
+		this_opt = va_arg(ap, const char *);
+		if (opt_bitarray[i]) {
+			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)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage();
@@ -112,6 +130,13 @@ static void parse_args(int argc, const char **argv)
 	if (cmd_opts.commit_argc < 2)
 		usage_with_options(usage_str, options);
 
+	if (cmd_opts.allow_ff) {
+		int opt_bitarray[] = {cmd_opts.signoff, cmd_opts.no_commit,
+				      cmd_opts.no_replay, cmd_opts.edit};
+		die_opt_incompatible(me, "--ff", 4, opt_bitarray, "--signoff",
+				"--no-commit", "-x", "--edit");
+	}
+
 	cmd_opts.commit_argv = argv;
 }
 
@@ -584,17 +609,6 @@ static int pick_commits(void)
 	struct commit *commit;
 	int res;
 
-	if (cmd_opts.allow_ff) {
-		if (cmd_opts.signoff)
-			die(_("cherry-pick --ff cannot be used with --signoff"));
-		if (cmd_opts.no_commit)
-			die(_("cherry-pick --ff cannot be used with --no-commit"));
-		if (cmd_opts.no_replay)
-			die(_("cherry-pick --ff cannot be used with -x"));
-		if (cmd_opts.edit)
-			die(_("cherry-pick --ff cannot be used with --edit"));
-	}
-
 	if ((res = read_and_refresh_cache()) ||
 		(res = prepare_revs(&revs)))
 		return res;
-- 
1.7.4.rc1.7.g2cf08.dirty
Junio C Hamano· Apr 11, 2011, 21:44 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH 05/11] revert: Catch incompatible command-line options early

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> +static void die_opt_incompatible(const char *me, const char *base_opt,
> +				int nargs, int opt_bitarray[], ...)
> +{

This feels way overkill to use vararg. This may be _one_ way to do what you want to do, and while I don't think it is the best way, it may be Ok.

"opt_bitarray[]" is a horrible name, though. It does not convey what the variable is about (its "purpose"). It doesn't even correctly describe the way that unspecified purpose happens to be implemented (claims to be bitarray but it is just an array of ints and what matters is if any bit is set in the array).

Show 13 quoted lines
>  static void parse_args(int argc, const char **argv)
>  {
>  	const char * const * usage_str = revert_or_cherry_pick_usage();
> @@ -112,6 +130,13 @@ static void parse_args(int argc, const char **argv)
>  	if (cmd_opts.commit_argc < 2)
>  		usage_with_options(usage_str, options);
>  
> +	if (cmd_opts.allow_ff) {
> +		int opt_bitarray[] = {cmd_opts.signoff, cmd_opts.no_commit,
> +				      cmd_opts.no_replay, cmd_opts.edit};
> +		die_opt_incompatible(me, "--ff", 4, opt_bitarray, "--signoff",
> +				"--no-commit", "-x", "--edit");
> +	}
Why not do it like this instead?
	struct incompatible {
        	unsigned option_bit;
                const char *option_name;
	} incompatible[] = {
		{ opts->signoff, "--signoff" },
                { opts->no_commit, "--no-commit" },
                ...
	};
	verify_compatible("me", "--ff", incompatible, ARRAY_SIZE(incompatible));

Or if you are shooting for ease-of-use, it might make sense to do it like this:

	verify_compatible("me", "--ff",
        		"--signoff", opts->signoff,
                        "--no-commit", opts->no_commit,
                        ...
                        NULL);

and make verify_compatible() a varargs function that takes two optional arguments at a time, i.e. const char *, followed by an int. Then there is no need for extra "int opt_bitarray[]" or "struct incompatible".

That would justify use of varargs, I think.
Ramkumar Ramachandra· May 8, 2011, 11:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/11] revert: Catch incompatible command-line options early

Hi Junio,
Junio C Hamano writes:
Show 38 quoted lines
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> > @@ -112,6 +130,13 @@ static void parse_args(int argc, const char **argv)
> >  	if (cmd_opts.commit_argc < 2)
> >  		usage_with_options(usage_str, options);
> >  
> > +	if (cmd_opts.allow_ff) {
> > +		int opt_bitarray[] = {cmd_opts.signoff, cmd_opts.no_commit,
> > +				      cmd_opts.no_replay, cmd_opts.edit};
> > +		die_opt_incompatible(me, "--ff", 4, opt_bitarray, "--signoff",
> > +				"--no-commit", "-x", "--edit");
> > +	}
> 
> Why not do it like this instead?
> 
> 	struct incompatible {
>         	unsigned option_bit;
>                 const char *option_name;
> 	} incompatible[] = {
> 		{ opts->signoff, "--signoff" },
>                 { opts->no_commit, "--no-commit" },
>                 ...
> 	};
> 	verify_compatible("me", "--ff", incompatible, ARRAY_SIZE(incompatible));
> 
> Or if you are shooting for ease-of-use, it might make sense to do it like
> this:
> 
> 	verify_compatible("me", "--ff",
>         		"--signoff", opts->signoff,
>                         "--no-commit", opts->no_commit,
>                         ...
>                         NULL);
> 
> and make verify_compatible() a varargs function that takes two optional
> arguments at a time, i.e. const char *, followed by an int.  Then there is
> no need for extra "int opt_bitarray[]" or "struct incompatible".
> 
> That would justify use of varargs, I think.

Now that you point it out, my original approach was unnecessarily cryptic and convoluted. I've followed this approach in my new series, and kept varargs -- the code looks much prettier now. Thanks :)

I can't justify changing the name from "die_opt_incompatible" to "verify_compatible" though; the name I've chosen seems to be more appropriate/ descriptive. Further, I think command-line parsing should always be the toplevel caller, and "die" is therefore more appropriate than "return error" here.

-- Ram
Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

[PATCH 06/11] revert: Implement parsing --continue, --abort and --skip

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 45 insertions(+), 1 deletions(-)
Show changes to builtin/revert.c +45 −1
diff --git a/builtin/revert.c b/builtin/revert.c
index facae24..b90f3d0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -39,6 +39,11 @@ static const char * const cherry_pick_usage[] = {
 static struct {
 	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;
@@ -98,8 +103,12 @@ static void die_opt_incompatible(const char *me, const char *base_opt,
 static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage();
+	const char *me;
 	int noop;
 	struct option options[] = {
+		OPT_BOOLEAN(0, "abort", &cmd_opts.abort_oper, "abort the current operation"),
+		OPT_BOOLEAN(0, "skip", &cmd_opts.skip_oper, "skip the current commit"),
+		OPT_BOOLEAN(0, "continue", &cmd_opts.continue_oper, "continue the current operation"),
 		OPT_BOOLEAN('n', "no-commit", &cmd_opts.no_commit, "don't automatically commit"),
 		OPT_BOOLEAN('e', "edit", &cmd_opts.edit, "edit the commit message"),
 		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
@@ -127,7 +136,42 @@ static void parse_args(int argc, const char **argv)
 	cmd_opts.commit_argc = parse_options(argc, argv, NULL, options, usage_str,
 				    PARSE_OPT_KEEP_ARGV0 |
 				    PARSE_OPT_KEEP_UNKNOWN);
-	if (cmd_opts.commit_argc < 2)
+
+	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
+
+	/* Check for incompatible command line arguments */
+	if (cmd_opts.abort_oper || cmd_opts.skip_oper || cmd_opts.continue_oper) {
+		char *this_oper;
+		int opt_bitarray[] = {cmd_opts.no_commit, cmd_opts.edit, noop,
+				      cmd_opts.signoff, cmd_opts.mainline,
+				      (cmd_opts.strategy ? 1 : 0),
+				      (cmd_opts.xopts ? 1 : 0),
+				      cmd_opts.no_replay, cmd_opts.allow_ff};
+		if (cmd_opts.abort_oper) {
+			this_oper = "--abort";
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.skip_oper, "--skip");
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.continue_oper, "--continue");
+		} else if (cmd_opts.skip_oper) {
+			this_oper = "--skip";
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.abort_oper, "--abort");
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.continue_oper, "--continue");
+		} else {
+			this_oper = "--continue";
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.abort_oper, "--abort");
+			die_opt_incompatible(me, this_oper, 1,
+					&cmd_opts.skip_oper, "--skip");
+		}
+		die_opt_incompatible(me, this_oper, 9, opt_bitarray, "--no-commit",
+				"--edit", "-r",	"--signoff", "--mainline",
+				"--strategy", "--strategy-option", "-x", "--ff");
+	}
+
+	else if (cmd_opts.commit_argc < 2)
 		usage_with_options(usage_str, options);
 
 	if (cmd_opts.allow_ff) {
-- 
1.7.4.rc1.7.g2cf08.dirty
Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

[PATCH 07/11] revert: Handle conflict resolutions more elegantly

Avoid calling die; return error instead.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 advice.c         |   14 ++++++++++++++
 advice.h         |    1 +
 builtin/revert.c |   40 ++++++++++++++++++----------------------
 3 files changed, 33 insertions(+), 22 deletions(-)
Show changes to 3 files +33 −22

advice.c, advice.h, builtin/revert.c

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 b90f3d0..e0352d4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -344,25 +344,20 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static int error_dirty_index()
+static int verify_resolution(const char *me)
 {
-	if (read_cache_unmerged()) {
-		die_resolve_conflict(cmd_opts.action == REVERT ? "revert" : "cherry-pick");
-	} else {
-		if (advice_commit_before_merge) {
-			if (cmd_opts.action == REVERT)
-				return error(_("Your local changes would be overwritten by revert.\n"
-					  "Please, commit your changes or stash them to proceed."));
-			else
-				return error(_("Your local changes would be overwritten by cherry-pick.\n"
-					  "Please, commit your changes or stash them to proceed."));
-		} else {
-			if (cmd_opts.action == REVERT)
-				return error(_("Your local changes would be overwritten by revert.\n"));
-			else
-				return error(_("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)
@@ -467,8 +462,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;
 	int res;
 
+	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
 	if (cmd_opts.no_commit) {
 		/*
 		 * We do not intend to commit immediately.  We just want to
@@ -481,8 +478,8 @@ static int do_pick_commit(struct commit *commit)
 	} else {
 		if (get_sha1("HEAD", head))
 			return error(_("You do not have a valid HEAD"));
-		if (index_differs_from("HEAD", 0))
-			return error_dirty_index();
+		if (index_differs_from("HEAD", 0) && !verify_resolution(me))
+			return error_dirty_worktree(me);
 	}
 	discard_cache();
 
@@ -521,8 +518,7 @@ static int do_pick_commit(struct commit *commit)
 		/* TRANSLATORS: The first %s will be "revert" or
 		   "cherry-pick", the second %s a SHA1 */
 		return error(_("%s: cannot parse parent commit %s"),
-			cmd_opts.action == REVERT ? "revert" : "cherry-pick",
-			sha1_to_hex(parent->object.sha1));
+			me, sha1_to_hex(parent->object.sha1));
 
 	if (get_message(commit, commit->buffer, &msg) != 0)
 		return error(_("Cannot get commit message for %s"),
-- 
1.7.4.rc1.7.g2cf08.dirty
Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

[PATCH 08/11] usage: Introduce error_errno correspoding to die_errno

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-compat-util.h |    2 ++
 usage.c           |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)
Show changes to 2 files +36 −0

git-compat-util.h, usage.c

diff --git a/git-compat-util.h b/git-compat-util.h
index 40498b3..00d2815 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -240,10 +240,12 @@ extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern NORETURN void die_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
+extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params));
+extern void set_error_routine(void (*routine)(const char *err, va_list params));
 
 extern int prefixcmp(const char *str, const char *prefix);
 extern int suffixcmp(const char *str, const char *suffix);
diff --git a/usage.c b/usage.c
index b5e67e3..8724b41 100644
--- a/usage.c
+++ b/usage.c
@@ -46,6 +46,11 @@ void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list param
 	die_routine = routine;
 }
 
+void set_error_routine(void (*routine)(const char *err, va_list params))
+{
+	error_routine = routine;
+}
+
 void NORETURN usagef(const char *err, ...)
 {
 	va_list params;
@@ -97,6 +102,35 @@ void NORETURN die_errno(const char *fmt, ...)
 	va_end(params);
 }
 
+int error_errno(const char *fmt, ...)
+{
+	va_list params;
+	char fmt_with_err[1024];
+	char str_error[256], *err;
+	int i, j;
+
+	err = strerror(errno);
+	for (i = j = 0; err[i] && j < sizeof(str_error) - 1; ) {
+		if ((str_error[j++] = err[i++]) != '%')
+			continue;
+		if (j < sizeof(str_error) - 1) {
+			str_error[j++] = '%';
+		} else {
+			/* No room to double the '%', so we overwrite it with
+			 * '\0' below */
+			j--;
+			break;
+		}
+	}
+	str_error[j] = 0;
+	snprintf(fmt_with_err, sizeof(fmt_with_err), "%s: %s", fmt, str_error);
+
+	va_start(params, fmt);
+	error_routine(fmt_with_err, params);
+	va_end(params);
+	return -1;
+}
+
 int error(const char *err, ...)
 {
 	va_list params;
-- 
1.7.4.rc1.7.g2cf08.dirty
Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

[PATCH 09/11] revert: Write head, todo, done files

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   94 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 88 insertions(+), 6 deletions(-)
Show changes to builtin/revert.c +88 −6
diff --git a/builtin/revert.c b/builtin/revert.c
index e0352d4..25969a5 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
@@ -643,21 +651,95 @@ static int read_and_refresh_cache(void)
 	return 0;
 }
 
+static void format_todo(struct strbuf *buf, struct commit_list *list)
+{
+	struct commit_list *cur = NULL;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	const char *sha1 = NULL;
+	const char *action;
+
+	action = (cmd_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))
+			die("Cannot get commit message for %s", sha1);
+
+		strbuf_addf(buf, "%s %s %s\n", action, sha1, msg.subject);
+	}
+}
+
+static int persist_initialize(unsigned char *head)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777))
+		return error_errno(_("Could not create sequencer directory '%s'"), SEQ_PATH);
+
+	if ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0)
+		return error_errno(_("Could not open '%s' for writing"), HEAD_FILE);
+
+	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 strbuf buf = STRBUF_INIT;
+	int fd;
+
+	if (!res)
+		return 0;
+
+	/* TODO file */
+	if ((fd = open(TODO_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0)
+		return error_errno(_("Could not open '%s' for writing"), TODO_FILE);
+
+	format_todo(&buf, todo_list);
+	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)
+		return error_errno(_("Could not open '%s' for writing"), DONE_FILE);
+
+	format_todo(&buf, done_list);
+	write_or_whine(fd, buf.buf, buf.len, DONE_FILE);
+	close(fd);
+	strbuf_release(&buf);
+	return res;
+}
+
 static int pick_commits(void)
 {
+	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()) ||
-		(res = prepare_revs(&revs)))
+		(res = prepare_revs(&revs)) ||
+		(res = persist_initialize(head)))
 		return res;
 
-	while ((commit = get_revision(&revs)) &&
-		!(res = do_pick_commit(commit)))
-		;
-
-	return res;
+	while ((commit = get_revision(&revs))) {
+		if (!(res = do_pick_commit(commit)))
+			commit_list_insert(commit, &done_list);
+		else {
+			commit_list_insert(commit, &revs.commits);
+			break;
+		}
+	}
+	return persist_todo_done(res, revs.commits, done_list);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
-- 
1.7.4.rc1.7.g2cf08.dirty
Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

[PATCH 10/11] revert: Give noop a default value while argument parsing

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to builtin/revert.c +2 −1
diff --git a/builtin/revert.c b/builtin/revert.c
index 25969a5..0720721 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -112,7 +112,8 @@ static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage();
 	const char *me;
-	int noop;
+	int noop = 0;
+
 	struct option options[] = {
 		OPT_BOOLEAN(0, "abort", &cmd_opts.abort_oper, "abort the current operation"),
 		OPT_BOOLEAN(0, "skip", &cmd_opts.skip_oper, "skip the current commit"),
-- 
1.7.4.rc1.7.g2cf08.dirty
Ramkumar Ramachandra· Apr 10, 2011, 15:11 UTC · re: Ramkumar Ramachandra · lore

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

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 47 insertions(+), 4 deletions(-)
Show changes to builtin/revert.c +47 −4
diff --git a/builtin/revert.c b/builtin/revert.c
index 0720721..c050d4f 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -743,6 +743,51 @@ static int pick_commits(void)
 	return persist_todo_done(res, revs.commits, done_list);
 }
 
+static int process_args(int argc, const char **argv)
+{
+	const char *me;
+	int fd;
+
+	parse_args(argc, argv);
+	me = (cmd_opts.action == REVERT ? "revert" : "cherry-pick");
+	if (cmd_opts.abort_oper) {
+		const char *args[3] = {NULL, NULL, NULL};
+		char head[DEFAULT_ABBREV];
+
+		args[0] = "rerere";
+		args[1] = "clear";
+		cmd_rerere(2, args, NULL);
+
+		if (!file_exists(HEAD_FILE))
+			goto error;
+
+		if ((fd = open(HEAD_FILE, O_RDONLY, 0666)) < 0)
+			return error_errno(_("Could not open '%s' for reading"), HEAD_FILE);
+		if (xread(fd, head, DEFAULT_ABBREV) < DEFAULT_ABBREV)
+			return error_errno(_("Corrupt '%s'"), HEAD_FILE);
+		close(fd);
+
+		args[0] = "reset";
+		args[1] = "--hard";
+		args[2] = head;
+		return cmd_reset(3, args, NULL);
+	}
+	else if (cmd_opts.skip_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+	else if (cmd_opts.continue_oper) {
+		if (!file_exists(TODO_FILE))
+			goto error;
+		return 0;
+	}
+
+	return pick_commits();
+error:
+	return error(_("No %s in progress"), me);
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	if (isatty(0))
@@ -752,9 +797,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	memset(&cmd_opts, 0, sizeof(cmd_opts));
 	cmd_opts.action = REVERT;
 	setenv(GIT_REFLOG_ACTION, "revert", 0);
-	parse_args(argc, argv);
 
-	return pick_commits();
+	return process_args(argc, argv);
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
@@ -763,7 +807,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	memset(&cmd_opts, 0, sizeof(cmd_opts));
 	cmd_opts.action = CHERRY_PICK;
 	setenv(GIT_REFLOG_ACTION, "cherry-pick", 0);
-	parse_args(argc, argv);
 
-	return pick_commits();
+	return process_args(argc, argv);
 }
-- 
1.7.4.rc1.7.g2cf08.dirty
Daniel Barkalow· Apr 10, 2011, 19:33 UTC · re: Ramkumar Ramachandra · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

On Sun, 10 Apr 2011, Ramkumar Ramachandra wrote:
Show 8 quoted lines
> Hi,
> 
> I've started working on building a sequencer for Git.  While the
> outline is described in [1], I'd like some additional clarifications.
> A big thanks to Christian's series [2] for the valuable roadmap.
> 
> Please note that 10/11 is not related to this series, but seems to be
> a minor nit that's required to make all existing tests pass.

That looks like an actual bug that only doesn't matter currently because the function is never called with enough junk on the stack.

> 0. Is the general flow alright?

I suspect it would be easier to review some of this with certain things squashed together; one patch that changes all of the variable references to what you want them to be is easier to understand than one that moves statics to function arguments, one that moves statics to struct fields, etc. Likewise, when you're converting some of the die() calls to error(), it's easier to review the patch if all of the die() calls that aren't changed in that patch don't get changed later in the series.

> 1. Is it okay to use this kind of adaptive error handling (calling
> 'die' in some places and returning error in other places), or should
> it be more uniform?

I think it should be systematic but not necessarily uniform. You should be able to give a guideline as to how to decide which to use (and you should probably actually give the guideline, so future developers make consistent choices). I think of "die" as being ideally for situations where the program can't understand what has happened well enough to know what to do about it.

Show 7 quoted lines
> 2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
> inelegant, mainly because of the command-line argument parsing
> overhead.  Re-implementing it using more low-level functions doesn't
> seem to be the way to go either: for example, 'reset --hard' has some
> additional logic of writing HEAD and ORIG_HEAD, which I don't want to
> duplicate.  Should I work on reworking parts of 'rerere.c' and
> 'revert.c', or is there some other way?
(ITYM cmd_reset here)

I think rerere.c should get a rerere_clear(). I think it would make sense to implement the reset locally; the abort ought to be undoing exactly those things that you did, and I'm not actually sure the ORIG_HEAD is entirely appropriate. You ought to be able to use cleanup functions that correspond to the functions you used to make the mess in the first place.

Show 6 quoted lines
> 3. From the format of the TODO and DONE files, one more thing should
> be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> format.  This part will go into the sequencer.  Then I'll use a
> cherry-pick specific file to keep the command-line options.  Yes, I'm
> trying to work on Daniel's idea [3] from the very start.  Is this a
> good idea?
I certainly like it. :)
> Thanks for reading.
You're welcome. :)
	-Daniel
*This .sig left intentionally blank*
Ramkumar Ramachandra· Apr 11, 2011, 08:55 UTC · re: Daniel Barkalow · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

Hi Daniel,
Daniel Barkalow writes:
Show 5 quoted lines
> > Please note that 10/11 is not related to this series, but seems to be
> > a minor nit that's required to make all existing tests pass.
> 
> That looks like an actual bug that only doesn't matter currently because 
> the function is never called with enough junk on the stack.

Fixing this bug was one is one of the hidden motives of the patch I send out just afterward [1].

Show 9 quoted lines
> > 0. Is the general flow alright?
> 
> I suspect it would be easier to review some of this with certain things 
> squashed together; one patch that changes all of the variable references 
> to what you want them to be is easier to understand than one that moves 
> statics to function arguments, one that moves statics to struct fields, 
> etc. Likewise, when you're converting some of the die() calls to error(), 
> it's easier to review the patch if all of the die() calls that aren't 
> changed in that patch don't get changed later in the series.

Agreed -- I'll post a reworked series shortly. Jonathan was finding it hard to follow as well.

Show 10 quoted lines
> > 1. Is it okay to use this kind of adaptive error handling (calling
> > 'die' in some places and returning error in other places), or should
> > it be more uniform?
> 
> I think it should be systematic but not necessarily uniform. You should be 
> able to give a guideline as to how to decide which to use (and you should 
> probably actually give the guideline, so future developers make consistent 
> choices). I think of "die" as being ideally for situations where the 
> program can't understand what has happened well enough to know what to do 
> about it.
Jonathan concurs.  Where should I document this?
Show 9 quoted lines
> > 2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
> > inelegant, mainly because of the command-line argument parsing
> > overhead.  Re-implementing it using more low-level functions doesn't
> > seem to be the way to go either: for example, 'reset --hard' has some
> > additional logic of writing HEAD and ORIG_HEAD, which I don't want to
> > duplicate.  Should I work on reworking parts of 'rerere.c' and
> > 'revert.c', or is there some other way?
> 
> (ITYM cmd_reset here)
Yeah, sorry.
Show 5 quoted lines
> I think rerere.c should get a rerere_clear(). I think it would make sense 
> to implement the reset locally; the abort ought to be undoing exactly 
> those things that you did, and I'm not actually sure the ORIG_HEAD is 
> entirely appropriate. You ought to be able to use cleanup functions that 
> correspond to the functions you used to make the mess in the first place.

Good suggestion -- rerere_clear is done [2]; now I have to figure out if it makes sense to write a library for reset.

-- Ram

[1]: http://article.gmane.org/gmane.comp.version-control.git/171267 [2]: http://article.gmane.org/gmane.comp.version-control.git/171314

Jonathan Nieder· Apr 10, 2011, 19:47 UTC · re: Ramkumar Ramachandra · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

Hi again,
Ramkumar Ramachandra wrote:
> I've started working on building a sequencer for Git.
Happy to see.
> 0. Is the general flow alright?

Not sure --- I don't have the big picture. Could you give a quick summary of the flow in the cover letter ("patch 1 does such-and-such, so patch 2 can do such-and-such, leading to...") to the next revision, and quick explanations of the purpose (i.e., why we should want each change) in the individual change descriptions?

> 1. Is it okay to use this kind of adaptive error handling (calling
> 'die' in some places and returning error in other places), or should
> it be more uniform?
'die' gets used in two ways (well, one way really):
 - to say "there is no sane way to recover from this failure".  For
   example, xmalloc dies if even after trying to free up memory,
   malloc still could not satisfy the request.
 - to say "so far we've been too lazy to implement recovery from
   this failure".  Or "while we *could* recover from this failure, no
   one has needed it, so let's not --- that code would just bitrot."

Therefore a mixture of 'die' and 'return error' seems inevitable. The dangerous mixtures to avoid are those likely to trip up callers (e.g., if all code paths 'return error' except one edge case).

Show 7 quoted lines
> 2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
> inelegant, mainly because of the command-line argument parsing
> overhead.  Re-implementing it using more low-level functions doesn't
> seem to be the way to go either: for example, 'reset --hard' has some
> additional logic of writing HEAD and ORIG_HEAD, which I don't want to
> duplicate.  Should I work on reworking parts of 'rerere.c' and
> 'revert.c', or is there some other way?

See "git log --grep=libify" for examples. Isn't rerere already libified? Ah, you need "rerere clear" --- I think a rerere_clear function alongside rerere_forget et al would make sense.

More generally, it should be feasible to expose a nice, simple API for any functionality you need (with params struct if necessary, etc). That's how many of the current APIs (revision walking, diffcore, ...) came about.

Show 6 quoted lines
> 3. From the format of the TODO and DONE files, one more thing should
> be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> format.  This part will go into the sequencer.  Then I'll use a
> cherry-pick specific file to keep the command-line options.  Yes, I'm
> trying to work on Daniel's idea [3] from the very start.  Is this a
> good idea?

This is still bouncing in my head. I think I like it --- is the idea that some day you could put commands like

	am topic.mbox

in your insn sheet, or do nested rebases with a --force-nested option? That does sound useful. How would one request "skip to the next operation in the outer rebase" on the command line? This is starting to feel like a debugger.

> 4. I have a feeling that I've broken translation strings.  Is there a
> README, plus a bunch of tests I can run to make sure that I've not
> broken anything?

If you put "GETTEXT_POISON = YesPlease" in your config.mak, the translations will be translated to gibberish when the GIT_GETTEXT_POISON envvar is set, so you can use the test suite as a sanity check. "make pot" can be used to get the translation template that translators will see.

As for a readme explaining how to use _, N_, and Q_, yes, I think that would be useful. I think Ævar's series has something like that (but targetted at translators) later on; it might make sense to prod him or me for a simpler explanation can be merged immediately. Until then, there is "git log gettext.h".

Regards, Jonathan

Daniel Barkalow· Apr 11, 2011, 01:16 UTC · re: Jonathan Nieder · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

On Sun, 10 Apr 2011, Jonathan Nieder wrote:
Show 16 quoted lines
> > 3. From the format of the TODO and DONE files, one more thing should
> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > format.  This part will go into the sequencer.  Then I'll use a
> > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > trying to work on Daniel's idea [3] from the very start.  Is this a
> > good idea?
> 
> This is still bouncing in my head.  I think I like it --- is the idea
> that some day you could put commands like
> 
> 	am topic.mbox
> 
> in your insn sheet, or do nested rebases with a --force-nested option?
> That does sound useful.  How would one request "skip to the next
> operation in the outer rebase" on the command line?  This is starting
> to feel like a debugger.

The most basic idea is that, in the case of a rebase where a patch fails to apply, there are actually two levels of operation that have to be deferred until after the human intervention: making the commit where the pick failed, and applying further patches. If there are going to be multiple higher-level operations (e.g., rebase and rebase -i), they should be able to share the completion of the pick operation, and a basic "git cherry-pick <sha1>" ought to be able to share it, too, so you can finish that up without having to remember what the command is. People should be able to implement custom higher-level operations (e.g., "cherry-pick all the commits attached to bugs in bugzilla that block the release tracker" or something), they should be able to rely on library code that knows how to finish the conflicted cherry-pick. (Another interesting example might be "reconstruct the linux-next tree", which is a series of merges, and which should have --continue share the post-intervention code from the single merge process before going on to do the custom higher-level thing.)

My feeling is that "--skip" is actually "abort the pick, but continue the rebase". I suppose there could be more than two levels, and people could want to skip a higher-level chunk, but that's something to get to when someone actually wants it; we've obviously already got the two-level situation now, so we can implement that.

	-Daniel
*This .sig left intentionally blank*
Jonathan Nieder· Apr 11, 2011, 06:42 UTC · re: Daniel Barkalow · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

Daniel Barkalow wrote:
> My feeling is that "--skip" is actually "abort the pick, but continue the 
> rebase". I suppose there could be more than two levels, and people could 
> want to skip a higher-level chunk, but that's something to get to when 
> someone actually wants it
Ah, I think I see better now.  What was confusing was
	git cherry-pick foo..bar

for which --skip would mean "abort the pick, but continue the pick". It fits very well into your story. There are two levels: "atomic" (for lack of a better word) operations, like a single cherry-pick, and sequences of operations.

(As for the more-than-two-levels story, I want it, but I agree with you that that is tangential to this project. I find myself attempting to go back and rearrange earlier commits during an interactive rebase very often.)

Ramkumar Ramachandra· Apr 11, 2011, 09:07 UTC · re: Jonathan Nieder · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

Hi Jonathan,
Jonathan Nieder writes:
Show 8 quoted lines
> Ramkumar Ramachandra wrote:
> > 0. Is the general flow alright?
> 
> Not sure --- I don't have the big picture.  Could you give a quick
> summary of the flow in the cover letter ("patch 1 does such-and-such,
> so patch 2 can do such-and-such, leading to...") to the next revision,
> and quick explanations of the purpose (i.e., why we should want each
> change) in the individual change descriptions?
Agreed.  I'll ask again after the next re-roll.
Show 17 quoted lines
> > 1. Is it okay to use this kind of adaptive error handling (calling
> > 'die' in some places and returning error in other places), or should
> > it be more uniform?
> 
> 'die' gets used in two ways (well, one way really):
> 
>  - to say "there is no sane way to recover from this failure".  For
>    example, xmalloc dies if even after trying to free up memory,
>    malloc still could not satisfy the request.
> 
>  - to say "so far we've been too lazy to implement recovery from
>    this failure".  Or "while we *could* recover from this failure, no
>    one has needed it, so let's not --- that code would just bitrot."
> 
> Therefore a mixture of 'die' and 'return error' seems inevitable.  The
> dangerous mixtures to avoid are those likely to trip up callers (e.g.,
> if all code paths 'return error' except one edge case).

My thoughts precisely. I was worried about what would happen in future when Git is completely lib'ified, but I suppose it makes little sense to think about that now.

Show 11 quoted lines
> > 2. In 11/11, I've used cmd_revert and cmd_rerere.  This is highly
> > inelegant, mainly because of the command-line argument parsing
> > overhead.  Re-implementing it using more low-level functions doesn't
> > seem to be the way to go either: for example, 'reset --hard' has some
> > additional logic of writing HEAD and ORIG_HEAD, which I don't want to
> > duplicate.  Should I work on reworking parts of 'rerere.c' and
> > 'revert.c', or is there some other way?
> 
> See "git log --grep=libify" for examples.  Isn't rerere already
> libified?  Ah, you need "rerere clear" --- I think a rerere_clear
> function alongside rerere_forget et al would make sense.
Done in [1].
> More generally, it should be feasible to expose a nice, simple API for
> any functionality you need (with params struct if necessary, etc).
> That's how many of the current APIs (revision walking, diffcore, ...)
> came about.
I see.  Okay, I'll see if it makes sense to craft an API for rebase.
Show 16 quoted lines
> > 3. From the format of the TODO and DONE files, one more thing should
> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > format.  This part will go into the sequencer.  Then I'll use a
> > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > trying to work on Daniel's idea [3] from the very start.  Is this a
> > good idea?
> 
> This is still bouncing in my head.  I think I like it --- is the idea
> that some day you could put commands like
> 
> 	am topic.mbox
> 
> in your insn sheet, or do nested rebases with a --force-nested option?
> That does sound useful.  How would one request "skip to the next
> operation in the outer rebase" on the command line?  This is starting
> to feel like a debugger.

I'll respond to this later in the thread, since Daniel has already said something. I just need help with crafting a nice instruction sheet now -- please join the discussion at [2].

Show 15 quoted lines
> > 4. I have a feeling that I've broken translation strings.  Is there a
> > README, plus a bunch of tests I can run to make sure that I've not
> > broken anything?
> 
> If you put "GETTEXT_POISON = YesPlease" in your config.mak, the
> translations will be translated to gibberish when the GIT_GETTEXT_POISON
> envvar is set, so you can use the test suite as a sanity check.
> "make pot" can be used to get the translation template that
> translators will see.
> 
> As for a readme explaining how to use _, N_, and Q_, yes, I think that
> would be useful.  I think Ævar's series has something like that (but
> targetted at translators) later on; it might make sense to prod him or
> me for a simpler explanation can be merged immediately.  Until then,
> there is "git log gettext.h".
Thanks!  Someone should really document this whole translations thing.
-- Ram

[1]: http://article.gmane.org/gmane.comp.version-control.git/171314 [2]: http://thread.gmane.org/gmane.comp.version-control.git/171255/focus=171307

Christian Couder· Apr 11, 2011, 03:18 UTC · re: Ramkumar Ramachandra · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:
> Hi,
> 
> I've started working on building a sequencer for Git.  

So you are starting the GSoC early! Great! When (or before) it really starts, just make sure you put your work on a public Git repository and you send status updates regularly (weekly if possible).

Show 6 quoted lines
> 3. From the format of the TODO and DONE files, one more thing should
> be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> format.  This part will go into the sequencer.  Then I'll use a
> cherry-pick specific file to keep the command-line options.  Yes, I'm
> trying to work on Daniel's idea [3] from the very start.  Is this a
> good idea?

I think that the TODO and DONE file format will need at one point to include options and it is simpler if this change is done early. Using a cherry-pick specific file to keep the options is not very generic for a sequencer that could be used for many things.

For example, as we have rebase --interactive, we will probably want to have cherry-pick --interactive, and when editing the TODO file we might want to use different cherry-pick options when picking different commits.

This would also make the different cherry-pick options available when using rebase --interactive once it uses the sequencer.

> [1]:
> http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170908
> [2]: http://thread.gmane.org/gmane.comp.version-control.git/162183 [3]:
> http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170834
[3] is missing here.

Thanks, Christian.

Ramkumar Ramachandra· Apr 11, 2011, 04:49 UTC · re: Christian Couder · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

Hi Christian,
Christian Couder writes:
Show 9 quoted lines
> On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:
> > Hi,
> > 
> > I've started working on building a sequencer for Git.  
> 
> So you are starting the GSoC early! Great!
> When (or before) it really starts, just make sure you put your work on a 
> public Git repository and you send status updates regularly (weekly if 
> possible).

Ofcourse. I've already discussed many of these issues last year [1]. The work corresponding to this particular series can be found in the 'sequencer' branch on my GitHub fork [2]. Since the results haven't been announced, and the coding period hasn't begun, this work should be treated like "normal work" -- I just wrote it this weekend.

Show 15 quoted lines
> > 3. From the format of the TODO and DONE files, one more thing should
> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > format.  This part will go into the sequencer.  Then I'll use a
> > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > trying to work on Daniel's idea [3] from the very start.  Is this a
> > good idea?
> 
> I think that the TODO and DONE file format will need at one point to include 
> options and it is simpler if this change is done early. Using a cherry-pick 
> specific file to keep the options is not very generic for a sequencer that could 
> be used for many things.
> 
> For example, as we have rebase --interactive, we will probably want to have 
> cherry-pick --interactive, and when editing the TODO file we might want to use 
> different cherry-pick options when picking different commits.

Point noted -- I shouldn't narrow down the various things I can do with a single commit early on and lock us into a more restrictive design. However, I'm not in favor of making it too generic; I certainly wouldn't like to edit an instruction sheet that looks like this:

cherry-pick -m 1 -s -r 83a4fe9 revert -n 3a6fe42 cherry-pick -x --ff dacfe41 cherry-pick -s recursive -Xpatience b31d4e2

It'll become impossible to tell which options are disallowed over what else, and it'll become a nightmare to debug when something goes wrong. My idea is that we add commit-specific options in an optional backward-compatible manner later:

pick 83a4fe9 revert 3a6fe42 # -n pick dacfe41 # -s pick b31d4e2

That way, there'll be two sets of options:
1. One "global" set of command-line switches that applies
to all the commits, which will be written to a command-specific
location.  The sequencer itself knows nothing about this.
2. Optional commit-specific stuff that's passed in the
form of a (modified) commit_list to the sequencer API to write to the
todo/ done files.
Do you like this idea?
Show 9 quoted lines
> This would also make the different cherry-pick options available when using 
> rebase --interactive once it uses the sequencer.
> 
> > [1]:
> > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170908
> > [2]: http://thread.gmane.org/gmane.comp.version-control.git/162183 [3]:
> > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170834
> 
> [3] is missing here.

Your email client is perhaps wrapping too aggressively? It's fine in my original email [3].

-- Ram

[1]: http://thread.gmane.org/gmane.comp.version-control.git/142623/focus=142821 [2]: https://github.com/artagnon/git [2]: http://article.gmane.org/gmane.comp.version-control.git/171255

Christian Couder· Apr 11, 2011, 06:20 UTC · re: Ramkumar Ramachandra · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

Hi Ram,
On Monday 11 April 2011 06:49:05 Ramkumar Ramachandra wrote:
Show 16 quoted lines
> Hi Christian,
> 
> Christian Couder writes:
> > On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:
> > > Hi,
> > > 
> > > I've started working on building a sequencer for Git.
> > 
> > So you are starting the GSoC early! Great!
> > When (or before) it really starts, just make sure you put your work on a
> > public Git repository and you send status updates regularly (weekly if
> > possible).
> 
> Ofcourse.  I've already discussed many of these issues last year [1].
> The work corresponding to this particular series can be found in the
> 'sequencer' branch on my GitHub fork [2].  
Great! Thanks!
> Since the results haven't
> been announced, and the coding period hasn't begun, this work should
> be treated like "normal work" -- I just wrote it this weekend.
Ok.
Show 27 quoted lines
> > > 3. From the format of the TODO and DONE files, one more thing should
> > > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > > format.  This part will go into the sequencer.  Then I'll use a
> > > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > > trying to work on Daniel's idea [3] from the very start.  Is this a
> > > good idea?
> > 
> > I think that the TODO and DONE file format will need at one point to
> > include options and it is simpler if this change is done early. Using a
> > cherry-pick specific file to keep the options is not very generic for a
> > sequencer that could be used for many things.
> > 
> > For example, as we have rebase --interactive, we will probably want to
> > have cherry-pick --interactive, and when editing the TODO file we might
> > want to use different cherry-pick options when picking different
> > commits.
> 
> Point noted -- I shouldn't narrow down the various things I can do
> with a single commit early on and lock us into a more restrictive
> design.  However, I'm not in favor of making it too generic; I
> certainly wouldn't like to edit an instruction sheet that looks like
> this:
> 
> cherry-pick -m 1 -s -r 83a4fe9
> revert -n 3a6fe42
> cherry-pick -x --ff dacfe41
> cherry-pick -s recursive -Xpatience b31d4e2
I wouldn't like either, but I would like it even less if it was like this:

pick 83a4fe9 # -m 1 -s -r revert 3a6fe42 # -n pick dacfe41 # -x --ff pick b31d4e2 # -s recursive -Xpatience

I mean that of course people should not use too many options for no good reason, but if they do need to use some options, it's better if they are shown like in a shell, as they will be more familiar with them this way.

There is no point of making options look different to prevent people from abusing them.

Show 15 quoted lines
> It'll become impossible to tell which options are disallowed over what
> else, and it'll become a nightmare to debug when something goes wrong.
> My idea is that we add commit-specific options in an optional
> backward-compatible manner later:
> 
> pick 83a4fe9
> revert 3a6fe42 # -n
> pick dacfe41 # -s
> pick b31d4e2
> 
> That way, there'll be two sets of options:
> 
> 1. One "global" set of command-line switches that applies
> to all the commits, which will be written to a command-specific
> location.  The sequencer itself knows nothing about this.

I don't see the point of this global set. And if the sequencer knows nothing about it, the user may not know about it too and so may not understand how things work.

Show 20 quoted lines
> 2. Optional commit-specific stuff that's passed in the
> form of a (modified) commit_list to the sequencer API to write to the
> todo/ done files.
> 
> Do you like this idea?
> 
> > This would also make the different cherry-pick options available when
> > using rebase --interactive once it uses the sequencer.
> > 
> > > [1]:
> > > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170
> > > 908 [2]: http://thread.gmane.org/gmane.comp.version-control.git/162183
> > > [3]:
> > > http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=17
> > > 0834
> > 
> > [3] is missing here.
> 
> Your email client is perhaps wrapping too aggressively? It's fine in
> my original email [3].

Yeah, sorry about that! Christian.

Ramkumar Ramachandra· Apr 11, 2011, 10:48 UTC · re: Christian Couder · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

Hi Christian,
Christian Couder writes:
Show 42 quoted lines
> On Monday 11 April 2011 06:49:05 Ramkumar Ramachandra wrote:
> > > > 3. From the format of the TODO and DONE files, one more thing should
> > > > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > > > format.  This part will go into the sequencer.  Then I'll use a
> > > > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > > > trying to work on Daniel's idea [3] from the very start.  Is this a
> > > > good idea?
> > > 
> > > I think that the TODO and DONE file format will need at one point to
> > > include options and it is simpler if this change is done early. Using a
> > > cherry-pick specific file to keep the options is not very generic for a
> > > sequencer that could be used for many things.
> > > 
> > > For example, as we have rebase --interactive, we will probably want to
> > > have cherry-pick --interactive, and when editing the TODO file we might
> > > want to use different cherry-pick options when picking different
> > > commits.
> > 
> > Point noted -- I shouldn't narrow down the various things I can do
> > with a single commit early on and lock us into a more restrictive
> > design.  However, I'm not in favor of making it too generic; I
> > certainly wouldn't like to edit an instruction sheet that looks like
> > this:
> > 
> > cherry-pick -m 1 -s -r 83a4fe9
> > revert -n 3a6fe42
> > cherry-pick -x --ff dacfe41
> > cherry-pick -s recursive -Xpatience b31d4e2
> 
> I wouldn't like either, but I would like it even less if it was like this:
> 
> pick 83a4fe9 # -m 1 -s -r 
> revert 3a6fe42 # -n 
> pick dacfe41 # -x --ff 
> pick b31d4e2 # -s recursive -Xpatience 
> 
> I mean that of course people should not use too many options for no good 
> reason, but if they do need to use some options, it's better if they are shown 
> like in a shell, as they will be more familiar with them this way.
> 
> There is no point of making options look different to prevent people from 
> abusing them.

You're right -- no point obscuring things. I can't help thinking that there must be a more elegant representation. I'll think about it for a while, and try to come up with something.

Show 19 quoted lines
> > It'll become impossible to tell which options are disallowed over what
> > else, and it'll become a nightmare to debug when something goes wrong.
> > My idea is that we add commit-specific options in an optional
> > backward-compatible manner later:
> > 
> > pick 83a4fe9
> > revert 3a6fe42 # -n
> > pick dacfe41 # -s
> > pick b31d4e2
> > 
> > That way, there'll be two sets of options:
> > 
> > 1. One "global" set of command-line switches that applies
> > to all the commits, which will be written to a command-specific
> > location.  The sequencer itself knows nothing about this.
> 
> I don't see the point of this global set. And if the sequencer knows nothing 
> about it, the user may not know about it too and so may not understand how 
> things work.

Hm. I originally wanted this so that each commit in the instruction sheet isn't polluted with the same command-line options, but this doesn't seem to be a good solution.

-- Ram
Daniel Barkalow· Apr 11, 2011, 05:30 UTC · re: Christian Couder · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

On Mon, 11 Apr 2011, Christian Couder wrote:
Show 11 quoted lines
> > 3. From the format of the TODO and DONE files, one more thing should
> > be clear- I'm trying to stick to a slight variation of the 'rebase -i'
> > format.  This part will go into the sequencer.  Then I'll use a
> > cherry-pick specific file to keep the command-line options.  Yes, I'm
> > trying to work on Daniel's idea [3] from the very start.  Is this a
> > good idea?
> 
> I think that the TODO and DONE file format will need at one point to include 
> options and it is simpler if this change is done early. Using a cherry-pick 
> specific file to keep the options is not very generic for a sequencer that could 
> be used for many things.

My idea is that cherry-pick (or merge, or whatever) would have its own state to store what it was trying to do when it got a conflict. This is to enable the cherry-pick (etc) to complete without needing to know anything about what called it. The sequencer's information would keep track of what was left to do, what was already complete, and how to abort the whole sequence.

It would be nice to get the following commands to work:
$ git cherry-pick <sha1>
Automatic cherry-pick failed.  After resolving the conflicts,
mark the corrected paths with 'git add <paths>' or 'git rm <paths>' and 
run 'git continue'
$ (resolve conflicts)
$ git continue
Finished one cherry-pick.
[master sha1b] Subject
 2 files changes, 2 insertions(+)

And, of course, if this happened as a step in a rebase, the rebase would continue after the cherry-pick finished. But it's annoying that, in order to finish a conflicted "git cherry-pick <branch>", you currently need to go back and find the instruction that says to commit it yourself, with the option "-c <sha1>" to retain authorship and message. And if you want to abort it, you need to remember "git reset --hard HEAD" (and maybe you also want "git rerere clear"). That's what cherry-pick should be keeping its own state for. This is also parallel to how "merge" works, with its information kept in private state files, including some things like the list of files that had conflicts which isn't trivial to reproduce after the user has resolved them.

	-Daniel
*This .sig left intentionally blank*
Jonathan Nieder· Apr 11, 2011, 05:38 UTC · re: Daniel Barkalow · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

Hi,
Daniel Barkalow wrote:
> But it's annoying that, in order 
> to finish a conflicted "git cherry-pick <branch>", you currently need to 
> go back and find the instruction that says to commit it yourself, with the 
> option "-c <sha1>" to retain authorship and message.

You might like v1.7.5-rc0~88^2 (Teach commit about CHERRY_PICK_HEAD, 2011-02-19).

> And if you want to 
> abort it, you need to remember "git reset --hard HEAD" (and maybe you also 
> want "git rerere clear").

Hm, I had assumed reset --hard (or "git reset --merge HEAD") would take care of the "rerere clear" but it seems that indeed it doesn't.

Daniel Barkalow· Apr 11, 2011, 06:34 UTC · re: Jonathan Nieder · lore

Re: [RFC PATCH 00/11] Sequencer Foundations

On Mon, 11 Apr 2011, Jonathan Nieder wrote:
Show 11 quoted lines
> Hi,
> 
> Daniel Barkalow wrote:
> 
> > But it's annoying that, in order 
> > to finish a conflicted "git cherry-pick <branch>", you currently need to 
> > go back and find the instruction that says to commit it yourself, with the 
> > option "-c <sha1>" to retain authorship and message.
> 
> You might like v1.7.5-rc0~88^2 (Teach commit about CHERRY_PICK_HEAD,
> 2011-02-19).

Ah, so I'm actually proposing that we not revert that series, as far as having cherry-pick-specific state is concerned. :) On the other hand, I'd like to have the logic for using that state be in the cherry-pick implementation, rather than having commit.c need to understand merge and cherry-pick (and revert, and applying a patch from email, and...). Which is to say, there should be a core state file that ends up with "cherry-pick" in it, and revert.c has registered that it handles that, and commit.c should call the registered function to use the saved state.

Also, git-rebase should be able to take advantage of the fact that other code knows how to start a cherry-pick, find that it has a conflict, tell the user, and use saved information to finish it.

Show 6 quoted lines
> > And if you want to 
> > abort it, you need to remember "git reset --hard HEAD" (and maybe you also 
> > want "git rerere clear").
> 
> Hm, I had assumed reset --hard (or "git reset --merge HEAD") would
> take care of the "rerere clear" but it seems that indeed it doesn't.
In fact, Ram's series makes sure to call both, which is how I knew.
	-Daniel
*This .sig left intentionally blank*

← back to recent threads