{"thread":{"id":"46696","subject":"[Bug] commit-tree shouldn't append an extra newline to commit messages","startedAt":"2017-09-01T18:59:39Z","lastAt":"2017-09-07T05:57:09Z","messageCount":10,"participants":["Ross Kabus","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"327507","messageId":"CAEVs+za9do_wXC12SSRznF9v9oGw3_Grq2EFDVf8nH1CRgM-Qw@mail.gmail.com","threadId":"46696","inReplyTo":null,"subject":"[Bug] commit-tree shouldn't append an extra newline to commit messages","fromName":"Ross Kabus","fromEmail":"rkabus@aerotech.com","sentAt":"2017-09-01T18:58:52Z","receivedAt":"2017-09-01T18:59:39Z","isPatch":false,"sender":{"key":"rkabus@aerotech.com","avatar":"https://avatars.githubusercontent.com/u/18420751?v=4"},"body":"Hello,\n\nWhen doing git commit-tree to manually create a commit object, it can be seen\nthat the resulting commit's message has an extra appended newline (\\n) that\nwas not present in the input for either argument -m or -F. This is both\nundesirable and inconsistent with the git commit porcelain command.\n\nIn code this happens in the file \"builtin\\commit-tree.c\" lines #80 and #105\n(these lines numbers are the same for master, maint, and next branches). It\nseems like the calls to \"strbuf_complete_line()\" should just be removed to\npreserve the original input message exactly. As far as I can tell removing\nthis call doesn't have any unintended side-effects.\n\ngit version 2.13.1.windows.2\n\nThanks,\nRoss\n"},{"id":"327526","messageId":"20170902083319.lcugfpkkk5lahieb@sigill.intra.peff.net","threadId":"46696","inReplyTo":"CAEVs+za9do_wXC12SSRznF9v9oGw3_Grq2EFDVf8nH1CRgM-Qw@mail.gmail.com","subject":"Re: [Bug] commit-tree shouldn't append an extra newline to commit messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-02T08:33:19Z","receivedAt":"2017-09-02T08:33:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 01, 2017 at 02:58:52PM -0400, Ross Kabus wrote:\n\n> When doing git commit-tree to manually create a commit object, it can be seen\n> that the resulting commit's message has an extra appended newline (\\n) that\n> was not present in the input for either argument -m or -F. This is both\n> undesirable and inconsistent with the git commit porcelain command.\n\nHmm. As a plumbing command, I'd expect commit-tree to generally _not_\nperform such niceties. And definitely it does not when taking the\nmessage in via stdin. In Git's original design, commit object bodies do\nnot even have to be text, though certainly the porcelain tools all\nassume they are.\n\nBut I am confused by your \"inconsistent with git commit porcelain\"\ncomment. The porcelain git-commit definitely _does_ add a newline if one\nisn't present (and in fact runs the whole thing through git-stripspace\nto clean up whitespace oddities).\n\nSo I don't think \"inconsistent with git-commit\" is a compelling\nargument, unless I'm missing something.\n\nI _could_ see an argument for \"commit-tree as plumbing should always\ntreat the message verbatim\". But since \"-F\" and \"-m\" have done the\nnewline-completion since their inception, I'm not sure it's a good idea\nto change them now. The current behavior also makes some sense, as it's\nconsistent with the matching options in git-commit (again, as far as I\ncan see; if you have a counter-example it would be interesting to see).\n\n-Peff\n"},{"id":"327582","messageId":"CAEVs+zbbWQuM-=5d04bkpTu38Mr4PyczskNhni5K1u_nzh-2Qw@mail.gmail.com","threadId":"46696","inReplyTo":"20170902083319.lcugfpkkk5lahieb@sigill.intra.peff.net","subject":"Re: [Bug] commit-tree shouldn't append an extra newline to commit messages","fromName":"Ross Kabus","fromEmail":"rkabus@aerotech.com","sentAt":"2017-09-05T15:09:01Z","receivedAt":"2017-09-05T15:09:50Z","isPatch":false,"sender":{"key":"rkabus@aerotech.com","avatar":"https://avatars.githubusercontent.com/u/18420751?v=4"},"body":"On Sat, Sep 2, 2017 at 4:33 AM, Jeff King wrote:\n\n> But I am confused by your \"inconsistent with git commit porcelain\"\n> comment. The porcelain git-commit definitely _does_ add a newline if one\n> isn't present (and in fact runs the whole thing through git-stripspace\n> to clean up whitespace oddities).\n\nOk I figured out my confusion. The repository I am working with did\ncommits with \"git commit --cleanup=verbatim\" thus do not have a newline.\nThis is why I thought there was an inconsistency.\n\n> So I don't think \"inconsistent with git-commit\" is a compelling\n> argument, unless I'm missing something.\n\nAgreed, but now I guess I would argue that it is inconsistent because\nit lacks a \"verbatim\" option like git-commit has. I would like to see\nan argument like this for commit-tree but a clean way to add this option\ndidn't immediately jump out at me.\n\n> And definitely it does not when taking the message in via stdin.\n\nI'm not seeing this, I see commit-tree as adding a new line even via\nstdin (and the code seems to corroborate this), unless I missed something.\n\n~Ross\n"},{"id":"327583","messageId":"20170905153636.tsmlq3wv7ztpc67z@sigill.intra.peff.net","threadId":"46696","inReplyTo":"CAEVs+zbbWQuM-=5d04bkpTu38Mr4PyczskNhni5K1u_nzh-2Qw@mail.gmail.com","subject":"Re: [Bug] commit-tree shouldn't append an extra newline to commit messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T15:36:36Z","receivedAt":"2017-09-05T15:36:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2017 at 11:09:01AM -0400, Ross Kabus wrote:\n\n> On Sat, Sep 2, 2017 at 4:33 AM, Jeff King wrote:\n> \n> > But I am confused by your \"inconsistent with git commit porcelain\"\n> > comment. The porcelain git-commit definitely _does_ add a newline if one\n> > isn't present (and in fact runs the whole thing through git-stripspace\n> > to clean up whitespace oddities).\n> \n> Ok I figured out my confusion. The repository I am working with did\n> commits with \"git commit --cleanup=verbatim\" thus do not have a newline.\n> This is why I thought there was an inconsistency.\n\nOK, that makes more sense. Though I think even with verbatim, \"-m\" will\nstill complete a line (it happens in opt_parse_m):\n\n $ git commit -q --allow-empty -m foo\n $ git cat-file commit HEAD | xxd\n ...\n 00000090: 3034 3030 0a0a 666f 6f0a 0400..foo.\n ^^\n\t\t\t\t newline\n\nThat makes sense, since the idea of \"-m\" is to take a one-liner, but you\nwould not typically provide the newline yourself via the shell.\n\nIt looks like \"-F\" does not do an explicit complete-line (so it would\ndepend on --cleanup to do it normally, but in verbatim mode would not).\nThat makes sense to me, as well (in a full file, usually you _would_\nhave the newline, or choose to omit it intentionally, and we should\nrespect that). So I'd argue that \"git commit -F\" is doing a reasonable\nthing, and \"commit-tree -F\" should probably change to match it (because\nit's inconsistent, and because if anything the plumbing commit-tree\nshould err more on the side of not touching its input than the porcelain\ncommit command).\n\n> > So I don't think \"inconsistent with git-commit\" is a compelling\n> > argument, unless I'm missing something.\n> \n> Agreed, but now I guess I would argue that it is inconsistent because\n> it lacks a \"verbatim\" option like git-commit has. I would like to see\n> an argument like this for commit-tree but a clean way to add this option\n> didn't immediately jump out at me.\n\nI think the default behavior of reading via stdin _is_ the verbatim\noption. And you can choose whether or not to pipe it through\ngit-stripspace to do more cleanup (and in fact, when git-commit was\nimplemented as a shell script long ago, that's exactly how it was\nimplemented). But...\n\n> > And definitely it does not when taking the message in via stdin.\n> \n> I'm not seeing this, I see commit-tree as adding a new line even via\n> stdin (and the code seems to corroborate this), unless I missed something.\n\nI'm not sure why you're seeing that. It seems verbatim to me:\n\n $ tree=$(git write-tree)\n $ commit=$(printf end | git commit-tree $tree)\n $ git cat-file commit $commit | xxd\n ...\n 00000090: 3034 3030 0a0a 656e 64 0400..end\n\n-Peff\n"},{"id":"327587","messageId":"CAEVs+zbCj0Zv0t4_WG6y2jcLoXwHy-Mu-LH31c_QgFaE9i3HtQ@mail.gmail.com","threadId":"46696","inReplyTo":"20170905153636.tsmlq3wv7ztpc67z@sigill.intra.peff.net","subject":"Re: [Bug] commit-tree shouldn't append an extra newline to commit messages","fromName":"Ross Kabus","fromEmail":"rkabus@aerotech.com","sentAt":"2017-09-05T16:57:21Z","receivedAt":"2017-09-05T16:58:11Z","isPatch":false,"sender":{"key":"rkabus@aerotech.com","avatar":"https://avatars.githubusercontent.com/u/18420751?v=4"},"body":"On Tue, Sep 5, 2017 at 11:36 AM, Jeff King wrote:\n\n> So I'd argue that \"git commit -F\" is doing a reasonable\n> thing, and \"commit-tree -F\" should probably change to match it (because\n> it's inconsistent, and because if anything the plumbing commit-tree\n> should err more on the side of not touching its input than the porcelain\n> commit command).\n\nI would agree that \"commit-tree -F\" should change to match the behavior of\n\"git commit -F --cleanup=verbatim\". I feel pretty strongly that this type of\ncleanup logic shouldn't be done in a plumbing command, though I'm not sure\nit is a big enough deal to change behavior/compatibility for everyone.\n\n> $ tree=$(git write-tree)\n> $ commit=$(printf end | git commit-tree $tree)\n> $ git cat-file commit $commit | xxd\n> ...\n> 00000090: 3034 3030 0a0a 656e 64 0400..end\n\nYup, confusion #2. I was using \"-F -\" which I see now is a different code\npath. Reading via stdin without \"-F -\" _is_ the verbatim option. This\ndifference burned someone else on the mailing list as well. See:\n\nhttps://public-inbox.org/git/CACAUoX6zT0wXxCLXK+sk0e4hgfD_A_EWWKvWnTOXK0-hzw7BUg@mail.gmail.com/\n\nProbably more reason that \"-F\" should be 'verbatim' by default.\n\n~Ross\n"},{"id":"327588","messageId":"20170905170311.yhcksrw2bxevd3hk@sigill.intra.peff.net","threadId":"46696","inReplyTo":"CAEVs+zbCj0Zv0t4_WG6y2jcLoXwHy-Mu-LH31c_QgFaE9i3HtQ@mail.gmail.com","subject":"Re: [Bug] commit-tree shouldn't append an extra newline to commit messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T17:03:12Z","receivedAt":"2017-09-05T17:03:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2017 at 12:57:21PM -0400, Ross Kabus wrote:\n\n> On Tue, Sep 5, 2017 at 11:36 AM, Jeff King wrote:\n> \n> > So I'd argue that \"git commit -F\" is doing a reasonable\n> > thing, and \"commit-tree -F\" should probably change to match it (because\n> > it's inconsistent, and because if anything the plumbing commit-tree\n> > should err more on the side of not touching its input than the porcelain\n> > commit command).\n> \n> I would agree that \"commit-tree -F\" should change to match the behavior of\n> \"git commit -F --cleanup=verbatim\". I feel pretty strongly that this type of\n> cleanup logic shouldn't be done in a plumbing command, though I'm not sure\n> it is a big enough deal to change behavior/compatibility for everyone.\n\nOK. Do you want to try your hand at a patch?\n\n> Yup, confusion #2. I was using \"-F -\" which I see now is a different code\n> path. Reading via stdin without \"-F -\" _is_ the verbatim option. This\n> difference burned someone else on the mailing list as well. See:\n\nAh, OK, your confusion makes more sense now.\n\n-Peff\n"},{"id":"327603","messageId":"CAEVs+zZRw3kW0C56NcJNXieCSTOa=uMxg6tDPjU2u+WDzhTLfw@mail.gmail.com","threadId":"46696","inReplyTo":"20170905170311.yhcksrw2bxevd3hk@sigill.intra.peff.net","subject":"Re: [Bug] commit-tree shouldn't append an extra newline to commit messages","fromName":"Ross Kabus","fromEmail":"rkabus@aerotech.com","sentAt":"2017-09-05T20:57:24Z","receivedAt":"2017-09-05T20:58:14Z","isPatch":false,"sender":{"key":"rkabus@aerotech.com","avatar":"https://avatars.githubusercontent.com/u/18420751?v=4"},"body":"From: Ross Kabus \nDate: Tue, 5 Sep 2017 13:54:52 -0400\nSubject: [PATCH] commit-tree: don't append a newline with -F\n\nThis change makes it such that commit-tree -F never appends a newline\ncharacter to the supplied commit message (either from file or stdin).\n\nPreviously, commit-tree -F would always append a newline character to\nthe text brought in from file or stdin. This has caused confusion in a\nnumber of ways:\n - This is a plumbing command and it is generally expected not to do\n text cleanup or other niceties.\n - stdin piping with \"-F -\" appends a newline but stdin piping without\n -F does not append a newline (inconsistent).\n - git-commit has the --cleanup=verbatim option that prevents appending\n a newline with its -F argument. There is no verbatim counterpart to\n commit-tree -F (inconsistent).\n---\n builtin/commit-tree.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 19e898fa4..2177251e2 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -102,7 +102,6 @@ int cmd_commit_tree(int argc, const char **argv,\nconst char *prefix)\n if (fd && close(fd))\n die_errno(\"git commit-tree: failed to close '%s'\",\n argv[i]);\n- strbuf_complete_line(&buffer);\n continue;\n }\n\n-- \n2.13.1.windows.2\n"},{"id":"327604","messageId":"CAEVs+zafc_=_xV3pAmFq+wCCc_Ex4HhTOLJBqyDnodVQ+pczBQ@mail.gmail.com","threadId":"46696","inReplyTo":"CAEVs+zZRw3kW0C56NcJNXieCSTOa=uMxg6tDPjU2u+WDzhTLfw@mail.gmail.com","subject":"Re: [Bug] commit-tree shouldn't append an extra newline to commit messages","fromName":"Ross Kabus","fromEmail":"rkabus@aerotech.com","sentAt":"2017-09-05T20:59:22Z","receivedAt":"2017-09-05T21:00:12Z","isPatch":false,"sender":{"key":"rkabus@aerotech.com","avatar":"https://avatars.githubusercontent.com/u/18420751?v=4"},"body":"Gmail mangled that patch of course...\nRoss Kabus\nSoftware Engineer\nwww.aerotech.com | 412-968-2833\n\n\nOn Tue, Sep 5, 2017 at 4:57 PM, Ross Kabus wrote:\n> From: Ross Kabus \n> Date: Tue, 5 Sep 2017 13:54:52 -0400\n> Subject: [PATCH] commit-tree: don't append a newline with -F\n>\n> This change makes it such that commit-tree -F never appends a newline\n> character to the supplied commit message (either from file or stdin).\n>\n> Previously, commit-tree -F would always append a newline character to\n> the text brought in from file or stdin. This has caused confusion in a\n> number of ways:\n> - This is a plumbing command and it is generally expected not to do\n> text cleanup or other niceties.\n> - stdin piping with \"-F -\" appends a newline but stdin piping without\n> -F does not append a newline (inconsistent).\n> - git-commit has the --cleanup=verbatim option that prevents appending\n> a newline with its -F argument. There is no verbatim counterpart to\n> commit-tree -F (inconsistent).\n> ---\n> builtin/commit-tree.c | 1 -\n> 1 file changed, 1 deletion(-)\n>\n> diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\n> index 19e898fa4..2177251e2 100644\n> --- a/builtin/commit-tree.c\n> +++ b/builtin/commit-tree.c\n> @@ -102,7 +102,6 @@ int cmd_commit_tree(int argc, const char **argv,\n> const char *prefix)\n> if (fd && close(fd))\n> die_errno(\"git commit-tree: failed to close '%s'\",\n> argv[i]);\n> - strbuf_complete_line(&buffer);\n> continue;\n> }\n>\n> --\n> 2.13.1.windows.2\n"},{"id":"327612","messageId":"xmqq3780byhw.fsf@gitster.mtv.corp.google.com","threadId":"46696","inReplyTo":"20170905170311.yhcksrw2bxevd3hk@sigill.intra.peff.net","subject":"Re: [Bug] commit-tree shouldn't append an extra newline to commit messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-05T21:56:43Z","receivedAt":"2017-09-05T21:56:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 05, 2017 at 12:57:21PM -0400, Ross Kabus wrote:\n>\n>> On Tue, Sep 5, 2017 at 11:36 AM, Jeff King wrote:\n>> \n>> > So I'd argue that \"git commit -F\" is doing a reasonable\n>> > thing, and \"commit-tree -F\" should probably change to match it (because\n>> > it's inconsistent, and because if anything the plumbing commit-tree\n>> > should err more on the side of not touching its input than the porcelain\n>> > commit command).\n>> \n>> I would agree that \"commit-tree -F\" should change to match the behavior of\n>> \"git commit -F --cleanup=verbatim\". I feel pretty strongly that this type of\n>> cleanup logic shouldn't be done in a plumbing command, though I'm not sure\n>> it is a big enough deal to change behavior/compatibility for everyone.\n>\n> OK. Do you want to try your hand at a patch?\n>\n>> Yup, confusion #2. I was using \"-F -\" which I see now is a different code\n>> path. Reading via stdin without \"-F -\" _is_ the verbatim option. This\n>> difference burned someone else on the mailing list as well. See:\n>\n> Ah, OK, your confusion makes more sense now.\n\nYeah, my initial knee-jerk reaction when I started reading the early\npart of the thread was that the use of strbuf_complete_line() is a\nstrong enough sign that this was intended behaviour, but the \"we get\ndifferent results between reading from standard input and pretending\n'-' is a file and reading from it\" made me change my mind. I agree\nthat dropping strbuf_complete_line() call from that codepath (and\nnowhere else, especially the \"-m\" oneline thing [*1*]) would be a\nbackward incompatible change that is acceptable.\n\nHere is what I am preparing to add to the What's cooking report when\nsuch a patch materializes ;-)\n\n * \"git commit-tree -F $file\" used to add terminating newline if the\n input ends with an incomplete line, but when the command reads\n the input from its standard input, we recorded what was given\n verbatim. The former has been changed to record the input with\n an incomplete line to make them consistent.\n\nSomething like that probably needs to be added to the release notes\nbut I am way being ahead of myself ;-)\n\nThanks, both, for sensible discussion.\n\n\n\n[Footnote]\n\n*1* The message from Ross you are responding to talks about\n \"cleanup\", but I tend to view the calls to *_complete_line() as\n more for \"the end-user convenience\". \"-m\" meant to take a\n one-liner from the command line should have it because exactly\n as you said, having to pass the terminating newline from the\n command line with \"-m\" is awkward. On the other hand, if the\n user/caller prepared the exact message in a file and wants to\n feed it either via \"-F\" or from the standard input, I agree that\n \"the end-user convenience\" would get in the way and it is\n preferrable to avoid it in the plumbing layer.\n"},{"id":"327690","messageId":"20170907055701.ujibki2jwmzhhycm@sigill.intra.peff.net","threadId":"46696","inReplyTo":"CAEVs+zZRw3kW0C56NcJNXieCSTOa=uMxg6tDPjU2u+WDzhTLfw@mail.gmail.com","subject":"Re: [Bug] commit-tree shouldn't append an extra newline to commit messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-07T05:57:01Z","receivedAt":"2017-09-07T05:57:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Thanks for following up. A few minor comments:\n\nOn Tue, Sep 05, 2017 at 04:57:24PM -0400, Ross Kabus wrote:\n\n> From: Ross Kabus \n> Date: Tue, 5 Sep 2017 13:54:52 -0400\n> Subject: [PATCH] commit-tree: don't append a newline with -F\n\nUsually you'd just omit these in favor of the email headers (and replace\nthe email subject with this one).\n\n> This change makes it such that commit-tree -F never appends a newline\n> character to the supplied commit message (either from file or stdin).\n> \n> Previously, commit-tree -F would always append a newline character to\n> the text brought in from file or stdin. This has caused confusion in a\n> number of ways:\n> - This is a plumbing command and it is generally expected not to do\n> text cleanup or other niceties.\n> - stdin piping with \"-F -\" appends a newline but stdin piping without\n> -F does not append a newline (inconsistent).\n> - git-commit has the --cleanup=verbatim option that prevents appending\n> a newline with its -F argument. There is no verbatim counterpart to\n> commit-tree -F (inconsistent).\n\nThis explanation all makes sense to me except for the last bit, which is\na bit subtle. I'd have said it more like:\n\n - git-commit does not specifically append a newline to the \"-F\"\n input. The issue is somewhat muddled by the fact that git-commit\n does pass the message through its --cleanup option, which may add\n such a newline. But for commit-tree to match \"commit --cleanup=verbatim\",\n we should not do so here.\n\n> ---\n> builtin/commit-tree.c | 1 -\n> 1 file changed, 1 deletion(-)\n\nYour patch needs to be signed-off; see the Developer's Certificate of\nOrigin section in Documentation/SubmittingPatches.\n\n> diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\n> index 19e898fa4..2177251e2 100644\n> --- a/builtin/commit-tree.c\n> +++ b/builtin/commit-tree.c\n> @@ -102,7 +102,6 @@ int cmd_commit_tree(int argc, const char **argv,\n> const char *prefix)\n> if (fd && close(fd))\n> die_errno(\"git commit-tree: failed to close '%s'\",\n> argv[i]);\n> - strbuf_complete_line(&buffer);\n> continue;\n\nAside from the whitespace damage, the patch itself looks good.\n\n-Peff\n"}]}