Re: [Bug] commit-tree shouldn't append an extra newline to commit messages
Thanks for following up. A few minor comments: On Tue, Sep 05, 2017 at 04:57:24PM -0400, Ross Kabus wrote: > From: Ross Kabus > 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
Re: [Bug] commit-tree shouldn't append an extra newline to commit messages
Jeff King 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 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
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 wrote: > From: Ross Kabus > 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
From: Ross Kabus 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
On Tue, Sep 05, 2017 at 12:57:21PM -0400, Ross Kabus wrote: > On Tue, Sep 5, 2017 at 11:36 AM, Jeff King 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
On Tue, Sep 5, 2017 at 11:36 AM, Jeff King 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 > ... > 0090: 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-hzw7...@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
On Tue, Sep 05, 2017 at 11:09:01AM -0400, Ross Kabus wrote: > On Sat, Sep 2, 2017 at 4:33 AM, Jeff King 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 ... 0090: 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 ... 0090: 3034 3030 0a0a 656e 64 0400..end -Peff
Re: [Bug] commit-tree shouldn't append an extra newline to commit messages
On Sat, Sep 2, 2017 at 4:33 AM, Jeff King 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
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