[PATCH] merge-options.txt: clarify meaning of various ff-related options

[PATCH] merge-options.txt: clarify meaning of various ff-related options

[Elijah Newren]

As discovered on the mailing list, some of the descriptions of the
ff-related options were unclear.  Try to be more precise with what these
options do.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
I noticed this patch sitting around in one of my branches, and noticed it
wasn't upstream.  I'm pretty sure I submitted it a few months back, but I
think it got lost in the cracks.  Resubmitting and I'll see if I can do a
better job following up on it.

 Documentation/merge-options.txt | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..b39df5f126 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -40,20 +40,22 @@ set to `no` at the beginning of them.
 	case of a merge conflict.
 
 --ff::
-	When the merge resolves as a fast-forward, only update the branch
-	pointer, without creating a merge commit.  This is the default
+	When the merge can resolve as a fast-forward, do so (only
+	update the branch pointer to match the merged branch; do not
+	create a merge commit).  When a fast forward update is not
+	possible, create a merge commit.  This is the default
 	behavior.
 
 --no-ff::
-	Create a merge commit even when the merge resolves as a
-	fast-forward.  This is the default behaviour when merging an
-	annotated (and possibly signed) tag that is not stored in
-	its natural place in 'refs/tags/' hierarchy.
+	Create a merge commit even when the merge could instead resolve
+	as a fast-forward.  This is the default behaviour when merging
+	an annotated (and possibly signed) tag that is not stored in its
+	natural place in 'refs/tags/' hierarchy.
 
 --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.
+	When possible, resolve the merge as a fast-forward (do not
+	create a merge commit).  When not possible, refuse to merge and
+	exit with a non-zero status.
 
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
-- 
2.23.0.38.gfc6987be7e

Re: [PATCH] merge-options.txt: clarify meaning of various ff-related options

[Sergey Organov]

Elijah Newren <newren@gmail.com> writes:

> As discovered on the mailing list, some of the descriptions of the
> ff-related options were unclear.  Try to be more precise with what these
> options do.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> I noticed this patch sitting around in one of my branches, and noticed it
> wasn't upstream.  I'm pretty sure I submitted it a few months back, but I
> think it got lost in the cracks.  Resubmitting and I'll see if I can do a
> better job following up on it.
>
>  Documentation/merge-options.txt | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 79a00d2a4a..b39df5f126 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -40,20 +40,22 @@ set to `no` at the beginning of them.
>  	case of a merge conflict.
>  
>  --ff::
> -	When the merge resolves as a fast-forward, only update the branch
> -	pointer, without creating a merge commit.  This is the default
> +	When the merge can resolve as a fast-forward, do so (only
> +	update the branch pointer to match the merged branch; do not
> +	create a merge commit).  When a fast forward update is not
> +	possible, create a merge commit.  This is the default
>  	behavior.
>  
>  --no-ff::
> -	Create a merge commit even when the merge resolves as a
> -	fast-forward.  This is the default behaviour when merging an
> -	annotated (and possibly signed) tag that is not stored in
> -	its natural place in 'refs/tags/' hierarchy.
> +	Create a merge commit even when the merge could instead resolve
> +	as a fast-forward.  This is the default behaviour when merging
> +	an annotated (and possibly signed) tag that is not stored in its
> +	natural place in 'refs/tags/' hierarchy.

Please notice that virtually all the other cases of
--something/--no-something are formatted like this:

--something::
--no-something::
        [descriptions]

So, even only for consistency, it seems to be better to have this the
same way:

  --ff::
  --no-ff::
  --ff-only::
        [descriptions]

that, as a bonus, will make it explicit and crystal clear that these 3
things are alternatives, and thus the last one on the command line takes
precedence.

-- Sergey

[PATCH v2] merge-options.txt: clarify meaning of various ff-related options

[Elijah Newren]

As discovered on the mailing list, some of the descriptions of the
ff-related options were unclear.  Try to be more precise with what these
options do.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
Changes since v1:
  * Grouped much like --option/--no-option items are to make it clearer
    that these are related options, as suggested by Sergey.

 Documentation/merge-options.txt | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..348d66f54b 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -39,21 +39,28 @@ set to `no` at the beginning of them.
 	to `MERGE_MSG` before being passed on to the commit machinery in the
 	case of a merge conflict.
 
---ff::
-	When the merge resolves as a fast-forward, only update the branch
-	pointer, without creating a merge commit.  This is the default
-	behavior.
-
+--ff-only::
 --no-ff::
-	Create a merge commit even when the merge resolves as a
-	fast-forward.  This is the default behaviour when merging an
-	annotated (and possibly signed) tag that is not stored in
-	its natural place in 'refs/tags/' hierarchy.
+--ff::
+	Whether to only allow resolving the merge as a fast forward
+	(only updating the branch pointer to match the merged branch
+	and not creating a merge commit), to never allow it (always
+	creating a merge commit), or to prefer it when possible.  The
+	default is --ff, except when merging an annotated (and
+	possibly signed) tag that is not stored in its natural place
+	in 'refs/tags/' hierarchy (in which case --no-ff is the
+	default).
++
+With --ff-only, resolve the merge as a fast-forward when possible
+(when the merged branch contains the current branch in its history).
+When not possible, refuse to merge and exit with a non-zero status.
++
+With --no-ff, create a merge commit in all cases, even when the merge
+could instead resolve as a fast-forward.
++
+With --ff, resolve the merge as a fast-forward when possible.  When not
+possible, create a merge commit.
 
---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.
 
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
-- 
2.23.0.9.g568fda2d03

Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options

[Martin Ågren]

Hi Elijah,

On Wed, 28 Aug 2019 at 17:52, Elijah Newren <newren@gmail.com> wrote:
> ---ff::
> -       When the merge resolves as a fast-forward, only update the branch
> -       pointer, without creating a merge commit.  This is the default
> -       behavior.
> -
> +--ff-only::
>  --no-ff::
> -       Create a merge commit even when the merge resolves as a
> -       fast-forward.  This is the default behaviour when merging an
> -       annotated (and possibly signed) tag that is not stored in
> -       its natural place in 'refs/tags/' hierarchy.
> +--ff::
> +       Whether to only allow resolving the merge as a fast forward
> +       (only updating the branch pointer to match the merged branch
> +       and not creating a merge commit), to never allow it (always
> +       creating a merge commit), or to prefer it when possible.  The
> +       default is --ff, except when merging an annotated (and

It would be great if you'd write this as `--ff` so that it got
monospaced in the rendered documentation. Same thing with `no-ff` later
in this paragraph and a few more times in the next three paragraphs that
you're adding.

> +       possibly signed) tag that is not stored in its natural place
> +       in 'refs/tags/' hierarchy (in which case --no-ff is the
> +       default).

I'd also write `refs/tags/`, but I realize that you're just inheriting
what is already here. If you'd rather punt on that, that's understood.
This whole document could need a look-over with respect to monospacing
anyway, but it seems unfortunate to introduce *new* non-monospaced
instances, especially for command-line options where it's pretty clear
how they should be handled (Documentation/CodingGuidelines, line ~610).

> ++
> +With --ff-only, resolve the merge as a fast-forward when possible
> +(when the merged branch contains the current branch in its history).
> +When not possible, refuse to merge and exit with a non-zero status.
> ++
> +With --no-ff, create a merge commit in all cases, even when the merge
> +could instead resolve as a fast-forward.
> ++
> +With --ff, resolve the merge as a fast-forward when possible.  When not
> +possible, create a merge commit.
>
> ---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.

I was sort of expecting these to be listed in the order "--ff, --no-ff,
--ff-only", and I see Sergey suggested the same ordering. The way your
proposed text reads does make sense though... Would it read as well
turning it over and going through the options in the other order? That's
the way it is before your patch, so you could argue "but people don't
grok that!". What your patch does well is to offer an overview before
describing each option in a bit more detail. Would that work with the
reversed direction as well (compared to this patch)? Dunno.

I wondered briefly whether it might make sense to float "The default is
`--no-ff`." to the top, but since it's really "The default ... unless
so-and-so", it would probably complicate things more than it'd help.

Martin

Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options

[Sergey Organov]

Hi,

Martin Ågren <martin.agren@gmail.com> writes:


[...]

>> ++
>> +With --ff-only, resolve the merge as a fast-forward when possible
>> +(when the merged branch contains the current branch in its history).
>> +When not possible, refuse to merge and exit with a non-zero status.
>> ++
>> +With --no-ff, create a merge commit in all cases, even when the merge
>> +could instead resolve as a fast-forward.
>> ++
>> +With --ff, resolve the merge as a fast-forward when possible.  When not
>> +possible, create a merge commit.
>>
>> ---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.
>
> I was sort of expecting these to be listed in the order "--ff, --no-ff,
> --ff-only", and I see Sergey suggested the same ordering. The way your
> proposed text reads does make sense though... Would it read as well
> turning it over and going through the options in the other order? That's
> the way it is before your patch, so you could argue "but people don't
> grok that!". What your patch does well is to offer an overview before
> describing each option in a bit more detail. Would that work with the
> reversed direction as well (compared to this patch)? Dunno.
>
> I wondered briefly whether it might make sense to float "The default is
> `--no-ff`." to the top, but since it's really "The default ... unless
> so-and-so", it would probably complicate things more than it'd help.

Dunno if it helps, but here is what I came up with somewhere in previous
discussions:

--ff::
--no-ff::
--ff-only::
	When the merge resolves as a fast-forward, only update the
	branch pointer (without creating a merge commit).  When a fast
	forward update is not possible, create a merge commit.  This is
	the default behavior, unless merging an annotated (and possibly
	signed) tag that is not stored in its natural place in
	'refs/tags/' hierarchy, in which case --no-ff is assumed.
+
With --no-ff create a merge commit even when the merge could instead
resolve as a fast-forward.
+
With --ff-only resolve the merge as a fast-forward (never create a merge
commit). When fast-forward is not possible, refuse to merge and exit
with non-zero status.

-- 
Sergey

Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options

[Martin Ågren]

Hiya,

On Wed, 28 Aug 2019 at 21:15, Sergey Organov <sorganov@gmail.com> wrote:
> > I was sort of expecting these to be listed in the order "--ff, --no-ff,
> > --ff-only", and I see Sergey suggested the same ordering. The way your
> > proposed text reads does make sense though... Would it read as well
> > turning it over and going through the options in the other order? That's
> > the way it is before your patch, so you could argue "but people don't
> > grok that!". What your patch does well is to offer an overview before
> > describing each option in a bit more detail. Would that work with the
> > reversed direction as well (compared to this patch)? Dunno.

> Dunno if it helps, but here is what I came up with somewhere in previous
> discussions:
>
> --ff::
> --no-ff::
> --ff-only::
>         When the merge resolves as a fast-forward, only update the
>         branch pointer (without creating a merge commit).  When a fast
>         forward update is not possible, create a merge commit.  This is
>         the default behavior, unless merging an annotated (and possibly
>         signed) tag that is not stored in its natural place in
>         'refs/tags/' hierarchy, in which case --no-ff is assumed.
> +
> With --no-ff create a merge commit even when the merge could instead
> resolve as a fast-forward.
> +
> With --ff-only resolve the merge as a fast-forward (never create a merge
> commit). When fast-forward is not possible, refuse to merge and exit
> with non-zero status.

I think this could make clear that the first paragraph talks about
`--ff`. I know that we often list `--foo` and `--no-foo`, say "This
option does bar" and leave it to the reader to figure out which is which.
Most of the time, that's fairly obvious though (foo=bar). With this
tri-state option, we might be past the point of fair obviousness.
Then again, seeing "ff" and "fast forward", it's not /that hard/ to
figure out.

Apart from that, this reads well. Of course with the usual caveat of
the topic being fresh on my mind, so in general, I'd be more likely to
understand what it is I'm reading. ;-)

Martin

Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options

[Elijah Newren]

On Wed, Aug 28, 2019 at 12:15 PM Sergey Organov <sorganov@gmail.com> wrote:
>
> Hi,
>
[...]
> Dunno if it helps, but here is what I came up with somewhere in previous
> discussions:
>
> --ff::
> --no-ff::
> --ff-only::
>         When the merge resolves as a fast-forward, only update the

I think this loose wording (that you just took from the original) is
problematic.  Saying that a "merge resolves as a fast-forward" seems
to imply that there are circumstances when a fast-forward is the only
option.  An _individual_ can decide to resolve a merge as a
fast-forward in some circumstances, but it's certainly not the only
choice in any circumstance.  If you want to keep this wording short,
you could replace "resolves" with "can be resolved".

>         branch pointer (without creating a merge commit).  When a fast

Only update the branch pointer to what?  (Yes, I know the original
text we were improving left this unclear, but it's worth noting.)

>         forward update is not possible, create a merge commit.  This is
>         the default behavior, unless merging an annotated (and possibly
>         signed) tag that is not stored in its natural place in
>         'refs/tags/' hierarchy, in which case --no-ff is assumed.

Maybe it's just me, but I think it takes extra human cycles to figure
out that this paragraph is referring just to the --ff case, and that
users might not be able to do so until after reading the next 2-3
sentences.  While more brief, I think it will cause people to need to
read the description for these three options twice, removing most the
savings from being shorter.  It'd be better if it could be re-worded
to not need re-reads.

> +
> With --no-ff create a merge commit even when the merge could instead
> resolve as a fast-forward.
> +
> With --ff-only resolve the merge as a fast-forward (never create a merge
> commit). When fast-forward is not possible, refuse to merge and exit
> with non-zero status.

Something else I was trying to address with my patch that perhaps you
can see a different way to tackle: Using the wording "when possible"
is probably going to make users wonder when a fast forward is
possible; the "can be resolved" wording tweak also makes it more
likely they will wonder about this.  Another question they will be
wondering about is what a fast forward is (which you partially
explain).  Some basic knowledge of both are probably very useful in
helping them decide which option to actually pick.  As such, I think
trying to explain the answers to these sub-questions will assist them
in knowing which option to use.  Simply inserting a couple phrases
(e.g. "when the merged branch contains the current branch in its
history", and "only update the branch pointer *to match the merged
branch* and do not create a merge commit") may help a lot.

Anyway, I'll send a v3 addressing Martin's comments; if you've got
further suggestions for streamlining or rearranging, though, please do
send them along.

[PATCH v3] merge-options.txt: clarify meaning of various ff-related options

[Elijah Newren]

As discovered on the mailing list, some of the descriptions of the
ff-related options were unclear.  Try to be more precise with what these
options do.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
Changes since v2:
  * reordered the options
  * typeset the option flags differently to ensure they are monospace in
    the rendered documentation

 Documentation/merge-options.txt | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..ed3804650b 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -40,20 +40,26 @@ set to `no` at the beginning of them.
 	case of a merge conflict.
 
 --ff::
-	When the merge resolves as a fast-forward, only update the branch
-	pointer, without creating a merge commit.  This is the default
-	behavior.
-
 --no-ff::
-	Create a merge commit even when the merge resolves as a
-	fast-forward.  This is the default behaviour when merging an
-	annotated (and possibly signed) tag that is not stored in
-	its natural place in 'refs/tags/' hierarchy.
-
 --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.
+	Whether to prefer resolving the merge as a fast forward (only
+	updating the branch pointer to match the merged branch and not
+	creating a merge commit), to never allow it (always creating a
+	merge commit), or to only allow fast forward updates.  The
+	default is `--ff`, except when merging an annotated (and
+	possibly signed) tag that is not stored in its natural place
+	in the `refs/tags/` hierarchy (in which case `--no-ff` is
+	assumed).
++
+With `--ff`, resolve the merge as a fast-forward when possible (when the
+merged branch contains the current branch in its history).  When not
+possible, create a merge commit.
++
+With `--no-ff`, create a merge commit in all cases, even when the merge
+could instead be resolved as a fast-forward.
++
+With `--ff-only`, resolve the merge as a fast-forward when possible.
+When not possible, refuse to merge and exit with a non-zero status.
 
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
-- 
2.23.0.3.g93492fb5e6.dirty

Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options

[Sergey Organov]

Elijah Newren <newren@gmail.com> writes:

> On Wed, Aug 28, 2019 at 12:15 PM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Hi,
>>
> [...]
>> Dunno if it helps, but here is what I came up with somewhere in previous
>> discussions:
>>
>> --ff::
>> --no-ff::
>> --ff-only::
>>         When the merge resolves as a fast-forward, only update the
>
> I think this loose wording (that you just took from the original) is
> problematic.  Saying that a "merge resolves as a fast-forward" seems
> to imply that there are circumstances when a fast-forward is the only
> option.  An _individual_ can decide to resolve a merge as a
> fast-forward in some circumstances, but it's certainly not the only
> choice in any circumstance.  If you want to keep this wording short,
> you could replace "resolves" with "can be resolved".
>
>>         branch pointer (without creating a merge commit).  When a fast
>
> Only update the branch pointer to what?  (Yes, I know the original
> text we were improving left this unclear, but it's worth noting.)
>
>>         forward update is not possible, create a merge commit.  This is
>>         the default behavior, unless merging an annotated (and possibly
>>         signed) tag that is not stored in its natural place in
>>         'refs/tags/' hierarchy, in which case --no-ff is assumed.
>
> Maybe it's just me, but I think it takes extra human cycles to figure
> out that this paragraph is referring just to the --ff case, and that
> users might not be able to do so until after reading the next 2-3
> sentences.  While more brief, I think it will cause people to need to
> read the description for these three options twice, removing most the
> savings from being shorter.  It'd be better if it could be re-worded
> to not need re-reads.
>
>> +
>> With --no-ff create a merge commit even when the merge could instead
>> resolve as a fast-forward.
>> +
>> With --ff-only resolve the merge as a fast-forward (never create a merge
>> commit). When fast-forward is not possible, refuse to merge and exit
>> with non-zero status.
>
> Something else I was trying to address with my patch that perhaps you
> can see a different way to tackle: Using the wording "when possible"
> is probably going to make users wonder when a fast forward is
> possible; the "can be resolved" wording tweak also makes it more
> likely they will wonder about this.  Another question they will be
> wondering about is what a fast forward is (which you partially
> explain).  Some basic knowledge of both are probably very useful in
> helping them decide which option to actually pick.  As such, I think
> trying to explain the answers to these sub-questions will assist them
> in knowing which option to use.  Simply inserting a couple phrases
> (e.g. "when the merged branch contains the current branch in its
> history", and "only update the branch pointer *to match the merged
> branch* and do not create a merge commit") may help a lot.
>
> Anyway, I'll send a v3 addressing Martin's comments; if you've got
> further suggestions for streamlining or rearranging, though, please do
> send them along.

Thanks for thorough reply!

My version was meant to show how to re-arrange the description
preserving original wording as much as possible, so your version should
be better, as it addresses other problems as well.

-- 
Sergey

Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options

[Sergey Organov]

Martin Ågren <martin.agren@gmail.com> writes:

> Hiya,

This one was new to me :-)

>
> On Wed, 28 Aug 2019 at 21:15, Sergey Organov <sorganov@gmail.com> wrote:
>> > I was sort of expecting these to be listed in the order "--ff, --no-ff,
>> > --ff-only", and I see Sergey suggested the same ordering. The way your
>> > proposed text reads does make sense though... Would it read as well
>> > turning it over and going through the options in the other order? That's
>> > the way it is before your patch, so you could argue "but people don't
>> > grok that!". What your patch does well is to offer an overview before
>> > describing each option in a bit more detail. Would that work with the
>> > reversed direction as well (compared to this patch)? Dunno.
>
>> Dunno if it helps, but here is what I came up with somewhere in previous
>> discussions:
>>
>> --ff::
>> --no-ff::
>> --ff-only::
>>         When the merge resolves as a fast-forward, only update the
>>         branch pointer (without creating a merge commit).  When a fast
>>         forward update is not possible, create a merge commit.  This is
>>         the default behavior, unless merging an annotated (and possibly
>>         signed) tag that is not stored in its natural place in
>>         'refs/tags/' hierarchy, in which case --no-ff is assumed.
>> +
>> With --no-ff create a merge commit even when the merge could instead
>> resolve as a fast-forward.
>> +
>> With --ff-only resolve the merge as a fast-forward (never create a merge
>> commit). When fast-forward is not possible, refuse to merge and exit
>> with non-zero status.
>
> I think this could make clear that the first paragraph talks about
> `--ff`. I know that we often list `--foo` and `--no-foo`, say "This
> option does bar" and leave it to the reader to figure out which is which.
> Most of the time, that's fairly obvious though (foo=bar). With this
> tri-state option, we might be past the point of fair obviousness.
> Then again, seeing "ff" and "fast forward", it's not /that hard/ to
> figure out.

Yeah, I've noticed that myself, but decided to keep it, as this was
meant to be a quick re-arrangement of already existing description, and
be as short as at all possible, as lengthy descriptions tend to obscure
problems.

Being this way it shows how difficult it is to document flawed design. I
guess the author of --ff-only would think twice should --ff/--no-ff be
originally documented similarly to the rest of 2-way options. At least
he'd likely call it --only-ff, as:

--ff::
--no-ff::
--only-ff::

looks much better. Yeah, I do realize all this is historical and such...

>
> Apart from that, this reads well. Of course with the usual caveat of
> the topic being fresh on my mind, so in general, I'd be more likely to
> understand what it is I'm reading. ;-)

... and I'm in even worse position here ;-)

-- 
Sergey

Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

>> +--ff::
>> +       Whether to only allow resolving the merge as a fast forward
>> +       (only updating the branch pointer to match the merged branch
>> +       and not creating a merge commit), to never allow it (always
>> +       creating a merge commit), or to prefer it when possible.  The
>> +       default is --ff, except when merging an annotated (and
>
> It would be great if you'd write this as `--ff` so that it got
> monospaced in the rendered documentation. ...
> I'd also write `refs/tags/`, but I realize that you're just inheriting
> what is already here. ...

These comments may sound nitpicky at times, and some parts may be
beyond the scope of the discussion on the patch and are better left
for a later time, but the consistency is valuable.

Thanks for mentioning them; and it would be appreciated if these
suggesions are followed through after the dust settles.

Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options

[Elijah Newren]

On Fri, Aug 30, 2019 at 12:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> >> +--ff::
> >> +       Whether to only allow resolving the merge as a fast forward
> >> +       (only updating the branch pointer to match the merged branch
> >> +       and not creating a merge commit), to never allow it (always
> >> +       creating a merge commit), or to prefer it when possible.  The
> >> +       default is --ff, except when merging an annotated (and
> >
> > It would be great if you'd write this as `--ff` so that it got
> > monospaced in the rendered documentation. ...
> > I'd also write `refs/tags/`, but I realize that you're just inheriting
> > what is already here. ...
>
> These comments may sound nitpicky at times, and some parts may be
> beyond the scope of the discussion on the patch and are better left
> for a later time, but the consistency is valuable.
>
> Thanks for mentioning them; and it would be appreciated if these
> suggesions are followed through after the dust settles.

I'm confused; these suggestions were incorporated into V3 already.  Am
I misunderstanding something here?

(But yes, I agree that Martin's reviews are valuable.)

Re: [PATCH v3] merge-options.txt: clarify meaning of various ff-related options

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
> index 79a00d2a4a..ed3804650b 100644
> --- a/Documentation/merge-options.txt
> +++ b/Documentation/merge-options.txt
> @@ -40,20 +40,26 @@ set to `no` at the beginning of them.
>  	case of a merge conflict.
>  
>  --ff::
>  --no-ff::
>  --ff-only::
> +	Whether to prefer resolving the merge as a fast forward (only
> +	updating the branch pointer to match the merged branch and not
> +	creating a merge commit), to never allow it (always creating a
> +	merge commit), or to only allow fast forward updates.  The
> +	default is `--ff`, except when merging an annotated (and
> +	possibly signed) tag that is not stored in its natural place
> +	in the `refs/tags/` hierarchy (in which case `--no-ff` is
> +	assumed).
> ++
> +With `--ff`, resolve the merge as a fast-forward when possible (when the
> +merged branch contains the current branch in its history).  When not
> +possible, create a merge commit.
> ++
> +With `--no-ff`, create a merge commit in all cases, even when the merge
> +could instead be resolved as a fast-forward.
> ++
> +With `--ff-only`, resolve the merge as a fast-forward when possible.
> +When not possible, refuse to merge and exit with a non-zero status.

I cannot shake the feeling that the above redundantly repeats the
same thing twice.

If we want to dedicate one paragraph for each of these options, we
can and should make the introductory paragraph lighter by saying
something like

	Specifies how a merge is handled when the merged-in history
	is already a descendant of the current history.  `--ff` is
	the default unless merging an annotated or signed tag that
	is not stored in the `refs/tags/` hierarchy, in which case
	`--no-ff` is the default.

Alternatively, we could sprinkle the actual option name in the first
paragraph and drop the last three paragraphs, while fattening the
description as necessary, e.g.

	Whether to prefer resolving the merge as a fast-forward and
	update the branch pointer to match the merged branch without
	creating an extra merge commit (`--ff`), never allow fast-forward
	and always creating an extra merge commit (`--no-ff`), or to
	only allow fast forward updates and reject when a merge
	commit needs to be created (`--ff-only`).  The default is ...

I think either approach shown above would reduce the redundancy.  I
do not care too deeply which one of these approaches is used myself,
but the redundancy feels a bit disturbing.

Thanks.

Re: [PATCH v3] merge-options.txt: clarify meaning of various ff-related options

[Eric Sunshine]

On Fri, Aug 30, 2019 at 3:57 PM Junio C Hamano <gitster@pobox.com> wrote:
> If we want to dedicate one paragraph for each of these options, we
> can and should make the introductory paragraph lighter by saying
> something like
>
>         Specifies how a merge is handled when the merged-in history
>         is already a descendant of the current history.  `--ff` is
>         the default unless merging an annotated or signed tag that
>         is not stored in the `refs/tags/` hierarchy, in which case
>         `--no-ff` is the default.
>
> Alternatively, we could sprinkle the actual option name in the first
> paragraph and drop the last three paragraphs, while fattening the
> description as necessary, e.g.
>
>         Whether to prefer resolving the merge as a fast-forward and
>         update the branch pointer to match the merged branch without
>         creating an extra merge commit (`--ff`), never allow fast-forward
>         and always creating an extra merge commit (`--no-ff`), or to
>         only allow fast forward updates and reject when a merge
>         commit needs to be created (`--ff-only`).  The default is ...
>
> I think either approach shown above would reduce the redundancy.  I
> do not care too deeply which one of these approaches is used myself,
> but the redundancy feels a bit disturbing.

I have not been paying close attention to this thread, but upon
reading your suggested rewrites, I find the lighter paragraph (the
first of your options), followed by the three short paragraphs -- each
dedicated to a distinct option -- easier to follow and grok. I think
that's because the lighter/shorter arrangement keeps the three cases
reasonably separate -- thus the reader is able to absorb and
understand each distinct option in isolation -- rather than having to
manually pluck out the meaning of each option from one long, run-on
sentence.

Re: [PATCH v2] merge-options.txt: clarify meaning of various ff-related options

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> On Fri, Aug 30, 2019 at 12:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Martin Ågren <martin.agren@gmail.com> writes:
>>
>> >> +--ff::
>> >> +       Whether to only allow resolving the merge as a fast forward
>> >> +       (only updating the branch pointer to match the merged branch
>> >> +       and not creating a merge commit), to never allow it (always
>> >> +       creating a merge commit), or to prefer it when possible.  The
>> >> +       default is --ff, except when merging an annotated (and
>> >
>> > It would be great if you'd write this as `--ff` so that it got
>> > monospaced in the rendered documentation. ...
>> > I'd also write `refs/tags/`, but I realize that you're just inheriting
>> > what is already here. ...
>>
>> These comments may sound nitpicky at times, and some parts may be
>> beyond the scope of the discussion on the patch and are better left
>> for a later time, but the consistency is valuable.
>>
>> Thanks for mentioning them; and it would be appreciated if these
>> suggesions are followed through after the dust settles.
>
> I'm confused; these suggestions were incorporated into V3 already.  Am
> I misunderstanding something here?

I was assuming that outside the context of the patch there also are
the same malformatted reference to refs/blah and --option "you're
just inheriting", which would be cleaned up outside of this topic
after the dust settles, if the suggestions are followed through.

[PATCH v4] merge-options.txt: clarify meaning of various ff-related options

[Elijah Newren]

As discovered on the mailing list, some of the descriptions of the
ff-related options were unclear.  Try to be more precise with what these
options do.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
Changes since v3:
  * Shorten the leading paragraph, as suggested by Junio (and Eric).

 Documentation/merge-options.txt | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 79a00d2a4a..94be34f941 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -40,20 +40,24 @@ set to `no` at the beginning of them.
 	case of a merge conflict.
 
 --ff::
-	When the merge resolves as a fast-forward, only update the branch
-	pointer, without creating a merge commit.  This is the default
-	behavior.
-
 --no-ff::
-	Create a merge commit even when the merge resolves as a
-	fast-forward.  This is the default behaviour when merging an
-	annotated (and possibly signed) tag that is not stored in
-	its natural place in 'refs/tags/' hierarchy.
-
 --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.
+	Specifies how a merge is handled when the merged-in history is
+	already a descendant of the current history.  `--ff` is the
+	default unless merging an annotated (and possibly signed) tag
+	that is not stored in its natural place in the `refs/tags/`
+	hierarchy, in which case `--no-ff` is assumed.
++
+With `--ff`, when possible resolve the merge as a fast-forward (only
+update the branch pointer to match the merged branch; do not create a
+merge commit).  When not possible (when the merged-in history is not a
+descendant of the current history), create a merge commit.
++
+With `--no-ff`, create a merge commit in all cases, even when the merge
+could instead be resolved as a fast-forward.
++
+With `--ff-only`, resolve the merge as a fast-forward when possible.
+When not possible, refuse to merge and exit with a non-zero status.
 
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
-- 
2.23.0.39.gf92d9de5c3