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

Hello,

When doing git commit-tree to manually create a commit object, it can be seen that the resulting commit's message has an extra appended newline (\n) that was not present in the input for either argument -m or -F. This is both undesirable and inconsistent with the git commit porcelain command.

In code this happens in the file "builtin\commit-tree.c" lines #80 and #105 (these lines numbers are the same for master, maint, and next branches). It seems like the calls to "strbuf_complete_line()" should just be removed to preserve the original input message exactly. As far as I can tell removing this call doesn't have any unintended side-effects.

git version 2.13.1.windows.2

Thanks, Ross

On Fri, Sep 01, 2017 at 02:58:52PM -0400, Ross Kabus wrote:

Hmm. As a plumbing command, I'd expect commit-tree to generally _not_ perform such niceties. And definitely it does not when taking the message in via stdin. In Git's original design, commit object bodies do not even have to be text, though certainly the porcelain tools all assume they are.

But I am confused by your "inconsistent with git commit porcelain" comment. The porcelain git-commit definitely _does_ add a newline if one isn't present (and in fact runs the whole thing through git-stripspace to clean up whitespace oddities).

So I don't think "inconsistent with git-commit" is a compelling argument, unless I'm missing something.

I _could_ see an argument for "commit-tree as plumbing should always treat the message verbatim". But since "-F" and "-m" have done the newline-completion since their inception, I'm not sure it's a good idea to change them now. The current behavior also makes some sense, as it's consistent with the matching options in git-commit (again, as far as I can see; if you have a counter-example it would be interesting to see).

-Peff

On Sat, Sep 2, 2017 at 4:33 AM, Jeff King <peff@peff.net> wrote:

Ok I figured out my confusion. The repository I am working with did commits with "git commit --cleanup=verbatim" thus do not have a newline. This is why I thought there was an inconsistency.

Agreed, but now I guess I would argue that it is inconsistent because it lacks a "verbatim" option like git-commit has. I would like to see an argument like this for commit-tree but a clean way to add this option didn't immediately jump out at me.

I'm not seeing this, I see commit-tree as adding a new line even via stdin (and the code seems to corroborate this), unless I missed something.

~Ross

On Tue, Sep 05, 2017 at 11:09:01AM -0400, Ross Kabus wrote:

OK, that makes more sense. Though I think even with verbatim, "-m" will still complete a line (it happens in opt_parse_m):

  $ git commit -q --allow-empty -m foo
  $ git cat-file commit HEAD | xxd
  ...
  00000090: 3034 3030 0a0a 666f 6f0a                 0400..foo.
                                  ^^
				  newline

That makes sense, since the idea of "-m" is to take a one-liner, but you would not typically provide the newline yourself via the shell.

It looks like "-F" does not do an explicit complete-line (so it would depend on --cleanup to do it normally, but in verbatim mode would not). That makes sense to me, as well (in a full file, usually you _would_ have the newline, or choose to omit it intentionally, and we should respect that). So I'd argue that "git commit -F" is doing a reasonable thing, and "commit-tree -F" should probably change to match it (because it's inconsistent, and because if anything the plumbing commit-tree should err more on the side of not touching its input than the porcelain commit command).

I think the default behavior of reading via stdin _is_ the verbatim option. And you can choose whether or not to pipe it through git-stripspace to do more cleanup (and in fact, when git-commit was implemented as a shell script long ago, that's exactly how it was implemented). But...

I'm not sure why you're seeing that. It seems verbatim to me:

  $ tree=$(git write-tree)
  $ commit=$(printf end | git commit-tree $tree)
  $ git cat-file commit $commit | xxd
  ...
  00000090: 3034 3030 0a0a 656e 64                   0400..end

-Peff

On Tue, Sep 5, 2017 at 11:36 AM, Jeff King <peff@peff.net> wrote:

I would agree that "commit-tree -F" should change to match the behavior of "git commit -F --cleanup=verbatim". I feel pretty strongly that this type of cleanup logic shouldn't be done in a plumbing command, though I'm not sure it is a big enough deal to change behavior/compatibility for everyone.

Yup, confusion #2. I was using "-F -" which I see now is a different code path. Reading via stdin without "-F -" _is_ the verbatim option. This difference burned someone else on the mailing list as well. See:

https://public-inbox.org/git/CACAUoX6zT0wXxCLXK+sk0e4hgfD_A_EWWKvWnTOXK0-hzw7BUg@mail.gmail.com/

Probably more reason that "-F" should be 'verbatim' by default.

~Ross

On Tue, Sep 05, 2017 at 12:57:21PM -0400, Ross Kabus wrote:

OK. Do you want to try your hand at a patch?

Ah, OK, your confusion makes more sense now.

-Peff

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

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).
---
 builtin/commit-tree.c | 1 -
 1 file changed, 1 deletion(-)

Gmail mangled that patch of course... Ross Kabus Software Engineer www.aerotech.com | 412-968-2833

On Tue, Sep 5, 2017 at 4:57 PM, Ross Kabus <rkabus@aerotech.com> wrote:

Thanks for following up. A few minor comments:

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

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

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.

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

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

-Peff

Jeff King <peff@peff.net> writes:

Yeah, my initial knee-jerk reaction when I started reading the early part of the thread was that the use of strbuf_complete_line() is a strong enough sign that this was intended behaviour, but the "we get different results between reading from standard input and pretending '-' is a file and reading from it" made me change my mind. I agree that dropping strbuf_complete_line() call from that codepath (and nowhere else, especially the "-m" oneline thing [*1*]) would be a backward incompatible change that is acceptable.

Here is what I am preparing to add to the What's cooking report when such a patch materializes ;-)

 * "git commit-tree -F $file" used to add terminating newline if the
   input ends with an incomplete line, but when the command reads
   the input from its standard input, we recorded what was given
   verbatim.  The former has been changed to record the input with
   an incomplete line to make them consistent.

Something like that probably needs to be added to the release notes but I am way being ahead of myself ;-)

Thanks, both, for sensible discussion.

[Footnote]

*1* The message from Ross you are responding to talks about
    "cleanup", but I tend to view the calls to *_complete_line() as
    more for "the end-user convenience".  "-m" meant to take a
    one-liner from the command line should have it because exactly
    as you said, having to pass the terminating newline from the
    command line with "-m" is awkward.  On the other hand, if the
    user/caller prepared the exact message in a file and wants to
    feed it either via "-F" or from the standard input, I agree that
    "the end-user convenience" would get in the way and it is
    preferrable to avoid it in the plumbing layer.