'git interpret-trailers' is tripped by comment characters other than '#'

'git interpret-trailers' is tripped by comment characters other than '#'

[Masahiro Yamada]

Hi.

When I tried to add ChangeId tag for Gerrit Code Review,
I noticed 'git interpret-trailers' went wrong
if a comment character other than '#' is used.


Quick Test Code
---------------

cat <<EOF | git -c trailer.ifexists=doNothing interpret-trailers \
      --trailer "Change-Id: new tag"
subject: this is commit subject

Blah Blah

Change-Id: old tag
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

; This is a comment line with non-default char

EOF

[result]

subject: this is commit subject

Blah Blah

Change-Id: old tag
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

; This is a comment line with non-default char

Change-Id: new tag



The same trailer 'Change-Id' was appended,
ignoring trailer.ifexists=doNothing



Details
-------

For Gerrit Code Review, we add "Change-Id" tag,
which will identify the patch though the review process.

For details, you can refer:
https://www.gerritcodereview.com/cmd-hook-commit-msg.html


Gerrit provides a shell script "commit-msg",
which is hooked from 'git commit'.

You can see its implementation here:
https://github.com/GerritCodeReview/gerrit/blob/v3.0.0/resources/com/google/gerrit/server/tools/root/hooks/commit-msg


It is implemented by using
'git interpret-trailers'.


I prefer ';' to '#" for commit comment lines.

So, I add the following:

[core]
        commentChar = ";"



With core.commentChar is set,
'git interpret-trailers' is so confused
that accumulate the same tag for every 'git commit --amend'.


I guess this should be fixed on the Git side.

Perhaps, 'git interpret-trailers' should be changed
to recognize core.commentChar ?



-- 
Best Regards
Masahiro Yamada

Re: 'git interpret-trailers' is tripped by comment characters other than '#'

[Jeff King]

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?

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 8ae40dec47..f101d092b8 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -10,6 +10,7 @@
 #include "parse-options.h"
 #include "string-list.h"
 #include "trailer.h"
+#include "config.h"
 
 static const char * const git_interpret_trailers_usage[] = {
 	N_("git interpret-trailers [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]"),
@@ -112,6 +113,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	git_config(git_default_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, options,
 			     git_interpret_trailers_usage, 0);
 

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.

-Peff

Re: 'git interpret-trailers' is tripped by comment characters other than '#'

[Christian Couder]

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.

> 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?

Thanks,
Christian.

Re: 'git interpret-trailers' is tripped by comment characters other than '#'

[Masahiro Yamada]

On Sat, Jun 15, 2019 at 12:08 AM 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?
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 8ae40dec47..f101d092b8 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -10,6 +10,7 @@
>  #include "parse-options.h"
>  #include "string-list.h"
>  #include "trailer.h"
> +#include "config.h"
>
>  static const char * const git_interpret_trailers_usage[] = {
>         N_("git interpret-trailers [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]"),
> @@ -112,6 +113,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
>                 OPT_END()
>         };
>
> +       git_config(git_default_config, NULL);
> +
>         argc = parse_options(argc, argv, prefix, options,
>                              git_interpret_trailers_usage, 0);
>

Yup, works for me.



> 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.

I see some sort of over-wrap between 'git stripspace --strip-comments'
and 'git interpret-trailers'

The 'commit-msg' hook could pass the message to 'git interpret-trailers'
after trimming all comment lines by using 'git stripspace --strip-comments'.

Anyway, users' hooks already rely on the behavior that
'git interpret-trailers' ignores lines starting with '#'.

So, respecting core.commentChar is the right fix, I believe.


BTW, why is commit-msg hook is passed with a message
without comments unstripped?
I think the straightforward behavior would be
to strip comments first, then invoke the commit hook.


Thanks.


-- 
Best Regards
Masahiro Yamada

Re: 'git interpret-trailers' is tripped by comment characters other than '#'

[Masahiro Yamada]

On Sat, Jun 15, 2019 at 5:41 PM Christian Couder
<christian.couder@gmail.com> 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.
>
> > 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?

Since 'git interpret-trailers' already ignores lines starting with '#',
is this option true by default?


> Thanks,
> Christian.



-- 
Best Regards
Masahiro Yamada

Re: 'git interpret-trailers' is tripped by comment characters other than '#'

[Christian Couder]

On Mon, Jun 17, 2019 at 6:33 AM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>
> On Sat, Jun 15, 2019 at 5:41 PM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> > > 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?
>
> Since 'git interpret-trailers' already ignores lines starting with '#',
> is this option true by default?

Sorry, I should have suggested something called --unstrip-comments or
--ignore-comment-char that would make 'git interpret-trailers' stop
stripping lines that start with the comment character.

Re: 'git interpret-trailers' is tripped by comment characters other than '#'

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> On Mon, Jun 17, 2019 at 6:33 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>>
>> On Sat, Jun 15, 2019 at 5:41 PM Christian Couder
>> <christian.couder@gmail.com> wrote:
>> >
>> > > 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?
>>
>> Since 'git interpret-trailers' already ignores lines starting with '#',
>> is this option true by default?
>
> Sorry, I should have suggested something called --unstrip-comments or
> --ignore-comment-char that would make 'git interpret-trailers' stop
> stripping lines that start with the comment character.

So, to summarize:

 - As the traditional behaviour is to strip comment, using the
   hardcoded definition of the comment char, i.e. '#', we do not
   switch the default.  Instead, a new command line option makes
   it pretend there is no comment char and nothing get stripped.

 - But the core.commentchar that does not override hardcoded
   definition is a bug, so we'd fix that along the lines of what
   Peff's patch outlined.

Anybody volunteering to do the honors?

Thanks.

Re: 'git interpret-trailers' is tripped by comment characters other than '#'

[Christian Couder]

On Mon, Jun 17, 2019 at 7:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > On Mon, Jun 17, 2019 at 6:33 AM Masahiro Yamada
> > <yamada.masahiro@socionext.com> wrote:
> >>
> >> On Sat, Jun 15, 2019 at 5:41 PM Christian Couder
> >> <christian.couder@gmail.com> wrote:
> >> >
> >> > > 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?
> >>
> >> Since 'git interpret-trailers' already ignores lines starting with '#',
> >> is this option true by default?
> >
> > Sorry, I should have suggested something called --unstrip-comments or
> > --ignore-comment-char that would make 'git interpret-trailers' stop
> > stripping lines that start with the comment character.
>
> So, to summarize:
>
>  - As the traditional behaviour is to strip comment, using the
>    hardcoded definition of the comment char, i.e. '#', we do not
>    switch the default.  Instead, a new command line option makes
>    it pretend there is no comment char and nothing get stripped.

Yeah, that's the idea of --unstrip-comments (or
--ignore-comment-char). I am ok with preparing and sending a patch to
add that, though it is not urgent and it would be nice if we could
agree with the name first.

>  - But the core.commentchar that does not override hardcoded
>    definition is a bug, so we'd fix that along the lines of what
>    Peff's patch outlined.

Yeah, not sure if Peff wants to resend his patch with a proper commit
message. I would be ok with doing it if he prefers.

Thanks,
Christian.

Re: 'git interpret-trailers' is tripped by comment characters other than '#'

[Masahiro Yamada]

On Tue, Jun 18, 2019 at 5:08 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Mon, Jun 17, 2019 at 7:31 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Christian Couder <christian.couder@gmail.com> writes:
> >
> > > On Mon, Jun 17, 2019 at 6:33 AM Masahiro Yamada
> > > <yamada.masahiro@socionext.com> wrote:
> > >>
> > >> On Sat, Jun 15, 2019 at 5:41 PM Christian Couder
> > >> <christian.couder@gmail.com> wrote:
> > >> >
> > >> > > 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?
> > >>
> > >> Since 'git interpret-trailers' already ignores lines starting with '#',
> > >> is this option true by default?
> > >
> > > Sorry, I should have suggested something called --unstrip-comments or
> > > --ignore-comment-char that would make 'git interpret-trailers' stop
> > > stripping lines that start with the comment character.
> >
> > So, to summarize:
> >
> >  - As the traditional behaviour is to strip comment, using the
> >    hardcoded definition of the comment char, i.e. '#', we do not
> >    switch the default.  Instead, a new command line option makes
> >    it pretend there is no comment char and nothing get stripped.
>
> Yeah, that's the idea of --unstrip-comments (or
> --ignore-comment-char). I am ok with preparing and sending a patch to
> add that, though it is not urgent and it would be nice if we could
> agree with the name first.
>
> >  - But the core.commentchar that does not override hardcoded
> >    definition is a bug, so we'd fix that along the lines of what
> >    Peff's patch outlined.
>
> Yeah, not sure if Peff wants to resend his patch with a proper commit
> message. I would be ok with doing it if he prefers.


Sounds good to me.

These are separate works.

Since the second one is a bug-fix, it can go in first.
(Peff's patch works for me)


The first one is a new feature, so we can take our time
to decide a preferred option name.




IMHO, --unstrip-* is a little bit confusing.

interpret-trailers does not strip anything from the output.

It is just like interpret-trailers does not take comment lines
into account when it determines the boundary between the commit
message body and trailers.


Just a idea:

--[no-]ignore-comments

   By default, comments (i.e. lines starting with '#' or a character
specified core.commentChar)
   are not taken into consideration when interpret-trailers determines which
   parts are trailers.
   Pass --no-ignore-comments if you want to treat all lines as the message body.
   --ignore-comments is the default.



Please feel free to put your ideas on the table!


-- 
Best Regards
Masahiro Yamada

[PATCH] interpret-trailers: load default config

[Jeff King]

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>
---
 builtin/interpret-trailers.c  |  3 ++
 t/t7513-interpret-trailers.sh | 70 +++++++++++++++++++++--------------
 2 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 8ae40dec47..f101d092b8 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -10,6 +10,7 @@
 #include "parse-options.h"
 #include "string-list.h"
 #include "trailer.h"
+#include "config.h"
 
 static const char * const git_interpret_trailers_usage[] = {
 	N_("git interpret-trailers [--in-place] [--trim-empty] [(--trailer <token>[(=|:)<value>])...] [<file>...]"),
@@ -112,6 +113,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	git_config(git_default_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, options,
 			     git_interpret_trailers_usage, 0);
 
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index c441861331..1da194e527 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -538,33 +538,49 @@ test_expect_success 'with 2 files arguments' '
 	test_cmp expected actual
 '
 
-test_expect_success 'with message that has comments' '
-	cat basic_message >message_with_comments &&
-	sed -e "s/ Z\$/ /" >>message_with_comments <<-\EOF &&
-		# comment
-
-		# other comment
-		Cc: Z
-		# yet another comment
-		Reviewed-by: Johan
-		Reviewed-by: Z
-		# last comment
-
-	EOF
-	cat basic_patch >>message_with_comments &&
-	cat basic_message >expected &&
-	cat >>expected <<-\EOF &&
-		# comment
-
-		Reviewed-by: Johan
-		Cc: Peff
-		# last comment
-
-	EOF
-	cat basic_patch >>expected &&
-	git interpret-trailers --trim-empty --trailer "Cc: Peff" message_with_comments >actual &&
-	test_cmp expected actual
-'
+# Cover multiple comment characters with the same test input.
+for char in "#" ";"; do
+	case "$char" in
+	"#")
+		# This is the default, so let's explicitly _not_
+		# set any config to make sure it behaves as we expect.
+		;;
+	*)
+		config="-c core.commentChar=$char"
+		;;
+	esac
+
+	test_expect_success "with message that has comments ($char)" '
+		cat basic_message >message_with_comments &&
+		sed -e "s/ Z\$/ /" \
+		    -e "s/#/$char/g" >>message_with_comments <<-EOF &&
+			# comment
+
+			# other comment
+			Cc: Z
+			# yet another comment
+			Reviewed-by: Johan
+			Reviewed-by: Z
+			# last comment
+
+		EOF
+		cat basic_patch >>message_with_comments &&
+		cat basic_message >expected &&
+		sed -e "s/#/$char/g" >>expected <<-\EOF &&
+			# comment
+
+			Reviewed-by: Johan
+			Cc: Peff
+			# last comment
+
+		EOF
+		cat basic_patch >>expected &&
+		git $config interpret-trailers \
+			--trim-empty --trailer "Cc: Peff" \
+			message_with_comments >actual &&
+		test_cmp expected actual
+	'
+done
 
 test_expect_success 'with message that has an old style conflict block' '
 	cat basic_message >message_with_comments &&
-- 
2.22.0.rc3.685.g5185838c9a

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

"tries to use it"?  Eh, it does use it, so, "the underlying trailer code
uses its value", would be the correct version of the last sentence.

The underlying trailer.c has two calls to git_config() to lazy-load
its own set of config variables (which is justified, as its caller
is not necessarily the "interpret-trailers" subcommand), but their
callbacks are not good places to call git_default_config() from for
obvious reasons.  It has to be done in "interpret-trailers" (and
other callers of the machinery should already have learned what
core.commentChar is with their own configuration calls) like the
patch I am reviewing does.

Looks perfect.  Thanks.

> 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>
> ---

>  builtin/interpret-trailers.c  |  3 ++
>  t/t7513-interpret-trailers.sh | 70 +++++++++++++++++++++--------------
>  2 files changed, 46 insertions(+), 27 deletions(-)

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

[Masahiro Yamada]

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

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

[Jeff King]

On Wed, Jun 19, 2019 at 07:24:10AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> "tries to use it"?  Eh, it does use it, so, "the underlying trailer code
> uses its value", would be the correct version of the last sentence.

Heh. Well, it does use the C variable, just not the one from the config.
:) But yeah, if you want to clarify the text while you apply, I'm fine
with that.

> The underlying trailer.c has two calls to git_config() to lazy-load
> its own set of config variables (which is justified, as its caller
> is not necessarily the "interpret-trailers" subcommand), but their
> callbacks are not good places to call git_default_config() from for
> obvious reasons.  It has to be done in "interpret-trailers" (and
> other callers of the machinery should already have learned what
> core.commentChar is with their own configuration calls) like the
> patch I am reviewing does.

Yeah, I didn't look too carefully into the loading that the trailer code
does for the reasons you gave. Thanks for laying it out clearly (and I
wouldn't mind if you stuck that in the commit message either).

-Peff