Re: [PATCH v2 4/4] format-patch: learn --infer-cover-letter option

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 20, 2019, 03:46 UTC
Message-ID: <CAPig+cRfk7jdhqRX2OiyftW0eaM040u6KgRPLUWin7Z3W1LgDA@mail.gmail.com>
In-Reply-To: <e682bd347a09d0e1293ec6bd495c38dac5006a19.1566258525.git.liu.denton@gmail.com>

On Mon, Aug 19, 2019 at 7:53 PM Denton Liu <liu.denton@gmail.com> wrote:

Show 6 quoted lines

> We used to populate the subject of the cover letter generated by
> git-format-patch with "*** SUBJECT HERE ***". However, if a user submits
> multiple patchsets, they may want to keep a consistent subject between
> rerolls.
>
> If git-format-patch is run with `--infer-cover-letter` or

s/letter/subject/

> `format.inferCoverSubject`, infer the subject for the cover letter from
> the top line(s) of a branch description, similar to how a subject is
> read from a commit message.

A possible rewrite of the entire commit message in imperative mood:

    Teach 'format-patch' to use the first line of the branch description
    as the Subject: of the generated cover letter, rather than
    "*** SUBJECT HERE ***", if --infer-cover-subject is specified (or the
    corresponding `format.inferCoverSubject` option is enabled). This
    complements existing inclusion of the branch description in the
    cover letter body.

A casual reader of this patch might wonder why this new useful behavior isn't default, so it might make sense for the commit message to further explain that making it default would potentially break existing tooling.

Show 7 quoted lines

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
> @@ -36,6 +36,10 @@ format.subjectPrefix::
> +format.inferCoverSubject::
> +       A boolean value which lets you enable the
> +       `--infer-cover-subject` option of format-patch by default.

As mentioned in my review of 3/4, it is common to mention the default value at the end of the paragraph. So, perhaps:

    A boolean that controls whether or not to take the first line of
    the branch description as the subject for the cover letter. See the
    `--infer-cover-subject` option in linkgit:git-format-patch[1].
    Default is false.

Show 9 quoted lines

> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> @@ -171,6 +172,14 @@ will want to ensure that threading is disabled for `git send-email`.
> +--[no-]infer-cover-subject::
> +       Instead of using the default "*** SUBJECT HERE ***" subject for
> +       the cover letter, infer the subject from the branch's
> +       description.
> ++
> +Similar to a commit message, the subject is inferred as the beginning of
> +the description up to and excluding the first blank line.

I think this can all be collapsed to the simpler:

    Use the beginning of the branch description (up to the first
    blank line) as the cover letter subject instead of the default
    "*** SUBJECT HERE ***".

or something.

> diff --git a/builtin/log.c b/builtin/log.c
> @@ -1577,6 +1589,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> +               OPT_BOOL(0, "infer-cover-subject", &infer_cover_subject,
> +                           N_("infer a cover letter subject from the branch description")),

Shorter: "infer cover letter subject from branch description"