[PATCH v2 00/12] add --ff option to cherry-pick

[PATCH v2 00/12] add --ff option to cherry-pick

[Christian Couder]

The goal of this patch series is to make it possible for "git cherry-pick"
to fast forward instead of creating a new commit if the cherry picked commit
has the same parent as the one we are cherry-picking on.

Changes since the previous series are the following:

- now --ff should work with merge commits and "-m <parent-num>" option
- added a no-op --no-ff option as request by Paolo
- removed for now the patch that used the new reset() function in
"builtin-merge.c"

Christian Couder (11):
  pick: refactor checking parent in a check_parent function
  pick: make check_parent function extern
  pick: move calling check_parent() ouside pick_commit()
  reset: refactor updating heads into a static function
  reset: refactor reseting in its own function
  reset: make reset function non static and declare it in "reset.h"
  revert: add --ff option to allow fast forward when cherry-picking
  cherry-pick: add tests for new --ff option
  Documentation: describe new cherry-pick --ff option
  cherry-pick: add a no-op --no-ff option to future proof scripts
  rebase -i: use new --ff cherry-pick option

Stephan Beyer (1):
  revert: libify cherry-pick and revert functionnality

 Documentation/git-cherry-pick.txt |   10 +-
 Makefile                          |    2 +
 builtin/reset.c                   |  178 ++++++++++++----------
 builtin/revert.c                  |  308 ++++++++++---------------------------
 git-rebase--interactive.sh        |   15 +--
 pick.c                            |  223 +++++++++++++++++++++++++++
 pick.h                            |   14 ++
 reset.h                           |   10 ++
 t/t3506-cherry-pick-ff.sh         |  106 +++++++++++++
 9 files changed, 545 insertions(+), 321 deletions(-)
 create mode 100644 pick.c
 create mode 100644 pick.h
 create mode 100644 reset.h
 create mode 100755 t/t3506-cherry-pick-ff.sh

[PATCH v2 01/12] revert: libify cherry-pick and revert functionnality

[Christian Couder]

From: Stephan Beyer <s-beyer@gmx.net>

The goal of this commit is to abstract out "git cherry-pick" and
"git revert" functionnality into a new pick_commit() function made
of code from "builtin-revert.c".

The new pick_commit() function is in a new "pick.c" file with an
associated "pick.h".

This function starts from the current index (not HEAD), and allow
the effect of one commit replayed (either forward or backward) to
that state, leaving the result in the index. So it makes it
possible to replay many commits to the index in sequence without
commiting in between.

This commit is made of code from the sequencer GSoC project:

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

(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)

And it contains some changes and comments suggested by Junio.

The original commit in the sequencer project that introduced
this change is: 94a568a78d243d7a6c13778bc6b7ac1eb46e48cc

Mentored-by: Daniel Barkalow <barkalow@iabervon.org>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Makefile         |    2 +
 builtin/revert.c |  272 +++++++++---------------------------------------------
 pick.c           |  218 +++++++++++++++++++++++++++++++++++++++++++
 pick.h           |   13 +++
 4 files changed, 277 insertions(+), 228 deletions(-)
 create mode 100644 pick.c
 create mode 100644 pick.h

diff --git a/Makefile b/Makefile
index b6f097e..4da977f 100644
--- a/Makefile
+++ b/Makefile
@@ -485,6 +485,7 @@ LIB_H += pack-refs.h
 LIB_H += pack-revindex.h
 LIB_H += parse-options.h
 LIB_H += patch-ids.h
+LIB_H += pick.h
 LIB_H += pkt-line.h
 LIB_H += progress.h
 LIB_H += quote.h
@@ -578,6 +579,7 @@ LIB_OBJS += parse-options.o
 LIB_OBJS += patch-delta.o
 LIB_OBJS += patch-ids.o
 LIB_OBJS += path.o
+LIB_OBJS += pick.o
 LIB_OBJS += pkt-line.o
 LIB_OBJS += preload-index.o
 LIB_OBJS += pretty.o
diff --git a/builtin/revert.c b/builtin/revert.c
index eff5268..3308de1 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -1,18 +1,14 @@
 #include "cache.h"
 #include "builtin.h"
-#include "object.h"
 #include "commit.h"
 #include "tag.h"
-#include "wt-status.h"
-#include "run-command.h"
 #include "exec_cmd.h"
 #include "utf8.h"
 #include "parse-options.h"
-#include "cache-tree.h"
 #include "diff.h"
 #include "revision.h"
 #include "rerere.h"
-#include "merge-recursive.h"
+#include "pick.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -35,26 +31,24 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_replay, no_commit, mainline, signoff;
-static enum { REVERT, CHERRY_PICK } action;
+static int edit, no_commit, mainline, signoff;
+static int flags;
 static struct commit *commit;
 static const char *commit_name;
 static int allow_rerere_auto;
 
-static const char *me;
-
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str =
-		action == REVERT ?  revert_usage : cherry_pick_usage;
+		flags & PICK_REVERSE ? revert_usage : cherry_pick_usage;
 	unsigned char sha1[20];
 	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('x', NULL, &no_replay, "append commit name when cherry-picking"),
+		OPT_BIT('x', NULL, &flags, "append commit name when cherry-picking", PICK_ADD_NOTE),
 		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"),
@@ -73,42 +67,12 @@ static void parse_args(int argc, const char **argv)
 		exit(1);
 }
 
-static char *get_oneline(const char *message)
-{
-	char *result;
-	const char *p = message, *abbrev, *eol;
-	int abbrev_len, oneline_len;
-
-	if (!p)
-		die ("Could not read commit message of %s",
-				sha1_to_hex(commit->object.sha1));
-	while (*p && (*p != '\n' || p[1] != '\n'))
-		p++;
-
-	if (*p) {
-		p += 2;
-		for (eol = p + 1; *eol && *eol != '\n'; eol++)
-			; /* do nothing */
-	} else
-		eol = p;
-	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	abbrev_len = strlen(abbrev);
-	oneline_len = eol - p;
-	result = xmalloc(abbrev_len + 5 + oneline_len);
-	memcpy(result, abbrev, abbrev_len);
-	memcpy(result + abbrev_len, "... ", 4);
-	memcpy(result + abbrev_len + 4, p, oneline_len);
-	result[abbrev_len + 4 + oneline_len] = '\0';
-	return result;
-}
-
 static char *get_encoding(const char *message)
 {
 	const char *p = message, *eol;
 
 	if (!p)
-		die ("Could not read commit message of %s",
-				sha1_to_hex(commit->object.sha1));
+		return NULL;
 	while (*p && *p != '\n') {
 		for (eol = p + 1; *eol && *eol != '\n'; eol++)
 			; /* do nothing */
@@ -124,30 +88,6 @@ 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)
-{
-	const char *p = message;
-	while (*p && (*p != '\n' || p[1] != '\n'))
-		p++;
-
-	if (!*p)
-		add_to_msg(sha1_to_hex(commit->object.sha1));
-
-	p += 2;
-	add_to_msg(p);
-	return;
-}
-
 static void set_author_ident_env(const char *message)
 {
 	const char *p = message;
@@ -210,7 +150,7 @@ static char *help_msg(const char *name)
 		"mark the corrected paths with 'git add <paths>' or 'git rm <paths>'\n"
 		"and commit the result");
 
-	if (action == CHERRY_PICK) {
+	if (!(flags & PICK_REVERSE)) {
 		strbuf_addf(&helpbuf, " with: \n"
 			"\n"
 			"        git commit -c %s\n",
@@ -221,14 +161,17 @@ static char *help_msg(const char *name)
 	return strbuf_detach(&helpbuf, NULL);
 }
 
-static struct tree *empty_tree(void)
+static void write_message(struct strbuf *msgbuf, const char *filename)
 {
-	struct tree *tree = xcalloc(1, sizeof(struct tree));
-
-	tree->object.parsed = 1;
-	tree->object.type = OBJ_TREE;
-	pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
-	return tree;
+	static struct lock_file msg_file;
+	int msg_fd;
+	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 NORETURN void die_dirty_index(const char *me)
@@ -246,175 +189,53 @@ static NORETURN void die_dirty_index(const char *me)
 
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
-	unsigned char head[20];
-	struct commit *base, *next, *parent;
-	int i, index_fd, clean;
-	char *oneline, *reencoded_message = NULL;
-	const char *message, *encoding;
-	char *defmsg = git_pathdup("MERGE_MSG");
-	struct merge_options o;
-	struct tree *result, *next_tree, *base_tree, *head_tree;
-	static struct lock_file index_lock;
+	const char *me;
+	struct strbuf msgbuf;
+	char *reencoded_message = NULL;
+	const char *encoding;
+	int failed;
 
 	git_config(git_default_config, NULL);
-	me = action == REVERT ? "revert" : "cherry-pick";
+	me = flags & PICK_REVERSE ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
 	parse_args(argc, argv);
 
-	/* this is copied from the shell script, but it's never triggered... */
-	if (action == REVERT && !no_replay)
-		die("revert is incompatible with replay");
-
 	if (read_cache() < 0)
 		die("git %s: failed to read the index", me);
-	if (no_commit) {
-		/*
-		 * We do not intend to commit immediately.  We just want to
-		 * merge the differences in, so let's compute the tree
-		 * that represents the "current" state for merge-recursive
-		 * to work on.
-		 */
-		if (write_cache_as_tree(head, 0, NULL))
-			die ("Your index file is unmerged.");
-	} else {
-		if (get_sha1("HEAD", head))
-			die ("You do not have a valid HEAD");
-		if (index_differs_from("HEAD", 0))
-			die_dirty_index(me);
-	}
-	discard_cache();
-
-	index_fd = hold_locked_index(&index_lock, 1);
-
-	if (!commit->parents) {
-		if (action == REVERT)
-			die ("Cannot revert a root commit");
-		parent = NULL;
-	}
-	else if (commit->parents->next) {
-		/* Reverting or cherry-picking a merge commit */
-		int cnt;
-		struct commit_list *p;
-
-		if (!mainline)
-			die("Commit %s is a merge but no -m option was given.",
-			    sha1_to_hex(commit->object.sha1));
-
-		for (cnt = 1, p = commit->parents;
-		     cnt != mainline && p;
-		     cnt++)
-			p = p->next;
-		if (cnt != mainline || !p)
-			die("Commit %s does not have parent %d",
-			    sha1_to_hex(commit->object.sha1), mainline);
-		parent = p->item;
-	} else if (0 < mainline)
-		die("Mainline was specified but commit %s is not a merge.",
-		    sha1_to_hex(commit->object.sha1));
-	else
-		parent = commit->parents->item;
+	if (!no_commit && index_differs_from("HEAD", 0))
+		die_dirty_index(me);
 
-	if (!(message = commit->buffer))
-		die ("Cannot get commit message for %s",
+	if (!commit->buffer)
+		return error("Cannot get commit message for %s",
 				sha1_to_hex(commit->object.sha1));
-
-	if (parent && parse_commit(parent) < 0)
-		die("%s: cannot parse parent commit %s",
-		    me, sha1_to_hex(parent->object.sha1));
-
-	/*
-	 * "commit" is an existing commit.  We would want to apply
-	 * the difference it introduces since its first parent "prev"
-	 * on top of the current HEAD if we are cherry-pick.  Or the
-	 * reverse of it if we are revert.
-	 */
-
-	msg_fd = hold_lock_file_for_update(&msg_file, defmsg,
-					   LOCK_DIE_ON_ERROR);
-
-	encoding = get_encoding(message);
+	encoding = get_encoding(commit->buffer);
 	if (!encoding)
 		encoding = "UTF-8";
 	if (!git_commit_encoding)
 		git_commit_encoding = "UTF-8";
-	if ((reencoded_message = reencode_string(message,
+	if ((reencoded_message = reencode_string(commit->buffer,
 					git_commit_encoding, encoding)))
-		message = reencoded_message;
-
-	oneline = get_oneline(message);
+		commit->buffer = reencoded_message;
 
-	if (action == REVERT) {
-		char *oneline_body = strchr(oneline, ' ');
-
-		base = commit;
-		next = parent;
-		add_to_msg("Revert \"");
-		add_to_msg(oneline_body + 1);
-		add_to_msg("\"\n\nThis reverts commit ");
-		add_to_msg(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));
-		}
-		add_to_msg(".\n");
-	} else {
-		base = parent;
-		next = commit;
-		set_author_ident_env(message);
-		add_message_to_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");
-		}
-	}
-
-	read_cache();
-	init_merge_options(&o);
-	o.branch1 = "HEAD";
-	o.branch2 = oneline;
-
-	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) {
-		add_to_msg("\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");
-				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);
+	failed = pick_commit(commit, mainline, flags, &msgbuf);
+	if (failed < 0) {
+		exit(1);
+	} else if (failed > 0) {
 		fprintf(stderr, "Automatic %s failed.%s\n",
 			me, help_msg(commit_name));
+		write_message(&msgbuf, git_path("MERGE_MSG"));
 		rerere(allow_rerere_auto);
 		exit(1);
 	}
-	if (commit_lock_file(&msg_file) < 0)
-		die ("Error wrapping up %s", defmsg);
+	if (!(flags & PICK_REVERSE))
+		set_author_ident_env(commit->buffer);
+	free(reencoded_message);
+
 	fprintf(stderr, "Finished one %s.\n", me);
 
+	write_message(&msgbuf, git_path("MERGE_MSG"));
+
 	/*
-	 *
 	 * If we are cherry-pick, and if the merge did not result in
 	 * hand-editing, we will hit this commit and inherit the original
 	 * author date and name.
@@ -432,14 +253,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			args[i++] = "-s";
 		if (!edit) {
 			args[i++] = "-F";
-			args[i++] = defmsg;
+			args[i++] = git_path("MERGE_MSG");
 		}
 		args[i] = NULL;
 		return execv_git_cmd(args);
 	}
-	free(reencoded_message);
-	free(defmsg);
-
 	return 0;
 }
 
@@ -447,14 +265,12 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	if (isatty(0))
 		edit = 1;
-	no_replay = 1;
-	action = REVERT;
+	flags = PICK_REVERSE | PICK_ADD_NOTE;
 	return revert_or_cherry_pick(argc, argv);
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
-	no_replay = 0;
-	action = CHERRY_PICK;
+	flags = 0;
 	return revert_or_cherry_pick(argc, argv);
 }
diff --git a/pick.c b/pick.c
new file mode 100644
index 0000000..1e1628a
--- /dev/null
+++ b/pick.c
@@ -0,0 +1,218 @@
+#include "cache.h"
+#include "commit.h"
+#include "run-command.h"
+#include "cache-tree.h"
+#include "pick.h"
+#include "merge-recursive.h"
+
+static struct commit *commit;
+
+static char *get_oneline(const char *message)
+{
+	char *result;
+	const char *p = message, *abbrev, *eol;
+	int abbrev_len, oneline_len;
+
+	if (!p)
+		return NULL;
+	while (*p && (*p != '\n' || p[1] != '\n'))
+		p++;
+
+	if (*p) {
+		p += 2;
+		for (eol = p + 1; *eol && *eol != '\n'; eol++)
+			; /* do nothing */
+	} else
+		eol = p;
+	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+	abbrev_len = strlen(abbrev);
+	oneline_len = eol - p;
+	result = xmalloc(abbrev_len + 5 + oneline_len);
+	memcpy(result, abbrev, abbrev_len);
+	memcpy(result + abbrev_len, "... ", 4);
+	memcpy(result + abbrev_len + 4, p, oneline_len);
+	result[abbrev_len + 4 + oneline_len] = '\0';
+	return result;
+}
+
+static void add_message_to_msg(struct strbuf *msg, const char *message)
+{
+	const char *p = message;
+	while (*p && (*p != '\n' || p[1] != '\n'))
+		p++;
+
+	if (!*p)
+		strbuf_addstr(msg, sha1_to_hex(commit->object.sha1));
+
+	p += 2;
+	strbuf_addstr(msg, p);
+	return;
+}
+
+static struct tree *empty_tree(void)
+{
+	struct tree *tree = xcalloc(1, sizeof(struct tree));
+
+	tree->object.parsed = 1;
+	tree->object.type = OBJ_TREE;
+	pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
+	return tree;
+}
+
+/*
+ * Pick changes introduced by "commit" argument into current working
+ * tree and index.
+ *
+ * It starts from the current index (not HEAD), and allow the effect
+ * of one commit replayed (either forward or backward) to that state,
+ * leaving the result in the index.
+ *
+ * You do not have to start from a commit, so you can replay many commits
+ * to the index in sequence without commiting in between to squash multiple
+ * steps if you wanted to.
+ *
+ * Return 0 on success.
+ * Return negative value on error before picking,
+ * and a positive value after picking,
+ * and return 1 if and only if a conflict occurs but no other error.
+ */
+int pick_commit(struct commit *pick_commit, int mainline, int flags,
+		struct strbuf *msg)
+{
+	unsigned char head[20];
+	struct commit *base, *next, *parent;
+	int i, index_fd, clean;
+	int ret = 0;
+	char *oneline;
+	const char *message;
+	struct merge_options o;
+	struct tree *result, *next_tree, *base_tree, *head_tree;
+	static struct lock_file index_lock;
+
+	strbuf_init(msg, 0);
+	commit = pick_commit;
+
+	/*
+	 * Let's compute the tree that represents the "current" state
+	 * for merge-recursive to work on.
+	 */
+	if (write_cache_as_tree(head, 0, NULL))
+		return error("Your index file is unmerged.");
+	discard_cache();
+
+	index_fd = hold_locked_index(&index_lock, 0);
+	if (index_fd < 0)
+		return error("Unable to create locked index: %s",
+			     strerror(errno));
+
+	if (!commit->parents) {
+		if (flags & PICK_REVERSE)
+			return error("Cannot revert a root commit");
+		parent = NULL;
+	}
+	else if (commit->parents->next) {
+		/* Reverting or cherry-picking a merge commit */
+		int cnt;
+		struct commit_list *p;
+
+		if (!mainline)
+			return error("Commit %s is a merge but no mainline 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)
+			return error("Commit %s does not have parent %d",
+				     sha1_to_hex(commit->object.sha1),
+				     mainline);
+		parent = p->item;
+	} else if (0 < mainline)
+		return error("Mainline was specified but commit %s is not a merge.",
+			     sha1_to_hex(commit->object.sha1));
+	else
+		parent = commit->parents->item;
+
+	if (!(message = commit->buffer))
+		return error("Cannot get commit message for %s",
+			     sha1_to_hex(commit->object.sha1));
+
+	if (parent && parse_commit(parent) < 0)
+		return error("Cannot parse parent commit %s",
+			     sha1_to_hex(parent->object.sha1));
+
+	oneline = get_oneline(message);
+
+	if (flags & PICK_REVERSE) {
+		char *oneline_body = strchr(oneline, ' ') + 1;
+
+		base = commit;
+		next = parent;
+		strbuf_addstr(msg, "Revert \"");
+		strbuf_addstr(msg, oneline_body);
+		strbuf_addstr(msg, "\"\n\nThis reverts commit ");
+		strbuf_addstr(msg, sha1_to_hex(commit->object.sha1));
+
+		if (commit->parents->next) {
+			strbuf_addstr(msg, ", reversing\nchanges made to ");
+			strbuf_addstr(msg, sha1_to_hex(parent->object.sha1));
+		}
+		strbuf_addstr(msg, ".\n");
+	} else {
+		base = parent;
+		next = commit;
+		add_message_to_msg(msg, message);
+		if (flags & PICK_ADD_NOTE) {
+			strbuf_addstr(msg, "(cherry picked from commit ");
+			strbuf_addstr(msg, sha1_to_hex(commit->object.sha1));
+			strbuf_addstr(msg, ")\n");
+		}
+	}
+
+	read_cache();
+	init_merge_options(&o);
+	o.branch1 = "HEAD";
+	o.branch2 = oneline;
+
+	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))) {
+		error("Unable to write new index file");
+		return 2;
+	}
+	rollback_lock_file(&index_lock);
+
+	if (!clean) {
+		strbuf_addstr(msg, "\nConflicts:\n\n");
+		for (i = 0; i < active_nr;) {
+			struct cache_entry *ce = active_cache[i++];
+			if (ce_stage(ce)) {
+				strbuf_addch(msg, '\t');
+				strbuf_addstr(msg, ce->name);
+				strbuf_addch(msg, '\n');
+				while (i < active_nr && !strcmp(ce->name,
+						active_cache[i]->name))
+					i++;
+			}
+		}
+		ret = 1;
+	}
+	free(oneline);
+
+	discard_cache();
+	if (read_cache() < 0) {
+		error("Cannot read the index");
+		return 2;
+	}
+
+	return ret;
+}
diff --git a/pick.h b/pick.h
new file mode 100644
index 0000000..7a74ad8
--- /dev/null
+++ b/pick.h
@@ -0,0 +1,13 @@
+#ifndef PICK_H
+#define PICK_H
+
+#include "commit.h"
+
+/* Pick flags: */
+#define PICK_REVERSE   1 /* pick the reverse changes ("revert") */
+#define PICK_ADD_NOTE  2 /* add note about original commit (unless conflict) */
+/* We don't need a PICK_QUIET. This is done by
+ *	setenv("GIT_MERGE_VERBOSITY", "0", 1); */
+extern int pick_commit(struct commit *commit, int mainline, int flags, struct strbuf *msg);
+
+#endif
-- 
1.7.0.321.g2d270

[PATCH v2 02/12] pick: refactor checking parent in a check_parent function

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 pick.c |   69 ++++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/pick.c b/pick.c
index 1e1628a..40673ca 100644
--- a/pick.c
+++ b/pick.c
@@ -59,6 +59,40 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
+static int check_parent(struct commit *commit, int mainline, int flags,
+			struct commit **parent)
+{
+	if (!commit->parents) {
+		if (flags & PICK_REVERSE)
+			return error("Cannot revert a root commit");
+	}
+	else if (commit->parents->next) {
+		/* Reverting or cherry-picking a merge commit */
+		int cnt;
+		struct commit_list *p;
+
+		if (!mainline)
+			return error("Commit %s is a merge but no mainline 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)
+			return error("Commit %s does not have parent %d",
+				     sha1_to_hex(commit->object.sha1),
+				     mainline);
+		*parent = p->item;
+	} else if (0 < mainline)
+		return error("Mainline was specified but commit %s is not a merge.",
+			     sha1_to_hex(commit->object.sha1));
+	else
+		*parent = commit->parents->item;
+
+	return 0;
+}
+
 /*
  * Pick changes introduced by "commit" argument into current working
  * tree and index.
@@ -80,8 +114,8 @@ int pick_commit(struct commit *pick_commit, int mainline, int flags,
 		struct strbuf *msg)
 {
 	unsigned char head[20];
-	struct commit *base, *next, *parent;
-	int i, index_fd, clean;
+	struct commit *base, *next, *parent = NULL;
+	int i, index_fd, clean, err;
 	int ret = 0;
 	char *oneline;
 	const char *message;
@@ -105,34 +139,9 @@ int pick_commit(struct commit *pick_commit, int mainline, int flags,
 		return error("Unable to create locked index: %s",
 			     strerror(errno));
 
-	if (!commit->parents) {
-		if (flags & PICK_REVERSE)
-			return error("Cannot revert a root commit");
-		parent = NULL;
-	}
-	else if (commit->parents->next) {
-		/* Reverting or cherry-picking a merge commit */
-		int cnt;
-		struct commit_list *p;
-
-		if (!mainline)
-			return error("Commit %s is a merge but no mainline 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)
-			return error("Commit %s does not have parent %d",
-				     sha1_to_hex(commit->object.sha1),
-				     mainline);
-		parent = p->item;
-	} else if (0 < mainline)
-		return error("Mainline was specified but commit %s is not a merge.",
-			     sha1_to_hex(commit->object.sha1));
-	else
-		parent = commit->parents->item;
+	err = check_parent(commit, mainline, flags, &parent);
+	if (err)
+		return err;
 
 	if (!(message = commit->buffer))
 		return error("Cannot get commit message for %s",
-- 
1.7.0.321.g2d270

[PATCH v2 03/12] pick: make check_parent function extern

[Christian Couder]

as it will be used outside pick.c in a later patch

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 pick.c |    2 +-
 pick.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/pick.c b/pick.c
index 40673ca..078b78d 100644
--- a/pick.c
+++ b/pick.c
@@ -59,7 +59,7 @@ static struct tree *empty_tree(void)
 	return tree;
 }
 
-static int check_parent(struct commit *commit, int mainline, int flags,
+int check_parent(struct commit *commit, int mainline, int flags,
 			struct commit **parent)
 {
 	if (!commit->parents) {
diff --git a/pick.h b/pick.h
index 7a74ad8..39af1de 100644
--- a/pick.h
+++ b/pick.h
@@ -9,5 +9,6 @@
 /* We don't need a PICK_QUIET. This is done by
  *	setenv("GIT_MERGE_VERBOSITY", "0", 1); */
 extern int pick_commit(struct commit *commit, int mainline, int flags, struct strbuf *msg);
+extern int check_parent(struct commit *commit, int mainline, int flags, struct commit **parent);
 
 #endif
-- 
1.7.0.321.g2d270

[PATCH v2 04/12] pick: move calling check_parent() ouside pick_commit()

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |    7 ++++++-
 pick.c           |   12 ++++--------
 pick.h           |    2 +-
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 3308de1..764cd41 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -193,6 +193,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	struct strbuf msgbuf;
 	char *reencoded_message = NULL;
 	const char *encoding;
+	struct commit *parent = NULL;
 	int failed;
 
 	git_config(git_default_config, NULL);
@@ -217,7 +218,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 					git_commit_encoding, encoding)))
 		commit->buffer = reencoded_message;
 
-	failed = pick_commit(commit, mainline, flags, &msgbuf);
+	failed = check_parent(commit, mainline, flags, &parent);
+	if (failed)
+		return failed;
+
+	failed = pick_commit(commit, parent, flags, &msgbuf);
 	if (failed < 0) {
 		exit(1);
 	} else if (failed > 0) {
diff --git a/pick.c b/pick.c
index 078b78d..d603c5b 100644
--- a/pick.c
+++ b/pick.c
@@ -60,7 +60,7 @@ static struct tree *empty_tree(void)
 }
 
 int check_parent(struct commit *commit, int mainline, int flags,
-			struct commit **parent)
+		 struct commit **parent)
 {
 	if (!commit->parents) {
 		if (flags & PICK_REVERSE)
@@ -110,12 +110,12 @@ int check_parent(struct commit *commit, int mainline, int flags,
  * and a positive value after picking,
  * and return 1 if and only if a conflict occurs but no other error.
  */
-int pick_commit(struct commit *pick_commit, int mainline, int flags,
+int pick_commit(struct commit *pick_commit, struct commit *parent, int flags,
 		struct strbuf *msg)
 {
 	unsigned char head[20];
-	struct commit *base, *next, *parent = NULL;
-	int i, index_fd, clean, err;
+	struct commit *base, *next;
+	int i, index_fd, clean;
 	int ret = 0;
 	char *oneline;
 	const char *message;
@@ -139,10 +139,6 @@ int pick_commit(struct commit *pick_commit, int mainline, int flags,
 		return error("Unable to create locked index: %s",
 			     strerror(errno));
 
-	err = check_parent(commit, mainline, flags, &parent);
-	if (err)
-		return err;
-
 	if (!(message = commit->buffer))
 		return error("Cannot get commit message for %s",
 			     sha1_to_hex(commit->object.sha1));
diff --git a/pick.h b/pick.h
index 39af1de..098f765 100644
--- a/pick.h
+++ b/pick.h
@@ -8,7 +8,7 @@
 #define PICK_ADD_NOTE  2 /* add note about original commit (unless conflict) */
 /* We don't need a PICK_QUIET. This is done by
  *	setenv("GIT_MERGE_VERBOSITY", "0", 1); */
-extern int pick_commit(struct commit *commit, int mainline, int flags, struct strbuf *msg);
+extern int pick_commit(struct commit *commit, struct commit *parent, int flags, struct strbuf *msg);
 extern int check_parent(struct commit *commit, int mainline, int flags, struct commit **parent);
 
 #endif
-- 
1.7.0.321.g2d270

[PATCH v2 05/12] reset: refactor updating heads into a static function

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/reset.c |   42 ++++++++++++++++++++++++++----------------
 1 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 2c3a69a..7af1cb1 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -229,7 +229,30 @@ static void die_if_unmerged_cache(int reset_type)
 	if (is_merge() || read_cache() < 0 || unmerged_cache())
 		die("Cannot do a %s reset in the middle of a merge.",
 		    reset_type_names[reset_type]);
+}
+
+/*
+ * Any resets update HEAD to the head being switched to,
+ * saving the previous head in ORIG_HEAD before.
+ */
+static int update_heads(unsigned char *sha1)
+{
+	unsigned char sha1_orig[20], *orig = NULL,
+		sha1_old_orig[20], *old_orig = NULL;
+	char msg[1024];
+
+	if (!get_sha1("ORIG_HEAD", sha1_old_orig))
+		old_orig = sha1_old_orig;
+	if (!get_sha1("HEAD", sha1_orig)) {
+		orig = sha1_orig;
+		prepend_reflog_action("updating ORIG_HEAD", msg, sizeof(msg));
+		update_ref(msg, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
+	}
+	else if (old_orig)
+		delete_ref("ORIG_HEAD", old_orig, 0);
+	prepend_reflog_action("updating HEAD", msg, sizeof(msg));
 
+	return update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 }
 
 int cmd_reset(int argc, const char **argv, const char *prefix)
@@ -237,10 +260,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
 	int patch_mode = 0;
 	const char *rev = "HEAD";
-	unsigned char sha1[20], *orig = NULL, sha1_orig[20],
-				*old_orig = NULL, sha1_old_orig[20];
+	unsigned char sha1[20];
 	struct commit *commit;
-	char *reflog_action, msg[1024];
+	char *reflog_action;
 	const struct option options[] = {
 		OPT__QUIET(&quiet),
 		OPT_SET_INT(0, "mixed", &reset_type,
@@ -350,19 +372,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 			die("Could not reset index file to revision '%s'.", rev);
 	}
 
-	/* Any resets update HEAD to the head being switched to,
-	 * saving the previous head in ORIG_HEAD before. */
-	if (!get_sha1("ORIG_HEAD", sha1_old_orig))
-		old_orig = sha1_old_orig;
-	if (!get_sha1("HEAD", sha1_orig)) {
-		orig = sha1_orig;
-		prepend_reflog_action("updating ORIG_HEAD", msg, sizeof(msg));
-		update_ref(msg, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
-	}
-	else if (old_orig)
-		delete_ref("ORIG_HEAD", old_orig, 0);
-	prepend_reflog_action("updating HEAD", msg, sizeof(msg));
-	update_ref_status = update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
+	update_ref_status = update_heads(sha1);
 
 	switch (reset_type) {
 	case HARD:
-- 
1.7.0.321.g2d270

[PATCH v2 06/12] reset: refactor reseting in its own function

[Christian Couder]

This splits the reseting logic away from the argument parsing.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/reset.c |  140 +++++++++++++++++++++++++++++--------------------------
 1 files changed, 74 insertions(+), 66 deletions(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 7af1cb1..b2239d2 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -255,70 +255,13 @@ static int update_heads(unsigned char *sha1)
 	return update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 }
 
-int cmd_reset(int argc, const char **argv, const char *prefix)
+static int reset(const char *rev, const char *prefix,
+		 int reset_type, int quiet, int patch_mode,
+		 int argc, const char **argv)
 {
-	int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
-	int patch_mode = 0;
-	const char *rev = "HEAD";
-	unsigned char sha1[20];
 	struct commit *commit;
-	char *reflog_action;
-	const struct option options[] = {
-		OPT__QUIET(&quiet),
-		OPT_SET_INT(0, "mixed", &reset_type,
-						"reset HEAD and index", MIXED),
-		OPT_SET_INT(0, "soft", &reset_type, "reset only HEAD", SOFT),
-		OPT_SET_INT(0, "hard", &reset_type,
-				"reset HEAD, index and working tree", HARD),
-		OPT_SET_INT(0, "merge", &reset_type,
-				"reset HEAD, index and working tree", MERGE),
-		OPT_SET_INT(0, "keep", &reset_type,
-				"reset HEAD but keep local changes", KEEP),
-		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
-		OPT_END()
-	};
-
-	git_config(git_default_config, NULL);
-
-	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
-						PARSE_OPT_KEEP_DASHDASH);
-	reflog_action = args_to_str(argv);
-	setenv("GIT_REFLOG_ACTION", reflog_action, 0);
-
-	/*
-	 * Possible arguments are:
-	 *
-	 * git reset [-opts] <rev> <paths>...
-	 * git reset [-opts] <rev> -- <paths>...
-	 * git reset [-opts] -- <paths>...
-	 * git reset [-opts] <paths>...
-	 *
-	 * At this point, argv[i] points immediately after [-opts].
-	 */
-
-	if (i < argc) {
-		if (!strcmp(argv[i], "--")) {
-			i++; /* reset to HEAD, possibly with paths */
-		} else if (i + 1 < argc && !strcmp(argv[i+1], "--")) {
-			rev = argv[i];
-			i += 2;
-		}
-		/*
-		 * Otherwise, argv[i] could be either <rev> or <paths> and
-		 * has to be unambiguous.
-		 */
-		else if (!get_sha1(argv[i], sha1)) {
-			/*
-			 * Ok, argv[i] looks like a rev; it should not
-			 * be a filename.
-			 */
-			verify_non_filename(prefix, argv[i]);
-			rev = argv[i++];
-		} else {
-			/* Otherwise we treat this as a filename */
-			verify_filename(prefix, argv[i]);
-		}
-	}
+	unsigned char sha1[20];
+	int update_ref_status;
 
 	if (get_sha1(rev, sha1))
 		die("Failed to resolve '%s' as a valid ref.", rev);
@@ -331,19 +274,19 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (patch_mode) {
 		if (reset_type != NONE)
 			die("--patch is incompatible with --{hard,mixed,soft}");
-		return interactive_reset(rev, argv + i, prefix);
+		return interactive_reset(rev, argv, prefix);
 	}
 
 	/* git reset tree [--] paths... can be used to
 	 * load chosen paths from the tree into the index without
 	 * affecting the working tree nor HEAD. */
-	if (i < argc) {
+	if (argc > 0) {
 		if (reset_type == MIXED)
 			warning("--mixed option is deprecated with paths.");
 		else if (reset_type != NONE)
 			die("Cannot do %s reset with paths.",
 					reset_type_names[reset_type]);
-		return read_from_tree(prefix, argv + i, sha1,
+		return read_from_tree(prefix, argv, sha1,
 				quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
 	}
 	if (reset_type == NONE)
@@ -389,7 +332,72 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 	remove_branch_state();
 
+	return update_ref_status;
+}
+
+int cmd_reset(int argc, const char **argv, const char *prefix)
+{
+	int i = 0, reset_type = NONE, quiet = 0;
+	int patch_mode = 0;
+	const char *rev = "HEAD";
+	unsigned char sha1[20];
+	char *reflog_action;
+	const struct option options[] = {
+		OPT__QUIET(&quiet),
+		OPT_SET_INT(0, "mixed", &reset_type,
+						"reset HEAD and index", MIXED),
+		OPT_SET_INT(0, "soft", &reset_type, "reset only HEAD", SOFT),
+		OPT_SET_INT(0, "hard", &reset_type,
+				"reset HEAD, index and working tree", HARD),
+		OPT_SET_INT(0, "merge", &reset_type,
+				"reset HEAD, index and working tree", MERGE),
+		OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
+		OPT_END()
+	};
+
+	git_config(git_default_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, options, git_reset_usage,
+						PARSE_OPT_KEEP_DASHDASH);
+	reflog_action = args_to_str(argv);
+	setenv("GIT_REFLOG_ACTION", reflog_action, 0);
 	free(reflog_action);
 
-	return update_ref_status;
+	/*
+	 * Possible arguments are:
+	 *
+	 * git reset [-opts] <rev> <paths>...
+	 * git reset [-opts] <rev> -- <paths>...
+	 * git reset [-opts] -- <paths>...
+	 * git reset [-opts] <paths>...
+	 *
+	 * At this point, argv[i] points immediately after [-opts].
+	 */
+
+	if (i < argc) {
+		if (!strcmp(argv[i], "--")) {
+			i++; /* reset to HEAD, possibly with paths */
+		} else if (i + 1 < argc && !strcmp(argv[i+1], "--")) {
+			rev = argv[i];
+			i += 2;
+		}
+		/*
+		 * Otherwise, argv[i] could be either <rev> or <paths> and
+		 * has to be unambiguous.
+		 */
+		else if (!get_sha1(argv[i], sha1)) {
+			/*
+			 * Ok, argv[i] looks like a rev; it should not
+			 * be a filename.
+			 */
+			verify_non_filename(prefix, argv[i]);
+			rev = argv[i++];
+		} else {
+			/* Otherwise we treat this as a filename */
+			verify_filename(prefix, argv[i]);
+		}
+	}
+
+	return reset(rev, prefix, reset_type, quiet, patch_mode,
+		     argc - i, argv + i);
 }
-- 
1.7.0.321.g2d270

[PATCH v2 07/12] reset: make reset function non static and declare it in "reset.h"

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/reset.c |    8 ++++----
 reset.h         |   10 ++++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)
 create mode 100644 reset.h

diff --git a/builtin/reset.c b/builtin/reset.c
index b2239d2..f1d7a5a 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -20,6 +20,7 @@
 #include "parse-options.h"
 #include "unpack-trees.h"
 #include "cache-tree.h"
+#include "reset.h"
 
 static const char * const git_reset_usage[] = {
 	"git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]",
@@ -27,7 +28,6 @@ static const char * const git_reset_usage[] = {
 	NULL
 };
 
-enum reset_type { MIXED, SOFT, HARD, MERGE, KEEP, NONE };
 static const char *reset_type_names[] = {
 	"mixed", "soft", "hard", "merge", "keep", NULL
 };
@@ -255,9 +255,9 @@ static int update_heads(unsigned char *sha1)
 	return update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
 }
 
-static int reset(const char *rev, const char *prefix,
-		 int reset_type, int quiet, int patch_mode,
-		 int argc, const char **argv)
+int reset(const char *rev, const char *prefix,
+	  int reset_type, int quiet, int patch_mode,
+	  int argc, const char **argv)
 {
 	struct commit *commit;
 	unsigned char sha1[20];
diff --git a/reset.h b/reset.h
new file mode 100644
index 0000000..05d2205
--- /dev/null
+++ b/reset.h
@@ -0,0 +1,10 @@
+#ifndef RESET_H
+#define RESET_H
+
+enum reset_type { MIXED, SOFT, HARD, MERGE, KEEP, NONE };
+
+int reset(const char *rev, const char *prefix,
+	  int reset_type, int quiet, int patch_mode,
+	  int argc, const char **argv);
+
+#endif
-- 
1.7.0.321.g2d270

[PATCH v2 08/12] revert: add --ff option to allow fast forward when cherry-picking

[Christian Couder]

As "git merge" fast forwards if possible, it seems sensible to
have such a feature for "git cherry-pick" too, especially as it
could be used in git-rebase--interactive.sh.

Maybe this option could be made the default in the long run, with
another --no-ff option to disable this default behavior, but that
could make some scripts backward incompatible and/or that would
require testing if some GIT_AUTHOR_* environment variables are
set. So we don't do that for now.

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

diff --git a/builtin/revert.c b/builtin/revert.c
index 764cd41..b3e1fea 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -9,6 +9,7 @@
 #include "revision.h"
 #include "rerere.h"
 #include "pick.h"
+#include "reset.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -31,7 +32,7 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_commit, mainline, signoff;
+static int edit, no_commit, mainline, signoff, ff_ok;
 static int flags;
 static struct commit *commit;
 static const char *commit_name;
@@ -39,6 +40,12 @@ static int allow_rerere_auto;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
+static void die_if_ff_incompatible(int opt_set, const char *opt_name)
+{
+	if (opt_set)
+		die ("options --ff and %s are incompatible", opt_name);
+}
+
 static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str =
@@ -51,6 +58,7 @@ static void parse_args(int argc, const char **argv)
 		OPT_BIT('x', NULL, &flags, "append commit name when cherry-picking", PICK_ADD_NOTE),
 		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
 		OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
+		OPT_BOOLEAN(0, "ff", &ff_ok, "allow fast forward"),
 		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
 		OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 		OPT_END(),
@@ -65,6 +73,12 @@ static void parse_args(int argc, const char **argv)
 	commit = lookup_commit_reference(sha1);
 	if (!commit)
 		exit(1);
+	if (ff_ok) {
+		die_if_ff_incompatible(edit, "--edit");
+		die_if_ff_incompatible(flags & PICK_ADD_NOTE, "-x");
+		die_if_ff_incompatible(no_commit, "--no-commit");
+		die_if_ff_incompatible(signoff, "--signoff");
+	}
 }
 
 static char *get_encoding(const char *message)
@@ -187,7 +201,7 @@ static NORETURN void die_dirty_index(const char *me)
 	}
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int revert_or_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	const char *me;
 	struct strbuf msgbuf;
@@ -206,6 +220,25 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	if (!no_commit && index_differs_from("HEAD", 0))
 		die_dirty_index(me);
 
+	failed = check_parent(commit, mainline, flags, &parent);
+	if (failed)
+		return failed;
+
+	if (ff_ok && parent) {
+		unsigned char head_sha1[20];
+
+		if (get_sha1("HEAD", head_sha1))
+			die("You do not have a valid HEAD.");
+
+		if (!hashcmp(parent->object.sha1, head_sha1)) {
+			char *hex = sha1_to_hex(commit->object.sha1);
+			int res = reset(hex, prefix, HARD, 0, 0, 0, NULL);
+			if (!res)
+				fprintf(stderr, "Fast-forward to %s\n", hex);
+			return res;
+		}
+	}
+
 	if (!commit->buffer)
 		return error("Cannot get commit message for %s",
 				sha1_to_hex(commit->object.sha1));
@@ -218,10 +251,6 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 					git_commit_encoding, encoding)))
 		commit->buffer = reencoded_message;
 
-	failed = check_parent(commit, mainline, flags, &parent);
-	if (failed)
-		return failed;
-
 	failed = pick_commit(commit, parent, flags, &msgbuf);
 	if (failed < 0) {
 		exit(1);
@@ -271,11 +300,11 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	if (isatty(0))
 		edit = 1;
 	flags = PICK_REVERSE | PICK_ADD_NOTE;
-	return revert_or_cherry_pick(argc, argv);
+	return revert_or_cherry_pick(argc, argv, prefix);
 }
 
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	flags = 0;
-	return revert_or_cherry_pick(argc, argv);
+	return revert_or_cherry_pick(argc, argv, prefix);
 }
-- 
1.7.0.321.g2d270

[PATCH v2 09/12] cherry-pick: add tests for new --ff option

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t3506-cherry-pick-ff.sh |   98 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 98 insertions(+), 0 deletions(-)
 create mode 100755 t/t3506-cherry-pick-ff.sh

diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
new file mode 100755
index 0000000..e17ae71
--- /dev/null
+++ b/t/t3506-cherry-pick-ff.sh
@@ -0,0 +1,98 @@
+#!/bin/sh
+
+test_description='test cherry-picking with --ff option'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo first > file1 &&
+	git add file1 &&
+	test_tick &&
+	git commit -m "first" &&
+	git tag first &&
+
+	git checkout -b other &&
+	echo second >> file1 &&
+	git add file1 &&
+	test_tick &&
+	git commit -m "second" &&
+	git tag second
+'
+
+test_expect_success 'cherry-pick using --ff fast forwards' '
+	git checkout master &&
+	git reset --hard first &&
+	test_tick &&
+	git cherry-pick --ff second &&
+	test "$(git rev-parse --verify HEAD)" = "$(git rev-parse --verify second)"
+'
+
+test_expect_success 'cherry-pick not using --ff does not fast forwards' '
+	git checkout master &&
+	git reset --hard first &&
+	test_tick &&
+	git cherry-pick second &&
+	test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify second)"
+'
+
+#
+# We setup the following graph:
+#
+#	      B---C
+#	     /   /
+#	first---A
+#
+# (This has been taken from t3502-cherry-pick-merge.sh)
+#
+test_expect_success 'merge setup' '
+	git checkout master &&
+	git reset --hard first &&
+	echo new line >A &&
+	git add A &&
+	test_tick &&
+	git commit -m "add line to A" A &&
+	git tag A &&
+	git checkout -b side first &&
+	echo new line >B &&
+	git add B &&
+	test_tick &&
+	git commit -m "add line to B" B &&
+	git tag B &&
+	git checkout master &&
+	git merge side &&
+	git tag C &&
+	git checkout -b new A
+'
+
+test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '
+	git reset --hard A -- &&
+	test_must_fail git cherry-pick --ff -m 1 B &&
+	git diff --exit-code A --
+'
+
+test_expect_success 'cherry pick a merge with --ff but without -m should fail' '
+	git reset --hard A -- &&
+	test_must_fail git cherry-pick --ff C &&
+	git diff --exit-code A --
+'
+
+test_expect_success 'cherry pick with --ff a merge (1)' '
+	git reset --hard A -- &&
+	git cherry-pick --ff -m 1 C &&
+	git diff --exit-code C &&
+	test "$(git rev-parse --verify HEAD)" = "$(git rev-parse --verify C)"
+'
+
+test_expect_success 'cherry pick with --ff a merge (2)' '
+	git reset --hard B -- &&
+	git cherry-pick --ff -m 2 C &&
+	git diff --exit-code C &&
+	test "$(git rev-parse --verify HEAD)" = "$(git rev-parse --verify C)"
+'
+
+test_expect_success 'cherry pick a merge relative to nonexistent parent with --ff should fail' '
+	git reset --hard B -- &&
+	test_must_fail git cherry-pick --ff -m 3 C
+'
+
+test_done
-- 
1.7.0.321.g2d270

[PATCH v2 10/12] Documentation: describe new cherry-pick --ff option

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-cherry-pick.txt |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 78f4714..d71607a 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -7,7 +7,7 @@ git-cherry-pick - Apply the change introduced by an existing commit
 
 SYNOPSIS
 --------
-'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] <commit>
+'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>
 
 DESCRIPTION
 -----------
@@ -70,6 +70,10 @@ effect to your index in a row.
 --signoff::
 	Add Signed-off-by line at the end of the commit message.
 
+--ff::
+	If the current HEAD is the same as the parent of the
+	cherry-pick'ed commit, then a fast forward to this commit will
+	be performed.
 
 Author
 ------
-- 
1.7.0.321.g2d270

[PATCH v2 11/12] cherry-pick: add a no-op --no-ff option to future proof scripts

[Christian Couder]

A --ff option to allow "git cherry-pick" to fast forward if
possible was added in a previous patch, and this behavior may
become the default one in the future.

So to future proof scripts that may rely on the current behavior
it is safer to add a --no-ff option to make sure that the current
behavior will be used.

Requested-by: Paolo Bonzini <bonzini@gnu.org>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-cherry-pick.txt |    6 +++++-
 builtin/revert.c                  |    4 +++-
 t/t3506-cherry-pick-ff.sh         |    8 ++++++++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index d71607a..dfc8243 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -7,7 +7,7 @@ git-cherry-pick - Apply the change introduced by an existing commit
 
 SYNOPSIS
 --------
-'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>
+'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--[no-]ff] <commit>
 
 DESCRIPTION
 -----------
@@ -75,6 +75,10 @@ effect to your index in a row.
 	cherry-pick'ed commit, then a fast forward to this commit will
 	be performed.
 
+--no-ff::
+	Does nothing right now, but in the future this may disallow
+	fast forward if it becomes the default behavior.
+
 Author
 ------
 Written by Junio C Hamano <gitster@pobox.com>
diff --git a/builtin/revert.c b/builtin/revert.c
index b3e1fea..1a650e4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -32,7 +32,7 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_commit, mainline, signoff, ff_ok;
+static int edit, no_commit, mainline, signoff, ff_ok, no_ff;
 static int flags;
 static struct commit *commit;
 static const char *commit_name;
@@ -59,6 +59,7 @@ static void parse_args(int argc, const char **argv)
 		OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
 		OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
 		OPT_BOOLEAN(0, "ff", &ff_ok, "allow fast forward"),
+		OPT_BOOLEAN(0, "no-ff", &no_ff, "disallow fast forward"),
 		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
 		OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 		OPT_END(),
@@ -78,6 +79,7 @@ static void parse_args(int argc, const char **argv)
 		die_if_ff_incompatible(flags & PICK_ADD_NOTE, "-x");
 		die_if_ff_incompatible(no_commit, "--no-commit");
 		die_if_ff_incompatible(signoff, "--signoff");
+		die_if_ff_incompatible(no_ff, "--no-ff");
 	}
 }
 
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index e17ae71..2d7e532 100755
--- a/t/t3506-cherry-pick-ff.sh
+++ b/t/t3506-cherry-pick-ff.sh
@@ -35,6 +35,14 @@ test_expect_success 'cherry-pick not using --ff does not fast forwards' '
 	test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify second)"
 '
 
+test_expect_success 'cherry-pick using --no-ff does not fast forwards' '
+	git checkout master &&
+	git reset --hard first &&
+	test_tick &&
+	git cherry-pick --no-ff second &&
+	test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify second)"
+'
+
 #
 # We setup the following graph:
 #
-- 
1.7.0.321.g2d270

[PATCH v2 12/12] rebase -i: use new --ff cherry-pick option

[Christian Couder]

This simplifies rebase -i a little bit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 git-rebase--interactive.sh |   15 +++------------
 1 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 415ae72..a57f043 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -230,8 +230,8 @@ do_with_author () {
 }
 
 pick_one () {
-	no_ff=
-	case "$1" in -n) sha1=$2; no_ff=t ;; *) sha1=$1 ;; esac
+	ff=--ff
+	case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
 	output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
 	test -d "$REWRITTEN" &&
 		pick_one_preserving_merges "$@" && return
@@ -240,16 +240,7 @@ pick_one () {
 		output git cherry-pick "$@"
 		return
 	fi
-	parent_sha1=$(git rev-parse --verify $sha1^) ||
-		die "Could not get the parent of $sha1"
-	current_sha1=$(git rev-parse --verify HEAD)
-	if test -z "$no_ff" && test "$current_sha1" = "$parent_sha1"
-	then
-		output git reset --hard $sha1
-		output warn Fast-forward to $(git rev-parse --short $sha1)
-	else
-		output git cherry-pick "$@"
-	fi
+	output git cherry-pick $ff "$@"
 }
 
 pick_one_preserving_merges () {
-- 
1.7.0.321.g2d270

Re: [PATCH v2 00/12] add --ff option to cherry-pick

[Junio C Hamano]

Thanks, but this seems to conflict with too many things in flight (it
applies cleanly on top of 'pu' but not on top of 'next').

Given that "rebase--interactive", which is the sole in-tree user of
cherry-pick, has its own fast-forwarding logic to skip call to it, it
seems to take too much time out of me to deal with the code churn for
dubious benefit---the series does not seem to solve any real problem.

After other topics have either graduated to 'master' or dropped out of
'pu', things might look differently, though.

Re: [PATCH v2 00/12] add --ff option to cherry-pick

[Daniel Barkalow]

On Sun, 28 Feb 2010, Junio C Hamano wrote:

> Given that "rebase--interactive", which is the sole in-tree user of
> cherry-pick, has its own fast-forwarding logic to skip call to it, it
> seems to take too much time out of me to deal with the code churn for
> dubious benefit---the series does not seem to solve any real problem.

"git cherry-pick" is a porcelain command, so it shouldn't be evaluated on 
the basis of in-tree users, but on whether people running it from the 
command line would find it helpful.

Of course, I don't think it's especially high-value there, either.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH v2 00/12] add --ff option to cherry-pick

[Christian Couder]

On Monday 01 March 2010 04:49:12 Junio C Hamano wrote:
> Thanks, but this seems to conflict with too many things in flight (it
> applies cleanly on top of 'pu' but not on top of 'next').
> 
> Given that "rebase--interactive", which is the sole in-tree user of
> cherry-pick, has its own fast-forwarding logic to skip call to it, it
> seems to take too much time out of me to deal with the code churn for
> dubious benefit---the series does not seem to solve any real problem.
> 
> After other topics have either graduated to 'master' or dropped out of
> 'pu', things might look differently, though.
 
Ok I will wait for something like a week, and then rebase on top of next and 
resend.

Thanks,
Christian.

Re: [PATCH v2 00/12] add --ff option to cherry-pick

[Johannes Sixt]

Christian Couder schrieb:
> The goal of this patch series is to make it possible for "git cherry-pick"
> to fast forward instead of creating a new commit if the cherry picked commit
> has the same parent as the one we are cherry-picking on.

Why don't you just divert to 'git merge --ff' in this case?

-- Hannes

(PS: Please _do_not_ Cc me if/when you resend this series.)

Re: [PATCH v2 00/12] add --ff option to cherry-pick

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> On Monday 01 March 2010 04:49:12 Junio C Hamano wrote:
>> Thanks, but this seems to conflict with too many things in flight (it
>> applies cleanly on top of 'pu' but not on top of 'next').
>> 
>> Given that "rebase--interactive", which is the sole in-tree user of
>> cherry-pick, has its own fast-forwarding logic to skip call to it, it
>> seems to take too much time out of me to deal with the code churn for
>> dubious benefit---the series does not seem to solve any real problem.
>> 
>> After other topics have either graduated to 'master' or dropped out of
>> 'pu', things might look differently, though.
>  
> Ok I will wait for something like a week, and then rebase on top of next and 
> resend.

Actually, waiting, rebasing and resending, without any simplification,
would be the worst thing you could do.  Perhaps the "waiting" time can be
used to think how this can be simplified not to be such a big churn.

For example, why wouldn't the core of a "cherry-pick --ff" be something
like the attached patch, which obviously does not have "fast_forward_to"
yet, but whose implementation should be obvious (the code should already
be in "merge --ff" fast forward codepath, although I didn't look)?

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

diff --git a/builtin-revert.c b/builtin-revert.c
index eff5268..50a40d3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -284,8 +284,6 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	}
 	discard_cache();
 
-	index_fd = hold_locked_index(&index_lock, 1);
-
 	if (!commit->parents) {
 		if (action == REVERT)
 			die ("Cannot revert a root commit");
@@ -314,6 +312,16 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	else
 		parent = commit->parents->item;
 
+	if (action == CHERRY_PICK && user_asked_fast_forward &&
+	    !no_commit &&
+	    !edit &&
+	    !no_replay
+	    parent &&
+	    !hashcmp(parent->object.sha1, head))
+		return fast_forward_to(parent);
+
+	index_fd = hold_locked_index(&index_lock, 1);
+
 	if (!(message = commit->buffer))
 		die ("Cannot get commit message for %s",
 				sha1_to_hex(commit->object.sha1));

Re: [PATCH v2 00/12] add --ff option to cherry-pick

[Paolo Bonzini]

On 03/01/2010 09:20 AM, Johannes Sixt wrote:
> Christian Couder schrieb:
>> The goal of this patch series is to make it possible for "git cherry-pick"
>> to fast forward instead of creating a new commit if the cherry picked commit
>> has the same parent as the one we are cherry-picking on.
>
> Why don't you just divert to 'git merge --ff' in this case?

Because the purpose of the series is exactly to replace

   if A^ == HEAD; then
     git merge --ff A   # or git reset --hard A
   else
     git cherry-pick A
   fi

with a single "git cherry-pick --ff A".

An alternative would be the old ff-strategy series which would allow 
something like

   git merge --ff=only A || git cherry-pick A

Paolo

Re: [PATCH v2 00/12] add --ff option to cherry-pick

[Christian Couder]

On Monday 01 March 2010 09:48:45 Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > On Monday 01 March 2010 04:49:12 Junio C Hamano wrote:
> >> Thanks, but this seems to conflict with too many things in flight (it
> >> applies cleanly on top of 'pu' but not on top of 'next').
> >>
> >> Given that "rebase--interactive", which is the sole in-tree user of
> >> cherry-pick, has its own fast-forwarding logic to skip call to it, it
> >> seems to take too much time out of me to deal with the code churn for
> >> dubious benefit---the series does not seem to solve any real problem.
> >>
> >> After other topics have either graduated to 'master' or dropped out of
> >> 'pu', things might look differently, though.
> >
> > Ok I will wait for something like a week, and then rebase on top of next
> > and resend.
> 
> Actually, waiting, rebasing and resending, without any simplification,
> would be the worst thing you could do.  Perhaps the "waiting" time can be
> used to think how this can be simplified not to be such a big churn.

Ok, I removed some refactoring that was not really needed for this.

> For example, why wouldn't the core of a "cherry-pick --ff" be something
> like the attached patch, which obviously does not have "fast_forward_to"
> yet, but whose implementation should be obvious (the code should already
> be in "merge --ff" fast forward codepath, although I didn't look)?

I tried to use the checkout_fast_forward() function from builtin/merge.c but 
unfortunately it doesn't work. It gives an error like that in the tests :

error: Your local changes to 'file1' would be overwritten by merge.  Aborting.
Please, commit your changes or stash them before you can merge.

and I don't really understand why. (Though I didn't spend a lot of time on 
this.)

So the next version still refactors builtin/reset.c to create and then use a 
reset() function.

Thanks,
Christian.

Re: [PATCH v2 00/12] add --ff option to cherry-pick

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> I tried to use the checkout_fast_forward() function from builtin/merge.c but 
> unfortunately it doesn't work. It gives an error like that in the tests :
>
> error: Your local changes to 'file1' would be overwritten by merge.  Aborting.
> Please, commit your changes or stash them before you can merge.
>
> and I don't really understand why. (Though I didn't spend a lot of time on 
> this.)

Shouldn't it be something like this?  The patch is still unnecessarily
large because I wanted to share options between revert and cherry-pick
while giving --ff only to cherry-pick, but I don't understand why it needs
a nine patch series worth of code churn.



 builtin-merge.c  |    2 +-
 builtin-revert.c |   44 +++++++++++++++++++++++++++++++++++++++++---
 cache.h          |    3 +++
 parse-options.c  |   15 +++++++++++++++
 parse-options.h  |    1 +
 5 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index 3aaec7b..c043066 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -667,7 +667,7 @@ static int count_unmerged_entries(void)
 	return ret;
 }
 
-static int checkout_fast_forward(unsigned char *head, unsigned char *remote)
+int checkout_fast_forward(const unsigned char *head, const unsigned char *remote)
 {
 	struct tree *trees[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
diff --git a/builtin-revert.c b/builtin-revert.c
index eff5268..bfe75c8 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -13,6 +13,7 @@
 #include "revision.h"
 #include "rerere.h"
 #include "merge-recursive.h"
+#include "refs.h"
 
 /*
  * This implements the builtins revert and cherry-pick.
@@ -35,7 +36,7 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_replay, no_commit, mainline, signoff;
+static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
 static enum { REVERT, CHERRY_PICK } action;
 static struct commit *commit;
 static const char *commit_name;
@@ -60,8 +61,19 @@ static void parse_args(int argc, const char **argv)
 		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
 		OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 		OPT_END(),
+		OPT_END(),
+		OPT_END(),
 	};
 
+	if (action == CHERRY_PICK) {
+		struct option cp_extra[] = {
+			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
+			OPT_END(),
+		};
+		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
+			die("program error");
+	}
+
 	if (parse_options(argc, argv, NULL, options, usage_str, 0) != 1)
 		usage_with_options(usage_str, options);
 
@@ -244,6 +256,17 @@ static NORETURN void die_dirty_index(const char *me)
 	}
 }
 
+static int fast_forward_to(const unsigned char *to, const unsigned char *from)
+{
+	struct ref_lock *ref_lock;
+
+	read_cache();
+	if (checkout_fast_forward(from, to))
+		exit(1); /* the callee should have complained already */
+	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
+	return write_ref_sha1(ref_lock, to, "cherry-pick");
+}
+
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
 	unsigned char head[20];
@@ -265,6 +288,17 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	if (action == REVERT && !no_replay)
 		die("revert is incompatible with replay");
 
+	if (action == CHERRY_PICK && allow_ff) {
+		if (signoff)
+			die("cherry-pick --ff cannot be used with --signoff");
+		if (no_commit)
+			die("cherry-pick --ff cannot be used with --no-commit");
+		if (no_replay)
+			die("cherry-pick --ff cannot be used with -x");
+		if (edit)
+			die("cherry-pick --ff cannot be used with --edit");
+	}
+
 	if (read_cache() < 0)
 		die("git %s: failed to read the index", me);
 	if (no_commit) {
@@ -284,8 +318,6 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	}
 	discard_cache();
 
-	index_fd = hold_locked_index(&index_lock, 1);
-
 	if (!commit->parents) {
 		if (action == REVERT)
 			die ("Cannot revert a root commit");
@@ -314,6 +346,10 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	else
 		parent = commit->parents->item;
 
+	if (action == CHERRY_PICK && allow_ff
+	    && !hashcmp(parent->object.sha1, head))
+		return fast_forward_to(commit->object.sha1, head);
+
 	if (!(message = commit->buffer))
 		die ("Cannot get commit message for %s",
 				sha1_to_hex(commit->object.sha1));
@@ -343,6 +379,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 
 	oneline = get_oneline(message);
 
+	index_fd = hold_locked_index(&index_lock, 1);
+
 	if (action == REVERT) {
 		char *oneline_body = strchr(oneline, ' ');
 
diff --git a/cache.h b/cache.h
index d454b7e..7272160 100644
--- a/cache.h
+++ b/cache.h
@@ -1040,4 +1040,7 @@ 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/parse-options.c b/parse-options.c
index c83035d..8546d85 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -659,3 +659,18 @@ int parse_opt_tertiary(const struct option *opt, const char *arg, int unset)
 	*target = unset ? 2 : 1;
 	return 0;
 }
+
+int parse_options_concat(struct option *dst, size_t dst_size, struct option *src)
+{
+	int i, j;
+
+	for (i = 0; i < dst_size; i++)
+		if (dst[i].type == OPTION_END)
+			break;
+	for (j = 0; i < dst_size; i++, j++) {
+		dst[i] = src[j];
+		if (src[j].type == OPTION_END)
+			return 0;
+	}
+	return -1;
+}
diff --git a/parse-options.h b/parse-options.h
index 9429f7e..7581e93 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -187,6 +187,7 @@ extern int parse_options_step(struct parse_opt_ctx_t *ctx,
 
 extern int parse_options_end(struct parse_opt_ctx_t *ctx);
 
+extern int parse_options_concat(struct option *dst, size_t, struct option *src);
 
 /*----- some often used options -----*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);

Re: [PATCH v2 00/12] add --ff option to cherry-pick

[Christian Couder]

On Thursday 04 March 2010 04:31:25 Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > I tried to use the checkout_fast_forward() function from builtin/merge.c
> > but unfortunately it doesn't work. It gives an error like that in the
> > tests :
> >
> > error: Your local changes to 'file1' would be overwritten by merge. 
> > Aborting. Please, commit your changes or stash them before you can merge.
> >
> > and I don't really understand why. (Though I didn't spend a lot of time
> > on this.)
> 
> Shouldn't it be something like this?  The patch is still unnecessarily
> large because I wanted to share options between revert and cherry-pick
> while giving --ff only to cherry-pick, but I don't understand why it needs
> a nine patch series worth of code churn.

Please have another look at the patch series and realize that only the first 4 
patches are (trivial) refactoring (that you call "code churn"), so I think 
it's not fair at all to say that it's "a nine patch series worth of code 
churn".

And it used to be accepted to do some refactoring, and IMHO some reasonable 
amount of refactoring is good, as long as it is well done of course. So there 
is no problem if you criticize the amount in term of line of codes or if you 
criticize the quality of refactoring. But I don't think the patch count is 
right metric (especially when applied to the whole series).

> +static int fast_forward_to(const unsigned char *to, const unsigned char
>  *from) +{
> +	struct ref_lock *ref_lock;
> +
> +	read_cache();
> +	if (checkout_fast_forward(from, to))
> +		exit(1); /* the callee should have complained already */
> +	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
> +	return write_ref_sha1(ref_lock, to, "cherry-pick");
> +}

Ok, so your patch adds new code to do the job, while my refactoring patches 
only move existing code into new functions. So I think the net result is that 
your patch in fact increases the complexity of the code base and duplicates 
existing functionality, while my patches reuse and increase readability of 
existing code.

Now it's true that your patch adds a very small amount of new code and maybe I 
am missing many things and your patch is much better for many reasons that I 
can't see. If that is the case I am sorry to be so stupid.

I also agree that it might not be the right time for refactoring, and that 
it's more work for reviewers, especially yourself, and you (and other 
reviewers) probably just need less work and not more, but in the long run I 
think that reusing existing code is better as that means less maintenance 
work.

Best regards,
Christian.

Re: [PATCH v2 00/12] add --ff option to cherry-pick

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

>> +static int fast_forward_to(const unsigned char *to, const unsigned char
>>  *from) +{
>> +	struct ref_lock *ref_lock;
>> +
>> +	read_cache();
>> +	if (checkout_fast_forward(from, to))
>> +		exit(1); /* the callee should have complained already */
>> +	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
>> +	return write_ref_sha1(ref_lock, to, "cherry-pick");
>> +}
>
> Ok, so your patch adds new code to do the job,...

Come on.

The above calls an _existing_ code to go from one rev to another, and
another _existing_ code to update the HEAD pointer with a reflog record.
I could have inlined this at its sole callsite just so that I won't give
you an excuse to quibble like you just did, but I thought that would make
the code less readable.

Also I thought that you knew better than that, and I wouldn't need such a
"quibble blocker".  Please don't disappoint me.

In other words, it was a demonstration that no refactoring was necessary
to do what you seem to have wanted to do.  We already had API functions
that exactly match what we wanted.  No need to add "pick.c" file, no need
to touch reset.c, no need to move the code around to add a check_parent()
that is called from only one place.

It is a separate issue if there are some other functions that are totally
irrelevant to the immediate goal of implementing your "cherry-pick --ff"
(which is the title of the series) that do something similar to what
checkout_fast_forward() does.  Maybe they need to be rewritten in terms of
checkout_fast_forward(); maybe all of them and checkout_fast_forward() can
be split into smaller pieces and get their logically freestanding parts
consolidated into a common helper function.  But by conflating that into
your series, you made a lot more work to review and judge the merit of it;
I don't think it was necessarily.

You saw a handful of "*-refactor" patches queued recently.  They were all
"function X, Y and Z do exactly the same thing; add one common code and
call it from these places".  That kind of patch does not _fix_ nor improve
anything observable from the outside, but at the same time it is easy to
review.  The only thing that is necessary is to verify the claim "X, Y and
Z do the same thing" and validate the refactored code do the same thing
for X, Y and Z, and the review can _end there_.  I think you've been here
long enough to realize the difference.

Re: [PATCH 3/6] revert: add --ff option to allow fast forward when cherry-picking

[Jonathan Nieder]

Junio C Hamano wrote:

> diff --git a/builtin-revert.c b/builtin-revert.c
> index eff5268..bfe75c8 100644
> --- a/builtin-revert.c
> +++ b/builtin-revert.c
> @@ -314,6 +346,10 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  	else
>  		parent = commit->parents->item;
>  
> +	if (action == CHERRY_PICK && allow_ff
> +	    && !hashcmp(parent->object.sha1, head))
> +		return fast_forward_to(commit->object.sha1, head);
> +
>  	if (!(message = commit->buffer))
>  		die ("Cannot get commit message for %s",
>  				sha1_to_hex(commit->object.sha1));

Here’s a small fix to go on top.

-- %< --
Subject: revert: fix tiny memory leak in cherry-pick --ff

We forgot to free defmsg when returning early for a fast-forward.

Fixing this should reduce noise during test suite runs with valgrind.
More importantly, once cherry-pick learns to pick multiple commits,
the amount of memory leaked would start to add up.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin-revert.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index 476f41e..9a3c14c 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -274,7 +274,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	int i, index_fd, clean;
 	char *oneline, *reencoded_message = NULL;
 	const char *message, *encoding;
-	char *defmsg = git_pathdup("MERGE_MSG");
+	char *defmsg = NULL;
 	struct merge_options o;
 	struct tree *result, *next_tree, *base_tree, *head_tree;
 	static struct lock_file index_lock;
@@ -364,6 +364,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	 * reverse of it if we are revert.
 	 */
 
+	defmsg = git_pathdup("MERGE_MSG");
 	msg_fd = hold_lock_file_for_update(&msg_file, defmsg,
 					   LOCK_DIE_ON_ERROR);
 
-- 
1.7.0.2