{"thread":{"id":"53929","subject":"Questions about trailer configuration semantics","startedAt":"2020-07-27T17:03:45Z","lastAt":"2020-07-28T16:40:49Z","messageCount":16,"participants":["Anders Waldenborg","Junio C Hamano","Christian Couder","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"402145","messageId":"87blk0rjob.fsf@0x63.nu","threadId":"53929","inReplyTo":null,"subject":"Questions about trailer configuration semantics","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-07-27T16:45:24Z","receivedAt":"2020-07-27T17:03:45Z","isPatch":false,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nI noticed some undocumented and (at least to me) surprising behavior in\ntrailers.c.\n\nWhen configuring a value in trailer..key it causes the trailer to\nbe normalized to that in \"git interpret-trailers --parse\".\nE.g:\n $ printf '\\naCKed: Zz\\n' | \\\n git -c 'trailer.Acked.key=Acked' interpret-trailers --parse\n will emit: \"Acked: Zz\"\n\nbut only if \"key\" is used, other config options doesn't cause it to be\nnormalized.\nE.g:\n $ printf '\\naCKed: Zz\\n' | \\\n git -c 'trailer.Acked.ifmissing=doNothing' interpret-trailers --parse\n will emit: \"aCKed: Zz\" (still lowercase a and uppercase CK)\n\n\nThen there is the replacement by config \"trailer.fix.key=Fixes\" which\nexpands \"fix\" to \"Fixes\". This happens when using \"--trailer 'fix = 123'\"\nwhich seems to be expected and useful behavior (albeit a bit unclear in\ndocumentation). But it also happens when parsing incoming trailers, e.g\nwith that config\n $ printf \"\\nFix: 1\\n\" | git interpret-trailers --parse\n will emit: \"Fixes: 1\"\n\n(token_from_item prefers order .key, incoming token, .name)\n\n\nThe most surprising thing is that it uses prefix matching when finding\nthey key in configuration. If I have \"trailer.reviewed.key=Reviewed-By\"\nit is possible to just '--trailer r=XYZ' and it will find the\nreviewed-by trailer as \"r\" is a prefix of reviewedby. This also applies\nto the \"--parse\". This in makes it impossible to have trailer keys that\nare prefix of each other (e.g: \"Acked\", \"Acked-Tests\", \"Acked-Docs\") if\nthere is multiple matching in configuration it will just pick the one\nthat happens to come first.\n\n(token_matches_item uses strncasecmp with token length)\n\n\nI guess these are the questions for the above observations:\n\n* Should normalization of spelling happen at all?\n\n* If so should it only happen when there is a .key config?\n\n* Should replacement to what is in .key happen also in --parse mode, or\n only for \"--trailer\"\n\n* The prefix matching gotta be a bug, right?\n\n\n\nHere is a patch to the tests showing these things.\n\n\n\nFrom 49a4bb64a7ebf1f2d50897a024deb86b4f8056b1 Mon Sep 17 00:00:00 2001\nFrom: Anders Waldenborg \nDate: Mon, 27 Jul 2020 18:34:37 +0200\nSubject: [PATCH] trailers: add tests for unclear/undocumented behavior\n\n---\n t/t7513-interpret-trailers.sh | 70 +++++++++++++++++++++++++++++++++++\n 1 file changed, 70 insertions(+)\n\ndiff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\nindex 2e6d406edf..d5d19cf89b 100755\n--- a/t/t7513-interpret-trailers.sh\n+++ b/t/t7513-interpret-trailers.sh\n@@ -99,6 +99,64 @@ test_expect_success 'with config option on the command line' '\n \ttest_cmp expected actual\n '\n\n+test_expect_success 'parse normalizes spelling and separators from configs with key' '\n+\tcat >patch <<-\\EOF &&\n+\t\tnon-trailer-line\n+\n+\t\tReviEweD-bY :abc\n+\t\tReviEwEd-bY) rst\n+\t\tReviEweD-BY ; xyz\n+\t\taCked-bY: not normalized\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\t\tReviewed-By: abc\n+\t\tReviewed-By: rst\n+\t\tReviewed-By: xyz\n+\t\taCked-bY: not normalized\n+\tEOF\n+\tgit \\\n+\t\t-c \"trailer.separators=:);\" \\\n+\t\t-c \"trailer.rb.key=Reviewed-By\" \\\n+\t\t-c \"trailer.Acked-By.ifmissing=doNothing\" \\\n+\t\tinterpret-trailers --parse patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# Matching currently is prefix matching, causing \"This-trailer\" to be normalized too\n+test_expect_failure 'config option matches exact only' '\n+\tcat >patch <<-\\EOF &&\n+\n+\t\tThis-trailer: a\n+\t\t b\n+\t\tThis-trailer-exact: b\n+\t\t c\n+\t\tThis-trailer-exact-plus-some: c\n+\t\t d\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\t\tThis-trailer: a b\n+\t\tTHIS-TRAILER-EXACT: b c\n+\t\tThis-trailer-exact-plus-some: c d\n+\tEOF\n+\tgit -c \"trailer.tte.key=THIS-TRAILER-EXACT\" interpret-trailers --parse patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# Matching currently uses the config key even if key value is different\n+test_expect_failure 'config option matches exact only' '\n+\tcat >patch <<-\\EOF &&\n+\n+\t\tTicket: 1234\n+\t\tReference-ticket: 99\n+\tEOF\n+\tcat >expected <<-\\EOF &&\n+\t\tTicket: 1234\n+\t\tReference-Ticket: 99\n+\tEOF\n+\tgit -c \"trailer.ticket.key=Reference-Ticket\" interpret-trailers --parse patch >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'with only a title in the message' '\n \tcat >expected <<-\\EOF &&\n \t\tarea: change\n@@ -473,6 +531,18 @@ test_expect_success 'with config setup' '\n \ttest_cmp expected actual\n '\n\n+# currently this matches the \"Acked-by: \" value in ack key set by previous test\n+test_expect_failure 'with config setup matches key exactly' '\n+\tcat >expected <<-\\EOF &&\n+\n+\t\tA: B\n+\tEOF\n+\tgit interpret-trailers --trailer \"A=10\" empty >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+\n+\n test_expect_success 'with config setup and \":=\" as separators' '\n \tgit config trailer.separators \":=\" &&\n \tgit config trailer.ack.key \"Acked-by= \" &&\n--\n2.25.1\n"},{"id":"402147","messageId":"xmqqr1swg9lc.fsf@gitster.c.googlers.com","threadId":"53929","inReplyTo":"87blk0rjob.fsf@0x63.nu","subject":"Re: Questions about trailer configuration semantics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-27T17:18:39Z","receivedAt":"2020-07-27T17:18:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"[Redirecting it to the resident expert of the trailers]\n\nAnders Waldenborg writes:\n\n> I noticed some undocumented and (at least to me) surprising behavior in\n> trailers.c.\n>\n> When configuring a value in trailer..key it causes the trailer to\n> be normalized to that in \"git interpret-trailers --parse\".\n> E.g:\n> $ printf '\\naCKed: Zz\\n' | \\\n> git -c 'trailer.Acked.key=Acked' interpret-trailers --parse\n> will emit: \"Acked: Zz\"\n>\n> but only if \"key\" is used, other config options doesn't cause it to be\n> normalized.\n> E.g:\n> $ printf '\\naCKed: Zz\\n' | \\\n> git -c 'trailer.Acked.ifmissing=doNothing' interpret-trailers --parse\n> will emit: \"aCKed: Zz\" (still lowercase a and uppercase CK)\n>\n>\n> Then there is the replacement by config \"trailer.fix.key=Fixes\" which\n> expands \"fix\" to \"Fixes\". This happens when using \"--trailer 'fix = 123'\"\n> which seems to be expected and useful behavior (albeit a bit unclear in\n> documentation). But it also happens when parsing incoming trailers, e.g\n> with that config\n> $ printf \"\\nFix: 1\\n\" | git interpret-trailers --parse\n> will emit: \"Fixes: 1\"\n>\n> (token_from_item prefers order .key, incoming token, .name)\n>\n>\n> The most surprising thing is that it uses prefix matching when finding\n> they key in configuration. If I have \"trailer.reviewed.key=Reviewed-By\"\n> it is possible to just '--trailer r=XYZ' and it will find the\n> reviewed-by trailer as \"r\" is a prefix of reviewedby. This also applies\n> to the \"--parse\". This in makes it impossible to have trailer keys that\n> are prefix of each other (e.g: \"Acked\", \"Acked-Tests\", \"Acked-Docs\") if\n> there is multiple matching in configuration it will just pick the one\n> that happens to come first.\n>\n> (token_matches_item uses strncasecmp with token length)\n>\n>\n> I guess these are the questions for the above observations:\n>\n> * Should normalization of spelling happen at all?\n>\n> * If so should it only happen when there is a .key config?\n>\n> * Should replacement to what is in .key happen also in --parse mode, or\n> only for \"--trailer\"\n>\n> * The prefix matching gotta be a bug, right?\n>\n>\n>\n> Here is a patch to the tests showing these things.\n>\n>\n>\n> From 49a4bb64a7ebf1f2d50897a024deb86b4f8056b1 Mon Sep 17 00:00:00 2001\n> From: Anders Waldenborg \n> Date: Mon, 27 Jul 2020 18:34:37 +0200\n> Subject: [PATCH] trailers: add tests for unclear/undocumented behavior\n>\n> ---\n> t/t7513-interpret-trailers.sh | 70 +++++++++++++++++++++++++++++++++++\n> 1 file changed, 70 insertions(+)\n>\n> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh\n> index 2e6d406edf..d5d19cf89b 100755\n> --- a/t/t7513-interpret-trailers.sh\n> +++ b/t/t7513-interpret-trailers.sh\n> @@ -99,6 +99,64 @@ test_expect_success 'with config option on the command line' '\n> \ttest_cmp expected actual\n> '\n>\n> +test_expect_success 'parse normalizes spelling and separators from configs with key' '\n> +\tcat >patch <<-\\EOF &&\n> +\t\tnon-trailer-line\n> +\n> +\t\tReviEweD-bY :abc\n> +\t\tReviEwEd-bY) rst\n> +\t\tReviEweD-BY ; xyz\n> +\t\taCked-bY: not normalized\n> +\tEOF\n> +\tcat >expected <<-\\EOF &&\n> +\t\tReviewed-By: abc\n> +\t\tReviewed-By: rst\n> +\t\tReviewed-By: xyz\n> +\t\taCked-bY: not normalized\n> +\tEOF\n> +\tgit \\\n> +\t\t-c \"trailer.separators=:);\" \\\n> +\t\t-c \"trailer.rb.key=Reviewed-By\" \\\n> +\t\t-c \"trailer.Acked-By.ifmissing=doNothing\" \\\n> +\t\tinterpret-trailers --parse patch >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +# Matching currently is prefix matching, causing \"This-trailer\" to be normalized too\n> +test_expect_failure 'config option matches exact only' '\n> +\tcat >patch <<-\\EOF &&\n> +\n> +\t\tThis-trailer: a\n> +\t\t b\n> +\t\tThis-trailer-exact: b\n> +\t\t c\n> +\t\tThis-trailer-exact-plus-some: c\n> +\t\t d\n> +\tEOF\n> +\tcat >expected <<-\\EOF &&\n> +\t\tThis-trailer: a b\n> +\t\tTHIS-TRAILER-EXACT: b c\n> +\t\tThis-trailer-exact-plus-some: c d\n> +\tEOF\n> +\tgit -c \"trailer.tte.key=THIS-TRAILER-EXACT\" interpret-trailers --parse patch >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +# Matching currently uses the config key even if key value is different\n> +test_expect_failure 'config option matches exact only' '\n> +\tcat >patch <<-\\EOF &&\n> +\n> +\t\tTicket: 1234\n> +\t\tReference-ticket: 99\n> +\tEOF\n> +\tcat >expected <<-\\EOF &&\n> +\t\tTicket: 1234\n> +\t\tReference-Ticket: 99\n> +\tEOF\n> +\tgit -c \"trailer.ticket.key=Reference-Ticket\" interpret-trailers --parse patch >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_expect_success 'with only a title in the message' '\n> \tcat >expected <<-\\EOF &&\n> \t\tarea: change\n> @@ -473,6 +531,18 @@ test_expect_success 'with config setup' '\n> \ttest_cmp expected actual\n> '\n>\n> +# currently this matches the \"Acked-by: \" value in ack key set by previous test\n> +test_expect_failure 'with config setup matches key exactly' '\n> +\tcat >expected <<-\\EOF &&\n> +\n> +\t\tA: B\n> +\tEOF\n> +\tgit interpret-trailers --trailer \"A=10\" empty >actual &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +\n> +\n> test_expect_success 'with config setup and \":=\" as separators' '\n> \tgit config trailer.separators \":=\" &&\n> \tgit config trailer.ack.key \"Acked-by= \" &&\n> --\n> 2.25.1\n"},{"id":"402150","messageId":"CAP8UFD1XV_jN10yOc2o4=5PtPcvT-RbxhY1H3swZz2r4g-Uzkw@mail.gmail.com","threadId":"53929","inReplyTo":"xmqqr1swg9lc.fsf@gitster.c.googlers.com","subject":"Re: Questions about trailer configuration semantics","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-07-27T18:37:26Z","receivedAt":"2020-07-27T18:37:41Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"[Adding Peff and Jonathan in Cc as they know also about this area of the code]\n\nOn Mon, Jul 27, 2020 at 7:18 PM Junio C Hamano wrote:\n>\n> [Redirecting it to the resident expert of the trailers]\n\nThanks!\n\n> Anders Waldenborg writes:\n>\n> > I noticed some undocumented and (at least to me) surprising behavior in\n> > trailers.c.\n> >\n> > When configuring a value in trailer..key it causes the trailer to\n> > be normalized to that in \"git interpret-trailers --parse\".\n> > E.g:\n> > $ printf '\\naCKed: Zz\\n' | \\\n> > git -c 'trailer.Acked.key=Acked' interpret-trailers --parse\n> > will emit: \"Acked: Zz\"\n\nYeah, I think that's nice, as it can make sure that the key appears in\nthe same way. It's true that it would be better if it would be\ndocumented.\n\n> > but only if \"key\" is used, other config options doesn't cause it to be\n> > normalized.\n> > E.g:\n> > $ printf '\\naCKed: Zz\\n' | \\\n> > git -c 'trailer.Acked.ifmissing=doNothing' interpret-trailers --parse\n> > will emit: \"aCKed: Zz\" (still lowercase a and uppercase CK)\n\nYeah, in this case we are not sure if \"Acked\" or \"aCKed\" is the right\nway to spell it.\n\n> > Then there is the replacement by config \"trailer.fix.key=Fixes\" which\n> > expands \"fix\" to \"Fixes\". This happens when using \"--trailer 'fix = 123'\"\n> > which seems to be expected and useful behavior (albeit a bit unclear in\n> > documentation). But it also happens when parsing incoming trailers, e.g\n> > with that config\n> > $ printf \"\\nFix: 1\\n\" | git interpret-trailers --parse\n> > will emit: \"Fixes: 1\"\n\nYeah, I think it allows for shortcuts and can help with standardizing\nthe keys in commit messages.\n\n> > (token_from_item prefers order .key, incoming token, .name)\n> >\n> >\n> > The most surprising thing is that it uses prefix matching when finding\n> > they key in configuration. If I have \"trailer.reviewed.key=Reviewed-By\"\n> > it is possible to just '--trailer r=XYZ' and it will find the\n> > reviewed-by trailer as \"r\" is a prefix of reviewedby. This also applies\n> > to the \"--parse\".\n\nYeah, that's also for shortcuts and standardization.\n\n> > This in makes it impossible to have trailer keys that\n> > are prefix of each other (e.g: \"Acked\", \"Acked-Tests\", \"Acked-Docs\") if\n> > there is multiple matching in configuration it will just pick the one\n> > that happens to come first.\n\nThat's a downside of the above. I agree that it might seem strange or\nbad. Perhaps an option could be added to implement a strict matching,\nif people really want it.\n\nAlso if you configure trailers in the \"Acked\", \"Acked-Tests\",\n\"Acked-Docs\" order, then any common prefix will pick \"Acked\" which\ncould be considered ok in my opinion.\n\n> > (token_matches_item uses strncasecmp with token length)\n> >\n> >\n> > I guess these are the questions for the above observations:\n> >\n> > * Should normalization of spelling happen at all?\n\nYes, I think it can help.\n\n> > * If so should it only happen when there is a .key config?\n\nYes, it can help too if that only happens when there is a .key config.\n\n> > * Should replacement to what is in .key happen also in --parse mode, or\n> > only for \"--trailer\"\n\nI think it's more consistent if it happens in both --parse and\n--trailer mode. I didn't implement --parse though.\n\n> > * The prefix matching gotta be a bug, right?\n\nNo, it's a feature ;-) Seriously I agree that this could be seen as a\ndownside, but I think it can be understood that the convenience is\nworth it. And in case someone is really annoyed by this, then adding\nan option for strict matching should not be very difficult.\n\n> > Here is a patch to the tests showing these things.\n\nThanks for the patch! I would be ok to add such a patch to the test\nsuite if it was sent like a regular patch (so with a commit message, a\nSigned-off-by: and so on) to the mailing list. While at it some\ndocumentation of the related behavior would also be very nice.\n"},{"id":"402154","messageId":"20200727194036.GA795313@coredump.intra.peff.net","threadId":"53929","inReplyTo":"CAP8UFD1XV_jN10yOc2o4=5PtPcvT-RbxhY1H3swZz2r4g-Uzkw@mail.gmail.com","subject":"Re: Questions about trailer configuration semantics","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-27T19:40:36Z","receivedAt":"2020-07-27T19:40:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 27, 2020 at 08:37:26PM +0200, Christian Couder wrote:\n\n> > > I noticed some undocumented and (at least to me) surprising behavior in\n> > > trailers.c.\n> > >\n> > > When configuring a value in trailer..key it causes the trailer to\n> > > be normalized to that in \"git interpret-trailers --parse\".\n> > > E.g:\n> > > $ printf '\\naCKed: Zz\\n' | \\\n> > > git -c 'trailer.Acked.key=Acked' interpret-trailers --parse\n> > > will emit: \"Acked: Zz\"\n> \n> Yeah, I think that's nice, as it can make sure that the key appears in\n> the same way. It's true that it would be better if it would be\n> documented.\n\nI'd note that this also happens without --parse.\n\n> > > Then there is the replacement by config \"trailer.fix.key=Fixes\" which\n> > > expands \"fix\" to \"Fixes\". This happens when using \"--trailer 'fix = 123'\"\n> > > which seems to be expected and useful behavior (albeit a bit unclear in\n> > > documentation). But it also happens when parsing incoming trailers, e.g\n> > > with that config\n> > > $ printf \"\\nFix: 1\\n\" | git interpret-trailers --parse\n> > > will emit: \"Fixes: 1\"\n> [...]\n> > > * Should replacement to what is in .key happen also in --parse mode, or\n> > > only for \"--trailer\"\n> \n> I think it's more consistent if it happens in both --parse and\n> --trailer mode. I didn't implement --parse though.\n\nI don't recall being aware of this prefix matching until this thread, so\nI doubt that the current behavior of --parse was something I tried for\nintentionally. It's mostly just using the existing code, plus a few\nextra options (listed in the docs). I'm not opposed to adding an option\nto do strict matching and/or avoid rewriting, and then possibly adding\nthat into --parse by default.\n\nI don't have much of an opinion on which behavior would be preferred.\nI've never actually had a use case for configuring trailer.*.key, as I\nusually am only looking at reading existing trailers to collect stats,\netc.\n\n-Peff\n"},{"id":"402159","messageId":"xmqqk0yog1lg.fsf@gitster.c.googlers.com","threadId":"53929","inReplyTo":"CAP8UFD1XV_jN10yOc2o4=5PtPcvT-RbxhY1H3swZz2r4g-Uzkw@mail.gmail.com","subject":"Re: Questions about trailer configuration semantics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-27T20:11:23Z","receivedAt":"2020-07-27T20:11:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> > $ printf '\\naCKed: Zz\\n' | \\\n>> > git -c 'trailer.Acked.key=Acked' interpret-trailers --parse\n>> > will emit: \"Acked: Zz\"\n>\n> Yeah, I think that's nice, as it can make sure that the key appears in\n> the same way. It's true that it would be better if it would be\n> documented.\n>\n>> > but only if \"key\" is used, other config options doesn't cause it to be\n>> > normalized.\n>> > E.g:\n>> > $ printf '\\naCKed: Zz\\n' | \\\n>> > git -c 'trailer.Acked.ifmissing=doNothing' interpret-trailers --parse\n>> > will emit: \"aCKed: Zz\" (still lowercase a and uppercase CK)\n>\n> Yeah, in this case we are not sure if \"Acked\" or \"aCKed\" is the right\n> way to spell it.\n\nOK, so in short, the trailer subsystem matches the second level of\nthe configuration variable name (e.g. \"Acked\") in a case insensitive\nway, and it does *not* normalize the case in the output. The .key \nrequest is a mechanism to replace the matched key with the specified\nstring, so there is *NO* case normalization in what Anders observed.\n\nIn other words,\n\n $ printf '\\naCKed: Zz\\n' | \\\n git -c 'trailer.Acked.key=Rejected' interpret-trailers --parse\n\nwould have emitted \"Rejected: Zz\".\n"},{"id":"402167","messageId":"87a6zkr5z7.fsf@0x63.nu","threadId":"53929","inReplyTo":"CAP8UFD1XV_jN10yOc2o4=5PtPcvT-RbxhY1H3swZz2r4g-Uzkw@mail.gmail.com","subject":"Re: Questions about trailer configuration semantics","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-07-27T21:41:16Z","receivedAt":"2020-07-27T21:41:28Z","isPatch":false,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nChristian Couder writes:\n\n>> > When configuring a value in trailer..key it causes the trailer to\n>> > be normalized to that in \"git interpret-trailers --parse\".\n>\n> Yeah, I think that's nice, as it can make sure that the key appears in\n> the same way. It's true that it would be better if it would be\n> documented.\n>\n>> > but only if \"key\" is used, other config options doesn't cause it to be\n>> > normalized.\n>\n> Yeah, in this case we are not sure if \"Acked\" or \"aCKed\" is the right\n> way to spell it.\n\nMakes sense.\n\nHowever I guess one alternative implementation would be that if\ntrailer.X.something is configured but not trailer.X.key it would work as\nif there was an implicit \"trailer.X.key=X\" configured. The name of the\nconfiguration value would specify the correct spelling.\n\n\n>> > Then there is the replacement by config \"trailer.fix.key=Fixes\" which\n>> > expands \"fix\" to \"Fixes\". This happens when using \"--trailer 'fix = 123'\"\n>> > which seems to be expected and useful behavior (albeit a bit unclear in\n>> > documentation). But it also happens when parsing incoming trailers, e.g\n>> > with that config\n>> > $ printf \"\\nFix: 1\\n\" | git interpret-trailers --parse\n>> > will emit: \"Fixes: 1\"\n>\n> Yeah, I think it allows for shortcuts and can help with standardizing\n> the keys in commit messages.\n\nMaybe what I'm missing is a clear picture of the different cases that\n\"git interpret-trailers\" is being used in. The \"--trailer x=10\" option\nseems clearly designed for human input trying to be as helpful as\npossible (e.g always allowing '=' regardless of separators\nconfigured). But when reading a message with trailers, is same\nhelpfulness useful? Or is it always only reading proper trailers\npreviously added by --trailer?\n\n\n\nThe standardization of trailers is interesting. If I want to standardize\n\"ReviewedBy\", \"Reviewer\", \"Code-Reviewer\" trailers to \"Reviewed-By\" I\nwould add these configs:\n\ntrailer.reviewer.key = Reviewed-By\ntrailer.ReviewedBy.key = Reviewed-By\ntrailer.Code-Reviewer.key = Reviewed-By\n\nSeems useful (and works out of the box as a msg-filter to\nfilter-branch). But the configuration seems a bit backwards, it is\nconfigured a 3 different trailer entries, rather that a single trailer\nentry with three aliases (or something like that).\n\n\n\n>> > (token_from_item prefers order .key, incoming token, .name)\n>> >\n>> >\n>> > The most surprising thing is that it uses prefix matching when finding\n>> > they key in configuration. If I have \"trailer.reviewed.key=Reviewed-By\"\n>> > it is possible to just '--trailer r=XYZ' and it will find the\n>> > reviewed-by trailer as \"r\" is a prefix of reviewedby. This also applies\n>> > to the \"--parse\".\n>\n> Yeah, that's also for shortcuts and standardization.\n>\n>> > This in makes it impossible to have trailer keys that\n>> > are prefix of each other (e.g: \"Acked\", \"Acked-Tests\", \"Acked-Docs\") if\n>> > there is multiple matching in configuration it will just pick the one\n>> > that happens to come first.\n>\n> That's a downside of the above. I agree that it might seem strange or\n> bad. Perhaps an option could be added to implement a strict matching,\n> if people really want it.\n>\n> Also if you configure trailers in the \"Acked\", \"Acked-Tests\",\n> \"Acked-Docs\" order, then any common prefix will pick \"Acked\" which\n> could be considered ok in my opinion.\n\nYeah, that works. But that dependency on order of configuration is quite\nsubtle imho.\n\nE.g consider:\n\n$ cat >msg <\n>> > (token_matches_item uses strncasecmp with token length)\n>> >\n>> >\n>> > I guess these are the questions for the above observations:\n>> >\n>> > * Should normalization of spelling happen at all?\n>\n> Yes, I think it can help.\n>\n>> > * If so should it only happen when there is a .key config?\n>\n> Yes, it can help too if that only happens when there is a .key config.\n>\n>> > * Should replacement to what is in .key happen also in --parse mode, or\n>> > only for \"--trailer\"\n>\n> I think it's more consistent if it happens in both --parse and\n> --trailer mode. I didn't implement --parse though.\n\n\nKeep in mind that the machinery used by interpret-trailers is also used\nby pretty '%(trailers)' so whatever normalization and shortcuts\nhappening also shows up there. Is shortcuts useful in that case too?\nThere the input is commit history, not some user input.\n\nE.g:\n$ git -c 'trailer.signed-off-by.key=Attest' \\\n show --pretty='format:%(trailers:key=Attest)' --no-patch \\\n 0172f7834a31ae7cb9873feaaaa63939352fa3ae\nAttest: Christian Couder \nAttest: Junio C Hamano \n\n\nThere is also some inconsistency here. If one use '%(trailers) the\nnormalization doesn't happen. Only if using '%(trailers:only)' or some\nother option.\n\n(because optimization in format_trailer_info:\n /* If we want the whole block untouched, we can take the fast path. */)\n\nI guess the fact that expansion happens also in these cases can get\nconfusing if I have configured a trailer \"Bug-Introduced-In\" locally and\nthe commit message contains \"Bug: 123\".\n\n'git log --pretty=format:\"%h% (trailers:key=Bug-Introduced-In,valueonly,separator=%x20)\"'\nwould pick up data from the wrong trailer just because I added\nconfiguration for Bug-Introduced-In trailer.\n\n\n\n>> > * The prefix matching gotta be a bug, right?\n>\n> No, it's a feature ;-) Seriously I agree that this could be seen as a\n> downside, but I think it can be understood that the convenience is\n> worth it. And in case someone is really annoyed by this, then adding\n> an option for strict matching should not be very difficult.\n>\n>> > Here is a patch to the tests showing these things.\n>\n> Thanks for the patch! I would be ok to add such a patch to the test\n> suite if it was sent like a regular patch (so with a commit message, a\n> Signed-off-by: and so on) to the mailing list. While at it some\n> documentation of the related behavior would also be very nice.\n\nShould I keep the \"test_expect_failure\" tests, or change into expecting\ncurrent behavior and switch them over to \"test_except_success\"?\n\nI'll see if I can do something about documentation.\n"},{"id":"402168","messageId":"878sf4r4au.fsf@0x63.nu","threadId":"53929","inReplyTo":"xmqqk0yog1lg.fsf@gitster.c.googlers.com","subject":"Re: Questions about trailer configuration semantics","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-07-27T22:17:39Z","receivedAt":"2020-07-27T22:17:47Z","isPatch":false,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJunio C Hamano writes:\n\n> Christian Couder writes:\n>> Yeah, in this case we are not sure if \"Acked\" or \"aCKed\" is the right\n>> way to spell it.\n>\n> OK, so in short, the trailer subsystem matches the second level of\n> the configuration variable name (e.g. \"Acked\") in a case insensitive\n> way\n\nFrom what I can understand it tries to match *both* on the second level\nAND the value of .key (trailers.c:token_matches_item)\n\n$ printf '\\na: 1\\nb: 2\\nc: 3\\n' | \\\n git -c 'trailer.A.key=B' interpret-trailers\nB: 1\nB: 2\nc: 3\n\nand then uses the .key value when outputting the result (by calling\ntrailer.c:token_from_item)\n\nI.e:\nit gets \"a: 1\", tries to find configuration for that, and finds\ntrailer.A because \"a\" (case insenitively) matches conf.name, therefore\nit outputs value of trailer.A.key + separator + \"1\"\n\nthen it gets \"b: 1\", and again finds trailer.A because \"b\" matches\nconf.key.\n\n> , and it does *not* normalize the case in the output. The .key\n> request is a mechanism to replace the matched key with the specified\n> string, so there is *NO* case normalization in what Anders observed.\n\nHmm. Maybe the \"matching\" vs \"outputting\" makes it clearer.\n\n\nGiven configuration trailer..key=\n\nWhen printing a parsed trailer, e.g from pretty format \"%(trailers)\",\n\"git interpret-trailers\" passthrough of existing trailers or addition of\na new trailer with --trailer: is used in output. If is not\nconfigured the trailer token is output the same way as it was in input.\n\nWhen finding a trailer, e.g for '--trailer x=y' or\ntrailer..where=before/after: matching is done against both\n and .\n\nWhen showing a single trailer with pretty format '%(trailers:key=X)' it\nis matched against only. (I guess one can see this as matching on\nthe formatted output).\n\n\n> In other words,\n>\n> $ printf '\\naCKed: Zz\\n' | \\\n> git -c 'trailer.Acked.key=Rejected' interpret-trailers --parse\n>\n> would have emitted \"Rejected: Zz\".\n\nIndeed.\n"},{"id":"402170","messageId":"xmqqa6zkfu3b.fsf@gitster.c.googlers.com","threadId":"53929","inReplyTo":"87a6zkr5z7.fsf@0x63.nu","subject":"Re: Questions about trailer configuration semantics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-27T22:53:28Z","receivedAt":"2020-07-27T22:53:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> However I guess one alternative implementation would be that if\n> trailer.X.something is configured but not trailer.X.key it would work as\n> if there was an implicit \"trailer.X.key=X\" configured. The name of the\n> configuration value would specify the correct spelling.\n\nI had the same thought but then a question struck and stopped me:\nwhat should happen if \"trailer.X.something\" and\n\"trailer.x.somethingelse\" are both defined?\n"},{"id":"402171","messageId":"875za8r2fu.fsf@0x63.nu","threadId":"53929","inReplyTo":"20200727194036.GA795313@coredump.intra.peff.net","subject":"Re: Questions about trailer configuration semantics","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-07-27T22:57:51Z","receivedAt":"2020-07-27T22:57:58Z","isPatch":false,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJeff King writes:\n\n> On Mon, Jul 27, 2020 at 08:37:26PM +0200, Christian Couder wrote:\n>\n>> > > I noticed some undocumented and (at least to me) surprising behavior in\n>> > > trailers.c.\n>> > >\n>> > > When configuring a value in trailer..key it causes the trailer to\n>> > > be normalized to that in \"git interpret-trailers --parse\".\n>> > > E.g:\n>> > > $ printf '\\naCKed: Zz\\n' | \\\n>> > > git -c 'trailer.Acked.key=Acked' interpret-trailers --parse\n>> > > will emit: \"Acked: Zz\"\n>>\n>> Yeah, I think that's nice, as it can make sure that the key appears in\n>> the same way. It's true that it would be better if it would be\n>> documented.\n>\n> I'd note that this also happens without --parse.\n\nRight, and it also happens with \"--only-input\" (part of \"--parse\")\n\n\"--only-input\" is documented as follows:\n\n Output only trailers that exist in the input; do not add any from the\n command-line or by following configured trailer.* rules.\n\n\n[]\n\n> I don't recall being aware of this prefix matching until this thread, so\n> I doubt that the current behavior of --parse was something I tried for\n> intentionally. It's mostly just using the existing code, plus a few\n> extra options (listed in the docs). I'm not opposed to adding an option\n> to do strict matching and/or avoid rewriting, and then possibly adding\n> that into --parse by default.\n\nWould that option also be set for the parsing done by \"%(trailers)\"\npretty format specifier?\n\n> I don't have much of an opinion on which behavior would be preferred.\n> I've never actually had a use case for configuring trailer.*.key, as I\n> usually am only looking at reading existing trailers to collect stats,\n> etc.\n\nI'm also mainly using in reading trailers (mostly with pretty format\n\"%(trailers:key=x)\") and then these convenience shortcuts doesn't really\nseem helpful, they rather add a small risk of mangling my data. Not that\nthis has caused any problems for me in practice.\n\n"},{"id":"402172","messageId":"xmqq5za8ftih.fsf@gitster.c.googlers.com","threadId":"53929","inReplyTo":"878sf4r4au.fsf@0x63.nu","subject":"Re: Questions about trailer configuration semantics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-27T23:05:58Z","receivedAt":"2020-07-27T23:06:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> From what I can understand it tries to match *both* on the second level\n> AND the value of .key (trailers.c:token_matches_item)\n\nYuck, I do not know what were we thinking to design the behaviour\nlike *that*. Or it may be simply buggy.\n\n> $ printf '\\na: 1\\nb: 2\\nc: 3\\n' | \\\n> git -c 'trailer.A.key=B' interpret-trailers\n> B: 1\n> B: 2\n> c: 3\n\nI can understand the first one (i.e. \"trailer.$name.$var\" try to\nmatch $name as case insensitively) but not the second one. There is\nnot an single rule for \"b\" trailer, and we should be getting the\nsame behaviour as the third line, i.e. the key not involved in\nrewriting is passed as-is.\n\n"},{"id":"402173","messageId":"87365cr1in.fsf@0x63.nu","threadId":"53929","inReplyTo":"xmqqa6zkfu3b.fsf@gitster.c.googlers.com","subject":"Re: Questions about trailer configuration semantics","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-07-27T23:17:36Z","receivedAt":"2020-07-27T23:17:42Z","isPatch":false,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJunio C Hamano writes:\n\n> I had the same thought but then a question struck and stopped me:\n> what should happen if \"trailer.X.something\" and\n> \"trailer.x.somethingelse\" are both defined?\n\nGood point.\n\nIf we follow the same reasoning as with what happens with prefix\nmatching (config order matters) the one that happens to be mentioned\nfirst in configuration wins.\n\nThe same thing actually happens today with .key:\n\ntrailer.foo.key=Hello\ntrailer.bar.key=HELLO\n\n$ printf \"\\nHELLo: hi\\n\" | \\\n git -c \"trailer.foo.key=Hello\" -c \"trailer.bar.key=HELLO\" interpret-trailers --parse\nHello: hi\n\nbut if we swap order of config:\n\n$ printf \"\\nHELLo: hi\\n\" | \\\n git -c \"trailer.bar.key=HELLO\" -c \"trailer.foo.key=Hello\" interpret-trailers --parse\nHELLO: hi\n\n"},{"id":"402175","messageId":"20200727234217.GA802697@coredump.intra.peff.net","threadId":"53929","inReplyTo":"875za8r2fu.fsf@0x63.nu","subject":"Re: Questions about trailer configuration semantics","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-27T23:42:17Z","receivedAt":"2020-07-27T23:42:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 28, 2020 at 12:57:51AM +0200, Anders Waldenborg wrote:\n\n> >> > > When configuring a value in trailer..key it causes the trailer to\n> >> > > be normalized to that in \"git interpret-trailers --parse\".\n> >> > > E.g:\n> >> > > $ printf '\\naCKed: Zz\\n' | \\\n> >> > > git -c 'trailer.Acked.key=Acked' interpret-trailers --parse\n> >> > > will emit: \"Acked: Zz\"\n> >>\n> >> Yeah, I think that's nice, as it can make sure that the key appears in\n> >> the same way. It's true that it would be better if it would be\n> >> documented.\n> >\n> > I'd note that this also happens without --parse.\n> \n> Right, and it also happens with \"--only-input\" (part of \"--parse\")\n> \n> \"--only-input\" is documented as follows:\n> \n> Output only trailers that exist in the input; do not add any from the\n> command-line or by following configured trailer.* rules.\n\nI think I meant there only that we wouldn't add new trailers (e.g., from\n\"trailers.*.ifMissing\"). But I do agree that it might be simpler if we\ncan just say \"we don't look at trailer..* config at all in\n--only-input mode. I _think_ that's true (we probably do look at\ntrailer.separators, but the rest of the token-specific ones look like\nthey're all about writing or modifying, not reading).\n\n> > I don't recall being aware of this prefix matching until this thread, so\n> > I doubt that the current behavior of --parse was something I tried for\n> > intentionally. It's mostly just using the existing code, plus a few\n> > extra options (listed in the docs). I'm not opposed to adding an option\n> > to do strict matching and/or avoid rewriting, and then possibly adding\n> > that into --parse by default.\n> \n> Would that option also be set for the parsing done by \"%(trailers)\"\n> pretty format specifier?\n\nI thnk %(trailers) isn't quite the same as \"--parse\", because you have\nto say \"only\" or \"unfold\" yourself. But yeah, that option should\ncertainly be available there, if not the default.\n\n> > I don't have much of an opinion on which behavior would be preferred.\n> > I've never actually had a use case for configuring trailer.*.key, as I\n> > usually am only looking at reading existing trailers to collect stats,\n> > etc.\n> \n> I'm also mainly using in reading trailers (mostly with pretty format\n> \"%(trailers:key=x)\") and then these convenience shortcuts doesn't really\n> seem helpful, they rather add a small risk of mangling my data. Not that\n> this has caused any problems for me in practice.\n\nYeah, pondering it a bit more, I think trailer..* doesn't really\nmake any sense for any reading operation (including %(trailers) or\n--parse). I guess it _could_ be useful to normalize names in some\ninstances, but it's as likely to confuse or break somebody as to help.\n\n-Peff\n"},{"id":"402176","messageId":"871rkwqzi3.fsf@0x63.nu","threadId":"53929","inReplyTo":"xmqq5za8ftih.fsf@gitster.c.googlers.com","subject":"Re: Questions about trailer configuration semantics","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2020-07-28T00:01:15Z","receivedAt":"2020-07-28T00:01:23Z","isPatch":false,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJunio C Hamano writes:\n\n> Anders Waldenborg writes:\n>\n>> From what I can understand it tries to match *both* on the second level\n>> AND the value of .key (trailers.c:token_matches_item)\n>\n> Yuck, I do not know what were we thinking to design the behaviour\n> like *that*. Or it may be simply buggy.\n>\n>> $ printf '\\na: 1\\nb: 2\\nc: 3\\n' | \\\n>> git -c 'trailer.A.key=B' interpret-trailers\n>> B: 1\n>> B: 2\n>> c: 3\n>\n> I can understand the first one (i.e. \"trailer.$name.$var\" try to\n> match $name as case insensitively) but not the second one. There is\n> not an single rule for \"b\" trailer, and we should be getting the\n> same behaviour as the third line, i.e. the key not involved in\n> rewriting is passed as-is.\n\nI'm not so sure about that. Matching sometimes needs to happen on .key\ntoo. If this configuration is supposed to be useful (and as \"shortcuts\"\nhas been mentioned before and is what tests does, I think it should be):\n\ntrailer.rb.key=Reviewed-By\ntrailer.rb.ifexists=addifdifferent\n\nthen matching must look at key, not name. As the value of .key is what\nwould have been written previously into the message.\n\nE.g:\n$ printf \"\\nReviewed-By: hi\\n\" | \\\n git -c \"trailer.rb.key=Reviewed-By\" \\\n -c \"trailer.rb.ifexists=addifdifferent\" \\\n interpret-trailers --trailer \"rb=hi\"\nReviewed-By: hi\n\n\n\nThe opposite case, matching on name in input message I'm not sure where\nit is useful.\n\n $ printf \"\\nrb: hi\\n\" | \\\n git -c \"trailer.rb.key=Reviewed-By\" \\\n -c \"trailer.rb.ifexists=addifdifferent\" \\\n interpret-trailers --trailer \"rb=hi\"\nReviewed-By: hi\n\n\n\nThe way I have understood it \".key\" can be used for some different\nthings:\n\n * Freeing up name to be used as a shortcut alias.\n * Defining the canonical capitalization when passing through trailers\n * Allowing specifying non default separator for that key.\n\nI wonder if those uses could be split up.\n\nSo instead of this configuration:\n\n [trailer \"rb\"]\n key=\"Reviewed-By> \"\n\nthat does all three of those. The configuration would be:\n\n [trailer \"Reviewed-By\"]\n separator=\"> \"\n canonicalize=true\n alias=rb\n\nthat way the \"name\" part of the config always is the correct spelling\nand capitalization. \"alias\" could easily be a list of multiple alias if\nthat is wanted. \"alias\" could be split into \"inputalias\" (matched\nagainst when reading trailers from stdin or a commit/tag message) and\n\"optalias\" (matched against when reading --trailer cmdline option, and\n%(trailers:key))\n"},{"id":"402179","messageId":"CAP8UFD1JZhNVDJ=fe-FLzmBqSbAwyaJuABK-G+-keL4LanZbpA@mail.gmail.com","threadId":"53929","inReplyTo":"87a6zkr5z7.fsf@0x63.nu","subject":"Re: Questions about trailer configuration semantics","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-07-28T07:07:18Z","receivedAt":"2020-07-28T07:07:33Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jul 27, 2020 at 11:41 PM Anders Waldenborg wrote:\n\n> Christian Couder writes:\n\n[...]\n\n> >> > Then there is the replacement by config \"trailer.fix.key=Fixes\" which\n> >> > expands \"fix\" to \"Fixes\". This happens when using \"--trailer 'fix = 123'\"\n> >> > which seems to be expected and useful behavior (albeit a bit unclear in\n> >> > documentation). But it also happens when parsing incoming trailers, e.g\n> >> > with that config\n> >> > $ printf \"\\nFix: 1\\n\" | git interpret-trailers --parse\n> >> > will emit: \"Fixes: 1\"\n> >\n> > Yeah, I think it allows for shortcuts and can help with standardizing\n> > the keys in commit messages.\n>\n> Maybe what I'm missing is a clear picture of the different cases that\n> \"git interpret-trailers\" is being used in. The \"--trailer x=10\" option\n> seems clearly designed for human input trying to be as helpful as\n> possible (e.g always allowing '=' regardless of separators\n> configured). But when reading a message with trailers, is same\n> helpfulness useful? Or is it always only reading proper trailers\n> previously added by --trailer?\n\nI guess it depends on the purpose of reading a message with trailers.\nIf you want to do stats for example, do you really want to consider\n\"Reviewed\", \"Reviewer\" and \"Reviewed-by\" as different trailers in your\nstats?\n\n> The standardization of trailers is interesting. If I want to standardize\n> \"ReviewedBy\", \"Reviewer\", \"Code-Reviewer\" trailers to \"Reviewed-By\" I\n> would add these configs:\n>\n> trailer.reviewer.key = Reviewed-By\n> trailer.ReviewedBy.key = Reviewed-By\n> trailer.Code-Reviewer.key = Reviewed-By\n>\n> Seems useful (and works out of the box as a msg-filter to\n> filter-branch). But the configuration seems a bit backwards, it is\n> configured a 3 different trailer entries, rather that a single trailer\n> entry with three aliases (or something like that).\n\nMaybe taking advantage of the shortcuts and using only the following could work:\n\ntrailer.review.key = Reviewed-By\ntrailer.code-review.key = Reviewed-By\n\n> >> > This in makes it impossible to have trailer keys that\n> >> > are prefix of each other (e.g: \"Acked\", \"Acked-Tests\", \"Acked-Docs\") if\n> >> > there is multiple matching in configuration it will just pick the one\n> >> > that happens to come first.\n> >\n> > That's a downside of the above. I agree that it might seem strange or\n> > bad. Perhaps an option could be added to implement a strict matching,\n> > if people really want it.\n> >\n> > Also if you configure trailers in the \"Acked\", \"Acked-Tests\",\n> > \"Acked-Docs\" order, then any common prefix will pick \"Acked\" which\n> > could be considered ok in my opinion.\n>\n> Yeah, that works. But that dependency on order of configuration is quite\n> subtle imho.\n\n[...]\n\nYeah, I agree that documenting this would be nice.\n\n> >> > * Should replacement to what is in .key happen also in --parse mode, or\n> >> > only for \"--trailer\"\n> >\n> > I think it's more consistent if it happens in both --parse and\n> > --trailer mode. I didn't implement --parse though.\n>\n> Keep in mind that the machinery used by interpret-trailers is also used\n> by pretty '%(trailers)' so whatever normalization and shortcuts\n> happening also shows up there. Is shortcuts useful in that case too?\n> There the input is commit history, not some user input.\n\nPretty %(trailer) was added later by someone else, but I guess it also\ndepends on the use case. Is it going to be used for stats? Is it\ninteresting to distinguish between very similar trailers in these\nstats?\n\nAnd again if people are interested in very strict processing, then\nadding an option for that could be the right thing to do.\n\n[...]\n\n> There is also some inconsistency here. If one use '%(trailers) the\n> normalization doesn't happen. Only if using '%(trailers:only)' or some\n> other option.\n>\n> (because optimization in format_trailer_info:\n> /* If we want the whole block untouched, we can take the fast path. */)\n\nMaybe that's a bug. Peff, it looks like you added the above comment.\nDo you think it's a bug?\n\n> I guess the fact that expansion happens also in these cases can get\n> confusing if I have configured a trailer \"Bug-Introduced-In\" locally and\n> the commit message contains \"Bug: 123\".\n>\n> 'git log --pretty=format:\"%h% (trailers:key=Bug-Introduced-In,valueonly,separator=%x20)\"'\n> would pick up data from the wrong trailer just because I added\n> configuration for Bug-Introduced-In trailer.\n\nYeah, I understand that it could be an issue.\n\n> >> > Here is a patch to the tests showing these things.\n> >\n> > Thanks for the patch! I would be ok to add such a patch to the test\n> > suite if it was sent like a regular patch (so with a commit message, a\n> > Signed-off-by: and so on) to the mailing list. While at it some\n> > documentation of the related behavior would also be very nice.\n>\n> Should I keep the \"test_expect_failure\" tests, or change into expecting\n> current behavior and switch them over to \"test_except_success\"?\n\nI think you should switch them over to \"test_except_success\".\n\n> I'll see if I can do something about documentation.\n\nThanks,\nChristian.\n"},{"id":"402198","messageId":"20200728154132.GA817108@coredump.intra.peff.net","threadId":"53929","inReplyTo":"CAP8UFD1JZhNVDJ=fe-FLzmBqSbAwyaJuABK-G+-keL4LanZbpA@mail.gmail.com","subject":"Re: Questions about trailer configuration semantics","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-28T15:41:32Z","receivedAt":"2020-07-28T15:41:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 28, 2020 at 09:07:18AM +0200, Christian Couder wrote:\n\n> > Maybe what I'm missing is a clear picture of the different cases that\n> > \"git interpret-trailers\" is being used in. The \"--trailer x=10\" option\n> > seems clearly designed for human input trying to be as helpful as\n> > possible (e.g always allowing '=' regardless of separators\n> > configured). But when reading a message with trailers, is same\n> > helpfulness useful? Or is it always only reading proper trailers\n> > previously added by --trailer?\n> \n> I guess it depends on the purpose of reading a message with trailers.\n> If you want to do stats for example, do you really want to consider\n> \"Reviewed\", \"Reviewer\" and \"Reviewed-by\" as different trailers in your\n> stats?\n\nMy guess would be that yes, you probably would want them to be\ndifferent. They _might_ be typos of each other, in which case\nnormalizing is helpful. But they might well have totally different\nmeanings. It will really depend on the project's use of the trailers,\nand I'd expect any stats-gathering to do that kind of normalization much\nlater. I.e., use Git to reliably get \"foo: bar\" output from all of the\ncommits, and then count them up in a perl script or whatever, lumping\ntogether like fields at that stage.\n\nIt's inevitable that you'll have to do some data cleanup like that\nanyway. Lumping together prefixes isn't flexible enough to coverall\ncases. Using trailer.*.key to manually map names covers more, but again\nnot all (e.g., if the project used to use \"foo\" but switched to \"bar\",\nbut the syntax of the value fields also changed at the same time, you'd\nneed to normalize those, too).\n\nSo to me it boils down to:\n\n - returning the data as verbatim as possible when reading existing\n trailers would give the least surprise to most people\n\n - real data collection is going to involve a separate post-processing\n step which is better done in a full programming language\n\n> > There is also some inconsistency here. If one use '%(trailers) the\n> > normalization doesn't happen. Only if using '%(trailers:only)' or some\n> > other option.\n> >\n> > (because optimization in format_trailer_info:\n> > /* If we want the whole block untouched, we can take the fast path. */)\n> \n> Maybe that's a bug. Peff, it looks like you added the above comment.\n> Do you think it's a bug?\n\nThe original %(trailers) was \"dump the trailers block verbatim\". But\nthat's not super useful for parsing individual trailers. So \"only\"\nactually starts parsing the individual trailers. I'd argue that the\n%(trailers) behavior is correct, but that %(trailers:only) should\nprobably be doing less (i.e., just parsing and reporting what it finds,\nbut not changing any names).\n\n-Peff\n"},{"id":"402216","messageId":"xmqqmu3jegon.fsf@gitster.c.googlers.com","threadId":"53929","inReplyTo":"20200728154132.GA817108@coredump.intra.peff.net","subject":"Re: Questions about trailer configuration semantics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-28T16:40:40Z","receivedAt":"2020-07-28T16:40:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It's inevitable that you'll have to do some data cleanup like that\n> anyway. Lumping together prefixes isn't flexible enough to coverall\n> cases. Using trailer.*.key to manually map names covers more, but again\n> not all (e.g., if the project used to use \"foo\" but switched to \"bar\",\n> but the syntax of the value fields also changed at the same time, you'd\n> need to normalize those, too).\n>\n> So to me it boils down to:\n>\n> - returning the data as verbatim as possible when reading existing\n> trailers would give the least surprise to most people\n>\n> - real data collection is going to involve a separate post-processing\n> step which is better done in a full programming language\n\nYeah, I agree that these are good guidelines to use when we think\nabout the feature.\n\n> The original %(trailers) was \"dump the trailers block verbatim\". But\n> that's not super useful for parsing individual trailers. So \"only\"\n> actually starts parsing the individual trailers. I'd argue that the\n> %(trailers) behavior is correct, but that %(trailers:only) should\n> probably be doing less (i.e., just parsing and reporting what it finds,\n> but not changing any names).\n\nYes, sounds sensible.\n"}]}