git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] interpret-trailers: load default config

From
Masahiro Yamada <yamada.masahiro@socionext.com>
Date
Jun 19, 2019, 15:52 UTC
Message-ID
<CAK7LNAQW9ObWv5WiiRk9dxrxoQgU=LoEP9FrTzmOTO5XF89aVA@mail.gmail.com>
In-Reply-To
<20190619033728.GA29027@sigill.intra.peff.net>
On Wed, Jun 19, 2019 at 12:37 PM Jeff King <peff@peff.net> wrote:
Show 55 quoted lines
>
> On Sat, Jun 15, 2019 at 10:41:44AM +0200, Christian Couder wrote:
>
> > On Fri, Jun 14, 2019 at 5:10 PM Jeff King <peff@peff.net> wrote:
> > >
> > > On Fri, Jun 14, 2019 at 08:35:04PM +0900, Masahiro Yamada wrote:
> > >
> > > > Perhaps, 'git interpret-trailers' should be changed
> > > > to recognize core.commentChar ?
> > >
> > > It looks like the trailer code does respect it, but the
> > > interpret-trailers program never loads the config. Does the patch below
> > > make your problem go away?
> >
> > It seems to me to be the right analysis and the right fix too.
>
> Thanks. Here it is (below) with a commit message and a test. I tried to
> build on the existing comment test, but the resulting diff is hard to
> read due to the indent change; try it with "-w".
>
> > > I do wonder if the trailer code is correct to always respect it, though.
> > > For example, in "git log" output we'd expect to see commit messages from
> > > people with all sorts of config. I suppose the point is that their
> > > comment characters wouldn't make it into the commit object at all, so
> > > the right answer there is probably not to look for comment characters at
> > > all.
> >
> > Would you suggest an option, maybe called `--ignore-comments` to ignore them?
>
> Yeah, though I think most callers of interpret-trailers would probably
> want the existing behavior. I'd be more concerned about the internal
> callers to the trailer code, like "git log --format=%(trailers)". I
> doubt it's that big a deal in practice, though. As I said above, the
> idea is that comments would be removed before making it into commit
> objects anyway. So we shouldn't be seeing comments, and so the code to
> recognize them is not likely to trigger (and I think it would be
> reasonably hard to trigger a false positive accidentally).
>
> If you or somebody else wants to dig into it, be my guest, but I don't
> think I'd prioritize it.
>
> -- >8 --
> Subject: [PATCH] interpret-trailers: load default config
>
> The interpret-trailers program does not do the usual loading of config
> via git_default_config(), and thus does not respect many of the usual
> options. In particular, we will not load core.commentChar, even though
> the underlying trailer code tries to do so.
>
> This can be seen in the accompanying test, where setting
> core.commentChar to anything besides "#" results in a failure to treat
> the comments correctly.
>
> Reported-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Jeff King <peff@peff.net>
Tested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Thanks.
-- 
Best Regards
Masahiro Yamada
Previous: Jeff KingNext: Masahiro Yamada
Message 12 of 13 in “'git interpret-trailers' is tripped by comment characters other than '#'”
  1. Masahiro YamadaJun 14, 2019
  2. Jeff KingJun 14, 2019
  3. Christian CouderJun 15, 2019
  4. Masahiro YamadaJun 17, 2019
  5. Christian CouderJun 17, 2019
  6. Junio C HamanoJun 17, 2019
  7. Christian CouderJun 17, 2019
  8. Masahiro YamadaJun 18, 2019
  9. interpret-trailers: load default configJeff King, Jun 19, 2019
  10. Junio C HamanoJun 19, 2019
  11. Jeff KingJun 19, 2019
  12. Masahiro YamadaJun 19, 2019
  13. Masahiro YamadaJun 17, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.