{"thread":{"id":"51314","subject":"'git interpret-trailers' is tripped by comment characters other than '#'","startedAt":"2019-06-14T11:40:30Z","lastAt":"2019-06-19T17:48:03Z","messageCount":13,"participants":["Masahiro Yamada","Jeff King","Christian Couder","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"377232","messageId":"CAK7LNAR=KhTZRFFXTbcZwe-+65fGnVB7NbmVbQ7ymbmChDuhAg@mail.gmail.com","threadId":"51314","inReplyTo":null,"subject":"'git interpret-trailers' is tripped by comment characters other than '#'","fromName":"Masahiro Yamada","fromEmail":"yamada.masahiro@socionext.com","sentAt":"2019-06-14T11:35:04Z","receivedAt":"2019-06-14T11:40:30Z","isPatch":false,"sender":{"key":"yamada.masahiro@socionext.com","avatar":"https://gravatar.com/avatar/c0898c739a908a662f8caf90c3f0f0b420df8c392b7ef0948c13746b474d9f9e?d=mp&s=160"},"body":"Hi.\n\nWhen I tried to add ChangeId tag for Gerrit Code Review,\nI noticed 'git interpret-trailers' went wrong\nif a comment character other than '#' is used.\n\n\nQuick Test Code\n---------------\n\ncat <\n\n; This is a comment line with non-default char\n\nEOF\n\n[result]\n\nsubject: this is commit subject\n\nBlah Blah\n\nChange-Id: old tag\nSigned-off-by: Masahiro Yamada \n\n; This is a comment line with non-default char\n\nChange-Id: new tag\n\n\n\nThe same trailer 'Change-Id' was appended,\nignoring trailer.ifexists=doNothing\n\n\n\nDetails\n-------\n\nFor Gerrit Code Review, we add \"Change-Id\" tag,\nwhich will identify the patch though the review process.\n\nFor details, you can refer:\nhttps://www.gerritcodereview.com/cmd-hook-commit-msg.html\n\n\nGerrit provides a shell script \"commit-msg\",\nwhich is hooked from 'git commit'.\n\nYou can see its implementation here:\nhttps://github.com/GerritCodeReview/gerrit/blob/v3.0.0/resources/com/google/gerrit/server/tools/root/hooks/commit-msg\n\n\nIt is implemented by using\n'git interpret-trailers'.\n\n\nI prefer ';' to '#\" for commit comment lines.\n\nSo, I add the following:\n\n[core]\n commentChar = \";\"\n\n\n\nWith core.commentChar is set,\n'git interpret-trailers' is so confused\nthat accumulate the same tag for every 'git commit --amend'.\n\n\nI guess this should be fixed on the Git side.\n\nPerhaps, 'git interpret-trailers' should be changed\nto recognize core.commentChar ?\n\n\n\n-- \nBest Regards\nMasahiro Yamada\n"},{"id":"377236","messageId":"20190614150758.GA22984@sigill.intra.peff.net","threadId":"51314","inReplyTo":"CAK7LNAR=KhTZRFFXTbcZwe-+65fGnVB7NbmVbQ7ymbmChDuhAg@mail.gmail.com","subject":"Re: 'git interpret-trailers' is tripped by comment characters other than '#'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-14T15:07:58Z","receivedAt":"2019-06-14T15:08:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 14, 2019 at 08:35:04PM +0900, Masahiro Yamada wrote:\n\n> Perhaps, 'git interpret-trailers' should be changed\n> to recognize core.commentChar ?\n\nIt looks like the trailer code does respect it, but the\ninterpret-trailers program never loads the config. Does the patch below\nmake your problem go away?\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 8ae40dec47..f101d092b8 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -10,6 +10,7 @@\n #include \"parse-options.h\"\n #include \"string-list.h\"\n #include \"trailer.h\"\n+#include \"config.h\"\n \n static const char * const git_interpret_trailers_usage[] = {\n \tN_(\"git interpret-trailers [--in-place] [--trim-empty] [(--trailer [(=|:)])...] [...]\"),\n@@ -112,6 +113,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tgit_config(git_default_config, NULL);\n+\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_interpret_trailers_usage, 0);\n \n\nI do wonder if the trailer code is correct to always respect it, though.\nFor example, in \"git log\" output we'd expect to see commit messages from\npeople with all sorts of config. I suppose the point is that their\ncomment characters wouldn't make it into the commit object at all, so\nthe right answer there is probably not to look for comment characters at\nall.\n\n-Peff\n"},{"id":"377293","messageId":"CAP8UFD2dhGqOxXJMTZhNSM5G4sp6PvKF+0R5KVk6YjAQi3Sccw@mail.gmail.com","threadId":"51314","inReplyTo":"20190614150758.GA22984@sigill.intra.peff.net","subject":"Re: 'git interpret-trailers' is tripped by comment characters other than '#'","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-15T08:41:44Z","receivedAt":"2019-06-15T08:41:59Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 14, 2019 at 5:10 PM Jeff King wrote:\n>\n> On Fri, Jun 14, 2019 at 08:35:04PM +0900, Masahiro Yamada wrote:\n>\n> > Perhaps, 'git interpret-trailers' should be changed\n> > to recognize core.commentChar ?\n>\n> It looks like the trailer code does respect it, but the\n> interpret-trailers program never loads the config. Does the patch below\n> make your problem go away?\n\nIt seems to me to be the right analysis and the right fix too.\n\n> I do wonder if the trailer code is correct to always respect it, though.\n> For example, in \"git log\" output we'd expect to see commit messages from\n> people with all sorts of config. I suppose the point is that their\n> comment characters wouldn't make it into the commit object at all, so\n> the right answer there is probably not to look for comment characters at\n> all.\n\nWould you suggest an option, maybe called `--ignore-comments` to ignore them?\n\nThanks,\nChristian.\n"},{"id":"377343","messageId":"CAK7LNATRQWz9CvosEDzNHCZHaxpzozAjGyo5VSKpQhui9zhSFQ@mail.gmail.com","threadId":"51314","inReplyTo":"CAP8UFD2dhGqOxXJMTZhNSM5G4sp6PvKF+0R5KVk6YjAQi3Sccw@mail.gmail.com","subject":"Re: 'git interpret-trailers' is tripped by comment characters other than '#'","fromName":"Masahiro Yamada","fromEmail":"yamada.masahiro@socionext.com","sentAt":"2019-06-17T04:32:59Z","receivedAt":"2019-06-17T04:33:50Z","isPatch":false,"sender":{"key":"yamada.masahiro@socionext.com","avatar":"https://gravatar.com/avatar/c0898c739a908a662f8caf90c3f0f0b420df8c392b7ef0948c13746b474d9f9e?d=mp&s=160"},"body":"On Sat, Jun 15, 2019 at 5:41 PM Christian Couder\n wrote:\n>\n> On Fri, Jun 14, 2019 at 5:10 PM Jeff King wrote:\n> >\n> > On Fri, Jun 14, 2019 at 08:35:04PM +0900, Masahiro Yamada wrote:\n> >\n> > > Perhaps, 'git interpret-trailers' should be changed\n> > > to recognize core.commentChar ?\n> >\n> > It looks like the trailer code does respect it, but the\n> > interpret-trailers program never loads the config. Does the patch below\n> > make your problem go away?\n>\n> It seems to me to be the right analysis and the right fix too.\n>\n> > I do wonder if the trailer code is correct to always respect it, though.\n> > For example, in \"git log\" output we'd expect to see commit messages from\n> > people with all sorts of config. I suppose the point is that their\n> > comment characters wouldn't make it into the commit object at all, so\n> > the right answer there is probably not to look for comment characters at\n> > all.\n>\n> Would you suggest an option, maybe called `--ignore-comments` to ignore them?\n\nSince 'git interpret-trailers' already ignores lines starting with '#',\nis this option true by default?\n\n\n> Thanks,\n> Christian.\n\n\n\n-- \nBest Regards\nMasahiro Yamada\n"},{"id":"377344","messageId":"CAK7LNASueRR6narhHqe_D93ewXhTPXokWSCvQRh+Q6=-_AB+3g@mail.gmail.com","threadId":"51314","inReplyTo":"20190614150758.GA22984@sigill.intra.peff.net","subject":"Re: 'git interpret-trailers' is tripped by comment characters other than '#'","fromName":"Masahiro Yamada","fromEmail":"yamada.masahiro@socionext.com","sentAt":"2019-06-17T04:32:51Z","receivedAt":"2019-06-17T04:39:49Z","isPatch":false,"sender":{"key":"yamada.masahiro@socionext.com","avatar":"https://gravatar.com/avatar/c0898c739a908a662f8caf90c3f0f0b420df8c392b7ef0948c13746b474d9f9e?d=mp&s=160"},"body":"On Sat, Jun 15, 2019 at 12:08 AM Jeff King wrote:\n>\n> On Fri, Jun 14, 2019 at 08:35:04PM +0900, Masahiro Yamada wrote:\n>\n> > Perhaps, 'git interpret-trailers' should be changed\n> > to recognize core.commentChar ?\n>\n> It looks like the trailer code does respect it, but the\n> interpret-trailers program never loads the config. Does the patch below\n> make your problem go away?\n>\n> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\n> index 8ae40dec47..f101d092b8 100644\n> --- a/builtin/interpret-trailers.c\n> +++ b/builtin/interpret-trailers.c\n> @@ -10,6 +10,7 @@\n> #include \"parse-options.h\"\n> #include \"string-list.h\"\n> #include \"trailer.h\"\n> +#include \"config.h\"\n>\n> static const char * const git_interpret_trailers_usage[] = {\n> N_(\"git interpret-trailers [--in-place] [--trim-empty] [(--trailer [(=|:)])...] [...]\"),\n> @@ -112,6 +113,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n> OPT_END()\n> };\n>\n> + git_config(git_default_config, NULL);\n> +\n> argc = parse_options(argc, argv, prefix, options,\n> git_interpret_trailers_usage, 0);\n>\n\nYup, works for me.\n\n\n\n> I do wonder if the trailer code is correct to always respect it, though.\n> For example, in \"git log\" output we'd expect to see commit messages from\n> people with all sorts of config. I suppose the point is that their\n> comment characters wouldn't make it into the commit object at all, so\n> the right answer there is probably not to look for comment characters at\n> all.\n\nI see some sort of over-wrap between 'git stripspace --strip-comments'\nand 'git interpret-trailers'\n\nThe 'commit-msg' hook could pass the message to 'git interpret-trailers'\nafter trimming all comment lines by using 'git stripspace --strip-comments'.\n\nAnyway, users' hooks already rely on the behavior that\n'git interpret-trailers' ignores lines starting with '#'.\n\nSo, respecting core.commentChar is the right fix, I believe.\n\n\nBTW, why is commit-msg hook is passed with a message\nwithout comments unstripped?\nI think the straightforward behavior would be\nto strip comments first, then invoke the commit hook.\n\n\nThanks.\n\n\n-- \nBest Regards\nMasahiro Yamada\n"},{"id":"377345","messageId":"CAP8UFD3_kKvBs=rMvBp-K-UPD5mCqVsHRZ1VqGYY7uR8G+H8SQ@mail.gmail.com","threadId":"51314","inReplyTo":"CAK7LNATRQWz9CvosEDzNHCZHaxpzozAjGyo5VSKpQhui9zhSFQ@mail.gmail.com","subject":"Re: 'git interpret-trailers' is tripped by comment characters other than '#'","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-17T05:03:29Z","receivedAt":"2019-06-17T05:03:44Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 17, 2019 at 6:33 AM Masahiro Yamada\n wrote:\n>\n> On Sat, Jun 15, 2019 at 5:41 PM Christian Couder\n> wrote:\n> >\n> > > I do wonder if the trailer code is correct to always respect it, though.\n> > > For example, in \"git log\" output we'd expect to see commit messages from\n> > > people with all sorts of config. I suppose the point is that their\n> > > comment characters wouldn't make it into the commit object at all, so\n> > > the right answer there is probably not to look for comment characters at\n> > > all.\n> >\n> > Would you suggest an option, maybe called `--ignore-comments` to ignore them?\n>\n> Since 'git interpret-trailers' already ignores lines starting with '#',\n> is this option true by default?\n\nSorry, I should have suggested something called --unstrip-comments or\n--ignore-comment-char that would make 'git interpret-trailers' stop\nstripping lines that start with the comment character.\n"},{"id":"377358","messageId":"xmqqtvcoruda.fsf@gitster-ct.c.googlers.com","threadId":"51314","inReplyTo":"CAP8UFD3_kKvBs=rMvBp-K-UPD5mCqVsHRZ1VqGYY7uR8G+H8SQ@mail.gmail.com","subject":"Re: 'git interpret-trailers' is tripped by comment characters other than '#'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-17T17:31:45Z","receivedAt":"2019-06-17T17:31:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Mon, Jun 17, 2019 at 6:33 AM Masahiro Yamada\n> wrote:\n>>\n>> On Sat, Jun 15, 2019 at 5:41 PM Christian Couder\n>> wrote:\n>> >\n>> > > I do wonder if the trailer code is correct to always respect it, though.\n>> > > For example, in \"git log\" output we'd expect to see commit messages from\n>> > > people with all sorts of config. I suppose the point is that their\n>> > > comment characters wouldn't make it into the commit object at all, so\n>> > > the right answer there is probably not to look for comment characters at\n>> > > all.\n>> >\n>> > Would you suggest an option, maybe called `--ignore-comments` to ignore them?\n>>\n>> Since 'git interpret-trailers' already ignores lines starting with '#',\n>> is this option true by default?\n>\n> Sorry, I should have suggested something called --unstrip-comments or\n> --ignore-comment-char that would make 'git interpret-trailers' stop\n> stripping lines that start with the comment character.\n\nSo, to summarize:\n\n - As the traditional behaviour is to strip comment, using the\n hardcoded definition of the comment char, i.e. '#', we do not\n switch the default. Instead, a new command line option makes\n it pretend there is no comment char and nothing get stripped.\n\n - But the core.commentchar that does not override hardcoded\n definition is a bug, so we'd fix that along the lines of what\n Peff's patch outlined.\n\nAnybody volunteering to do the honors?\n\nThanks.\n\n"},{"id":"377371","messageId":"CAP8UFD3EaR=PRsN9sEEAOOdqQxPeZ1wiiWrVeBKQv8AW1zY1ig@mail.gmail.com","threadId":"51314","inReplyTo":"xmqqtvcoruda.fsf@gitster-ct.c.googlers.com","subject":"Re: 'git interpret-trailers' is tripped by comment characters other than '#'","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-17T20:07:50Z","receivedAt":"2019-06-17T20:08:05Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jun 17, 2019 at 7:31 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > On Mon, Jun 17, 2019 at 6:33 AM Masahiro Yamada\n> > wrote:\n> >>\n> >> On Sat, Jun 15, 2019 at 5:41 PM Christian Couder\n> >> wrote:\n> >> >\n> >> > > I do wonder if the trailer code is correct to always respect it, though.\n> >> > > For example, in \"git log\" output we'd expect to see commit messages from\n> >> > > people with all sorts of config. I suppose the point is that their\n> >> > > comment characters wouldn't make it into the commit object at all, so\n> >> > > the right answer there is probably not to look for comment characters at\n> >> > > all.\n> >> >\n> >> > Would you suggest an option, maybe called `--ignore-comments` to ignore them?\n> >>\n> >> Since 'git interpret-trailers' already ignores lines starting with '#',\n> >> is this option true by default?\n> >\n> > Sorry, I should have suggested something called --unstrip-comments or\n> > --ignore-comment-char that would make 'git interpret-trailers' stop\n> > stripping lines that start with the comment character.\n>\n> So, to summarize:\n>\n> - As the traditional behaviour is to strip comment, using the\n> hardcoded definition of the comment char, i.e. '#', we do not\n> switch the default. Instead, a new command line option makes\n> it pretend there is no comment char and nothing get stripped.\n\nYeah, that's the idea of --unstrip-comments (or\n--ignore-comment-char). I am ok with preparing and sending a patch to\nadd that, though it is not urgent and it would be nice if we could\nagree with the name first.\n\n> - But the core.commentchar that does not override hardcoded\n> definition is a bug, so we'd fix that along the lines of what\n> Peff's patch outlined.\n\nYeah, not sure if Peff wants to resend his patch with a proper commit\nmessage. I would be ok with doing it if he prefers.\n\nThanks,\nChristian.\n"},{"id":"377382","messageId":"CAK7LNARd7AeaBqFh3AgfbcjcgjGsGpK=0xK_bgpbe9FevA6qHA@mail.gmail.com","threadId":"51314","inReplyTo":"CAP8UFD3EaR=PRsN9sEEAOOdqQxPeZ1wiiWrVeBKQv8AW1zY1ig@mail.gmail.com","subject":"Re: 'git interpret-trailers' is tripped by comment characters other than '#'","fromName":"Masahiro Yamada","fromEmail":"yamada.masahiro@socionext.com","sentAt":"2019-06-18T02:41:19Z","receivedAt":"2019-06-18T02:42:09Z","isPatch":false,"sender":{"key":"yamada.masahiro@socionext.com","avatar":"https://gravatar.com/avatar/c0898c739a908a662f8caf90c3f0f0b420df8c392b7ef0948c13746b474d9f9e?d=mp&s=160"},"body":"On Tue, Jun 18, 2019 at 5:08 AM Christian Couder\n wrote:\n>\n> On Mon, Jun 17, 2019 at 7:31 PM Junio C Hamano wrote:\n> >\n> > Christian Couder writes:\n> >\n> > > On Mon, Jun 17, 2019 at 6:33 AM Masahiro Yamada\n> > > wrote:\n> > >>\n> > >> On Sat, Jun 15, 2019 at 5:41 PM Christian Couder\n> > >> wrote:\n> > >> >\n> > >> > > I do wonder if the trailer code is correct to always respect it, though.\n> > >> > > For example, in \"git log\" output we'd expect to see commit messages from\n> > >> > > people with all sorts of config. I suppose the point is that their\n> > >> > > comment characters wouldn't make it into the commit object at all, so\n> > >> > > the right answer there is probably not to look for comment characters at\n> > >> > > all.\n> > >> >\n> > >> > Would you suggest an option, maybe called `--ignore-comments` to ignore them?\n> > >>\n> > >> Since 'git interpret-trailers' already ignores lines starting with '#',\n> > >> is this option true by default?\n> > >\n> > > Sorry, I should have suggested something called --unstrip-comments or\n> > > --ignore-comment-char that would make 'git interpret-trailers' stop\n> > > stripping lines that start with the comment character.\n> >\n> > So, to summarize:\n> >\n> > - As the traditional behaviour is to strip comment, using the\n> > hardcoded definition of the comment char, i.e. '#', we do not\n> > switch the default. Instead, a new command line option makes\n> > it pretend there is no comment char and nothing get stripped.\n>\n> Yeah, that's the idea of --unstrip-comments (or\n> --ignore-comment-char). I am ok with preparing and sending a patch to\n> add that, though it is not urgent and it would be nice if we could\n> agree with the name first.\n>\n> > - But the core.commentchar that does not override hardcoded\n> > definition is a bug, so we'd fix that along the lines of what\n> > Peff's patch outlined.\n>\n> Yeah, not sure if Peff wants to resend his patch with a proper commit\n> message. I would be ok with doing it if he prefers.\n\n\nSounds good to me.\n\nThese are separate works.\n\nSince the second one is a bug-fix, it can go in first.\n(Peff's patch works for me)\n\n\nThe first one is a new feature, so we can take our time\nto decide a preferred option name.\n\n\n\n\nIMHO, --unstrip-* is a little bit confusing.\n\ninterpret-trailers does not strip anything from the output.\n\nIt is just like interpret-trailers does not take comment lines\ninto account when it determines the boundary between the commit\nmessage body and trailers.\n\n\nJust a idea:\n\n--[no-]ignore-comments\n\n By default, comments (i.e. lines starting with '#' or a character\nspecified core.commentChar)\n are not taken into consideration when interpret-trailers determines which\n parts are trailers.\n Pass --no-ignore-comments if you want to treat all lines as the message body.\n --ignore-comments is the default.\n\n\n\nPlease feel free to put your ideas on the table!\n\n\n-- \nBest Regards\nMasahiro Yamada\n"},{"id":"377480","messageId":"20190619033728.GA29027@sigill.intra.peff.net","threadId":"51314","inReplyTo":"CAP8UFD2dhGqOxXJMTZhNSM5G4sp6PvKF+0R5KVk6YjAQi3Sccw@mail.gmail.com","subject":"[PATCH] interpret-trailers: load default config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-19T03:37:28Z","receivedAt":"2019-06-19T03:37:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jun 15, 2019 at 10:41:44AM +0200, Christian Couder wrote:\n\n> On Fri, Jun 14, 2019 at 5:10 PM Jeff King wrote:\n> >\n> > On Fri, Jun 14, 2019 at 08:35:04PM +0900, Masahiro Yamada wrote:\n> >\n> > > Perhaps, 'git interpret-trailers' should be changed\n> > > to recognize core.commentChar ?\n> >\n> > It looks like the trailer code does respect it, but the\n> > interpret-trailers program never loads the config. Does the patch below\n> > make your problem go away?\n> \n> It seems to me to be the right analysis and the right fix too.\n\nThanks. Here it is (below) with a commit message and a test. I tried to\nbuild on the existing comment test, but the resulting diff is hard to\nread due to the indent change; try it with \"-w\".\n\n> > I do wonder if the trailer code is correct to always respect it, though.\n> > For example, in \"git log\" output we'd expect to see commit messages from\n> > people with all sorts of config. I suppose the point is that their\n> > comment characters wouldn't make it into the commit object at all, so\n> > the right answer there is probably not to look for comment characters at\n> > all.\n> \n> Would you suggest an option, maybe called `--ignore-comments` to ignore them?\n\nYeah, though I think most callers of interpret-trailers would probably\nwant the existing behavior. I'd be more concerned about the internal\ncallers to the trailer code, like \"git log --format=%(trailers)\". I\ndoubt it's that big a deal in practice, though. As I said above, the\nidea is that comments would be removed before making it into commit\nobjects anyway. So we shouldn't be seeing comments, and so the code to\nrecognize them is not likely to trigger (and I think it would be\nreasonably hard to trigger a false positive accidentally).\n\nIf you or somebody else wants to dig into it, be my guest, but I don't\nthink I'd prioritize it.\n\n-- >8 --\nSubject: [PATCH] interpret-trailers: load default config\n\nThe interpret-trailers program does not do the usual loading of config\nvia git_default_config(), and thus does not respect many of the usual\noptions. In particular, we will not load core.commentChar, even though\nthe underlying trailer code tries to do so.\n\nThis can be seen in the accompanying test, where setting\ncore.commentChar to anything besides \"#\" results in a failure to treat\nthe comments correctly.\n\nReported-by: Masahiro Yamada \nSigned-off-by: Jeff King \n---\n builtin/interpret-trailers.c | 3 ++\n t/t7513-interpret-trailers.sh | 70 +++++++++++++++++++++--------------\n 2 files changed, 46 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c\nindex 8ae40dec47..f101d092b8 100644\n--- a/builtin/interpret-trailers.c\n+++ b/builtin/interpret-trailers.c\n@@ -10,6 +10,7 @@\n #include \"parse-options.h\"\n #include \"string-list.h\"\n #include \"trailer.h\"\n+#include \"config.h\"\n \n static const char * const git_interpret_trailers_usage[] = {\n \tN_(\"git interpret-trailers [--in-place] [--trim-empty] [(--trailer [(=|:)])...] [...]\"),\n@@ -112,6 +113,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tgit_config(git_default_config, NULL);\n+\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t git_interpret_trailers_usage, 0);\n \ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex c441861331..1da194e527 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -538,33 +538,49 @@ test_expect_success 'with 2 files arguments' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'with message that has comments' '\n-\tcat basic_message >message_with_comments &&\n-\tsed -e \"s/ Z\\$/ /\" >>message_with_comments <<-\\EOF &&\n-\t\t# comment\n-\n-\t\t# other comment\n-\t\tCc: Z\n-\t\t# yet another comment\n-\t\tReviewed-by: Johan\n-\t\tReviewed-by: Z\n-\t\t# last comment\n-\n-\tEOF\n-\tcat basic_patch >>message_with_comments &&\n-\tcat basic_message >expected &&\n-\tcat >>expected <<-\\EOF &&\n-\t\t# comment\n-\n-\t\tReviewed-by: Johan\n-\t\tCc: Peff\n-\t\t# last comment\n-\n-\tEOF\n-\tcat basic_patch >>expected &&\n-\tgit interpret-trailers --trim-empty --trailer \"Cc: Peff\" message_with_comments >actual &&\n-\ttest_cmp expected actual\n-'\n+# Cover multiple comment characters with the same test input.\n+for char in \"#\" \";\"; do\n+\tcase \"$char\" in\n+\t\"#\")\n+\t\t# This is the default, so let's explicitly _not_\n+\t\t# set any config to make sure it behaves as we expect.\n+\t\t;;\n+\t*)\n+\t\tconfig=\"-c core.commentChar=$char\"\n+\t\t;;\n+\tesac\n+\n+\ttest_expect_success \"with message that has comments ($char)\" '\n+\t\tcat basic_message >message_with_comments &&\n+\t\tsed -e \"s/ Z\\$/ /\" \\\n+\t\t -e \"s/#/$char/g\" >>message_with_comments <<-EOF &&\n+\t\t\t# comment\n+\n+\t\t\t# other comment\n+\t\t\tCc: Z\n+\t\t\t# yet another comment\n+\t\t\tReviewed-by: Johan\n+\t\t\tReviewed-by: Z\n+\t\t\t# last comment\n+\n+\t\tEOF\n+\t\tcat basic_patch >>message_with_comments &&\n+\t\tcat basic_message >expected &&\n+\t\tsed -e \"s/#/$char/g\" >>expected <<-\\EOF &&\n+\t\t\t# comment\n+\n+\t\t\tReviewed-by: Johan\n+\t\t\tCc: Peff\n+\t\t\t# last comment\n+\n+\t\tEOF\n+\t\tcat basic_patch >>expected &&\n+\t\tgit $config interpret-trailers \\\n+\t\t\t--trim-empty --trailer \"Cc: Peff\" \\\n+\t\t\tmessage_with_comments >actual &&\n+\t\ttest_cmp expected actual\n+\t'\n+done\n \n test_expect_success 'with message that has an old style conflict block' '\n \tcat basic_message >message_with_comments &&\n-- \n2.22.0.rc3.685.g5185838c9a\n\n"},{"id":"377521","messageId":"xmqqwohhr6ut.fsf@gitster-ct.c.googlers.com","threadId":"51314","inReplyTo":"20190619033728.GA29027@sigill.intra.peff.net","subject":"Re: [PATCH] interpret-trailers: load default config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-19T14:24:10Z","receivedAt":"2019-06-19T14:24:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Subject: [PATCH] interpret-trailers: load default config\n>\n> The interpret-trailers program does not do the usual loading of config\n> via git_default_config(), and thus does not respect many of the usual\n> options. In particular, we will not load core.commentChar, even though\n> the underlying trailer code tries to do so.\n\n\"tries to use it\"? Eh, it does use it, so, \"the underlying trailer code\nuses its value\", would be the correct version of the last sentence.\n\nThe underlying trailer.c has two calls to git_config() to lazy-load\nits own set of config variables (which is justified, as its caller\nis not necessarily the \"interpret-trailers\" subcommand), but their\ncallbacks are not good places to call git_default_config() from for\nobvious reasons. It has to be done in \"interpret-trailers\" (and\nother callers of the machinery should already have learned what\ncore.commentChar is with their own configuration calls) like the\npatch I am reviewing does.\n\nLooks perfect. Thanks.\n\n> This can be seen in the accompanying test, where setting\n> core.commentChar to anything besides \"#\" results in a failure to treat\n> the comments correctly.\n>\n> Reported-by: Masahiro Yamada \n> Signed-off-by: Jeff King \n> ---\n\n> builtin/interpret-trailers.c | 3 ++\n> t/t7513-interpret-trailers.sh | 70 +++++++++++++++++++++--------------\n> 2 files changed, 46 insertions(+), 27 deletions(-)\n\n"},{"id":"377538","messageId":"CAK7LNAQW9ObWv5WiiRk9dxrxoQgU=LoEP9FrTzmOTO5XF89aVA@mail.gmail.com","threadId":"51314","inReplyTo":"20190619033728.GA29027@sigill.intra.peff.net","subject":"Re: [PATCH] interpret-trailers: load default config","fromName":"Masahiro Yamada","fromEmail":"yamada.masahiro@socionext.com","sentAt":"2019-06-19T15:52:09Z","receivedAt":"2019-06-19T15:52:58Z","isPatch":true,"sender":{"key":"yamada.masahiro@socionext.com","avatar":"https://gravatar.com/avatar/c0898c739a908a662f8caf90c3f0f0b420df8c392b7ef0948c13746b474d9f9e?d=mp&s=160"},"body":"On Wed, Jun 19, 2019 at 12:37 PM Jeff King wrote:\n>\n> On Sat, Jun 15, 2019 at 10:41:44AM +0200, Christian Couder wrote:\n>\n> > On Fri, Jun 14, 2019 at 5:10 PM Jeff King wrote:\n> > >\n> > > On Fri, Jun 14, 2019 at 08:35:04PM +0900, Masahiro Yamada wrote:\n> > >\n> > > > Perhaps, 'git interpret-trailers' should be changed\n> > > > to recognize core.commentChar ?\n> > >\n> > > It looks like the trailer code does respect it, but the\n> > > interpret-trailers program never loads the config. Does the patch below\n> > > make your problem go away?\n> >\n> > It seems to me to be the right analysis and the right fix too.\n>\n> Thanks. Here it is (below) with a commit message and a test. I tried to\n> build on the existing comment test, but the resulting diff is hard to\n> read due to the indent change; try it with \"-w\".\n>\n> > > I do wonder if the trailer code is correct to always respect it, though.\n> > > For example, in \"git log\" output we'd expect to see commit messages from\n> > > people with all sorts of config. I suppose the point is that their\n> > > comment characters wouldn't make it into the commit object at all, so\n> > > the right answer there is probably not to look for comment characters at\n> > > all.\n> >\n> > Would you suggest an option, maybe called `--ignore-comments` to ignore them?\n>\n> Yeah, though I think most callers of interpret-trailers would probably\n> want the existing behavior. I'd be more concerned about the internal\n> callers to the trailer code, like \"git log --format=%(trailers)\". I\n> doubt it's that big a deal in practice, though. As I said above, the\n> idea is that comments would be removed before making it into commit\n> objects anyway. So we shouldn't be seeing comments, and so the code to\n> recognize them is not likely to trigger (and I think it would be\n> reasonably hard to trigger a false positive accidentally).\n>\n> If you or somebody else wants to dig into it, be my guest, but I don't\n> think I'd prioritize it.\n>\n> -- >8 --\n> Subject: [PATCH] interpret-trailers: load default config\n>\n> The interpret-trailers program does not do the usual loading of config\n> via git_default_config(), and thus does not respect many of the usual\n> options. In particular, we will not load core.commentChar, even though\n> the underlying trailer code tries to do so.\n>\n> This can be seen in the accompanying test, where setting\n> core.commentChar to anything besides \"#\" results in a failure to treat\n> the comments correctly.\n>\n> Reported-by: Masahiro Yamada \n> Signed-off-by: Jeff King \n\nTested-by: Masahiro Yamada \n\nThanks.\n\n-- \nBest Regards\nMasahiro Yamada\n"},{"id":"377543","messageId":"20190619174752.GA27834@sigill.intra.peff.net","threadId":"51314","inReplyTo":"xmqqwohhr6ut.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] interpret-trailers: load default config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-19T17:47:52Z","receivedAt":"2019-06-19T17:48:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 19, 2019 at 07:24:10AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Subject: [PATCH] interpret-trailers: load default config\n> >\n> > The interpret-trailers program does not do the usual loading of config\n> > via git_default_config(), and thus does not respect many of the usual\n> > options. In particular, we will not load core.commentChar, even though\n> > the underlying trailer code tries to do so.\n> \n> \"tries to use it\"? Eh, it does use it, so, \"the underlying trailer code\n> uses its value\", would be the correct version of the last sentence.\n\nHeh. Well, it does use the C variable, just not the one from the config.\n:) But yeah, if you want to clarify the text while you apply, I'm fine\nwith that.\n\n> The underlying trailer.c has two calls to git_config() to lazy-load\n> its own set of config variables (which is justified, as its caller\n> is not necessarily the \"interpret-trailers\" subcommand), but their\n> callbacks are not good places to call git_default_config() from for\n> obvious reasons. It has to be done in \"interpret-trailers\" (and\n> other callers of the machinery should already have learned what\n> core.commentChar is with their own configuration calls) like the\n> patch I am reviewing does.\n\nYeah, I didn't look too carefully into the loading that the trailer code\ndoes for the reasons you gave. Thanks for laying it out clearly (and I\nwouldn't mind if you stuck that in the commit message either).\n\n-Peff\n"}]}