[PATCH v4 0/7] add --ff option to cherry-pick

[PATCH v4 0/7] add --ff option to cherry-pick

[Christian Couder]

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

The big change since previous version is that the first patches to implement
the --ff option are based on what Junio suggested.

Christian Couder (4):
  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

Junio C Hamano (3):
  parse-options: add parse_options_concat() to concat options
  builtin/merge: make checkout_fast_forward() non static
  revert: add --ff option to allow fast forward when cherry-picking

 Documentation/git-cherry-pick.txt |   10 +++-
 builtin/merge.c                   |    2 +-
 builtin/revert.c                  |   47 +++++++++++++++-
 cache.h                           |    3 +
 git-rebase--interactive.sh        |   15 +----
 parse-options.c                   |   15 +++++
 parse-options.h                   |    1 +
 t/t3506-cherry-pick-ff.sh         |  106 +++++++++++++++++++++++++++++++++++++
 8 files changed, 182 insertions(+), 17 deletions(-)
 create mode 100755 t/t3506-cherry-pick-ff.sh

[PATCH v4 1/7] parse-options: add parse_options_concat() to concat options

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>


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

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);
-- 
1.7.0.1.307.g861f4

[PATCH v4 2/7] builtin/merge: make checkout_fast_forward() non static

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>

and also export it in "cache.h".

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

diff --git a/builtin/merge.c b/builtin/merge.c
index 3aaec7b..c043066 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -667,7 +667,7 @@ static int count_unmerged_entries(void)
 	return ret;
 }
 
-static int checkout_fast_forward(unsigned char *head, unsigned char *remote)
+int checkout_fast_forward(const unsigned char *head, const unsigned char *remote)
 {
 	struct tree *trees[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
diff --git a/cache.h b/cache.h
index 89f6a40..0608b55 100644
--- a/cache.h
+++ b/cache.h
@@ -1054,4 +1054,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 */
-- 
1.7.0.1.307.g861f4

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

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>

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 |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index eff5268..476f41e 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 (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,9 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	else
 		parent = commit->parents->item;
 
+	if (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 +378,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, ' ');
 
-- 
1.7.0.1.307.g861f4

[PATCH v4 4/7] cherry-pick: add tests for new --ff option

[Christian Couder]

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

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

[PATCH v4 5/7] Documentation: describe new cherry-pick --ff option

[Christian Couder]

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

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

[PATCH v4 6/7] cherry-pick: add a no-op --no-ff option to future proof scripts

[Christian Couder]

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

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

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

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index d71607a..dfc8243 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -7,7 +7,7 @@ git-cherry-pick - Apply the change introduced by an existing commit
 
 SYNOPSIS
 --------
-'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>
+'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--[no-]ff] <commit>
 
 DESCRIPTION
 -----------
@@ -75,6 +75,10 @@ effect to your index in a row.
 	cherry-pick'ed commit, then a fast forward to this commit will
 	be performed.
 
+--no-ff::
+	Does nothing right now, but in the future this may disallow
+	fast forward if it becomes the default behavior.
+
 Author
 ------
 Written by Junio C Hamano <gitster@pobox.com>
diff --git a/builtin/revert.c b/builtin/revert.c
index 476f41e..0651e34 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -36,7 +36,7 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
+static int edit, no_replay, no_commit, mainline, signoff, allow_ff, no_ff;
 static enum { REVERT, CHERRY_PICK } action;
 static struct commit *commit;
 static const char *commit_name;
@@ -63,11 +63,13 @@ static void parse_args(int argc, const char **argv)
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
+		OPT_END(),
 	};
 
 	if (action == CHERRY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
+			OPT_BOOLEAN(0, "no-ff", &no_ff, "disallow fast forward"),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
@@ -297,6 +299,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			die("cherry-pick --ff cannot be used with -x");
 		if (edit)
 			die("cherry-pick --ff cannot be used with --edit");
+		if (no_ff)
+			die("cherry-pick --ff cannot be used with --no-ff");
 	}
 
 	if (read_cache() < 0)
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.1.307.g861f4

[PATCH v4 7/7] rebase -i: use new --ff cherry-pick option

[Christian Couder]

This simplifies rebase -i a little bit.

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

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 3e4fd14..92d19f5 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.7.0.1.307.g861f4

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> From: Junio C Hamano <gitster@pobox.com>
>
> 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,

I never said that.

For scripted use, like we can see in your [PATCH 7/7], "cherry-pick --ff"
could be a good ingredient (but even then the calling Porcelain script may
already know that what is being picked is a direct descendent (rebase -i
certainly should, as the last commit it processed from the todo file is at
the HEAD), but for manual use case it just feels silly to assume that the
end user is so stupid that s/he doesn't know the commit being picked might
be a direct descendant, which would be the only reason why making --ff the
default might make sense.

And adding --no-ff and sprinkling that to scripts, only to support that
default change as a future possibility, feels doubly silly.

What was the real motive/use case of "cherry-pick --ff"?

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

[Paolo Bonzini]

On 03/07/2010 04:55 AM, Junio C Hamano wrote:
> What was the real motive/use case of "cherry-pick --ff"?

Avoiding pointless separation of histories.  It's true that nowadays 
git-patch-id will make a good job of reconciling them, but they do look 
ugly in gitk.

Paolo

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

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> On 03/07/2010 04:55 AM, Junio C Hamano wrote:
>> What was the real motive/use case of "cherry-pick --ff"?
>
> Avoiding pointless separation of histories.  It's true that nowadays
> git-patch-id will make a good job of reconciling them, but they do
> look ugly in gitk.

Sorry, but that is a not-very-useful XY answer.

Why were _you_ using "cherry-pick" on a commit that is an immediate child
of the current commit?  What were you trying to achieve by blindly using
cherry-pick even in such a case?

I am sort-of guessing that "blindly" is coming from a script that doesn't
bother to check if the commit you are cherry-picking is a direct child,
and I do not think it is such a bad thing to allow scripts to blindly call
cherry-pick that way and at the same time avoid making cherry-picked
commits that are unnecessary.  So in that sense I am not opposed to having
an option to allow "--ff".

But if that is the real motivation, then making --ff default is a wrong
thing to do, as existing scripts knew and trusted cherry-pick will _not_
fast-forward, and the ones that do want fast-forward have already checked
just like "rebase -i" does.  Changing the default like Chriscool suggested
would not help anybody.

That is what I wanted to find out.

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

[Christian Couder]

On Sunday 07 March 2010 10:05:04 Junio C Hamano wrote:
> Paolo Bonzini <bonzini@gnu.org> writes:
> > On 03/07/2010 04:55 AM, Junio C Hamano wrote:
> >> What was the real motive/use case of "cherry-pick --ff"?
> >
> > Avoiding pointless separation of histories.  It's true that nowadays
> > git-patch-id will make a good job of reconciling them, but they do
> > look ugly in gitk.
> 
> Sorry, but that is a not-very-useful XY answer.
> 
> Why were _you_ using "cherry-pick" on a commit that is an immediate child
> of the current commit?  What were you trying to achieve by blindly using
> cherry-pick even in such a case?
> 
> I am sort-of guessing that "blindly" is coming from a script that doesn't
> bother to check if the commit you are cherry-picking is a direct child,
> and I do not think it is such a bad thing to allow scripts to blindly call
> cherry-pick that way and at the same time avoid making cherry-picked
> commits that are unnecessary.  So in that sense I am not opposed to having
> an option to allow "--ff".
> 
> But if that is the real motivation, then making --ff default is a wrong
> thing to do, as existing scripts knew and trusted cherry-pick will _not_
> fast-forward, and the ones that do want fast-forward have already checked
> just like "rebase -i" does.  Changing the default like Chriscool suggested
> would not help anybody.

The wording is: "Maybe this option could be made the default in the long run, 
..."

I didn't wrote something like: "This option should be made the default in the 
long run, ..."

I really didn't know if it should or should not be made the default now or in 
the long run, but I thought I had to talk about it.

Why? Because Paolo said that he would like it to become the default, and also 
because if we implement "git cherry-pick A..B" and people start to use it a 
lot, they may find it boring to have to always add --ff to avoid creating lots 
of spurious commit (instead of reusing existing ones).

Now if you want me to remove this part of the commit message, I will do it.
But my opinion is that it is interesting to document that we thought about 
making it the default now or in the future.

Do you prefer something like:

"Making this option the default was discussed on the mailing list but we 
decided that it is best left as non default for now. Though we recognized that 
it is best to have a --no-ff option too to future proof scripts in case the 
decision to make fast forwarding the default is made in the future."

?

Best regards,
Christian.