Re: [Bug] commit-tree shouldn't append an extra newline to commit messages

From: Jeff King <peff@peff.net>
Date: Sep 7, 2017, 05:57 UTC
Message-ID: <20170907055701.ujibki2jwmzhhycm@sigill.intra.peff.net>
In-Reply-To: <CAEVs+zZRw3kW0C56NcJNXieCSTOa=uMxg6tDPjU2u+WDzhTLfw@mail.gmail.com>

Thanks for following up. A few minor comments:

On Tue, Sep 05, 2017 at 04:57:24PM -0400, Ross Kabus wrote:

> From: Ross Kabus <rkabus@aerotech.com>
> Date: Tue, 5 Sep 2017 13:54:52 -0400
> Subject: [PATCH] commit-tree: don't append a newline with -F

Usually you'd just omit these in favor of the email headers (and replace the email subject with this one).

Show 13 quoted lines

> This change makes it such that commit-tree -F never appends a newline
> character to the supplied commit message (either from file or stdin).
> 
> Previously, commit-tree -F would always append a newline character to
> the text brought in from file or stdin. This has caused confusion in a
> number of ways:
>   - This is a plumbing command and it is generally expected not to do
>     text cleanup or other niceties.
>   - stdin piping with "-F -" appends a newline but stdin piping without
>     -F does not append a newline (inconsistent).
>   - git-commit has the --cleanup=verbatim option that prevents appending
>     a newline with its -F argument. There is no verbatim counterpart to
>     commit-tree -F (inconsistent).

This explanation all makes sense to me except for the last bit, which is a bit subtle. I'd have said it more like:

  - git-commit does not specifically append a newline to the "-F"
    input. The issue is somewhat muddled by the fact that git-commit
    does pass the message through its --cleanup option, which may add
    such a newline. But for commit-tree to match "commit --cleanup=verbatim",
    we should not do so here.

> ---
>  builtin/commit-tree.c | 1 -
>  1 file changed, 1 deletion(-)

Your patch needs to be signed-off; see the Developer's Certificate of Origin section in Documentation/SubmittingPatches.

Show 11 quoted lines

> diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
> index 19e898fa4..2177251e2 100644
> --- a/builtin/commit-tree.c
> +++ b/builtin/commit-tree.c
> @@ -102,7 +102,6 @@ int cmd_commit_tree(int argc, const char **argv,
> const char *prefix)
>   if (fd && close(fd))
>   die_errno("git commit-tree: failed to close '%s'",
>    argv[i]);
> - strbuf_complete_line(&buffer);
>   continue;

Aside from the whitespace damage, the patch itself looks good.

-Peff