Junio C Hamano wrote:
> Björn Gustavsson <bgustavsson@xxxxxxxxx> writes:
>>
>> +--ff-only::
>> + Refuse to merge unless the merge can be resolved as a
>> + fast-forward.
>
> Do you or do you not allow "already up to date"? I think it makes sense
> to allow it, but it is unclear from these two lines.
I do allow it. I will change the description to the following in the
re-roll:
--ff-only::
Refuse to merge and exit with a non-zero status unless the
current `HEAD` is already up-to-date or the merge can be
resolved as a fast-forward.
>
>> @@ -874,6 +877,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>> option_commit = 0;
>> }
>>
>> + if (!allow_fast_forward && fast_forward_only)
>> + die("You cannot combine --no-ff with --ff-only.");
>
> Are these the only nonsensical combinations? How should this interact
> with other options, e.g. --squash or --message?
They are the only options I can think of that flatly contradict each other.
Combining --squash and --ff-only will succeed if the current HEAD can be
fast-forwarded and will abort otherwise. I don't know how useful that
would be in practice, but I see no strong reason to forbid it.
The -m option will always be ignored, of course, and there will be
the usual warning if fast-forward is possible:
Fast forward (no commit created; -m option ignored)
I don't think there is any need to explicitly forbid the combination
of -m and --ff-only.
I should probably update the commit message in the re-roll to include
the information in the previous paragraphs.
>> @@ -969,8 +975,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>> }
>>
>> for (i = 0; i < use_strategies_nr; i++) {
>> - if (use_strategies[i]->attr & NO_FAST_FORWARD)
>> + if (use_strategies[i]->attr & NO_FAST_FORWARD) {
>> allow_fast_forward = 0;
>> + if (fast_forward_only)
>> + die("You cannot combine --ff-only with the merge strategy '%s'.", use_strategies[i]->name);
>> + }
>
> I am not convinced this tests the right condition nor it is placed at the
> right place in the codepath---even if a specified strategy happens to
> allow fast-forward, wouldn't it be nonsense to say
>
> $ git merge --ff-only -s resolve that-one
>
> in the first place? Note that I am not saying "I am convinced this is
> wrong."
Re-thinking it, I think that the test should be removed. It seemed like
a good idea at the time to point out which strategy that prevented the
fast-forward, but if there is a list of merge strategies, the test will prevent
--ff-only to succeed if *any* of merge strategies cannot fast-forward.
(Also, but I am not sure about this, a merge strategy that does not allow
fast-forward might allow up-to-date.)
Therefore, I will remove the test in the re-roll.
Thanks for the comments!
/Björn
--
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