[PATCH v3 0/9] more changes to port rebase -i to C using sequencer code

[PATCH v3 0/9] more changes to port rebase -i to C using sequencer code

[Christian Couder]

This is another reroll of the series I first sent on August the 12th.

The only changes since v2 are in the commit messages. References to the
pick() function have been replaced by references to pick_commit(), and I
tried to describe the purpose of the patch first. 

Christian Couder (5):
  sequencer: add "--fast-forward" option to "git sequencer--helper"
  sequencer: let "git sequencer--helper" callers set "allow_dirty"
  rebase -i: use "git sequencer--helper --fast-forward"
  pick: libify "pick_help_msg()"
  rebase -i: use "git sequencer--helper --cherry-pick"

Stephan Beyer (4):
  sequencer: add "do_fast_forward()" to perform a fast forward
  revert: libify pick
  sequencer: add "do_commit()" and related functions
  sequencer: add "--cherry-pick" option to "git sequencer--helper"

 Makefile                    |    2 +
 builtin-revert.c            |  293 ++++++-----------------------------------
 builtin-sequencer--helper.c |  305 +++++++++++++++++++++++++++++++++++++++++--
 git-rebase--interactive.sh  |   22 ++--
 pick.c                      |  232 ++++++++++++++++++++++++++++++++
 pick.h                      |   14 ++
 6 files changed, 599 insertions(+), 269 deletions(-)
 create mode 100644 pick.c
 create mode 100644 pick.h

[PATCH v3 1/9] sequencer: add "do_fast_forward()" to perform a fast forward

[Christian Couder]

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

This code is taken from the sequencer GSoC project:

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

    (commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20)

but the messages have been changed to be the same as those
displayed by git-rebase--interactive.sh.

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>
---
 builtin-sequencer--helper.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/builtin-sequencer--helper.c b/builtin-sequencer--helper.c
index be030bc..0cd7e98 100644
--- a/builtin-sequencer--helper.c
+++ b/builtin-sequencer--helper.c
@@ -171,6 +171,15 @@ static struct commit *get_commit(const char *arg)
 	return lookup_commit_reference(sha1);
 }
 
+static int do_fast_forward(const unsigned char *sha)
+{
+	if (reset_almost_hard(sha))
+		return error("Cannot fast forward to %s", sha1_to_hex(sha));
+	if (verbosity > 1)
+		printf("Fast forward to %s\n", sha1_to_hex(sha));
+	return 0;
+}
+
 static int set_verbosity(int verbose)
 {
 	char tmp[] = "0";
-- 
1.6.4.271.ge010d

[PATCH v3 2/9] sequencer: add "--fast-forward" option to "git sequencer--helper"

[Christian Couder]

This new option uses the "do_fast_forward()" function to perform
a fast forward.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-sequencer--helper.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/builtin-sequencer--helper.c b/builtin-sequencer--helper.c
index 0cd7e98..bd72f65 100644
--- a/builtin-sequencer--helper.c
+++ b/builtin-sequencer--helper.c
@@ -19,6 +19,7 @@ static unsigned char head_sha1[20];
 static const char * const git_sequencer_helper_usage[] = {
 	"git sequencer--helper --make-patch <commit>",
 	"git sequencer--helper --reset-hard <commit> <reflog-msg> <verbosity>",
+	"git sequencer--helper --fast-forward <commit> <reflog-msg> <verbosity>",
 	NULL
 };
 
@@ -218,11 +219,14 @@ int cmd_sequencer__helper(int argc, const char **argv, const char *prefix)
 {
 	char *patch_commit = NULL;
 	char *reset_commit = NULL;
+	char *ff_commit = NULL;
 	struct option options[] = {
 		OPT_STRING(0, "make-patch", &patch_commit, "commit",
 			   "create a patch from commit"),
 		OPT_STRING(0, "reset-hard", &reset_commit, "commit",
 			   "reset to commit"),
+		OPT_STRING(0, "fast-forward", &ff_commit, "commit",
+			   "fast forward to commit"),
 		OPT_END()
 	};
 
@@ -239,15 +243,16 @@ int cmd_sequencer__helper(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 
-	if (reset_commit) {
+	if (ff_commit || reset_commit) {
 		unsigned char sha1[20];
+		char *commit = ff_commit ? ff_commit : reset_commit;
 
 		if (argc != 2)
 			usage_with_options(git_sequencer_helper_usage,
 					   options);
 
-		if (get_sha1(reset_commit, sha1)) {
-			error("Could not find '%s'", reset_commit);
+		if (get_sha1(commit, sha1)) {
+			error("Could not find '%s'", commit);
 			return 1;
 		}
 
@@ -258,7 +263,10 @@ int cmd_sequencer__helper(int argc, const char **argv, const char *prefix)
 			return 1;
 		}
 
-		return reset_almost_hard(sha1);
+		if (ff_commit)
+			return do_fast_forward(sha1);
+		else
+			return reset_almost_hard(sha1);
 	}
 
 	usage_with_options(git_sequencer_helper_usage, options);
-- 
1.6.4.271.ge010d

[PATCH v3 3/9] sequencer: let "git sequencer--helper" callers set "allow_dirty"

[Christian Couder]

This flag can be set when using --reset-hard or --fast-forward, and
in this case changes in the work tree will be kept.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-sequencer--helper.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin-sequencer--helper.c b/builtin-sequencer--helper.c
index bd72f65..71a7fef 100644
--- a/builtin-sequencer--helper.c
+++ b/builtin-sequencer--helper.c
@@ -18,8 +18,10 @@ static unsigned char head_sha1[20];
 
 static const char * const git_sequencer_helper_usage[] = {
 	"git sequencer--helper --make-patch <commit>",
-	"git sequencer--helper --reset-hard <commit> <reflog-msg> <verbosity>",
-	"git sequencer--helper --fast-forward <commit> <reflog-msg> <verbosity>",
+	"git sequencer--helper --reset-hard <commit> <reflog-msg> "
+		"<verbosity> [<allow-dirty>]",
+	"git sequencer--helper --fast-forward <commit> <reflog-msg> "
+		"<verbosity> [<allow-dirty>]",
 	NULL
 };
 
@@ -247,7 +249,7 @@ int cmd_sequencer__helper(int argc, const char **argv, const char *prefix)
 		unsigned char sha1[20];
 		char *commit = ff_commit ? ff_commit : reset_commit;
 
-		if (argc != 2)
+		if (argc != 2 && argc != 3)
 			usage_with_options(git_sequencer_helper_usage,
 					   options);
 
@@ -263,6 +265,9 @@ int cmd_sequencer__helper(int argc, const char **argv, const char *prefix)
 			return 1;
 		}
 
+		if (argc == 3 && *argv[2] && strcmp(argv[2], "0"))
+			allow_dirty = 1;
+
 		if (ff_commit)
 			return do_fast_forward(sha1);
 		else
-- 
1.6.4.271.ge010d

[PATCH v3 4/9] rebase -i: use "git sequencer--helper --fast-forward"

[Christian Couder]

when fast forwarding.

Note that in the first hunk of this patch, there was this line:

test "a$1" = a-n && output git reset --soft $current_sha1

but the test always failed.

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

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0041994..7651fd6 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -154,11 +154,8 @@ pick_one () {
 		die "Could not get the parent of $sha1"
 	current_sha1=$(git rev-parse --verify HEAD)
 	if test "$no_ff$current_sha1" = "$parent_sha1"; then
-		git sequencer--helper --reset-hard $sha1 \
+		git sequencer--helper --fast-forward $sha1 \
 			"$GIT_REFLOG_ACTION" "$VERBOSE"
-		test "a$1" = a-n && output git reset --soft $current_sha1
-		sha1=$(git rev-parse --short $sha1)
-		output warn Fast forward to $sha1
 	else
 		output git cherry-pick "$@"
 	fi
@@ -238,10 +235,8 @@ pick_one_preserving_merges () {
 	done
 	case $fast_forward in
 	t)
-		output warn "Fast forward to $sha1"
-		git sequencer--helper --reset-hard $sha1 \
-			"$GIT_REFLOG_ACTION" "$VERBOSE" ||
-			die "Cannot fast forward to $sha1"
+		git sequencer--helper --fast-forward $sha1 \
+			"$GIT_REFLOG_ACTION" "$VERBOSE" || exit
 		;;
 	f)
 		first_parent=$(expr "$new_parents" : ' \([^ ]*\)')
-- 
1.6.4.271.ge010d

[PATCH v3 5/9] revert: libify pick

[Christian Couder]

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

The goal of this commit is to abstract out pick 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 commit is made of code from the sequencer GSoC project:

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

(commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)

And it contains some changes 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           |  210 +++++++++++++++++++++++++++++++++++++++++
 pick.h           |   13 +++
 4 files changed, 269 insertions(+), 228 deletions(-)
 create mode 100644 pick.c
 create mode 100644 pick.h

diff --git a/Makefile b/Makefile
index 98dd01d..5e76083 100644
--- a/Makefile
+++ b/Makefile
@@ -445,6 +445,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
@@ -533,6 +534,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 151aa6a..4797ac5 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,25 +31,23 @@ 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 *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];
 	const char *arg;
 	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"),
@@ -77,42 +71,12 @@ static void parse_args(int argc, const char **argv)
 		die ("'%s' does not point to a commit", arg);
 }
 
-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 */
@@ -128,30 +92,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;
@@ -214,7 +154,7 @@ static char *help_msg(const unsigned char *sha1)
 	       "mark the corrected paths with 'git add <paths>' "
 	       "or 'git rm <paths>' and commit the result.");
 
-	if (action == CHERRY_PICK) {
+	if (!(flags & PICK_REVERSE)) {
 		sprintf(helpbuf + strlen(helpbuf),
 			"\nWhen commiting, use the option "
 			"'-c %s' to retain authorship and message.",
@@ -223,187 +163,68 @@ static char *help_msg(const unsigned char *sha1)
 	return helpbuf;
 }
 
-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;
+	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 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: cannot %s", me);
-	}
-	discard_cache();
-
-	index_fd = hold_locked_index(&index_lock, 1);
+	if (!no_commit && index_differs_from("HEAD", 0))
+		die ("Dirty index: cannot %s", me);
 
-	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 (!(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);
-
-	if (action == REVERT) {
-		char *oneline_body = strchr(oneline, ' ');
+		commit->buffer = reencoded_message;
 
-		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->object.sha1));
+		write_message(&msgbuf, git_path("MERGE_MSG"));
 		rerere();
 		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.
@@ -421,14 +242,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;
 }
 
@@ -436,14 +254,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..058b877
--- /dev/null
+++ b/pick.c
@@ -0,0 +1,210 @@
+#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.
+ *
+ * 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, ' ');
+
+		base = commit;
+		next = parent;
+		strbuf_addstr(msg, "Revert \"");
+		strbuf_addstr(msg, oneline_body + 1);
+		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_addstr(msg, "\t");
+				strbuf_addstr(msg, ce->name);
+				strbuf_addstr(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.6.4.271.ge010d

[PATCH v3 6/9] pick: libify "pick_help_msg()"

[Christian Couder]

This function gives an help message when pick or revert failed.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-revert.c |   23 +----------------------
 pick.c           |   22 ++++++++++++++++++++++
 pick.h           |    1 +
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/builtin-revert.c b/builtin-revert.c
index 4797ac5..e5250bd 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -142,27 +142,6 @@ static void set_author_ident_env(const char *message)
 			sha1_to_hex(commit->object.sha1));
 }
 
-static char *help_msg(const unsigned char *sha1)
-{
-	static char helpbuf[1024];
-	char *msg = getenv("GIT_CHERRY_PICK_HELP");
-
-	if (msg)
-		return msg;
-
-	strcpy(helpbuf, "  After resolving the conflicts,\n"
-	       "mark the corrected paths with 'git add <paths>' "
-	       "or 'git rm <paths>' and commit the result.");
-
-	if (!(flags & PICK_REVERSE)) {
-		sprintf(helpbuf + strlen(helpbuf),
-			"\nWhen commiting, use the option "
-			"'-c %s' to retain authorship and message.",
-			find_unique_abbrev(sha1, DEFAULT_ABBREV));
-	}
-	return helpbuf;
-}
-
 static void write_message(struct strbuf *msgbuf, const char *filename)
 {
 	struct lock_file msg_file;
@@ -211,7 +190,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		exit(1);
 	} else if (failed > 0) {
 		fprintf(stderr, "Automatic %s failed.%s\n",
-			me, help_msg(commit->object.sha1));
+			me, pick_help_msg(commit->object.sha1, flags));
 		write_message(&msgbuf, git_path("MERGE_MSG"));
 		rerere();
 		exit(1);
diff --git a/pick.c b/pick.c
index 058b877..4f882bb 100644
--- a/pick.c
+++ b/pick.c
@@ -208,3 +208,25 @@ int pick_commit(struct commit *pick_commit, int mainline, int flags,
 
 	return ret;
 }
+
+char *pick_help_msg(const unsigned char *sha1, int flags)
+{
+	static char helpbuf[1024];
+	char *msg = getenv("GIT_CHERRY_PICK_HELP");
+
+	if (msg)
+		return msg;
+
+	strcpy(helpbuf, "  After resolving the conflicts,\n"
+	       "mark the corrected paths with 'git add <paths>' "
+	       "or 'git rm <paths>' and commit the result.");
+
+	if (!(flags & PICK_REVERSE)) {
+		sprintf(helpbuf + strlen(helpbuf),
+			"\nWhen commiting, use the option "
+			"'-c %s' to retain authorship and message.",
+			find_unique_abbrev(sha1, DEFAULT_ABBREV));
+	}
+	return helpbuf;
+}
+
diff --git a/pick.h b/pick.h
index 7a74ad8..115541a 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 char *pick_help_msg(const unsigned char *sha1, int flags);
 
 #endif
-- 
1.6.4.271.ge010d

[PATCH v3 7/9] sequencer: add "do_commit()" and related functions

[Christian Couder]

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

It adds "struct commit_info", the "next_commit" static variable and
the following functions:

        - do_commit()
        - set_author_info()
        - set_message_source()
        - set_pick_subject()
        - write_commit_summary_into()

This makes it possible to prepare and perform a commit (without
forking and execing "git commit").

This patch adds some code that comes from the sequencer GSoC project:

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

(commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)

Compared to the sequencer project, the only change is that "mark"
related (3 lines long) code has been removed from do_commit().

    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>
---
 builtin-sequencer--helper.c |  214 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 214 insertions(+), 0 deletions(-)

diff --git a/builtin-sequencer--helper.c b/builtin-sequencer--helper.c
index 71a7fef..61a8f2e 100644
--- a/builtin-sequencer--helper.c
+++ b/builtin-sequencer--helper.c
@@ -5,26 +5,69 @@
 #include "refs.h"
 #include "diff.h"
 #include "unpack-trees.h"
+#include "string-list.h"
+#include "pick.h"
+#include "rerere.h"
+#include "dir.h"
+#include "cache-tree.h"
+#include "utf8.h"
 
 #define SEQ_DIR "rebase-merge"
 
 #define PATCH_FILE	git_path(SEQ_DIR "/patch")
+#define MERGE_MSG	git_path("MERGE_MSG")
+#define SQUASH_MSG	git_path("SQUASH_MSG")
+
+/**********************************************************************
+ * Data structures
+ */
+
+struct user_info {
+	const char *name;
+	const char *mail;
+	const char *time; /* "<timestamp> <timezone>" */
+};
+
+struct commit_info {
+	struct user_info author; /* author info */
+	struct user_info committer; /* not used, but for easy extendability */
+	const char *encoding; /* encoding */
+	char *subject; /* basically the first line of the summary */
+	struct strbuf summary; /* the commit message */
+	char *source; /* source of the commit message, either
+		       * "message", "merge", "squash" or a commit SHA1 */
+	char *patch; /* a patch */
+	struct string_list parents; /* list of parents' hex'ed sha1 ids */
+};
+
+/**********************************************************************
+ * Global variables
+ */
 
 static char *reflog;
 
+static int squash_count = 0;
+
 static int allow_dirty = 0, verbosity = 1, advice = 1;
 
 static unsigned char head_sha1[20];
 
+static struct commit_info next_commit;
+
 static const char * const git_sequencer_helper_usage[] = {
 	"git sequencer--helper --make-patch <commit>",
 	"git sequencer--helper --reset-hard <commit> <reflog-msg> "
 		"<verbosity> [<allow-dirty>]",
 	"git sequencer--helper --fast-forward <commit> <reflog-msg> "
 		"<verbosity> [<allow-dirty>]",
+	"git sequencer--helper --cherry-pick <commit> [<do-not-commit>]",
 	NULL
 };
 
+/**********************************************************************
+ * Sequencer functions
+ */
+
 static int parse_and_init_tree_desc(const unsigned char *sha1,
 				    struct tree_desc *desc)
 {
@@ -162,6 +205,157 @@ static void make_patch(struct commit *commit)
 	free(args);
 }
 
+/* Commit current index with information next_commit onto parent_sha1. */
+static int do_commit(unsigned char *parent_sha1)
+{
+	int failed;
+	unsigned char tree_sha1[20];
+	unsigned char commit_sha1[20];
+	struct strbuf sbuf;
+	const char *reencoded = NULL;
+
+	if (squash_count) {
+		squash_count = 0;
+		if (file_exists(SQUASH_MSG))
+			unlink(SQUASH_MSG);
+	}
+
+	if (!index_differs_from("HEAD", 0) &&
+	    !next_commit.parents.nr)
+		return error("No changes! Do you really want an empty commit?");
+
+	if (!next_commit.author.name || !next_commit.author.mail)
+		return error("Internal error: Author information not set properly.");
+
+	if (write_cache_as_tree(tree_sha1, 0, NULL))
+		return 1;
+
+	if (!next_commit.encoding)
+		next_commit.encoding = xstrdup("utf-8");
+	if (!git_commit_encoding)
+		git_commit_encoding = "utf-8";
+
+	strbuf_init(&sbuf, 8192); /* should avoid reallocs for the headers */
+	strbuf_addf(&sbuf, "tree %s\n", sha1_to_hex(tree_sha1));
+	if (parent_sha1)
+		strbuf_addf(&sbuf, "parent %s\n", sha1_to_hex(parent_sha1));
+	if (next_commit.parents.nr) {
+		int i;
+		for (i = 0; i < next_commit.parents.nr; ++i)
+			strbuf_addf(&sbuf, "parent %s\n",
+					next_commit.parents.items[i].string);
+	}
+	if (!next_commit.author.time) {
+		char time[50];
+		datestamp(time, sizeof(time));
+		next_commit.author.time = xstrdup(time);
+	}
+
+	stripspace(&next_commit.summary, 1);
+
+	/* if encodings differ, reencode whole buffer */
+	if (strcasecmp(git_commit_encoding, next_commit.encoding)) {
+		if ((reencoded = reencode_string(next_commit.author.name,
+				git_commit_encoding, next_commit.encoding))) {
+			free((void *)next_commit.author.name);
+			next_commit.author.name = reencoded;
+		}
+		if ((reencoded = reencode_string(next_commit.summary.buf,
+				git_commit_encoding, next_commit.encoding))) {
+			strbuf_reset(&next_commit.summary);
+			strbuf_addstr(&next_commit.summary, reencoded);
+		}
+	}
+	strbuf_addf(&sbuf, "author %s <%s> %s\n", next_commit.author.name,
+			next_commit.author.mail, next_commit.author.time);
+	strbuf_addf(&sbuf, "committer %s\n", git_committer_info(0));
+	if (!is_encoding_utf8(git_commit_encoding))
+		strbuf_addf(&sbuf, "encoding %s\n", git_commit_encoding);
+	strbuf_addch(&sbuf, '\n');
+	strbuf_addbuf(&sbuf, &next_commit.summary);
+	if (sbuf.buf[sbuf.len-1] != '\n')
+		strbuf_addch(&sbuf, '\n');
+
+	failed = write_sha1_file(sbuf.buf, sbuf.len, commit_type, commit_sha1);
+	strbuf_release(&sbuf);
+	if (failed)
+		return 1;
+
+	if (verbosity > 1)
+		printf("Created %scommit %s\n",
+			parent_sha1 || next_commit.parents.nr ? "" : "initial ",
+			sha1_to_hex(commit_sha1));
+
+	if (update_ref(reflog, "HEAD", commit_sha1, NULL, 0, 0))
+		return error("Could not update HEAD to %s.",
+						sha1_to_hex(commit_sha1));
+
+	return 0;
+}
+
+/*
+ * Fill next_commit.author according to ident.
+ * Ident may have one of the following forms:
+ * 	"name <e-mail> timestamp timezone\n..."
+ * 	"name <e-mail> timestamp timezone"
+ * 	"name <e-mail>"
+ */
+static void set_author_info(const char *ident)
+{
+	const char *tmp1 = strstr(ident, " <");
+	const char *tmp2;
+	char *data;
+	if (!tmp1)
+		return;
+	tmp2 = strstr(tmp1+2, ">");
+	if (!tmp2)
+		return;
+	if (tmp2[1] != 0 && tmp2[1] != ' ')
+		return;
+
+	data = xmalloc(strlen(ident)); /* a trivial upper bound */
+
+	snprintf(data, tmp1-ident+1, "%s", ident);
+	next_commit.author.name = xstrdup(data);
+	snprintf(data, tmp2-tmp1-1, "%s", tmp1+2);
+	next_commit.author.mail = xstrdup(data);
+
+	if (tmp2[1] == 0) {
+		free(data);
+		return;
+	}
+
+	tmp1 = strpbrk(tmp2+2, "\r\n");
+	if (!tmp1)
+		tmp1 = tmp2 + strlen(tmp2);
+
+	snprintf(data, tmp1-tmp2-1, "%s", tmp2+2);
+	next_commit.author.time = xstrdup(data);
+	free(data);
+}
+
+static void set_message_source(const char *source)
+{
+	if (next_commit.source)
+		free(next_commit.source);
+	next_commit.source = xstrdup(source);
+}
+
+/* Set subject, an information for the case of conflict */
+static void set_pick_subject(const char *hex, struct commit *commit)
+{
+	const char *tmp = strstr(commit->buffer, "\n\n");
+	if (tmp) {
+		const char *eol;
+		int len = strlen(hex);
+		tmp += 2;
+		eol = strchrnul(tmp, '\n');
+		next_commit.subject = xmalloc(eol - tmp + len + 5);
+		snprintf(next_commit.subject, eol - tmp + len + 5, "%s... %s",
+								hex, tmp);
+	}
+}
+
 /* Return a commit object of "arg" */
 static struct commit *get_commit(const char *arg)
 {
@@ -198,6 +392,26 @@ static int set_verbosity(int verbose)
 	return 0;
 }
 
+static int write_commit_summary_into(const char *filename)
+{
+	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+	int fd = hold_lock_file_for_update(lock, filename, 0);
+	if (fd < 0)
+		return error("Unable to create '%s.lock': %s", filename,
+							strerror(errno));
+	if (write_in_full(fd, next_commit.summary.buf,
+			      next_commit.summary.len) < 0)
+		return error("Could not write to %s: %s",
+						filename, strerror(errno));
+	if (commit_lock_file(lock) < 0)
+		return error("Error wrapping up %s", filename);
+	return 0;
+}
+
+/**********************************************************************
+ * Builtin sequencer helper functions
+ */
+
 /* v should be "" or "t" or "\d" */
 static int parse_verbosity(const char *v)
 {
-- 
1.6.4.271.ge010d

[PATCH v3 8/9] sequencer: add "--cherry-pick" option to "git sequencer--helper"

[Christian Couder]

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

This patch adds some code that comes from the sequencer GSoC project:

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

(commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)

Most of the code from do_cherry_pick() is taken from the
sequencer insn_pick_act() function.

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>
---
 builtin-sequencer--helper.c |   71 +++++++++++++++++++++++++++++++++++++------
 1 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/builtin-sequencer--helper.c b/builtin-sequencer--helper.c
index 61a8f2e..291ba18 100644
--- a/builtin-sequencer--helper.c
+++ b/builtin-sequencer--helper.c
@@ -60,7 +60,8 @@ static const char * const git_sequencer_helper_usage[] = {
 		"<verbosity> [<allow-dirty>]",
 	"git sequencer--helper --fast-forward <commit> <reflog-msg> "
 		"<verbosity> [<allow-dirty>]",
-	"git sequencer--helper --cherry-pick <commit> [<do-not-commit>]",
+	"git sequencer--helper --cherry-pick <commit> <reflog-msg> "
+		"<verbosity> [<do-not-commit>]",
 	NULL
 };
 
@@ -288,7 +289,7 @@ static int do_commit(unsigned char *parent_sha1)
 
 	if (update_ref(reflog, "HEAD", commit_sha1, NULL, 0, 0))
 		return error("Could not update HEAD to %s.",
-						sha1_to_hex(commit_sha1));
+			     sha1_to_hex(commit_sha1));
 
 	return 0;
 }
@@ -408,6 +409,46 @@ static int write_commit_summary_into(const char *filename)
 	return 0;
 }
 
+static int do_cherry_pick(char *cp_commit, int no_commit)
+{
+	struct commit *commit;
+	int failed;
+	const char *author;
+
+	if (get_sha1("HEAD", head_sha1))
+		return error("You do not have a valid HEAD.");
+
+	commit = get_commit(cp_commit);
+	if (!commit)
+		return 1;
+
+	set_pick_subject(cp_commit, commit);
+
+	failed = pick_commit(commit, 0, 0, &next_commit.summary);
+
+	set_message_source(sha1_to_hex(commit->object.sha1));
+	author = strstr(commit->buffer, "\nauthor ");
+	if (author)
+		set_author_info(author + 8);
+
+	/* We do not want extra Conflicts: lines on cherry-pick,
+	   so just take the old commit message. */
+	if (failed) {
+		strbuf_setlen(&next_commit.summary, 0);
+		strbuf_addstr(&next_commit.summary,
+			      strstr(commit->buffer, "\n\n") + 2);
+		rerere();
+		make_patch(commit);
+		write_commit_summary_into(MERGE_MSG);
+		return error(pick_help_msg(commit->object.sha1, 0));
+	}
+
+	if (!no_commit && do_commit(head_sha1))
+		return error("Could not commit.");
+
+	return 0;
+}
+
 /**********************************************************************
  * Builtin sequencer helper functions
  */
@@ -436,6 +477,7 @@ int cmd_sequencer__helper(int argc, const char **argv, const char *prefix)
 	char *patch_commit = NULL;
 	char *reset_commit = NULL;
 	char *ff_commit = NULL;
+	char *cp_commit = NULL;
 	struct option options[] = {
 		OPT_STRING(0, "make-patch", &patch_commit, "commit",
 			   "create a patch from commit"),
@@ -443,6 +485,8 @@ int cmd_sequencer__helper(int argc, const char **argv, const char *prefix)
 			   "reset to commit"),
 		OPT_STRING(0, "fast-forward", &ff_commit, "commit",
 			   "fast forward to commit"),
+		OPT_STRING(0, "cherry-pick", &cp_commit, "commit",
+			   "cherry pick commit"),
 		OPT_END()
 	};
 
@@ -459,19 +503,15 @@ int cmd_sequencer__helper(int argc, const char **argv, const char *prefix)
 		return 0;
 	}
 
-	if (ff_commit || reset_commit) {
+	if (cp_commit || ff_commit || reset_commit) {
 		unsigned char sha1[20];
-		char *commit = ff_commit ? ff_commit : reset_commit;
+		char *commit;
+		int opt_arg = 0;
 
 		if (argc != 2 && argc != 3)
 			usage_with_options(git_sequencer_helper_usage,
 					   options);
 
-		if (get_sha1(commit, sha1)) {
-			error("Could not find '%s'", commit);
-			return 1;
-		}
-
 		reflog = (char *)argv[0];
 
 		if (parse_verbosity(argv[1])) {
@@ -480,7 +520,18 @@ int cmd_sequencer__helper(int argc, const char **argv, const char *prefix)
 		}
 
 		if (argc == 3 && *argv[2] && strcmp(argv[2], "0"))
-			allow_dirty = 1;
+			opt_arg = 1;
+
+		if (cp_commit)
+			return do_cherry_pick(cp_commit, opt_arg);
+
+		allow_dirty = opt_arg;
+
+		commit = ff_commit ? ff_commit : reset_commit;
+		if (get_sha1(commit, sha1)) {
+			error("Could not find '%s'", commit);
+			return 1;
+		}
 
 		if (ff_commit)
 			return do_fast_forward(sha1);
-- 
1.6.4.271.ge010d

[PATCH v3 9/9] rebase -i: use "git sequencer--helper --cherry-pick"

[Christian Couder]

instead of "git cherry-pick", as this will make it easier to
port "git-rebase--interactive.sh" to C.

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

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 7651fd6..349ca50 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -147,7 +147,8 @@ pick_one () {
 		pick_one_preserving_merges "$@" && return
 	if test ! -z "$REBASE_ROOT"
 	then
-		output git cherry-pick "$@"
+		git sequencer--helper --cherry-pick $sha1 \
+			"$GIT_REFLOG_ACTION" "$VERBOSE" $no_ff
 		return
 	fi
 	parent_sha1=$(git rev-parse --verify $sha1^) ||
@@ -157,7 +158,8 @@ pick_one () {
 		git sequencer--helper --fast-forward $sha1 \
 			"$GIT_REFLOG_ACTION" "$VERBOSE"
 	else
-		output git cherry-pick "$@"
+		git sequencer--helper --cherry-pick $sha1 \
+			"$GIT_REFLOG_ACTION" "$VERBOSE" $no_ff
 	fi
 }
 
@@ -269,7 +271,10 @@ pick_one_preserving_merges () {
 			fi
 			;;
 		*)
-			output git cherry-pick "$@" ||
+			no_commit=
+			test "a$1" = "a-n" && no_commit=t
+			git sequencer--helper --cherry-pick $sha1 \
+				"$GIT_REFLOG_ACTION" "$VERBOSE" $no_commit ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
 		esac
-- 
1.6.4.271.ge010d

Re: [PATCH v3 1/9] sequencer: add "do_fast_forward()" to perform a fast forward

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> From: Stephan Beyer <s-beyer@gmx.net>
>
> This code is taken from the sequencer GSoC project:
>
>     git://repo.or.cz/git/sbeyer.git
>
>     (commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20)
>
> but the messages have been changed to be the same as those
> displayed by git-rebase--interactive.sh.

Hmm, forgot to amend, or perhaps you sent out a wrong series?

The log message does not explain:

 - why the patch adds a new static function that nobody calls;
 - what the new function is good for;

which are the most important things in order to defend the change.

"The messages have been changed to..." hints that the original commit by
Stephan had different messages produced, perhaps so that it can be used in
a different context.  I hoped, in an ideal world, perhaps Stephan defended
why the change was relevant to his project in some way, and because you
are using it in a different context that needs modification of the
message, perhaps Stephan's defense of his commit could be reworded to
defend your change here.

So I decided to take a look at the quoted commit to see if I can reword
this mess.

But the quoted commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20 does not
even seem to be a commit that corresponds to this change.  It is a merge
from upstream.

    commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20
    Merge: 0c73ae7 99ddd24
    Author: Stephan Beyer <s-beyer@gmx.net>
    Date:   Wed May 20 10:54:37 2009 +0200

        Merge branch 'junio/master' into seq-builtin-dev

        After this merge f79d4c8 "teach git-am to apply a patch to an unborn
        branch" has to be reimplemented in sequencer by allowing the "patch"
        insn on unborn branches.
        The related test in t/t4150-am.sh is set to test_expect_failure.

        Conflicts:
            git-am.sh

It does not help that the function that is crucial to the implemention of
this new function, reset_almost_hard(), is not explained at all in the
earlier commit in the previous series (36f692b (sequencer: add
"reset_almost_hard()" and related functions, 2009-08-05).  

The log message to the commit does not even hint in what sense "almost"
the function is, iow, in what situation it behaves exactly like "reset
--hard", and in what other situation it doesn't, and more importantly why
that distinction is there.  I thought I asked these questions when the
previous series was submitted, but I do not remember ever seeing
satisfactory answers to them.

I am afraid that the whole cc/sequencer-rebase-i series needs a serious
reroll before it gets near 'next'.

Before giving up, I'll quickly re-review how (un)readable the log of each
commit is in the series.  The following comments are mostly about the log
messages, which are supposed to entice people to review the code, and more
importantly, used as part of the release notes to summarize what the newly
added toys are about.  If they are horrible, the code has little chance to
be even read, and I'll have a hard time merging the series up into a new
release.



6db6551 sequencer: add "builtin-sequencer--helper.c"

Good.

b512803 sequencer: add "make_patch" function to save a patch

Passably okay, but the limitation that it always writes into a file with a
fixed name "$SEQ_DIR/patch" should be noted in the log.

0ccc92b sequencer: free memory used in "make_patch" function

Should be squashed to the previous.

f121b06 rebase -i: use "git sequencer--helper --make-patch"

Good.

36f692b sequencer: add "reset_almost_hard()" and related functions

Horrible.  See above.

9d41fbd sequencer: add comments about reset_almost_hard()

Should be squashed to the previous---lift some text to justify the
existence of the function in the commit log message.  Even though
allow_dirty is referred to in the comment as affecting the behaviour, it
is unclear who sets that global variable using what interface, making the
reader suspect that maybe it should be a function parameter instead of a
global (but the other parts of the helper may also look at allow_dirty
and the internal implementation might be--I am just guessing--simpler this
way, in which case _that_ should be explained and justified).

022a9e7 sequencer: add "--reset-hard" option to "git sequencer--helper"

This by itself is Okay, provided if 36f692b were made readable.  Then you
can expect the reader to know why reset_almost_hard() needs to be there,
and you need an interface to that function.  Until then, it is totally
unclear why you need this, instead of using "reset --hard" itself.

ad28459 rebase -i: use "git sequencer--helper --reset-hard"

Ditto.

e4b3f0f sequencer: add "do_fast_forward()" to perform a fast forward

See above.

1d88073 sequencer: add "--fast-forward" option to "git sequencer--helper"

Okay.

6eff656 sequencer: let "git sequencer--helper" callers set "allow_dirty"

Why?  What for?

877ddc1 rebase -i: use "git sequencer--helper --fast-forward"

It is unclear how this relates to the previous one, nor why it is more
appropriate than "reset-hard" it replaces.

ff312f0 revert: libify pick

Almost good.

ab67716 pick: libify "pick_help_msg()"

Good.

d871b0e sequencer: add "do_commit()" and related functions

We can see from "git show" what static functions that are never called in
this commit are added, but nobody explains why they are needed.  For
example, do_commit() may create a new commit object, but does not share
the code with what "git commit" and/or "git commit-tree" do?  If so, how?
If not, why not?

ac5fc4d sequencer: add "--cherry-pick" option to "git sequencer--helper"

Passably okay.  I can see ff312f0 made about a half of cherry-pick
accessible to the sequencer, and this patch uses it to finish the other
half, although that is not explained in the log message.  Also it is
unclear why the resulting "libified" code does not share more
infrastructure with "git cherry-pick" itself (and "git revert").

664c7ab rebase -i: use "git sequencer--helper --cherry-pick"

Passably okay, even though it is not quite convincing why using
sequencer-helper's cherry-pick option makes it easier to later port the
script, than keeping calls to cherry-pick.

Re: [PATCH v3 1/9] sequencer: add "do_fast_forward()" to perform a fast forward

[Christian Couder]

On Saturday 22 August 2009, Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > From: Stephan Beyer <s-beyer@gmx.net>
> >
> > This code is taken from the sequencer GSoC project:
> >
> >     git://repo.or.cz/git/sbeyer.git
> >
> >     (commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20)
> >
> > but the messages have been changed to be the same as those
> > displayed by git-rebase--interactive.sh.
>
> Hmm, forgot to amend, or perhaps you sent out a wrong series?

Your comments on the v2 series were in a message replying to patch 5/9, so I 
amended only 5/9 and after it, because I thought that you had already 
reviewed those before 5/9 and they were ok.

> The log message does not explain:
>
>  - why the patch adds a new static function that nobody calls;
>  - what the new function is good for;
>
> which are the most important things in order to defend the change.

Yes, but the function contains only 5 lines of code and it mostly only calls 
reset_almost_hard() that is already documented by a big comment before its 
definition.

> "The messages have been changed to..." hints that the original commit by
> Stephan had different messages produced, perhaps so that it can be used
> in a different context.  I hoped, in an ideal world, perhaps Stephan
> defended why the change was relevant to his project in some way, and
> because you are using it in a different context that needs modification
> of the message, perhaps Stephan's defense of his commit could be reworded
> to defend your change here.
>
> So I decided to take a look at the quoted commit to see if I can reword
> this mess.
>
> But the quoted commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20 does not
> even seem to be a commit that corresponds to this change.  It is a merge
> from upstream.
>
>     commit e7b8dab0c2a73ade92017a52bb1405ea1534ef20
>     Merge: 0c73ae7 99ddd24
>     Author: Stephan Beyer <s-beyer@gmx.net>
>     Date:   Wed May 20 10:54:37 2009 +0200
>
>         Merge branch 'junio/master' into seq-builtin-dev
>
>         After this merge f79d4c8 "teach git-am to apply a patch to an
> unborn branch" has to be reimplemented in sequencer by allowing the
> "patch" insn on unborn branches.
>         The related test in t/t4150-am.sh is set to test_expect_failure.
>
>         Conflicts:
>             git-am.sh

This is the commit at the point where I took the code of the function, not 
the commit that introduce the function. This is because some functions like 
pick() in patch 5/9 evolve a lot in the sequencer repo after they are 
introduced.

> It does not help that the function that is crucial to the implemention of
> this new function, reset_almost_hard(), is not explained at all in the
> earlier commit in the previous series (36f692b (sequencer: add
> "reset_almost_hard()" and related functions, 2009-08-05).
>
> The log message to the commit does not even hint in what sense "almost"
> the function is, iow, in what situation it behaves exactly like "reset
> --hard", and in what other situation it doesn't, and more importantly why
> that distinction is there.  I thought I asked these questions when the
> previous series was submitted, but I do not remember ever seeing
> satisfactory answers to them.

You asked questions about reset_almost_hard() and I added the big comment 
before its definition, see:

http://git.kernel.org/?p=git/git.git;a=commitdiff;h=9d41fbd28f1ba3d07cd2e0f547521f9dced4cfd2

As you merged the series in pu, I thought that it was ok.

> I am afraid that the whole cc/sequencer-rebase-i series needs a serious
> reroll before it gets near 'next'.

Ok, I will reroll everything to try to improve commit messages.

> Before giving up, I'll quickly re-review how (un)readable the log of each
> commit is in the series.  The following comments are mostly about the log
> messages, which are supposed to entice people to review the code, and
> more importantly, used as part of the release notes to summarize what the
> newly added toys are about.  If they are horrible, the code has little
> chance to be even read, and I'll have a hard time merging the series up
> into a new release.

About the release notes, as "git sequencer--helper" is not for public use, 
and as many functions like reset_almost_hard() are static, perhaps it would 
be ok to have only something like:

(developers)

- parts of "git rebase -i" have been ported to C using "git 
sequencer--helper"

- cherry-pick and revert functionality is available using new functions 
declared in "pick.h"

> 6db6551 sequencer: add "builtin-sequencer--helper.c"
>
> Good.
>
> b512803 sequencer: add "make_patch" function to save a patch
>
> Passably okay, but the limitation that it always writes into a file with
> a fixed name "$SEQ_DIR/patch" should be noted in the log.

Ok.

> 0ccc92b sequencer: free memory used in "make_patch" function
>
> Should be squashed to the previous.

Will do.

> f121b06 rebase -i: use "git sequencer--helper --make-patch"
>
> Good.
>
> 36f692b sequencer: add "reset_almost_hard()" and related functions
>
> Horrible.  See above.
>
> 9d41fbd sequencer: add comments about reset_almost_hard()
>
> Should be squashed to the previous---lift some text to justify the
> existence of the function in the commit log message.

Will do.

> Even though 
> allow_dirty is referred to in the comment as affecting the behaviour, it
> is unclear who sets that global variable using what interface, making the
> reader suspect that maybe it should be a function parameter instead of a
> global (but the other parts of the helper may also look at allow_dirty
> and the internal implementation might be--I am just guessing--simpler
> this way, in which case _that_ should be explained and justified).

Will have a look.

> 022a9e7 sequencer: add "--reset-hard" option to "git sequencer--helper"
>
> This by itself is Okay, provided if 36f692b were made readable.  Then you
> can expect the reader to know why reset_almost_hard() needs to be there,
> and you need an interface to that function.  Until then, it is totally
> unclear why you need this, instead of using "reset --hard" itself.

Ok.

> ad28459 rebase -i: use "git sequencer--helper --reset-hard"
>
> Ditto.
>
> e4b3f0f sequencer: add "do_fast_forward()" to perform a fast forward
>
> See above.
>
> 1d88073 sequencer: add "--fast-forward" option to "git sequencer--helper"
>
> Okay.
>
> 6eff656 sequencer: let "git sequencer--helper" callers set "allow_dirty"
>
> Why?  What for?

Daniel asked me to make it available so people interested can test. I will 
state this in the commit message.

> 877ddc1 rebase -i: use "git sequencer--helper --fast-forward"
>
> It is unclear how this relates to the previous one, nor why it is more
> appropriate than "reset-hard" it replaces.

It is more appropriate because it makes the rebase -i shell code a little 
bit shorter. I will add that to the commit message.

> ff312f0 revert: libify pick
>
> Almost good.

Will try to improve.

> ab67716 pick: libify "pick_help_msg()"
>
> Good.
>
> d871b0e sequencer: add "do_commit()" and related functions
>
> We can see from "git show" what static functions that are never called in
> this commit are added, but nobody explains why they are needed.  For
> example, do_commit() may create a new commit object, but does not share
> the code with what "git commit" and/or "git commit-tree" do?  If so, how?
> If not, why not?

Will have a look.

> ac5fc4d sequencer: add "--cherry-pick" option to "git sequencer--helper"
>
> Passably okay.  I can see ff312f0 made about a half of cherry-pick
> accessible to the sequencer, and this patch uses it to finish the other
> half, although that is not explained in the log message.  Also it is
> unclear why the resulting "libified" code does not share more
> infrastructure with "git cherry-pick" itself (and "git revert").

Will have a look.

> 664c7ab rebase -i: use "git sequencer--helper --cherry-pick"
>
> Passably okay, even though it is not quite convincing why using
> sequencer-helper's cherry-pick option makes it easier to later port the
> script, than keeping calls to cherry-pick.

Will have a look.

Thanks,
Christian.

Re: [PATCH v3 1/9] sequencer: add "do_fast_forward()" to perform a fast forward

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

>> Hmm, forgot to amend, or perhaps you sent out a wrong series?
>
> Your comments on the v2 series were in a message replying to patch 5/9, so I 
> amended only 5/9 and after it, because I thought that you had already 
> reviewed those before 5/9 and they were ok.

Sorry, but in general I expect readers of this list to be intelligent
enough and capable of extrapolating, so that I do not have to spell out
the same issue over and over again ;-)

> As you merged the series in pu, I thought that it was ok.

Heh, being queued to 'pu' does not mean any more than just that we do not
want to lose what the author spent efforts to produce, and we want to give
reviewers easier access to the end result.  If it is Ok, it would have
been in 'next'.

That's why topics initially queued in 'pu' sometimes move to 'stalled'
state in "What's in" report and then eventually get ejected.

The comment you added only says what it does, namely, that it does two
different things, based on the allow_dirty global variable that is
underdocumented.  It does not explain _why_ it is a good thing, in what
circumstance the caller would want "almost" semantics and in what other
circumstance the caller would want a "real" hard reset.

>> I am afraid that the whole cc/sequencer-rebase-i series needs a serious
>> reroll before it gets near 'next'.
>
> Ok, I will reroll everything to try to improve commit messages.

It would be a good idea; it may expose a wrong abstraction or an
insufficient refactoring (e.g. adding yet another function to make a
commit object that runs parallel to what we already have).

Thanks.