Re: [PATCH 3/5] Update documentations for git-log to include the new --merges option and also its corresponding config option.

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 22, 2015, 21:17 UTC
Message-ID: <CAPig+cTQK_NF9pULobpggg8eXEKN_exj2buBEmk26TKY=DkapA@mail.gmail.com>
In-Reply-To: <1427048921-28677-3-git-send-email-koosha@posteo.de>

On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:

> Subject: Update documentations for git-log to include the new
> --merges option and also its corresponding config option.

The Subject: should be a short summary of the change; ideally less than 70 or 72 characters. The rest of the commit message can flesh out the description if necessary. The subject on this patch is far too long. Also, drop capitalization, mention the area you're touching, and drop the final period. You might say instead:

    Documentation: mention git-log --merges= and log.merges

In this case, it's probably not necessary to write anything else in the commit message. The summary says enough, despite its conciseness.

More below.

Show 12 quoted lines

> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> index 1f7bc67..506125a 100644
> --- a/Documentation/git-log.txt
> +++ b/Documentation/git-log.txt
> @@ -190,6 +190,9 @@ log.showroot::
>         `git log -p` output would be shown without a diff attached.
>         The default is `true`.
>
> +log.merges::
> +    Default for `--merges` option. Defaults to `show`.

To disambiguate this from the --merges option, you should probably spell it out explicitly as `--merges=` rather than just `--merges`.

Show 16 quoted lines

>  mailmap.*::
>         See linkgit:git-shortlog[1].
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 4ed8587..398e657 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -99,6 +99,12 @@ if it is part of the log message.
>  --merges::
>         Print only merge commits. This is exactly the same as `--min-parents=2`.
>
> +--merges=show|hide|only::
> +       If show is specified, merge commits will be shown in conjunction with
> +       other commits. If hide is specified, it will work like `--no-merges`.
> +       If only is specified, it will work like `--merges`. The default option
> +       is show.

By "The default option is show", do you mean when --merges= is not specified? Perhaps it would be better to phrase it as:

    Default is `show` if `--merges=` is not specified.

> +
>  --no-merges::
>         Do not print commits with more than one parent. This is
>         exactly the same as `--max-parents=1`.

It might make more sense to rewrite the --merges and --no-merges options in terms of the new --merges= option. For instance, something like this:

     --merges::
        Shorthand for `--merges=only`.

    --merges={show|hide|only}::
        `show`: show merge and non-merge commits

        `hide`: omit merge commits; same as `--max-parents=1`

        `only`: show only merge commits; same as `--min-parents=2`

        Default is `show` if `--merges=` is not specified.

     --no-merges::
        Shorthand for `--merges=hide`.

Alternately, --merges and --merges= could be combined like this (taking inspiration from --decorate[=]):

    --merges[=show|hide|only]::

But then you need additional explanation about what --merges means without the '=' and following keyword.

> --
> 2.3.3.263.g095251d.dirty