On Saturday 06 February 2010 00:57:18 Junio C Hamano wrote:
> Christian Couder <chriscool@xxxxxxxxxxxxx> 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.
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html