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

Christian Couder [off-list ref] writes:

On Monday 01 March 2010 04:49:12 Junio C Hamano wrote:

quoted

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

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

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

 
Ok I will wait for something like a week, and then rebase on top of next and 
resend.

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

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

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

diff --git a/builtin-revert.c b/builtin-revert.c
index eff5268..50a40d3 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c

@@ -284,8 +284,6 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	}
 	discard_cache();
 
-	index_fd = hold_locked_index(&index_lock, 1);
-
 	if (!commit->parents) {
 		if (action == REVERT)
 			die ("Cannot revert a root commit");

@@ -314,6 +312,16 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	else
 		parent = commit->parents->item;
 
+	if (action == CHERRY_PICK && user_asked_fast_forward &&
+	    !no_commit &&
+	    !edit &&
+	    !no_replay
+	    parent &&
+	    !hashcmp(parent->object.sha1, head))
+		return fast_forward_to(parent);
+
+	index_fd = hold_locked_index(&index_lock, 1);
+
 	if (!(message = commit->buffer))
 		die ("Cannot get commit message for %s",
 				sha1_to_hex(commit->object.sha1));

On Monday 01 March 2010 09:48:45 Junio C Hamano wrote:

Christian Couder [off-list ref] writes:

quoted

On Monday 01 March 2010 04:49:12 Junio C Hamano wrote:

quoted

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

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

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

Ok I will wait for something like a week, and then rebase on top of next
and resend.

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

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

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

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

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

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

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

Thanks,
Christian.