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

From: Christian Couder <chriscool@tuxfamily.org>
Date: Feb 2, 2010, 05:13 UTC
Message-ID: <201002020613.48067.chriscool@tuxfamily.org>
In-Reply-To: <4B66E53D.8050204@gnu.org>

On lundi 01 février 2010, Paolo Bonzini wrote:

Show 7 quoted lines

> On 02/01/2010 01:43 PM, Christian Couder wrote:
> > 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.
>
> A better objection is that GIT_COMMITTER_* is respected by |git
> cherry-pick" but not by "git cherry-pick --ff",

Yes, indeed! Good catch!

> 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.

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 would disable fast forward even if none of the above env variable is set.

Show 6 quoted lines

> 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?

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 variables) to decide if we will fast forward or not.

> 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.

Yeah, I will add the same treatment for these options.

> 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.

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

Thanks, Christian.