[PATCH 0/5] add a --strategy option to "git cherry-pick"

[PATCH 0/5] add a --strategy option to "git cherry-pick"

[Christian Couder]

The purpose of this series is to make it possible to test how "git cherry-pick"
would work with another merge strategy than the recursive one.

Junio suggested:

"It might not be a bad idea to teach a hidden primarily-for-debugging
option to "cherry-pick" to let it use resolve instead of recursive for
cases like this."

The changes since the previous RFC version are the following:

- the commit messages of the first 2 patches have been improved a little,
- some calls to free_commit_list() have been added to the last patch,
- the series was rebased recently on top of next.

Christian Couder (5):
  revert: use strbuf to refactor the code that writes the merge message
  revert: refactor merge recursive code into its own function
  merge: refactor code that calls "git merge-STRATEGY"
  merge: make function try_merge_command non static
  revert: add "--strategy" option to choose merge strategy

 builtin/merge.c   |   81 +++++++++++++-----------
 builtin/revert.c  |  179 +++++++++++++++++++++++++++++++----------------------
 cache.h           |    3 -
 merge-recursive.h |    4 +
 4 files changed, 154 insertions(+), 113 deletions(-)

[PATCH 1/5] revert: use strbuf to refactor the code that writes the merge message

[Christian Couder]

The code in this commit was written by Stephan Beyer for the sequencer
GSoC project:

    git://repo.or.cz/git/sbeyer.git

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   69 ++++++++++++++++++++++++++---------------------------
 1 files changed, 34 insertions(+), 35 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 778a56e..b8ee045 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -169,28 +169,17 @@ static char *get_encoding(const char *message)
 	return NULL;
 }
 
-static struct lock_file msg_file;
-static int msg_fd;
-
-static void add_to_msg(const char *string)
-{
-	int len = strlen(string);
-	if (write_in_full(msg_fd, string, len) < 0)
-		die_errno ("Could not write to MERGE_MSG");
-}
-
-static void add_message_to_msg(const char *message)
+static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
 {
 	const char *p = message;
 	while (*p && (*p != '\n' || p[1] != '\n'))
 		p++;
 
 	if (!*p)
-		add_to_msg(sha1_to_hex(commit->object.sha1));
+		strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
 
 	p += 2;
-	add_to_msg(p);
-	return;
+	strbuf_addstr(msgbuf, p);
 }
 
 static void set_author_ident_env(const char *message)
@@ -266,6 +255,19 @@ static char *help_msg(const char *name)
 	return strbuf_detach(&helpbuf, NULL);
 }
 
+static void write_message(struct strbuf *msgbuf, const char *filename)
+{
+	static struct lock_file msg_file;
+
+	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
+					       LOCK_DIE_ON_ERROR);
+	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
+		die_errno("Could not write to %s.", filename);
+	strbuf_release(msgbuf);
+	if (commit_lock_file(&msg_file) < 0)
+		die("Error wrapping up %s", filename);
+}
+
 static struct tree *empty_tree(void)
 {
 	struct tree *tree = xcalloc(1, sizeof(struct tree));
@@ -311,6 +313,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	struct merge_options o;
 	struct tree *result, *next_tree, *base_tree, *head_tree;
 	static struct lock_file index_lock;
+	struct strbuf msgbuf = STRBUF_INIT;
 
 	git_config(git_default_config, NULL);
 	me = action == REVERT ? "revert" : "cherry-pick";
@@ -398,8 +401,6 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	 */
 
 	defmsg = git_pathdup("MERGE_MSG");
-	msg_fd = hold_lock_file_for_update(&msg_file, defmsg,
-					   LOCK_DIE_ON_ERROR);
 
 	index_fd = hold_locked_index(&index_lock, 1);
 
@@ -408,27 +409,27 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		base_label = msg.label;
 		next = parent;
 		next_label = msg.parent_label;
-		add_to_msg("Revert \"");
-		add_to_msg(msg.subject);
-		add_to_msg("\"\n\nThis reverts commit ");
-		add_to_msg(sha1_to_hex(commit->object.sha1));
+		strbuf_addstr(&msgbuf, "Revert \"");
+		strbuf_addstr(&msgbuf, msg.subject);
+		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
+		strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 
 		if (commit->parents->next) {
-			add_to_msg(", reversing\nchanges made to ");
-			add_to_msg(sha1_to_hex(parent->object.sha1));
+			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
+			strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
 		}
-		add_to_msg(".\n");
+		strbuf_addstr(&msgbuf, ".\n");
 	} else {
 		base = parent;
 		base_label = msg.parent_label;
 		next = commit;
 		next_label = msg.label;
 		set_author_ident_env(msg.message);
-		add_message_to_msg(msg.message);
+		add_message_to_msg(&msgbuf, msg.message);
 		if (no_replay) {
-			add_to_msg("(cherry picked from commit ");
-			add_to_msg(sha1_to_hex(commit->object.sha1));
-			add_to_msg(")\n");
+			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+			strbuf_addstr(&msgbuf, ")\n");
 		}
 	}
 
@@ -453,27 +454,25 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	rollback_lock_file(&index_lock);
 
 	if (!clean) {
-		add_to_msg("\nConflicts:\n\n");
+		strbuf_addstr(&msgbuf, "\nConflicts:\n\n");
 		for (i = 0; i < active_nr;) {
 			struct cache_entry *ce = active_cache[i++];
 			if (ce_stage(ce)) {
-				add_to_msg("\t");
-				add_to_msg(ce->name);
-				add_to_msg("\n");
+				strbuf_addch(&msgbuf, '\t');
+				strbuf_addstr(&msgbuf, ce->name);
+				strbuf_addch(&msgbuf, '\n');
 				while (i < active_nr && !strcmp(ce->name,
 						active_cache[i]->name))
 					i++;
 			}
 		}
-		if (commit_lock_file(&msg_file) < 0)
-			die ("Error wrapping up %s", defmsg);
+		write_message(&msgbuf, defmsg);
 		fprintf(stderr, "Automatic %s failed.%s\n",
 			me, help_msg(commit_name));
 		rerere(allow_rerere_auto);
 		exit(1);
 	}
-	if (commit_lock_file(&msg_file) < 0)
-		die ("Error wrapping up %s", defmsg);
+	write_message(&msgbuf, defmsg);
 	fprintf(stderr, "Finished one %s.\n", me);
 
 	/*
-- 
1.7.0.3.454.gd9aeb

[PATCH 2/5] revert: refactor merge recursive code into its own function

[Christian Couder]

The code that is used to do a recursive merge is extracted from
the revert_or_cherry_pick() function and put into a new
do_recursive_merge() function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |  105 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 58 insertions(+), 47 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index b8ee045..8219ee2 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -302,17 +302,69 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)
 	return write_ref_sha1(ref_lock, to, "cherry-pick");
 }
 
+static void do_recursive_merge(struct commit *base, struct commit *next,
+			       const char *base_label, const char *next_label,
+			       unsigned char *head, struct strbuf *msgbuf,
+			       char *defmsg)
+{
+	struct merge_options o;
+	struct tree *result, *next_tree, *base_tree, *head_tree;
+	int clean, index_fd;
+	static struct lock_file index_lock;
+
+	index_fd = hold_locked_index(&index_lock, 1);
+
+	read_cache();
+	init_merge_options(&o);
+	o.ancestor = base ? base_label : "(empty tree)";
+	o.branch1 = "HEAD";
+	o.branch2 = next ? next_label : "(empty tree)";
+
+	head_tree = parse_tree_indirect(head);
+	next_tree = next ? next->tree : empty_tree();
+	base_tree = base ? base->tree : empty_tree();
+
+	clean = merge_trees(&o,
+			    head_tree,
+			    next_tree, base_tree, &result);
+
+	if (active_cache_changed &&
+	    (write_cache(index_fd, active_cache, active_nr) ||
+	     commit_locked_index(&index_lock)))
+		die("%s: Unable to write new index file", me);
+	rollback_lock_file(&index_lock);
+
+	if (!clean) {
+		int i;
+		strbuf_addstr(msgbuf, "\nConflicts:\n\n");
+		for (i = 0; i < active_nr;) {
+			struct cache_entry *ce = active_cache[i++];
+			if (ce_stage(ce)) {
+				strbuf_addch(msgbuf, '\t');
+				strbuf_addstr(msgbuf, ce->name);
+				strbuf_addch(msgbuf, '\n');
+				while (i < active_nr && !strcmp(ce->name,
+						active_cache[i]->name))
+					i++;
+			}
+		}
+		write_message(msgbuf, defmsg);
+		fprintf(stderr, "Automatic %s failed.%s\n",
+			me, help_msg(commit_name));
+		rerere(allow_rerere_auto);
+		exit(1);
+	}
+	write_message(msgbuf, defmsg);
+	fprintf(stderr, "Finished one %s.\n", me);
+}
+
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
 	const char *base_label, *next_label;
-	int i, index_fd, clean;
 	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
 	char *defmsg = NULL;
-	struct merge_options o;
-	struct tree *result, *next_tree, *base_tree, *head_tree;
-	static struct lock_file index_lock;
 	struct strbuf msgbuf = STRBUF_INIT;
 
 	git_config(git_default_config, NULL);
@@ -402,8 +454,6 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 	defmsg = git_pathdup("MERGE_MSG");
 
-	index_fd = hold_locked_index(&index_lock, 1);
-
 	if (action == REVERT) {
 		base = commit;
 		base_label = msg.label;
@@ -433,47 +483,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		}
 	}
 
-	read_cache();
-	init_merge_options(&o);
-	o.ancestor = base ? base_label : "(empty tree)";
-	o.branch1 = "HEAD";
-	o.branch2 = next ? next_label : "(empty tree)";
-
-	head_tree = parse_tree_indirect(head);
-	next_tree = next ? next->tree : empty_tree();
-	base_tree = base ? base->tree : empty_tree();
-
-	clean = merge_trees(&o,
-			    head_tree,
-			    next_tree, base_tree, &result);
-
-	if (active_cache_changed &&
-	    (write_cache(index_fd, active_cache, active_nr) ||
-	     commit_locked_index(&index_lock)))
-		die("%s: Unable to write new index file", me);
-	rollback_lock_file(&index_lock);
-
-	if (!clean) {
-		strbuf_addstr(&msgbuf, "\nConflicts:\n\n");
-		for (i = 0; i < active_nr;) {
-			struct cache_entry *ce = active_cache[i++];
-			if (ce_stage(ce)) {
-				strbuf_addch(&msgbuf, '\t');
-				strbuf_addstr(&msgbuf, ce->name);
-				strbuf_addch(&msgbuf, '\n');
-				while (i < active_nr && !strcmp(ce->name,
-						active_cache[i]->name))
-					i++;
-			}
-		}
-		write_message(&msgbuf, defmsg);
-		fprintf(stderr, "Automatic %s failed.%s\n",
-			me, help_msg(commit_name));
-		rerere(allow_rerere_auto);
-		exit(1);
-	}
-	write_message(&msgbuf, defmsg);
-	fprintf(stderr, "Finished one %s.\n", me);
+	do_recursive_merge(base, next, base_label, next_label,
+			   head, &msgbuf, defmsg);
 
 	/*
 	 *
-- 
1.7.0.3.454.gd9aeb

[PATCH 3/5] merge: refactor code that calls "git merge-STRATEGY"

[Christian Couder]

In the try_merge_strategy() function, when the strategy is "recursive"
or "subtree", the merge_recursive() function is called.
Otherwise we launch a "git merge-STRATEGY" process.

To make it possible to reuse code that launches a "git merge-STRATEGY"
process, this patch refactors this code into a new try_merge_command()
function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/merge.c |   81 ++++++++++++++++++++++++++++++------------------------
 1 files changed, 45 insertions(+), 36 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index c043066..f163d53 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -548,13 +548,53 @@ static void write_tree_trivial(unsigned char *sha1)
 		die("git write-tree failed to write a tree");
 }
 
-static int try_merge_strategy(const char *strategy, struct commit_list *common,
-			      const char *head_arg)
+static int try_merge_command(const char *strategy, struct commit_list *common,
+			     const char *head_arg, struct commit_list *remotes)
 {
 	const char **args;
 	int i = 0, x = 0, ret;
 	struct commit_list *j;
 	struct strbuf buf = STRBUF_INIT;
+
+	args = xmalloc((4 + xopts_nr + commit_list_count(common) +
+			commit_list_count(remotes)) * sizeof(char *));
+	strbuf_addf(&buf, "merge-%s", strategy);
+	args[i++] = buf.buf;
+	for (x = 0; x < xopts_nr; x++) {
+		char *s = xmalloc(strlen(xopts[x])+2+1);
+		strcpy(s, "--");
+		strcpy(s+2, xopts[x]);
+		args[i++] = s;
+	}
+	for (j = common; j; j = j->next)
+		args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
+	args[i++] = "--";
+	args[i++] = head_arg;
+	for (j = remotes; j; j = j->next)
+		args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
+	args[i] = NULL;
+	ret = run_command_v_opt(args, RUN_GIT_CMD);
+	strbuf_release(&buf);
+	i = 1;
+	for (x = 0; x < xopts_nr; x++)
+		free((void *)args[i++]);
+	for (j = common; j; j = j->next)
+		free((void *)args[i++]);
+	i += 2;
+	for (j = remotes; j; j = j->next)
+		free((void *)args[i++]);
+	free(args);
+	discard_cache();
+	if (read_cache() < 0)
+		die("failed to read the cache");
+	resolve_undo_clear();
+
+	return ret;
+}
+
+static int try_merge_strategy(const char *strategy, struct commit_list *common,
+			      const char *head_arg)
+{
 	int index_fd;
 	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
 
@@ -567,12 +607,13 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 	rollback_lock_file(lock);
 
 	if (!strcmp(strategy, "recursive") || !strcmp(strategy, "subtree")) {
-		int clean;
+		int clean, x = 0;
 		struct commit *result;
 		struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
 		int index_fd;
 		struct commit_list *reversed = NULL;
 		struct merge_options o;
+		struct commit_list *j;
 
 		if (remoteheads->next) {
 			error("Not handling anything other than two heads merge.");
@@ -612,39 +653,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		rollback_lock_file(lock);
 		return clean ? 0 : 1;
 	} else {
-		args = xmalloc((4 + xopts_nr + commit_list_count(common) +
-					commit_list_count(remoteheads)) * sizeof(char *));
-		strbuf_addf(&buf, "merge-%s", strategy);
-		args[i++] = buf.buf;
-		for (x = 0; x < xopts_nr; x++) {
-			char *s = xmalloc(strlen(xopts[x])+2+1);
-			strcpy(s, "--");
-			strcpy(s+2, xopts[x]);
-			args[i++] = s;
-		}
-		for (j = common; j; j = j->next)
-			args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
-		args[i++] = "--";
-		args[i++] = head_arg;
-		for (j = remoteheads; j; j = j->next)
-			args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
-		args[i] = NULL;
-		ret = run_command_v_opt(args, RUN_GIT_CMD);
-		strbuf_release(&buf);
-		i = 1;
-		for (x = 0; x < xopts_nr; x++)
-			free((void *)args[i++]);
-		for (j = common; j; j = j->next)
-			free((void *)args[i++]);
-		i += 2;
-		for (j = remoteheads; j; j = j->next)
-			free((void *)args[i++]);
-		free(args);
-		discard_cache();
-		if (read_cache() < 0)
-			die("failed to read the cache");
-		resolve_undo_clear();
-		return ret;
+		return try_merge_command(strategy, common, head_arg, remoteheads);
 	}
 }
 
-- 
1.7.0.3.454.gd9aeb

[PATCH 4/5] merge: make function try_merge_command non static

[Christian Couder]

and while at it let's move declaration of checkout_fast_forward()
into "merge-recursive.h" instead of "cache.h"

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/merge.c   |    4 ++--
 cache.h           |    3 ---
 merge-recursive.h |    4 ++++
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index f163d53..fc240f9 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -548,8 +548,8 @@ static void write_tree_trivial(unsigned char *sha1)
 		die("git write-tree failed to write a tree");
 }
 
-static int try_merge_command(const char *strategy, struct commit_list *common,
-			     const char *head_arg, struct commit_list *remotes)
+int try_merge_command(const char *strategy, struct commit_list *common,
+		      const char *head_arg, struct commit_list *remotes)
 {
 	const char **args;
 	int i = 0, x = 0, ret;
diff --git a/cache.h b/cache.h
index 32c18b1..b065721 100644
--- a/cache.h
+++ b/cache.h
@@ -1059,7 +1059,4 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix);
 char *alias_lookup(const char *alias);
 int split_cmdline(char *cmdline, const char ***argv);
 
-/* builtin/merge.c */
-int checkout_fast_forward(const unsigned char *from, const unsigned char *to);
-
 #endif /* CACHE_H */
diff --git a/merge-recursive.h b/merge-recursive.h
index d1192f5..9eb7e2d 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -54,4 +54,8 @@ int merge_recursive_generic(struct merge_options *o,
 void init_merge_options(struct merge_options *o);
 struct tree *write_tree_from_memory(struct merge_options *o);
 
+/* builtin/merge.c */
+int checkout_fast_forward(const unsigned char *from, const unsigned char *to);
+int try_merge_command(const char *strategy, struct commit_list *common, const char *head_arg, struct commit_list *remotes);
+
 #endif
-- 
1.7.0.3.454.gd9aeb

[PATCH 5/5] revert: add "--strategy" option to choose merge strategy

[Christian Couder]

This patch makes it possible to use a different merge strategy when
cherry-picking. This is usefull mainly for debugging purposes as it
allows to see if some failures are caused by the merge strategy used or
not.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   25 +++++++++++++++++++++++--
 1 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 8219ee2..bbaa937 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -43,6 +43,7 @@ static const char *commit_name;
 static int allow_rerere_auto;
 
 static const char *me;
+static const char *strategy;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -62,6 +63,7 @@ static void parse_args(int argc, const char **argv)
 		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_END(),
 		OPT_END(),
 		OPT_END(),
@@ -483,8 +485,27 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		}
 	}
 
-	do_recursive_merge(base, next, base_label, next_label,
-			   head, &msgbuf, defmsg);
+	if (!strategy || !strcmp(strategy, "recursive") || action == REVERT)
+		do_recursive_merge(base, next, base_label, next_label,
+				   head, &msgbuf, defmsg);
+	else {
+		int res;
+		struct commit_list *common = NULL;
+		struct commit_list *remotes = NULL;
+		write_message(&msgbuf, defmsg);
+		commit_list_insert(base, &common);
+		commit_list_insert(next, &remotes);
+		res = try_merge_command(strategy, common,
+					sha1_to_hex(head), remotes);
+		free_commit_list(common);
+		free_commit_list(remotes);
+		if (res) {
+			fprintf(stderr, "Automatic %s with strategy %s failed.%s\n",
+				me, strategy, help_msg(commit_name));
+			rerere(allow_rerere_auto);
+			exit(1);
+		}
+	}
 
 	/*
 	 *
-- 
1.7.0.3.454.gd9aeb