| Message ID | 20190619033728.GA29027@sigill.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | interpret-trailers: load default config | expand |
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(-)
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.
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
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 &&