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

From: Christian Couder <chriscool@tuxfamily.org>
Date: Feb 28, 2010, 22:22 UTC
Message-ID: <201002282322.54856.chriscool@tuxfamily.org>
In-Reply-To: <7vpr4jnsm9.fsf@alter.siamese.dyndns.org>

On Saturday 06 February 2010 00:57:18 Junio C Hamano wrote:

Show 21 quoted lines

> Christian Couder <chriscool@tuxfamily.org> writes:
> > 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.
> >
> >
> > +	if (!(flags & PICK_REVERSE) && ff_ok && commit->parents) {
> > +		unsigned char head_sha1[20];
> > +		if (get_sha1("HEAD", head_sha1))
> > +			die("You do not have a valid HEAD.");
> > +		if (!hashcmp(commit->parents->item->object.sha1, head_sha1)) {
> > +			char *hex = sha1_to_hex(commit->object.sha1);
> 
> With this check, you are saying:
> 
> 	If we are cherry-picking commit $C, and if the current HEAD is
>         the first parent of $C, then just reset to $C instead of running a
>         cherry-pick.
> 
> I didn't check if you have access to commit->parents->item->object.sha1 at
> this point in the codepath, though (e.g. have you parsed "commit" yet?).

Yes it is parsed by the call to lookup_commit_reference() in parse_args().

> If the goal is to make untouched 'pick' in rebase-i to fast forward
> without actually running cherry-picking, perhaps it is much cleaner to do
> this kind of comparison in the caller of cherry-pick (i.e. rebase-i and
> anything that runs cherry-pick)?

At least Daniel Barkalow and Paolo Bonzini have stated before that they would find natural that git cherry-pick itself can fast forward.

The argument in the commit message is that as "git merge" can and does fast forward by default it seems strange that "git cherry-pick" cannot fast forward. It's not just because rebase-i tries to fast forward, as I think it is just simpler to fast forward if possible when using cherry-pick by hand.

> The whole series is titled as if "cherry-pick --ff" is the primary goal,
> but I am puzzled why earlier patches in the series were necessary for this
> change.

Earlier patches are just refactoring and making the needed functions extern to make the implementation of cherry-pick --ff possible and clean.

Show 12 quoted lines

> One more thing, in the same part of the code:
> >> +	if (!(flags & PICK_REVERSE) && ff_ok && commit->parents) {
> >> +		unsigned char head_sha1[20];
> >> +		if (get_sha1("HEAD", head_sha1))
> >> +			die("You do not have a valid HEAD.");
> >> +		if (!hashcmp(commit->parents->item->object.sha1, head_sha1)) {
> >> +			char *hex = sha1_to_hex(commit->object.sha1);
> 
> Is there a need to check commit->parents->next?
> 
> Should this code work the same way if the commit being cherry-picked is a
> merge?  Should "-m <parent-num>" option affect this codepath in any way?

Yeah I had not even checked what happens with merge commits and/or "-m <parent-num>". The next version of the patch series takes care of that.

Thanks, Christian.