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

From: Martin Ågren <martin.agren@gmail.com>
Date: Aug 28, 2019, 18:45 UTC
Message-ID: <CAN0heSoqy4sCY8NUWKuEkXwe2XxnYAN6Mn2N75hYwfQ_5WGYdQ@mail.gmail.com>
In-Reply-To: <20190828155131.29821-1-newren@gmail.com>

Hi Elijah,

On Wed, 28 Aug 2019 at 17:52, Elijah Newren <newren@gmail.com> wrote:

Show 17 quoted lines

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

Show 15 quoted lines

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