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

[Masahiro Yamada <yamada.masahiro@socionext.com>]

On Wed, Jun 19, 2019 at 12:37 PM Jeff King <peff@peff.net> wrote:
>
> 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