The goal of this series is to add a --ff fast forward otion to cherry-pick
and use it in "rebase -i".

There is no documentation yet and the commit messages are too short but
this will be improved if the series looks worthwhile.

The first patch in this series is a refactoring patch that is not really
needed, but as it looks like a good refactoring/cleanup anyway I left it in.

Christian Couder (5):
  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
  rebase -i: use new --ff cherry-pick option

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

 Makefile                   |    2 +
 builtin-reset.c            |  175 +++++++++++++++------------
 builtin-revert.c           |  293 ++++++++++----------------------------------
 git-rebase--interactive.sh |   15 +--
 pick.c                     |  218 ++++++++++++++++++++++++++++++++
 pick.h                     |   13 ++
 reset.h                    |   10 ++
 7 files changed, 407 insertions(+), 319 deletions(-)
 create mode 100644 pick.c
 create mode 100644 pick.h
 create mode 100644 reset.h

--

[ message continues ]

This splits the reseting logic away from the argument parsing.

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

diff --git a/builtin-reset.c b/builtin-reset.c
index 3569695..bf97931 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -235,68 +235,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_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 ...
[ message continues ]

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

diff --git a/builtin-reset.c b/builtin-reset.c
index bf97931..b004f9a 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] [-q] [<commit>]",
@@ -27,7 +28,6 @@ static const char * const git_reset_usage[] = {
 	NULL
 };
 
-enum reset_type { MIXED, SOFT, HARD, MERGE, NONE };
 static const char *reset_type_names[] = { "mixed", "soft", "hard", "merge", NULL };
 
 static char *args_to_str(const char **argv)
@@ -235,9 +235,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..c8497e4
--- /dev/null
+++ b/reset.h
@@ -0,0 +1,10 @@
+#ifndef RESET_H
+#define RESET_H
+
+enum reset_type { MIXED, SOFT, HARD, MERGE, NONE };
+
+int reset(const char *rev, const char *prefix,
+	  int reset_type, int quiet, int patch_mode,
+	  int argc, const char **argv);
+
+#endif
-- 
1.6.6.1.557.g77031


--

[ message continues ]

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

diff --git a/builtin-revert.c b/builtin-revert.c
index 3cbeab7..3f92515 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 int allow_rerere_auto;
@@ -51,6 +52,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(),
@@ -71,6 +73,8 @@ static void parse_args(int argc, const char **argv)
 	}
 	if (commit->object.type != OBJ_COMMIT)
 		die ("'%s' does not point to a commit", arg);
+	if (ff_ok && no_commit)
+		die ("options --ff and --no-commit are incompatible");
 }
 
 static char *get_encoding(const char *message)
@@ -191,7 +195,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;
@@ -209,6 +213,19 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	if (!no_commit && index_differs_from("HEAD", 0))
 		die_dirty_index(me);
 
+	if (!(flags & PICK_REVERSE) && ff_ok && commit->parents) ...
[ message continues ]

> +		OPT_BOOLEAN(0, "ff",&ff_ok, "allow fast forward"),

Why should this not be the default?  Instead, you'd add --no-ff.  This 
would simplify 6/6 further, like

   eval sha1=\$$#
   ...
   output git cherry-pick "$@"

Paolo
--

[ message continues ]

Maybe it could be the default, but in this case it should be made
compatible with -n option
(and perhaps other options) for backward compatibility, and this would
probably need more
involved changes.

Thanks,
--

[ message continues ]

A better objection is that GIT_COMMITTER_* is respected by |git 
cherry-pick" but not by "git cherry-pick --ff", and that even without 
setting the variables, "git cherry-pick" will pick a new commit date but 
"git cherry-pick --ff" wouldn't.  The latter, I think is the only 
difference that is worth pondering further.

My impression is that a user would never have any problem with 
fast-forwarding.  For scripts probably the same is true (the typical 
scenario for scripts is probably very similar to what "git rebase -i" 
does), but it can still be a potential backwards-incompatibility in case 
the script is *expecting* cherry-picking to generate a new SHA1.  How 
broken can we consider this expectation?


That said, to reply to your question, of course -n would disable it, and 
so would -x, -s and possibly -e.  But then, the same applies to --ff: 
your patch forbids "-n --ff", but -x, -s and -e would need the same 
treatment.

Note that "-e --ff" would error out; however if --ff would be the 
default, "-e" would probably choose between fast-forward and 
non-fast-forward depending on whether the commit message was edited.

Paolo
--

[ message continues ]

Because --no-ff could be used when the GIT_COMMITTER_* and GIT_AUTHOR_* env 
variable should be respected? Or because we should check if one of these 
env variable is set and, if that is the case, we should not fast forward?

As I think it would be a big backward incompatibility to force people to 
update their scripts to add --no-ff, I think you probably suggest the 
latter. This mean that we could have both --ff and --no-ff. --ff could 
force fast forward even if some of the above env variables are set. --no-ff 

I am not too worried by this expectation, but I think that, as it looks like 
we will need --ff anyway, it is safer to start by implementing --ff like I 
did and then later we can implement --no-ff and change the default (when 
neither --ff nor --no-ff is used) to look at env variables (or config 


Yeah, but let's change the default later please.

Thanks,
Christian.
--

[ message continues ]

I wish it could be the former, at least in the long run; after all git 
merge fast-forwards by default, and it doesn't adjust its behavior if 
GIT_COMMITTER_* is passed.

Anyway, your plan of starting with --ff and changing the default to 
--no-ff later seems fine.  Maybe you can add --no-ff now already, though 

Sure.

Paolo
--

[ message continues ]

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

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c2f6089..6b224a6 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -222,8 +222,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
@@ -232,16 +232,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.6.6.1.557.g77031

--

[ message continues ]

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

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

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

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

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

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

(at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079)

And it contains some changes and comments suggested by Junio.

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

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

diff --git a/Makefile b/Makefile
index d624530..71901c8 100644
--- a/Makefile
+++ b/Makefile
@@ -458,6 +458,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
@@ -550,6 +551,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 ...
[ message continues ]

Hi,


I guess that this patch would look nicer using --patience. In its current 
form, it would put too big a dent in my Git time budget, sorry, I will not 
review it.

Ciao,
Dscho
--

[ message continues ]

Why not do the oneline_body + 1 during the strchr()? Seems like 

use strbuf_addch() for characters.

--->8----

diff --git a/pick.c b/pick.c
index bb04c68..1e1628a 100644
--- a/pick.c
+++ b/pick.c
@@ -145,12 +145,12 @@ int pick_commit(struct commit *pick_commit, int mainline,
         oneline = get_oneline(message);

         if (flags&  PICK_REVERSE) {
-               char *oneline_body = strchr(oneline, ' ');
+               char *oneline_body = strchr(oneline, ' ') + 1;

                 base = commit;
                 next = parent;
                 strbuf_addstr(msg, "Revert \"");
-               strbuf_addstr(msg, oneline_body + 1);
+               strbuf_addstr(msg, oneline_body);
                 strbuf_addstr(msg, "\"\n\nThis reverts commit ");
                 strbuf_addstr(msg, sha1_to_hex(commit->object.sha1));

@@ -196,9 +196,9 @@ int pick_commit(struct commit *pick_commit, int mainline, in
                 for (i = 0; i<  active_nr;) {
                         struct cache_entry *ce = active_cache[i++];
                         if (ce_stage(ce)) {
-                               strbuf_addstr(msg, "\t");
+                               strbuf_addch(msg, '\t');
                                 strbuf_addstr(msg, ce->name);
-                               strbuf_addstr(msg, "\n");
+                               strbuf_addch(msg, '\n');
                                 while (i<  active_nr&&  !strcmp(ce->name,
                                                 active_cache[i]->name))
                                         i++;


--

[ message continues ]

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

diff --git a/builtin-reset.c b/builtin-reset.c
index 0f5022e..3569695 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -211,15 +211,38 @@ static void prepend_reflog_action(const char *action, char *buf, size_t size)
 		warning("Reflog action message too long: %.*s...", 50, buf);
 }
 
+/*
+ * 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)
 {
 	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,
@@ -321,19 +344,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	else if (reset_index_file(sha1, reset_type, quiet))
 		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 ...
[ message continues ]