interpret-trailers: load default config
diff mbox series

Message ID 20190619033728.GA29027@sigill.intra.peff.net
State New, archived
Headers show
Series
  • interpret-trailers: load default config
Related show

Commit Message

Jeff King June 19, 2019, 3:37 a.m. UTC
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(-)

Comments

Junio C Hamano June 19, 2019, 2:24 p.m. UTC | #1
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(-)
Masahiro Yamada June 19, 2019, 3:52 p.m. UTC | #2
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.
Jeff King June 19, 2019, 5:47 p.m. UTC | #3
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

Patch
diff mbox series

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