[v2] merge-options.txt: clarify meaning of various ff-related options
diff mbox series

Message ID 20190828155131.29821-1-newren@gmail.com
State New
Headers show
Series
  • [v2] merge-options.txt: clarify meaning of various ff-related options
Related show

Commit Message

Elijah Newren Aug. 28, 2019, 3:51 p.m. UTC
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(-)

Comments

Martin Ågren Aug. 28, 2019, 6:45 p.m. UTC | #1
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
Sergey Organov Aug. 28, 2019, 7:15 p.m. UTC | #2
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.
Martin Ågren Aug. 28, 2019, 7:53 p.m. UTC | #3
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
Elijah Newren Aug. 28, 2019, 10:51 p.m. UTC | #4
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.
Sergey Organov Aug. 29, 2019, 9:15 a.m. UTC | #5
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 Organov Aug. 29, 2019, 9:35 a.m. UTC | #6
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 ;-)
Junio C Hamano Aug. 30, 2019, 7:45 p.m. UTC | #7
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.
Elijah Newren Aug. 30, 2019, 7:48 p.m. UTC | #8
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.)
Junio C Hamano Aug. 30, 2019, 8:27 p.m. UTC | #9
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
diff mbox series

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>]::