[PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch

[PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch

[Samuel Bronson]

Without this, pull becomes unusable for merging branches when the config
option `merge.ff` is set to `only`.

Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 git-pull.sh |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 9868a0b..a09fcbc 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -76,6 +76,8 @@ do
 		no_ff=--no-ff ;;
 	--ff-only)
 		ff_only=--ff-only ;;
+	--no-ff-only)
+		ff_only=--no-ff-only ;;
 	-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\
 		--strateg=*|--strategy=*|\
 	-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)
-- 
1.7.7.3

[PATCH/RFC 2/2] merge, pull: Document the --no-ff-only merge option

[Samuel Bronson]

Now that --no-ff-only will work with pull and not just with merge, we
should probably tell the users about it.

Signed-off-by: Samuel Bronson <naesten@gmail.com>
---
 Documentation/merge-options.txt |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 6bd0b04..2adc4f5 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -56,9 +56,13 @@ With --no-squash perform the merge and commit the result. This
 option can be used to override --squash.
 
 --ff-only::
+--no-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.
++
+With --no-ff-only, don't refuse. This is can be used to override
+--ff-only, or the "`only`" value for the `merge.ff` configuration option.
 
 -s <strategy>::
 --strategy=<strategy>::
-- 
1.7.7.3

Re: [PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch

[Junio C Hamano]

Samuel Bronson <naesten@gmail.com> writes:

> Without this, pull becomes unusable for merging branches when the config
> option `merge.ff` is set to `only`.
>
> Signed-off-by: Samuel Bronson <naesten@gmail.com>

I wonder why you need this. We have "git config --unset merge.ff" after
all. From purely mechanstic point of view, being able to temporarily
defeat a configuration variable makes perfect sense, but from the point of
view of workflow, I am not sure if it is a good thing to even allow it in
the first place in this particular case.

Setting merge.ff to 'only' means you are following along other people's
work and making nothing new on your own in this particular repository, no?
Hence you won't be asking the upstream to pull from this repository, which
in turn means that even if you made a merge by temporarily defeating the
configuration setting with this patch, your future pulls will no longer
fast forward, until somehow the upstream gets hold of your merge commit.

By the way (this is a digression), I also have to say --no-ff-only is too
*ugly* as a UI element, even though I know "git merge" already allows it
by accident.

"ff" is a tristate. By default, fast-forward is done when appropriate, and
people can _refuse_ to fast-forward and force Git to create an extra merge
commit. Or if you are strictly following along, you can say you _require_
fast-forward and reject anything else. So it may make the UI saner if we
updated the UI to allow users to say:

	--ff=normal	the default
        --ff=never	same as --no-ff that forces an extra merge commit
        --ff=required	same as --ff-only

while keeping the current --ff-only and --no-ff as backward compatibility
wart.

I dunno.

Re: [PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch

[Samuel Bronson]

On Wed, Nov 30, 2011 at 11:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Samuel Bronson <naesten@gmail.com> writes:
>
>> Without this, pull becomes unusable for merging branches when the config
>> option `merge.ff` is set to `only`.
>>
>> Signed-off-by: Samuel Bronson <naesten@gmail.com>
>
> I wonder why you need this. We have "git config --unset merge.ff" after
> all. From purely mechanstic point of view, being able to temporarily
> defeat a configuration variable makes perfect sense, but from the point of
> view of workflow, I am not sure if it is a good thing to even allow it in
> the first place in this particular case.
>
> Setting merge.ff to 'only' means you are following along other people's
> work and making nothing new on your own in this particular repository, no?
> Hence you won't be asking the upstream to pull from this repository, which
> in turn means that even if you made a merge by temporarily defeating the
> configuration setting with this patch, your future pulls will no longer
> fast forward, until somehow the upstream gets hold of your merge commit.

Actually, I wanted to set merge.ff to only avoid *accidentally*
merging origin/master onto my local master, when I would likely to
prefer to either rebase my work onto it, or retroactively put my work
in a branch and merge that *into* the real master branch; I would then
override only when I explicitly did want a merge.

(I actually have commit access to the repository in question, but wish
to avoid making others' commits to master look as if they were on side
branches.)

> By the way (this is a digression), I also have to say --no-ff-only is too
> *ugly* as a UI element, even though I know "git merge" already allows it
> by accident.
>
> "ff" is a tristate. By default, fast-forward is done when appropriate, and
> people can _refuse_ to fast-forward and force Git to create an extra merge
> commit. Or if you are strictly following along, you can say you _require_
> fast-forward and reject anything else. So it may make the UI saner if we
> updated the UI to allow users to say:
>
>        --ff=normal     the default
>        --ff=never      same as --no-ff that forces an extra merge commit
>        --ff=required   same as --ff-only
>
> while keeping the current --ff-only and --no-ff as backward compatibility
> wart.

Hmm, yes, I had noticed that it was a tristate (merge.ff clearly is),
and I guess --no-ff-only is a pretty ugly flag. I do have to ask,
though: why give --ff these new values? Wouldn't it make more sense to
reuse the values accepted by merge.ff; namely, 'true' (the implied
default), 'false', and 'only'?

Otherwise, this looks like a very nice way to implement what I want: I
guess it is probably a mistake that the existing (documented) flags do
not behave in this way?

Re: [PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch

[Junio C Hamano]

Samuel Bronson <naesten@gmail.com> writes:

> Hmm, yes, I had noticed that it was a tristate (merge.ff clearly is),
> and I guess --no-ff-only is a pretty ugly flag. I do have to ask,
> though: why give --ff these new values? Wouldn't it make more sense to
> reuse the values accepted by merge.ff; namely, 'true' (the implied
> default), 'false', and 'only'?

The 'true' and 'false' values to merge.ff are carry-over from the days
when it was a boolean, _not_ a tristate. If we were to make the UI more
rational by making it clear that this is not a boolean, it is a good time
for us to aim a bit higher than merely repeating the mistakes we made in
the past due to historical accident. In other words, we could add a
synonym for the "default" mode in addition to "--ff=true" (and for the
"always merge" mode in addition to "--ff=false") that makes it clear that
the value is _not_ a boolean [*1*]. If we were to go the "--ff=<value>"
route, we have to add support for other ways to spell boolean 'true'
(e.g. 'yes', '1', and 'on') anyway, so it is not that much extra work to
do so, I would think.

> Otherwise, this looks like a very nice way to implement what I want: I
> guess it is probably a mistake that the existing (documented) flags do
> not behave in this way?

Yeah, right now if you say "merge --ff-only --no-ff", we say these are
mutually exclusive (which is true), but if you think about the tristate
nature of the 'ff' option and spell it differently in your head, i.e.
"merge --ff=only --ff=never", it is reasonable to argue that we should
apply the usual "last one overrides" rule and behave as if "merge --no-ff"
were given (for the purpose of "last one overrides", the configured
defaults can be treated as if they come very early on the command line).
After all "merge --no-ff --ff" does seem to use the "last one overrides"
rule.

[Footnote]

*1* Perhaps 'allowed' instead of 'normal' (which I wrote out of thin-air;
I do not have any strong preference on the actual values) may be a better
choice for such a "this is not a boolean" spelling for the default mode.

Re: [PATCH/RFC 1/2] pull: pass the --no-ff-only flag through to merge, not fetch

[Samuel Bronson]

On Thu, Dec 1, 2011 at 1:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Samuel Bronson <naesten@gmail.com> writes:
>
>> Hmm, yes, I had noticed that it was a tristate (merge.ff clearly is),
>> and I guess --no-ff-only is a pretty ugly flag. I do have to ask,
>> though: why give --ff these new values? Wouldn't it make more sense to
>> reuse the values accepted by merge.ff; namely, 'true' (the implied
>> default), 'false', and 'only'?
>
> The 'true' and 'false' values to merge.ff are carry-over from the days
> when it was a boolean, _not_ a tristate. If we were to make the UI more
> rational by making it clear that this is not a boolean, it is a good time
> for us to aim a bit higher than merely repeating the mistakes we made in
> the past due to historical accident. In other words, we could add a
> synonym for the "default" mode in addition to "--ff=true" (and for the
> "always merge" mode in addition to "--ff=false") that makes it clear that
> the value is _not_ a boolean [*1*]. If we were to go the "--ff=<value>"
> route, we have to add support for other ways to spell boolean 'true'
> (e.g. 'yes', '1', and 'on') anyway, so it is not that much extra work to
> do so, I would think.

Sure, that makes sense. I was just a little worried that you might be
(accidentally) proposing that --ff use a different set of names than
merge.ff for a moment there...

>> Otherwise, this looks like a very nice way to implement what I want: I
>> guess it is probably a mistake that the existing (documented) flags do
>> not behave in this way?
>
> Yeah, right now if you say "merge --ff-only --no-ff", we say these are
> mutually exclusive (which is true), but if you think about the tristate
> nature of the 'ff' option and spell it differently in your head, i.e.
> "merge --ff=only --ff=never", it is reasonable to argue that we should
> apply the usual "last one overrides" rule and behave as if "merge --no-ff"
> were given (for the purpose of "last one overrides", the configured
> defaults can be treated as if they come very early on the command line).
> After all "merge --no-ff --ff" does seem to use the "last one overrides"
> rule.

Yes, I totally agree that it would make more sense that way; I
certainly tried that before I even began to look at any of the code.

> [Footnote]
>
> *1* Perhaps 'allowed' instead of 'normal' (which I wrote out of thin-air;
> I do not have any strong preference on the actual values) may be a better
> choice for such a "this is not a boolean" spelling for the default mode.