threads / patch / 22859

v2, 12 partsadd --ff option to cherry-pick

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

## tl;dr

24 messages between Feb 28, 2010 and Mar 21, 2010. Diffs are folded; open one to read it.

replies: 23people: 6as markdown or json

Christian Couder· Feb 28, 2010, 22:21 UTC · lore

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
Christian Couder· Feb 28, 2010, 22:21 UTC · re: Christian Couder · lore

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

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
Show changes to 4 files +277 −228

Makefile, builtin/revert.c, pick.c, 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
Christian Couder· Feb 28, 2010, 22:21 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 pick.c |   69 ++++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 39 insertions(+), 30 deletions(-)
Show changes to pick.c +39 −30
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
Christian Couder· Feb 28, 2010, 22:21 UTC · re: Christian Couder · lore

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

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(-)
Show changes to 2 files +2 −1

pick.c, pick.h

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
Christian Couder· Feb 28, 2010, 22:21 UTC · re: Christian Couder · lore

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

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(-)
Show changes to 3 files +11 −10

builtin/revert.c, pick.c, pick.h

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
Christian Couder· Feb 28, 2010, 22:22 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/reset.c |   42 ++++++++++++++++++++++++++----------------
 1 files changed, 26 insertions(+), 16 deletions(-)
Show changes to builtin/reset.c +26 −16
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
Christian Couder· Feb 28, 2010, 22:22 UTC · re: Christian Couder · lore

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

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(-)
Show changes to builtin/reset.c +74 −66
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
Christian Couder· Feb 28, 2010, 22:22 UTC · re: Christian Couder · lore

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

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
Show changes to 2 files +14 −4

builtin/reset.c, 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
Christian Couder· Feb 28, 2010, 22:22 UTC · re: Christian Couder · lore

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

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(-)
Show changes to builtin/revert.c +37 −8
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
Christian Couder· Feb 28, 2010, 22:22 UTC · re: Christian Couder · lore

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

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
Show changes to t/t3506-cherry-pick-ff.sh +98 −0
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
Christian Couder· Feb 28, 2010, 22:22 UTC · re: Christian Couder · lore

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

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-cherry-pick.txt |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
Show changes to Documentation/git-cherry-pick.txt +5 −1
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
Christian Couder· Feb 28, 2010, 22:22 UTC · re: Christian Couder · lore

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

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(-)
Show changes to 3 files +16 −2

Documentation/git-cherry-pick.txt, builtin/revert.c, t/t3506-cherry-pick-ff.sh

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
Christian Couder· Feb 28, 2010, 22:22 UTC · re: Christian Couder · lore

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

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(-)
Show changes to git-rebase--interactive.sh +3 −12
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
Junio C Hamano· Mar 1, 2010, 03:49 UTC · re: Christian Couder · lore

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

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.

Daniel Barkalow· Mar 1, 2010, 04:42 UTC · re: Junio C Hamano · lore

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

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*
Christian Couder· Mar 1, 2010, 07:00 UTC · re: Junio C Hamano · lore

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

On Monday 01 March 2010 04:49:12 Junio C Hamano wrote:
Show 10 quoted lines
> 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.

Junio C Hamano· Mar 1, 2010, 08:48 UTC · re: Christian Couder · lore

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

Christian Couder <chriscool@tuxfamily.org> writes:
Show 14 quoted lines
> 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(-)
Show changes to builtin-revert.c +10 −2
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));
Christian Couder· Mar 4, 2010, 02:06 UTC · re: Junio C Hamano · lore

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

On Monday 01 March 2010 09:48:45 Junio C Hamano wrote:
Show 19 quoted lines
> 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.

Junio C Hamano· Mar 4, 2010, 03:31 UTC · re: Christian Couder · lore

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

Christian Couder <chriscool@tuxfamily.org> writes:
Show 8 quoted lines
> 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(-)
Show changes to 5 files +61 −4

builtin-merge.c, builtin-revert.c, cache.h, parse-options.c, parse-options.h

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);
Christian Couder· Mar 4, 2010, 06:55 UTC · re: Junio C Hamano · lore

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

On Thursday 04 March 2010 04:31:25 Junio C Hamano wrote:
Show 15 quoted lines
> 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).

Show 10 quoted lines
> +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.

Junio C Hamano· Mar 4, 2010, 20:48 UTC · re: Christian Couder · lore

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

Christian Couder <chriscool@tuxfamily.org> writes:
Show 12 quoted lines
>> +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.

Jonathan Nieder· Mar 21, 2010, 02:01 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 15 quoted lines
> 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(-)
Show changes to builtin-revert.c +2 −1
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
Johannes Sixt· Mar 1, 2010, 08:20 UTC · re: Christian Couder · lore

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

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.)
Paolo Bonzini· Mar 1, 2010, 12:34 UTC · re: Johannes Sixt · lore

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

On 03/01/2010 09:20 AM, Johannes Sixt wrote:
Show 6 quoted lines
> 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

← back to recent threads