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

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

[Ross Kabus]

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

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

[Jeff King]

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

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

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

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

[Ross Kabus]

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

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

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.

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

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.

> And definitely it does not when taking the message in via stdin.

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

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

[Jeff King]

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

> On Sat, Sep 2, 2017 at 4:33 AM, Jeff King <peff@peff.net> wrote:
> 
> > 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).
> 
> 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.

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

> > So I don't think "inconsistent with git-commit" is a compelling
> > argument, unless I'm missing something.
> 
> 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 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...

> > And definitely it does not when taking the message in via stdin.
> 
> 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.

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

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

[Ross Kabus]

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

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

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

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

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

[Jeff King]

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

> On Tue, Sep 5, 2017 at 11:36 AM, Jeff King <peff@peff.net> wrote:
> 
> > 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 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.

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

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

Ah, OK, your confusion makes more sense now.

-Peff

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

[Ross Kabus]

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

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;
  }

-- 
2.13.1.windows.2

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

[Ross Kabus]

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:
> 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(-)
>
> 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;
>   }
>
> --
> 2.13.1.windows.2

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 05, 2017 at 12:57:21PM -0400, Ross Kabus wrote:
>
>> On Tue, Sep 5, 2017 at 11:36 AM, Jeff King <peff@peff.net> wrote:
>> 
>> > 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 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.
>
> OK. Do you want to try your hand at a patch?
>
>> 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:
>
> Ah, OK, your confusion makes more sense now.

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.

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

[Jeff King]

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

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

> 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