{"thread":{"id":"49703","subject":"[PATCH] pretty: Add %(trailer:X) to display single trailer","startedAt":"2018-10-28T13:31:07Z","lastAt":"2019-02-02T09:14:28Z","messageCount":69,"participants":["Anders Waldenborg","Junio C Hamano","Jeff King","Eric Sunshine","Оля Тележная"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"361840","messageId":"20181028125025.30952-1-anders@0x63.nu","threadId":"49703","inReplyTo":null,"subject":"[PATCH] pretty: Add %(trailer:X) to display single trailer","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-10-28T12:50:25Z","receivedAt":"2018-10-28T13:31:07Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"This new format placeholder allows displaying only a single\ntrailer. The formatting done is similar to what is done for\n--decorate/%d using parentheses and comma separation.\n\nIt's intended use is for things like ticket references in trailers.\n\nSo with a commit with a message like:\n\n > Some good commit\n >\n > Ticket: XYZ-123\n\nrunning:\n\n $ git log --pretty=\"%H %s% (trailer:Ticket)\"\n\nwill give:\n\n > 123456789a Some good commit (Ticket: XYZ-123)\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 4 ++++\n pretty.c | 16 +++++++++++++\n t/t4205-log-pretty-formats.sh | 40 ++++++++++++++++++++++++++++++++\n trailer.c | 18 ++++++++++++--\n trailer.h | 1 +\n 5 files changed, 77 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 6109ef09aa..a46d0c0717 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -211,6 +211,10 @@ endif::git-rev-list[]\n If the `unfold` option is given, behave as if interpret-trailer's\n `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do\n both.\n+- %(trailer:): display the specified trailer in parentheses (like\n+ %d does for refnames). If there are multiple entries of that trailer\n+ they are shown comma separated. If there are no matching trailers\n+ nothing is displayed.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex 8ca29e9281..61ae34ced4 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1324,6 +1324,22 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t}\n \t}\n \n+\tif (skip_prefix(placeholder, \"(trailer:\", &arg)) {\n+\t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\t\topts.no_divider = 1;\n+\t\topts.only_trailers = 1;\n+\t\topts.unfold = 1;\n+\n+\t\tconst char *end = strchr(arg, ')');\n+\t\tif (!end)\n+\t\t\treturn 0;\n+\n+\t\topts.filter_trailer = xstrndup(arg, end - arg);\n+\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n+\t\tfree(opts.filter_trailer);\n+\t\treturn end - placeholder + 1;\n+\t}\n+\n \treturn 0;\t/* unknown placeholder */\n }\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 978a8a66ff..e929f820e7 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -598,6 +598,46 @@ test_expect_success ':only and :unfold work together' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailer:foo) shows that trailer' '\n+\tgit log --no-walk --pretty=\"%(trailer:Acked-By)\" >actual &&\n+\t{\n+\t\techo \"(Acked-By: A U Thor )\"\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailer:nonexistant) becomes empty' '\n+\tgit log --no-walk --pretty=\"x%(trailer:Nacked-By)x\" >actual &&\n+\t{\n+\t\techo \"xx\"\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '% (trailer:nonexistant) with space becomes empty' '\n+\tgit log --no-walk --pretty=\"x% (trailer:Nacked-By)x\" >actual &&\n+\t{\n+\t\techo \"xx\"\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '% (trailer:foo) with space adds space before' '\n+\tgit log --no-walk --pretty=\"x% (trailer:Acked-By)x\" >actual &&\n+\t{\n+\t\techo \"x (Acked-By: A U Thor )x\"\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailer:foo) with multiple lines becomes comma separated and unwrapped' '\n+\tgit log --no-walk --pretty=\"%(trailer:Signed-Off-By)\" >actual &&\n+\t{\n+\t\techo \"(Signed-Off-By: A U Thor , A U Thor )\"\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex 0796f326b3..d337bca8dd 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1138,6 +1138,7 @@ static void format_trailer_info(struct strbuf *out,\n \t\treturn;\n \t}\n \n+\tint printed_first = 0;\n \tfor (i = 0; i < info->trailer_nr; i++) {\n \t\tchar *trailer = info->trailers[i];\n \t\tssize_t separator_pos = find_separator(trailer, separators);\n@@ -1150,7 +1151,19 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tif (opts->unfold)\n \t\t\t\tunfold_value(&val);\n \n-\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\tif (opts->filter_trailer) {\n+\t\t\t\tif (!strcasecmp (tok.buf, opts->filter_trailer)) {\n+\t\t\t\t\tif (!printed_first) {\n+\t\t\t\t\t\tstrbuf_addf(out, \"(%s: \", opts->filter_trailer);\n+\t\t\t\t\t\tprinted_first = 1;\n+\t\t\t\t\t} else {\n+\t\t\t\t\t\tstrbuf_addstr(out, \", \");\n+\t\t\t\t\t}\n+\t\t\t\t\tstrbuf_addstr(out, val.buf);\n+\t\t\t\t}\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\n \n@@ -1158,7 +1171,8 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tstrbuf_addstr(out, trailer);\n \t\t}\n \t}\n-\n+\tif (printed_first)\n+\t\tstrbuf_addstr(out, \")\");\n }\n \n void format_trailers_from_commit(struct strbuf *out, const char *msg,\ndiff --git a/trailer.h b/trailer.h\nindex b997739649..852c79d449 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -72,6 +72,7 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tchar *filter_trailer;\n };\n \n #define PROCESS_TRAILER_OPTIONS_INIT {0}\n-- \n2.17.1\n\n"},{"id":"361871","messageId":"xmqqo9bd5pcx.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"20181028125025.30952-1-anders@0x63.nu","subject":"Re: [PATCH] pretty: Add %(trailer:X) to display single trailer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-29T04:49:34Z","receivedAt":"2018-10-29T04:49:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> This new format placeholder allows displaying only a single\n> trailer. The formatting done is similar to what is done for\n> --decorate/%d using parentheses and comma separation.\n>\n> It's intended use is for things like ticket references in trailers.\n>\n> So with a commit with a message like:\n>\n> > Some good commit\n> >\n> > Ticket: XYZ-123\n>\n> running:\n>\n> $ git log --pretty=\"%H %s% (trailer:Ticket)\"\n>\n> will give:\n>\n> > 123456789a Some good commit (Ticket: XYZ-123)\n\nSounds useful, but a few questions off the top of my head are:\n\n - How would this work together with existing %(trailers:...)?\n\n - Can't this be made to a new option, in addition to existing\n 'only' and 'unfold', to existing %(trailer:...)? If not, what\n are the missing pieces that we need to add in order to make that\n possible?\n\nThe latter is especially true as from the surface, it smell like\nthat the whole reason why this patch introduces a new placeholder\nwith confusingly simliar name is because the patch did not bother to\nthink of a way to make it fit there as an enhancement of it.\n\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index 6109ef09aa..a46d0c0717 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -211,6 +211,10 @@ endif::git-rev-list[]\n> If the `unfold` option is given, behave as if interpret-trailer's\n> `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do\n> both.\n> +- %(trailer:): display the specified trailer in parentheses (like\n> + %d does for refnames). If there are multiple entries of that trailer\n> + they are shown comma separated. If there are no matching trailers\n> + nothing is displayed.\n\n\nAs this list is sorted roughly alphabetically for short ones, I\nthink it is better to keep that order for the longer ones that begin\nwith \"%(\". This should be instead inserted before the description\nfor the existing \"%(trailers[:options])\".\n\nAssuming that we want this %(trailer) separate from %(trailers),\nthat is, of course.\n\n> diff --git a/pretty.c b/pretty.c\n> index 8ca29e9281..61ae34ced4 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1324,6 +1324,22 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> \t\t}\n> \t}\n> \n> +\tif (skip_prefix(placeholder, \"(trailer:\", &arg)) {\n> +\t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n> +\t\topts.no_divider = 1;\n> +\t\topts.only_trailers = 1;\n> +\t\topts.unfold = 1;\n\nThis makes me suspect that it would be very nice if this is\nimplemented as a new \"option\" to the existing \"%(trailers[:option])\"\nthing. It does mostly identical thing as the existing code.\n\n> +\t\tconst char *end = strchr(arg, ')');\n\nAvoid decl-after-statement.\n\n> +\t\tif (!end)\n> +\t\t\treturn 0;\n> +\n> +\t\topts.filter_trailer = xstrndup(arg, end - arg);\n> +\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n> +\t\tfree(opts.filter_trailer);\n> +\t\treturn end - placeholder + 1;\n> +\t}\n> +\n> \treturn 0;\t/* unknown placeholder */\n> }\n> \n> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n> index 978a8a66ff..e929f820e7 100755\n> --- a/t/t4205-log-pretty-formats.sh\n> +++ b/t/t4205-log-pretty-formats.sh\n> @@ -598,6 +598,46 @@ test_expect_success ':only and :unfold work together' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'pretty format %(trailer:foo) shows that trailer' '\n> +\tgit log --no-walk --pretty=\"%(trailer:Acked-By)\" >actual &&\n> +\t{\n> +\t\techo \"(Acked-By: A U Thor )\"\n> +\t} >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success '%(trailer:nonexistant) becomes empty' '\n> +\tgit log --no-walk --pretty=\"x%(trailer:Nacked-By)x\" >actual &&\n> +\t{\n> +\t\techo \"xx\"\n> +\t} >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success '% (trailer:nonexistant) with space becomes empty' '\n> +\tgit log --no-walk --pretty=\"x% (trailer:Nacked-By)x\" >actual &&\n> +\t{\n> +\t\techo \"xx\"\n> +\t} >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success '% (trailer:foo) with space adds space before' '\n> +\tgit log --no-walk --pretty=\"x% (trailer:Acked-By)x\" >actual &&\n> +\t{\n> +\t\techo \"x (Acked-By: A U Thor )x\"\n> +\t} >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nThese are both good positive-negative pairs of tests.\n\n> +test_expect_success '%(trailer:foo) with multiple lines becomes comma separated and unwrapped' '\n> +\tgit log --no-walk --pretty=\"%(trailer:Signed-Off-By)\" >actual &&\n> +\t{\n> +\t\techo \"(Signed-Off-By: A U Thor , A U Thor )\"\n> +\t} >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nThis also tells me that it is a bad design to add this as a separate\nnew feature that takes the trailer key as an end-user suppied value.\nThere is no way to extend this to other needs, such as \"do similar\nthing as %(trailer:foo) does by default, but do not unwrap; give two\nor more 'Signed-off-by:' separately)\".\n\nI wonder why something like %(trailers:comma,token=foo) were not\nconsidered. %(trailers:only,token=foo,token=bar) might even be a good\nway to grab only Foo: and Bar: trailers in the order they appear in\nthe original commit, filtering out all the other trailers and non-trailer\ntext in the log message.\n\n> diff --git a/trailer.c b/trailer.c\n> index 0796f326b3..d337bca8dd 100644\n> --- a/trailer.c\n> +++ b/trailer.c\n> @@ -1138,6 +1138,7 @@ static void format_trailer_info(struct strbuf *out,\n> \t\treturn;\n> \t}\n> \n> +\tint printed_first = 0;\n\ndecl-afer-stmt.\n\n> \tfor (i = 0; i < info->trailer_nr; i++) {\n> \t\tchar *trailer = info->trailers[i];\n> \t\tssize_t separator_pos = find_separator(trailer, separators);\n> @@ -1150,7 +1151,19 @@ static void format_trailer_info(struct strbuf *out,\n> \t\t\tif (opts->unfold)\n> \t\t\t\tunfold_value(&val);\n> \n> -\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n> +\t\t\tif (opts->filter_trailer) {\n> +\t\t\t\tif (!strcasecmp (tok.buf, opts->filter_trailer)) {\n> +\t\t\t\t\tif (!printed_first) {\n> +\t\t\t\t\t\tstrbuf_addf(out, \"(%s: \", opts->filter_trailer);\n> +\t\t\t\t\t\tprinted_first = 1;\n> +\t\t\t\t\t} else {\n> +\t\t\t\t\t\tstrbuf_addstr(out, \", \");\n> +\t\t\t\t\t}\n> +\t\t\t\t\tstrbuf_addstr(out, val.buf);\n> +\t\t\t\t}\n> +\t\t\t} else {\n> +\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n> +\t\t\t}\n> \t\t\tstrbuf_release(&tok);\n> \t\t\tstrbuf_release(&val);\n> \n> @@ -1158,7 +1171,8 @@ static void format_trailer_info(struct strbuf *out,\n> \t\t\tstrbuf_addstr(out, trailer);\n> \t\t}\n> \t}\n> -\n> +\tif (printed_first)\n> +\t\tstrbuf_addstr(out, \")\");\n> }\n> \n> void format_trailers_from_commit(struct strbuf *out, const char *msg,\n> diff --git a/trailer.h b/trailer.h\n> index b997739649..852c79d449 100644\n> --- a/trailer.h\n> +++ b/trailer.h\n> @@ -72,6 +72,7 @@ struct process_trailer_options {\n> \tint only_input;\n> \tint unfold;\n> \tint no_divider;\n> +\tchar *filter_trailer;\n> };\n> \n> #define PROCESS_TRAILER_OPTIONS_INIT {0}\n"},{"id":"361897","messageId":"20181029141402.GA17668@sigill.intra.peff.net","threadId":"49703","inReplyTo":"20181028125025.30952-1-anders@0x63.nu","subject":"Re: [PATCH] pretty: Add %(trailer:X) to display single trailer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-29T14:14:03Z","receivedAt":"2018-10-29T14:14:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 28, 2018 at 01:50:25PM +0100, Anders Waldenborg wrote:\n\n> This new format placeholder allows displaying only a single\n> trailer. The formatting done is similar to what is done for\n> --decorate/%d using parentheses and comma separation.\n\nDisplaying a single trailer makes sense as a goal. It was one of the\nthings I considered when working on %(trailers), actually, but I ended\nup needing something a bit more flexible (hence the ability to dump the\ntrailers in a parse-able format, where I feed them to another script).\nBut your ticket example makes sense for just ordinary log displays.\n\nJunio's review already covered my biggest question, which is why not\nsomething like \"%(trailers:key=ticket)\". And likewise making things like\ncomma-separation options.\n\nBut my second question is whether we want to provide something more\nflexible than the always-parentheses that \"%d\" provides. That has been a\nproblem in the past when people want to format the decoration in some\nother way.\n\nWe have formatting magic for \"if this thing is non-empty, then show this\nprefix\" in the for-each-ref formatter, but I'm not sure that we do in\nthe commit pretty-printer beyond \"% \". I wonder if we could/should add a\na placeholder for \"if this thing is non-empty, put in a space and\nenclose it in parentheses\".\n\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> index 6109ef09aa..a46d0c0717 100644\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -211,6 +211,10 @@ endif::git-rev-list[]\n> If the `unfold` option is given, behave as if interpret-trailer's\n> `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do\n> both.\n> +- %(trailer:): display the specified trailer in parentheses (like\n> + %d does for refnames). If there are multiple entries of that trailer\n> + they are shown comma separated. If there are no matching trailers\n> + nothing is displayed.\n\nIt might be worth specifying how this match is done. I'm thinking\nspecifically of whether it's case-sensitive, but I wonder if there\nshould be any allowance for other normalization (e.g., allowing a regex\nto match \"coauthored-by\" and \"co-authored-by\" or something).\n\n-Peff\n"},{"id":"361949","messageId":"CADsOX3Cbn7jjqFERptxMm59mn0qYnkf9bmFvJS20VBPedZHwqQ@mail.gmail.com","threadId":"49703","inReplyTo":"20181029141402.GA17668@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: Add %(trailer:X) to display single trailer","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-10-29T17:05:34Z","receivedAt":"2018-10-29T23:02:11Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nOn Mon, Oct 29, 2018 at 3:14 PM Jeff King wrote:\n> Junio's review already covered my biggest question, which is why not\n> something like \"%(trailers:key=ticket)\". And likewise making things like\n> comma-separation options.\n\nJeff, Junio,\n\nthanks!\n\nYour questions pretty much matches what I (and a colleague I discussed\nthis with before posting) was concerned about.\n\nMy first try actually had it as an option to \"trailers\". But it got a\nbit messy with the argument parsing, and the fact that there was a\nfast path making it work when only specified. I did not want to spend\nlot of time reworking fixing that before I had some feedback, so I\nwent for a smallest possible patch to float the idea with (a patch is\nworth a 1000 words).\n\nI'll start by reworking my patch to handle %(trailers:key=X) (I'll\nassume keys never contain ')' or ','), and ignore any formatting until\nthe way forward there is decided (see below).\n\n> But my second question is whether we want to provide something more\n> flexible than the always-parentheses that \"%d\" provides. That has been a\n> problem in the past when people want to format the decoration in some\n> other way.\n\nMaybe just like +/-/space can be used directly after %, a () pair can\nbe allowed.. E.g \"%d\" would just be an alias for \"%()D\", and for\ntrailers it would be something like \"%()(trailers:key=foo)\"\n\nThere is another special cased placeholder %f (sanitized subject line,\nsuitable for a filename). Which also could be changed to be a format\nspecifiier, allowing sanitize any thing, e.g \"%!an\" for sanitized\nauthor name.\n\nIs even the linebreak to commaseparation a generic thing?\n\"% ,()(trailers:key=Ticket)\" it starts go look a bit silly.\n\nThen there are the padding modifiers. %<() %<|(). They operate on next\nplaceholder. \"%<(10)%s\" Is that a better syntax?\n\"%()%(trailers:key=Ticket,comma)\"\n\nI can also imagine moving all these modifiers into a generic modifier\nsyntax in brackets (and keeping old for backwards compat)\n%[lpad=10,ltrunc=10]s == %<(10,trunc)%s\n%[nonempty-prefix=\"%n\"]GS == %+GS\n%[nonempty-prefix=\" (\",nonempty-suffix=\")\"]D == %d\nWhich would mean something like this for tickets thing:\n%[nonempty-prefix=\" (Tickets:\",nonempty-suffix=\")\",commaseparatelines](trailers:key=Ticket,nokey)\nwhich is kinda verbose.\n\n> We have formatting magic for \"if this thing is non-empty, then show this\n> prefix\" in the for-each-ref formatter, but I'm not sure that we do in\n> the commit pretty-printer beyond \"% \". I wonder if we could/should add a\n> a placeholder for \"if this thing is non-empty, put in a space and\n> enclose it in parentheses\".\n\nWould there be any interest in consolidating those formatters? Even\nthough they are totally separate beasts today. I think having all\nattributes available on long form (e.g \"%(authorname)\") in addition to\nexisting short forms in pretty-formatter would make sense.\n\n anders\n"},{"id":"362128","messageId":"20181031202708.GA13021@sigill.intra.peff.net","threadId":"49703","inReplyTo":"CADsOX3Cbn7jjqFERptxMm59mn0qYnkf9bmFvJS20VBPedZHwqQ@mail.gmail.com","subject":"Re: [PATCH] pretty: Add %(trailer:X) to display single trailer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-31T20:27:08Z","receivedAt":"2018-10-31T20:27:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 29, 2018 at 06:05:34PM +0100, Anders Waldenborg wrote:\n\n> I'll start by reworking my patch to handle %(trailers:key=X) (I'll\n> assume keys never contain ')' or ','), and ignore any formatting until\n> the way forward there is decided (see below).\n\nIMHO that is probably an acceptable tradeoff. We haven't really made any\nrules for quoting arbitrary values in other %() sequences. I think it's\nsomething we may want to have eventually, but as long as the rule for\nnow is \"you can't do that\", I think it would be OK to loosen it later.\n\n> > But my second question is whether we want to provide something more\n> > flexible than the always-parentheses that \"%d\" provides. That has been a\n> > problem in the past when people want to format the decoration in some\n> > other way.\n> \n> Maybe just like +/-/space can be used directly after %, a () pair can\n> be allowed.. E.g \"%d\" would just be an alias for \"%()D\", and for\n> trailers it would be something like \"%()(trailers:key=foo)\"\n\nYeah, I was thinking that \"(\" was taken as a special character, but I\nguess immediately followed by \")\" it is easy to parse left-to-right with\nno ambiguity.\n\nWould it include the leading space, too? It would be nice if it could be\ncombined with \"% \" in an orthogonal way. I guess in theory \"% ()D\" would\nwork, but it may need some tweaks to the parsing.\n\n> There is another special cased placeholder %f (sanitized subject line,\n> suitable for a filename). Which also could be changed to be a format\n> specifiier, allowing sanitize any thing, e.g \"%!an\" for sanitized\n> author name.\n\nYeah, I agree we should be able to sanitize anything. It's not strictly\nrelated to your patch, though, so you may or may not want to go down\nthis rabbit hole. :)\n\n> Is even the linebreak to commaseparation a generic thing?\n> \"% ,()(trailers:key=Ticket)\" it starts go look a bit silly.\n\nIn theory, yeah. I agree it's getting a bit magical.\n\n> Then there are the padding modifiers. %<() %<|(). They operate on next\n> placeholder. \"%<(10)%s\" Is that a better syntax?\n> \"%()%(trailers:key=Ticket,comma)\"\n> \n> I can also imagine moving all these modifiers into a generic modifier\n> syntax in brackets (and keeping old for backwards compat)\n> %[lpad=10,ltrunc=10]s == %<(10,trunc)%s\n> %[nonempty-prefix=\"%n\"]GS == %+GS\n> %[nonempty-prefix=\" (\",nonempty-suffix=\")\"]D == %d\n> Which would mean something like this for tickets thing:\n> %[nonempty-prefix=\" (Tickets:\",nonempty-suffix=\")\",commaseparatelines](trailers:key=Ticket,nokey)\n> which is kinda verbose.\n\nYes. I had dreams of eventually stuffing all of those as options into\nthe placeholders themselves. So \"%s\" would eventually have a long-form\nof \"%(subject)\", and in that syntax it could be:\n\n %(subject:lpad=10,filename)\n\nor something. I'm not completely opposed to:\n\n %[lpad=10,filename]%(subject)\n\nwhich keeps the \"formatting\" arguments out of the regular placeholders.\nOn the other hand, if the rule were not \"this affects the next\nplaceholder\" but had a true ending mark, then we could make a real\nparse-tree out of it, and format chunks of placeholders. E.g.:\n\n %(format:lpad=30,filename)%(subject) %(authordate)%(end)\n\nwould pad and format the whole string with two placeholders. I know that\ngoing down this road eventually involves reinventing XML, but I think\nhaving an actual tree structure may not be an unreasonable thing to\nshoot for.\n\nI dunno. You certainly don't need to solve all of these issues for what\nyou want to do. My main concern for now is to avoid introducing new\nsyntax that we'll be stuck with forever, even though it may later become\nredundant (or worse, create parsing ambiguities).\n\n> > We have formatting magic for \"if this thing is non-empty, then show this\n> > prefix\" in the for-each-ref formatter, but I'm not sure that we do in\n> > the commit pretty-printer beyond \"% \". I wonder if we could/should add a\n> > a placeholder for \"if this thing is non-empty, put in a space and\n> > enclose it in parentheses\".\n> \n> Would there be any interest in consolidating those formatters? Even\n> though they are totally separate beasts today. I think having all\n> attributes available on long form (e.g \"%(authorname)\") in addition to\n> existing short forms in pretty-formatter would make sense.\n\nYes, there's great interest. :)\n\nThe formats are not mutually incompatible at this point, so we should be\nable to come up with a unified language that maintains backwards\ncompatibility. One of the tricky parts is that some of the formatters\nhave more information than others (for-each-ref has a ref, which may\nresolve to any object type; cat-file has objects only; --pretty has only\ncommits).\n\nThis was the subject of last year's Outreachy work. There's still a ways\nto go, but you can find some of the previous discussions and work by\nsearching for Olga's work in the archive:\n\n https://public-inbox.org/git/?q=olga+telezhnaya\n\nI've also cc'd her here, as she's still been doing some work since then.\n\n-Peff\n"},{"id":"362136","messageId":"87a7mtlnzr.fsf@0x63.nu","threadId":"49703","inReplyTo":"20181031202708.GA13021@sigill.intra.peff.net","subject":"Re: [PATCH] pretty: Add %(trailer:X) to display single trailer","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-10-31T23:01:28Z","receivedAt":"2018-10-31T23:01:34Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJeff King writes:\n\n> On the other hand, if the rule were not \"this affects the next\n> placeholder\" but had a true ending mark, then we could make a real\n> parse-tree out of it, and format chunks of placeholders. E.g.:\n>\n> %(format:lpad=30,filename)%(subject) %(authordate)%(end)\n>\n> would pad and format the whole string with two placeholders. I know that\n> going down this road eventually involves reinventing XML, but I think\n> having an actual tree structure may not be an unreasonable thing to\n> shoot for.\n\nYes. I'm thinking that with [] for formatting specifiers and () for\nplaceholders, {} would be available for nesting. E.g:\n\n %[lpad=30,mangle]{%(subject) %ad%}\n\n\n> My main concern for now is to avoid introducing new\n> syntax that we'll be stuck with forever, even though it may later become\n> redundant (or worse, create parsing ambiguities).\n\nAgreed.\n\nI'm planning to work on the initial \"trailer:key=\" part later this\nweek. Maybe I can play around with different formatting options and see\nhow it affects the parser.\n"},{"id":"362204","messageId":"20181101184219.GA2918@sigill.intra.peff.net","threadId":"49703","inReplyTo":"87a7mtlnzr.fsf@0x63.nu","subject":"Re: [PATCH] pretty: Add %(trailer:X) to display single trailer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-01T18:42:19Z","receivedAt":"2018-11-01T18:42:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 01, 2018 at 12:01:28AM +0100, Anders Waldenborg wrote:\n\n> Jeff King writes:\n> \n> > On the other hand, if the rule were not \"this affects the next\n> > placeholder\" but had a true ending mark, then we could make a real\n> > parse-tree out of it, and format chunks of placeholders. E.g.:\n> >\n> > %(format:lpad=30,filename)%(subject) %(authordate)%(end)\n> >\n> > would pad and format the whole string with two placeholders. I know that\n> > going down this road eventually involves reinventing XML, but I think\n> > having an actual tree structure may not be an unreasonable thing to\n> > shoot for.\n> \n> Yes. I'm thinking that with [] for formatting specifiers and () for\n> placeholders, {} would be available for nesting. E.g:\n> \n> %[lpad=30,mangle]{%(subject) %ad%}\n\nHmm. That's kind of ugly, but probably not really any uglier than any of\nthe things I showed. And it has the advantage that we could implement\n%[] now, and later extend it (well, I guess we'd want to make sure that\n\"%[lpad=30]{foo}\" does not treat the curly braces literally, since we'd\neventually make them syntactically significant).\n\n> I'm planning to work on the initial \"trailer:key=\" part later this\n> week. Maybe I can play around with different formatting options and see\n> how it affects the parser.\n\nGreat! Thanks for working on this.\n\n-Peff\n"},{"id":"362394","messageId":"20181104152232.20671-1-anders@0x63.nu","threadId":"49703","inReplyTo":"20181028125025.30952-1-anders@0x63.nu","subject":"[PATCH v2 0/5] %(trailers) improvements in pretty format","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-04T15:22:27Z","receivedAt":"2018-11-04T15:23:54Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"This adds support for three new options to %(trailers):\n * key -- show only trailers with specified key\n * nokey -- don't show key part of trailers\n * separator -- allow specifying custom separator between trailers\n\n\nAnders Waldenborg (5):\n pretty: single return path in %(trailers) handling\n pretty: allow showing specific trailers\n pretty: add support for \"nokey\" option in %(trailers)\n pretty: extract fundamental placeholders to separate function\n pretty: add support for separator option in %(trailers)\n\n Documentation/pretty-formats.txt | 17 +++++---\n pretty.c | 71 ++++++++++++++++++++++++++------\n t/t4205-log-pretty-formats.sh | 60 +++++++++++++++++++++++++++\n trailer.c | 28 ++++++++++---\n trailer.h | 3 ++\n 5 files changed, 156 insertions(+), 23 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"362395","messageId":"20181104152232.20671-2-anders@0x63.nu","threadId":"49703","inReplyTo":"20181104152232.20671-1-anders@0x63.nu","subject":"[PATCH v2 1/5] pretty: single return path in %(trailers) handling","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-04T15:22:28Z","receivedAt":"2018-11-04T15:23:54Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"No functional change intended.\n\nThis change may not seem useful on its own, but upcoming commits will do\nmemory allocation in there, and a single return path makes deallocation\neasier.\n\nSigned-off-by: Anders Waldenborg \n---\n pretty.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex b83a3ecd2..aa03d5b23 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1312,6 +1312,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n \n@@ -1328,8 +1329,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t}\n \t\tif (*arg == ')') {\n \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n-\t\t\treturn arg - placeholder + 1;\n+\t\t\tret = arg - placeholder + 1;\n \t\t}\n+\t\treturn ret;\n \t}\n \n \treturn 0;\t/* unknown placeholder */\n-- \n2.17.1\n\n"},{"id":"362396","messageId":"20181104152232.20671-3-anders@0x63.nu","threadId":"49703","inReplyTo":"20181104152232.20671-1-anders@0x63.nu","subject":"[PATCH v2 2/5] pretty: allow showing specific trailers","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-04T15:22:29Z","receivedAt":"2018-11-04T15:24:33Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"Adds a new \"key=X\" option to \"%(trailers)\" which will cause it to only\nprint trailers lines which matches the specified key.\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 13 +++++----\n pretty.c | 15 ++++++++++-\n t/t4205-log-pretty-formats.sh | 45 ++++++++++++++++++++++++++++++++\n trailer.c | 8 +++---\n trailer.h | 1 +\n 5 files changed, 73 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 417b638cd..8326fc45e 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -209,11 +209,14 @@ endif::git-rev-list[]\n respectively, but padding both sides (i.e. the text is centered)\n - %(trailers[:options]): display the trailers of the body as interpreted\n by linkgit:git-interpret-trailers[1]. The `trailers` string may be\n- followed by a colon and zero or more comma-separated options. If the\n- `only` option is given, omit non-trailer lines from the trailer block.\n- If the `unfold` option is given, behave as if interpret-trailer's\n- `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do\n- both.\n+ followed by a colon and zero or more comma-separated options. The\n+ allowed options are `only` which omits non-trailer lines from the\n+ trailer block, `unfold` to make it behave as if interpret-trailer's\n+ `--unfold` option was given, and `key=T` to only show trailers with\n+ specified key (matching is done\n+ case-insensitively). E.g. `%(trailers:only,unfold)` unfolds and\n+ shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`\n+ unfolds and shows trailer lines with key `Reviewed-by`.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex aa03d5b23..cdca9dce2 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\topts.only_trailers = 1;\n \t\t\t\telse if (match_placeholder_arg(arg, \"unfold\", &arg))\n \t\t\t\t\topts.unfold = 1;\n-\t\t\t\telse\n+\t\t\t\telse if (skip_prefix(arg, \"key=\", &arg)) {\n+\t\t\t\t\tconst char *end = arg + strcspn(arg, \",)\");\n+\n+\t\t\t\t\tif (opts.filter_key)\n+\t\t\t\t\t\tfree(opts.filter_key);\n+\n+\t\t\t\t\topts.filter_key = xstrndup(arg, end - arg);\n+\t\t\t\t\targ = end;\n+\t\t\t\t\tif (*arg == ',')\n+\t\t\t\t\t\targ++;\n+\n+\t\t\t\t\topts.only_trailers = 1;\n+\t\t\t\t} else\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\n@@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n \t\t\tret = arg - placeholder + 1;\n \t\t}\n+\t\tfree(opts.filter_key);\n \t\treturn ret;\n \t}\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 978a8a66f..0f5207242 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '\n+\tgit log --no-walk --pretty=\"%(trailers:key=Acked-by)\" >actual &&\n+\t{\n+\t\techo \"Acked-by: A U Thor \" &&\n+\t\techo\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '\n+\tgit log --no-walk --pretty=\"%(trailers:key=AcKed-bY)\" >actual &&\n+\t{\n+\t\techo \"Acked-by: A U Thor \" &&\n+\t\techo\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=nonexistant) becomes empty' '\n+\tgit log --no-walk --pretty=\"x%(trailers:key=Nacked-by)x\" >actual &&\n+\t{\n+\t\techo \"xx\"\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '\n+\tgit log --no-walk --pretty=\"%(trailers:key=Signed-Off-by)\" >actual &&\n+\t{\n+\t\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '\n+\tgit log --no-walk --pretty=\"%(trailers:key=Signed-Off-by,unfold)\" >actual &&\n+\t{\n+\t\techo \"Signed-off-by: A U Thor \" &&\n+\t\techo \"Signed-off-by: A U Thor \" &&\n+\t\techo\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex 0796f326b..cbbb553e4 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tstruct strbuf val = STRBUF_INIT;\n \n \t\t\tparse_trailer(&tok, &val, NULL, trailer, separator_pos);\n-\t\t\tif (opts->unfold)\n-\t\t\t\tunfold_value(&val);\n+\t\t\tif (!opts->filter_key || !strcasecmp (tok.buf, opts->filter_key)) {\n+\t\t\t\tif (opts->unfold)\n+\t\t\t\t\tunfold_value(&val);\n \n-\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\n \ndiff --git a/trailer.h b/trailer.h\nindex b99773964..d052d02ae 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -72,6 +72,7 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tchar *filter_key;\n };\n \n #define PROCESS_TRAILER_OPTIONS_INIT {0}\n-- \n2.17.1\n\n"},{"id":"362397","messageId":"20181104152232.20671-4-anders@0x63.nu","threadId":"49703","inReplyTo":"20181104152232.20671-1-anders@0x63.nu","subject":"[PATCH v2 3/5] pretty: add support for \"nokey\" option in %(trailers)","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-04T15:22:30Z","receivedAt":"2018-11-04T15:24:35Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"With the new \"key=\" option to %trailers it often makes little sense to\nshow the key, as it by definition already is know which trailer is\nprinted there. This new \"nokey\" option makes it omit key trailer key\nwhen printing trailers.\n\nE.g.:\n $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,nokey)' aaaa88182\nwill show:\n > upload-pack: fix broken if/else chain in config callback\n > Jeff King \n > Junio C Hamano \n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 11 ++++++-----\n pretty.c | 2 ++\n t/t4205-log-pretty-formats.sh | 9 +++++++++\n trailer.c | 6 ++++--\n trailer.h | 1 +\n 5 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 8326fc45e..e115e355d 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -212,11 +212,12 @@ endif::git-rev-list[]\n followed by a colon and zero or more comma-separated options. The\n allowed options are `only` which omits non-trailer lines from the\n trailer block, `unfold` to make it behave as if interpret-trailer's\n- `--unfold` option was given, and `key=T` to only show trailers with\n- specified key (matching is done\n- case-insensitively). E.g. `%(trailers:only,unfold)` unfolds and\n- shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`\n- unfolds and shows trailer lines with key `Reviewed-by`.\n+ `--unfold` option was given, `key=T` to only show trailers with\n+ specified key (matching is done case-insensitively), and `nokey`\n+ which makes it skip over the key part of the trailer and only show\n+ value. E.g. `%(trailers:only,unfold)` unfolds and shows all trailer\n+ lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows\n+ trailer lines with key `Reviewed-by`.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex cdca9dce2..f87ba4f18 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1323,6 +1323,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\topts.only_trailers = 1;\n \t\t\t\telse if (match_placeholder_arg(arg, \"unfold\", &arg))\n \t\t\t\t\topts.unfold = 1;\n+\t\t\t\telse if (match_placeholder_arg(arg, \"nokey\", &arg))\n+\t\t\t\t\topts.no_key = 1;\n \t\t\t\telse if (skip_prefix(arg, \"key=\", &arg)) {\n \t\t\t\t\tconst char *end = arg + strcspn(arg, \",)\");\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 0f5207242..e7de3b18a 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -643,6 +643,15 @@ test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:key=foo,nokey) shows only value' '\n+\tgit log --no-walk --pretty=\"%(trailers:key=Acked-by,nokey)\" >actual &&\n+\t{\n+\t\techo \"A U Thor \" &&\n+\t\techo\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex cbbb553e4..4f19c34cb 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tif (!opts->filter_key || !strcasecmp (tok.buf, opts->filter_key)) {\n \t\t\t\tif (opts->unfold)\n \t\t\t\t\tunfold_value(&val);\n-\n-\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t\tif (opts->no_key)\n+\t\t\t\t\tstrbuf_addf(out, \"%s\\n\", val.buf);\n+\t\t\t\telse\n+\t\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n \t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\ndiff --git a/trailer.h b/trailer.h\nindex d052d02ae..83de87ee9 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -72,6 +72,7 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tint no_key;\n \tchar *filter_key;\n };\n \n-- \n2.17.1\n\n"},{"id":"362398","messageId":"20181104152232.20671-5-anders@0x63.nu","threadId":"49703","inReplyTo":"20181104152232.20671-1-anders@0x63.nu","subject":"[PATCH v2 4/5] pretty: extract fundamental placeholders to separate function","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-04T15:22:31Z","receivedAt":"2018-11-04T15:24:35Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"No functional change intended\n\nSigned-off-by: Anders Waldenborg \n---\n pretty.c | 37 ++++++++++++++++++++++++++-----------\n 1 file changed, 26 insertions(+), 11 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex f87ba4f18..9fdddce9d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1074,6 +1074,27 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,\n \treturn 0;\n }\n \n+static size_t format_fundamental(struct strbuf *sb, /* in UTF-8 */\n+\t\t\t\t const char *placeholder,\n+\t\t\t\t void *context)\n+{\n+\tint ch;\n+\n+\tswitch (placeholder[0]) {\n+\tcase 'n':\t\t/* newline */\n+\t\tstrbuf_addch(sb, '\\n');\n+\t\treturn 1;\n+\tcase 'x':\n+\t\t/* %x00 == NUL, %x0a == LF, etc. */\n+\t\tch = hex2chr(placeholder + 1);\n+\t\tif (ch < 0)\n+\t\t\treturn 0;\n+\t\tstrbuf_addch(sb, ch);\n+\t\treturn 3;\n+\t}\n+\treturn 0;\n+}\n+\n static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\tconst char *placeholder,\n \t\t\t\tvoid *context)\n@@ -1083,9 +1104,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tconst char *msg = c->message;\n \tstruct commit_list *p;\n \tconst char *arg;\n-\tint ch;\n+\tsize_t res;\n \n \t/* these are independent of the commit */\n+\tres = format_fundamental(sb, placeholder, NULL);\n+\tif (res)\n+\t\treturn res;\n+\n \tswitch (placeholder[0]) {\n \tcase 'C':\n \t\tif (starts_with(placeholder + 1, \"(auto)\")) {\n@@ -1104,16 +1129,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t */\n \t\t\treturn ret;\n \t\t}\n-\tcase 'n':\t\t/* newline */\n-\t\tstrbuf_addch(sb, '\\n');\n-\t\treturn 1;\n-\tcase 'x':\n-\t\t/* %x00 == NUL, %x0a == LF, etc. */\n-\t\tch = hex2chr(placeholder + 1);\n-\t\tif (ch < 0)\n-\t\t\treturn 0;\n-\t\tstrbuf_addch(sb, ch);\n-\t\treturn 3;\n \tcase 'w':\n \t\tif (placeholder[1] == '(') {\n \t\t\tunsigned long width = 0, indent1 = 0, indent2 = 0;\n-- \n2.17.1\n\n"},{"id":"362399","messageId":"20181104152232.20671-6-anders@0x63.nu","threadId":"49703","inReplyTo":"20181104152232.20671-1-anders@0x63.nu","subject":"[PATCH v2 5/5] pretty: add support for separator option in %(trailers)","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-04T15:22:32Z","receivedAt":"2018-11-04T15:24:40Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"By default trailer lines are terminated by linebreaks ('\\n'). By\nspecifying the new 'separator' option they will instead be separated by\nuser provided string and have separator semantics rather than terminator\nsemantics. The separator string can contain the fundamental formatting\ncodes %n and %xNN allowing it to be things that are otherwise hard to\ntype as %x00, or command and end-parenthesis which would break parsing.\n\nE.g:\n $ git log --pretty='%(trailers:key=Reviewed-by,nokey,separator=%x00)'\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 13 ++++++++-----\n pretty.c | 13 +++++++++++++\n t/t4205-log-pretty-formats.sh | 6 ++++++\n trailer.c | 20 +++++++++++++++++---\n trailer.h | 1 +\n 5 files changed, 45 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex e115e355d..3312850e6 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -213,11 +213,14 @@ endif::git-rev-list[]\n allowed options are `only` which omits non-trailer lines from the\n trailer block, `unfold` to make it behave as if interpret-trailer's\n `--unfold` option was given, `key=T` to only show trailers with\n- specified key (matching is done case-insensitively), and `nokey`\n- which makes it skip over the key part of the trailer and only show\n- value. E.g. `%(trailers:only,unfold)` unfolds and shows all trailer\n- lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows\n- trailer lines with key `Reviewed-by`.\n+ specified key (matching is done case-insensitively), `nokey` which\n+ makes it skip over the key part of the trailer and only show value\n+ and `separator` which allows specifying an alternative separator\n+ than the default line\n+ break. E.g. `%(trailers:only,unfold,separator=%x00)` unfolds and\n+ shows all trailer lines separated by NUL character,\n+ `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer lines\n+ with key `Reviewed-by`.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex 9fdddce9d..f73a2b0dc 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1327,6 +1327,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\t\tstruct strbuf sepbuf = STRBUF_INIT;\n \t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n@@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\t\targ++;\n \n \t\t\t\t\topts.only_trailers = 1;\n+\t\t\t\t} else if (skip_prefix(arg, \"separator=\", &arg)) {\n+\t\t\t\t\tsize_t seplen = strcspn(arg, \",)\");\n+\t\t\t\t\tstrbuf_reset(&sepbuf);\n+\t\t\t\t\tchar *fmt = xstrndup(arg, seplen);\n+\t\t\t\t\tstrbuf_expand(&sepbuf, fmt, format_fundamental, NULL);\n+\t\t\t\t\tfree(fmt);\n+\t\t\t\t\topts.separator = &sepbuf;\n+\n+\t\t\t\t\targ += seplen;\n+\t\t\t\t\tif (*arg == ',')\n+\t\t\t\t\t\targ++;\n \t\t\t\t} else\n \t\t\t\t\tbreak;\n \t\t\t}\n@@ -1360,6 +1372,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n \t\t\tret = arg - placeholder + 1;\n \t\t}\n+\t\tstrbuf_release (&sepbuf);\n \t\tfree(opts.filter_key);\n \t\treturn ret;\n \t}\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex e7de3b18a..71218d22e 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -652,6 +652,12 @@ test_expect_success '%(trailers:key=foo,nokey) shows only value' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:separator) changes separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n+\tprintf \"XSigned-off-by: A U Thor \\0Acked-by: A U Thor \\0[ v2 updated patch description ]\\0Signed-off-by: A U Thor X\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex 4f19c34cb..a79e4e36a 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\tconst struct trailer_info *info,\n \t\t\t\tconst struct process_trailer_options *opts)\n {\n+\tint first_printed = 0;\n \tsize_t i;\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n-\tif (!opts->only_trailers && !opts->unfold) {\n+\tif (!opts->only_trailers && !opts->unfold && !opts->separator) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1150,16 +1151,29 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tif (!opts->filter_key || !strcasecmp (tok.buf, opts->filter_key)) {\n \t\t\t\tif (opts->unfold)\n \t\t\t\t\tunfold_value(&val);\n+\t\t\t\tif (opts->separator && first_printed)\n+\t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (opts->no_key)\n-\t\t\t\t\tstrbuf_addf(out, \"%s\\n\", val.buf);\n+\t\t\t\t\tstrbuf_addf(out, \"%s\", val.buf);\n \t\t\t\telse\n-\t\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t\t\tstrbuf_addf(out, \"%s: %s\", tok.buf, val.buf);\n+\t\t\t\tif (!opts->separator)\n+\t\t\t\t\tstrbuf_addch(out, '\\n');\n+\n+\t\t\t\tfirst_printed = 1;\n \t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\n \n \t\t} else if (!opts->only_trailers) {\n+\t\t\tif (opts->separator && first_printed) {\n+\t\t\t\tstrbuf_addbuf(out, opts->separator);\n+\t\t\t}\n \t\t\tstrbuf_addstr(out, trailer);\n+\t\t\tif (opts->separator) {\n+\t\t\t\tstrbuf_rtrim(out);\n+\t\t\t}\n+\t\t\tfirst_printed = 1;\n \t\t}\n \t}\n \ndiff --git a/trailer.h b/trailer.h\nindex 83de87ee9..0e9d89660 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -74,6 +74,7 @@ struct process_trailer_options {\n \tint no_divider;\n \tint no_key;\n \tchar *filter_key;\n+\tconst struct strbuf *separator;\n };\n \n #define PROCESS_TRAILER_OPTIONS_INIT {0}\n-- \n2.17.1\n\n"},{"id":"362405","messageId":"CAPig+cSfwUJ8thYW+dq1qjT8X_f78DzAzfb_Xd3CMxO=9juz=w@mail.gmail.com","threadId":"49703","inReplyTo":"20181104152232.20671-1-anders@0x63.nu","subject":"Re: [PATCH v2 0/5] %(trailers) improvements in pretty format","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-04T17:40:12Z","receivedAt":"2018-11-04T17:42:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 4, 2018 at 10:23 AM Anders Waldenborg wrote:\n> This adds support for three new options to %(trailers):\n> * key -- show only trailers with specified key\n> * nokey -- don't show key part of trailers\n> * separator -- allow specifying custom separator between trailers\n\nIf \"key\" is for including particular trailers, intuition might lead\npeople to think that \"nokey\" is for excluding certain trailers.\nPerhaps a different name for \"nokey\", such as \"valueonly\" or\n\"stripkey\", would be better.\n"},{"id":"362409","messageId":"CAPig+cQeUxxvgNGVc_iZ4v0U77obFu6-q0QbtzTJdnEep8eq+Q@mail.gmail.com","threadId":"49703","inReplyTo":"20181104152232.20671-3-anders@0x63.nu","subject":"Re: [PATCH v2 2/5] pretty: allow showing specific trailers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-04T18:14:34Z","receivedAt":"2018-11-04T18:14:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 4, 2018 at 10:24 AM Anders Waldenborg wrote:\n> Adds a new \"key=X\" option to \"%(trailers)\" which will cause it to only\n> print trailers lines which matches the specified key.\n>\n> Signed-off-by: Anders Waldenborg \n> ---\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> @@ -209,11 +209,14 @@ endif::git-rev-list[]\n> - %(trailers[:options]): display the trailers of the body as interpreted\n> by linkgit:git-interpret-trailers[1]. The `trailers` string may be\n> + followed by a colon and zero or more comma-separated options. The\n> + allowed options are `only` which omits non-trailer lines from the\n> + trailer block, `unfold` to make it behave as if interpret-trailer's\n> + `--unfold` option was given, and `key=T` to only show trailers with\n> + specified key (matching is done\n> + case-insensitively).\n\nDoes the user have to include the colon when specifying of\n'key='? I can see from peeking at the implementation that the\ncolon must not be used, but this should be documented. Should the code\ntolerate a trailing colon? (Genuine question; it's easy to do and\nwould be more user-friendly.)\n\nDoes 'key=', do a full or partial match on trailers? And, if\npartial, is the match anchored at the start or can it match anywhere\nin the trailer key? I see from the implementation that it does a full\nmatch, but this behavior should be documented.\n\nWhat happens if 'key=...' is specified multiple times? Are the\nmultiple keys conjunctive? Disjunctive? Last-wins? I can see from the\nimplementation that it is last-wins, but this behavior should be\ndocumented. (I wonder how painful it will be for people who want to\nmatch multiple keys. This doesn't have to be answered yet, as the\nbehavior can always be loosened later to allow multiple-key matching\nsince the current syntax does not disallow such expansion.)\n\nThinking further on the last two points, should be a regular expression?\n\n> + shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`\n> + unfolds and shows trailer lines with key `Reviewed-by`.\n> diff --git a/pretty.c b/pretty.c\n> @@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> + opts.filter_key = xstrndup(arg, end - arg);\n> @@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> format_trailers_from_commit(sb, msg + c->subject_off, &opts);\n> }\n> + free(opts.filter_key);\n\nIf I understand correctly, this is making a copy of so that it\nwill be NUL-terminated since the code added to trailer.c uses a simple\nstrcasecmp() to match it. Would it make sense to avoid the copy by\nadding fields 'opts.filter_key' and 'opts.filter_key_len' and using\nstrncasecmp() instead? (Genuine question; not necessarily a request\nfor change.)\n\n> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '\n> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '\n> + git log --no-walk --pretty=\"%(trailers:key=Acked-by)\" >actual &&\n> + {\n> + echo \"Acked-by: A U Thor \" &&\n> + echo\n> + } >expect &&\n> + test_cmp expect actual\n> +'\n\nI guess these new tests are modeled after one or two existing tests\nwhich use a series of 'echo' statements, but an alternative would be:\n\n cat <<-\\EOF >expect &&\n Acked-by: A U Thor \n\n EOF\n\nor, even:\n\n test_write_lines \"Acked-by: A U Thor \" \"\" &&\n\nthough, that's probably less readable.\n"},{"id":"362443","messageId":"xmqqtvkwjn1k.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"20181104152232.20671-5-anders@0x63.nu","subject":"Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-05T02:06:15Z","receivedAt":"2018-11-05T02:06:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> No functional change intended\n>\n> Signed-off-by: Anders Waldenborg \n> ---\n> pretty.c | 37 ++++++++++++++++++++++++++-----------\n> 1 file changed, 26 insertions(+), 11 deletions(-)\n\nI do not think \"fundamental\" is the best name for this, but I agree\nthat it would be useful to split the helpers into one that is\n\"constant across commits\" and the other one that is \"per commit\".\n\n> diff --git a/pretty.c b/pretty.c\n> index f87ba4f18..9fdddce9d 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1074,6 +1074,27 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,\n> \treturn 0;\n> }\n> \n> +static size_t format_fundamental(struct strbuf *sb, /* in UTF-8 */\n> +\t\t\t\t const char *placeholder,\n> +\t\t\t\t void *context)\n> +{\n> +\tint ch;\n> +\n> +\tswitch (placeholder[0]) {\n> +\tcase 'n':\t\t/* newline */\n> +\t\tstrbuf_addch(sb, '\\n');\n> +\t\treturn 1;\n> +\tcase 'x':\n> +\t\t/* %x00 == NUL, %x0a == LF, etc. */\n> +\t\tch = hex2chr(placeholder + 1);\n> +\t\tif (ch < 0)\n> +\t\t\treturn 0;\n> +\t\tstrbuf_addch(sb, ch);\n> +\t\treturn 3;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> \t\t\t\tconst char *placeholder,\n> \t\t\t\tvoid *context)\n> @@ -1083,9 +1104,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> \tconst char *msg = c->message;\n> \tstruct commit_list *p;\n> \tconst char *arg;\n> -\tint ch;\n> +\tsize_t res;\n> \n> \t/* these are independent of the commit */\n> +\tres = format_fundamental(sb, placeholder, NULL);\n> +\tif (res)\n> +\t\treturn res;\n> +\n> \tswitch (placeholder[0]) {\n> \tcase 'C':\n> \t\tif (starts_with(placeholder + 1, \"(auto)\")) {\n> @@ -1104,16 +1129,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> \t\t\t */\n> \t\t\treturn ret;\n> \t\t}\n> -\tcase 'n':\t\t/* newline */\n> -\t\tstrbuf_addch(sb, '\\n');\n> -\t\treturn 1;\n> -\tcase 'x':\n> -\t\t/* %x00 == NUL, %x0a == LF, etc. */\n> -\t\tch = hex2chr(placeholder + 1);\n> -\t\tif (ch < 0)\n> -\t\t\treturn 0;\n> -\t\tstrbuf_addch(sb, ch);\n> -\t\treturn 3;\n> \tcase 'w':\n> \t\tif (placeholder[1] == '(') {\n> \t\t\tunsigned long width = 0, indent1 = 0, indent2 = 0;\n"},{"id":"362444","messageId":"xmqqpnvkjmtu.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"20181104152232.20671-6-anders@0x63.nu","subject":"Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-05T02:10:53Z","receivedAt":"2018-11-05T02:10:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> @@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> \t\t\t\t\t\targ++;\n> \n> \t\t\t\t\topts.only_trailers = 1;\n> +\t\t\t\t} else if (skip_prefix(arg, \"separator=\", &arg)) {\n> +\t\t\t\t\tsize_t seplen = strcspn(arg, \",)\");\n> +\t\t\t\t\tstrbuf_reset(&sepbuf);\n> +\t\t\t\t\tchar *fmt = xstrndup(arg, seplen);\n> +\t\t\t\t\tstrbuf_expand(&sepbuf, fmt, format_fundamental, NULL);\n\nThis somehow feels akin to using end-user supplied param to printf(3)\nas its format argument e.g.\n\n\tint main(int ac, char *av) {\n\t\tprintf(av[1]);\n\t\treturn 0;\n\t}\n\nwhich is not a good idea. Is there a mechanism with which we can\nensure that the separator= specification will never come from\npotentially malicious sources (e.g. not used to show things on webpage\nallowing random folks who access he site to supply custom format)?\n\n"},{"id":"362448","messageId":"xmqqa7mojibg.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"CAPig+cQeUxxvgNGVc_iZ4v0U77obFu6-q0QbtzTJdnEep8eq+Q@mail.gmail.com","subject":"Re: [PATCH v2 2/5] pretty: allow showing specific trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-05T03:48:19Z","receivedAt":"2018-11-05T03:48:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Does the user have to include the colon when specifying of\n> 'key='? I can see from peeking at the implementation that the\n> colon must not be used, but this should be documented. Should the code\n> tolerate a trailing colon? (Genuine question; it's easy to do and\n> would be more user-friendly.)\n>\n> Does 'key=', do a full or partial match on trailers? And, if\n> partial, is the match anchored at the start or can it match anywhere\n> in the trailer key? I see from the implementation that it does a full\n> match, but this behavior should be documented.\n>\n> What happens if 'key=...' is specified multiple times? Are the\n> multiple keys conjunctive? Disjunctive? Last-wins? I can see from the\n> implementation that it is last-wins, but this behavior should be\n> documented. (I wonder how painful it will be for people who want to\n> match multiple keys. This doesn't have to be answered yet, as the\n> behavior can always be loosened later to allow multiple-key matching\n> since the current syntax does not disallow such expansion.)\n>\n> Thinking further on the last two points, should be a regular expression?\n\nAnother thing that needs to be clarified in the document would be\ncase sensitivity. People sometimes spell \"Signed-Off-By:\" by\nmistake (or is it by malice?).\n\nI do suspect that the parser should just make a list of sought-after\nkeys, not doing \"last-one-wins\", as that won't be very difficult to\ndo and makes what happens when given multiple keys trivially obvious.\n"},{"id":"362449","messageId":"CAPig+cS8-7-6MzuUcTTPMOUBEGuJiPPui5hCECOAu7vDx0irLg@mail.gmail.com","threadId":"49703","inReplyTo":"xmqqa7mojibg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/5] pretty: allow showing specific trailers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-05T03:52:35Z","receivedAt":"2018-11-05T03:52:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 4, 2018 at 10:48 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > Does the user have to include the colon when specifying of\n> > 'key='?\n> > Does 'key=', do a full or partial match on trailers?\n> > What happens if 'key=...' is specified multiple times?\n> > Thinking further on the last two points, should be a regular expression?\n>\n> Another thing that needs to be clarified in the document would be\n> case sensitivity. People sometimes spell \"Signed-Off-By:\" by\n> mistake (or is it by malice?).\n\nThe documentation does say parenthetically \"(matching is done\ncase-insensitively)\", so I think that's already covered. Or did you\nhave something else in mind?\n"},{"id":"362453","messageId":"xmqqzhuohzrh.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"20181104152232.20671-3-anders@0x63.nu","subject":"Re: [PATCH v2 2/5] pretty: allow showing specific trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-05T05:14:26Z","receivedAt":"2018-11-05T05:14:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> +\t\t\t\telse if (skip_prefix(arg, \"key=\", &arg)) {\n> +\t\t\t\t\tconst char *end = arg + strcspn(arg, \",)\");\n> +\n> +\t\t\t\t\tif (opts.filter_key)\n> +\t\t\t\t\t\tfree(opts.filter_key);\n\nCall the free() unconditionally, cf. contrib/coccinelle/free.cocci.\n"},{"id":"362454","messageId":"xmqqr2g0hzlf.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"20181104152232.20671-6-anders@0x63.nu","subject":"Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-05T05:18:04Z","receivedAt":"2018-11-05T05:18:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> +\t\t\t\tif (opts->separator && first_printed)\n> +\t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n> \t\t\t\tif (opts->no_key)\n> -\t\t\t\t\tstrbuf_addf(out, \"%s\\n\", val.buf);\n> +\t\t\t\t\tstrbuf_addf(out, \"%s\", val.buf);\n\nAvoid addf with \"%s\" alone as a formatter; instead say\n\n\tstrbuf_addstr(out, val.buf);\n\ncf. contrib/coccinelle/strbuf.cocci\n"},{"id":"362475","messageId":"878t28knld.fsf@0x63.nu","threadId":"49703","inReplyTo":"CAPig+cSfwUJ8thYW+dq1qjT8X_f78DzAzfb_Xd3CMxO=9juz=w@mail.gmail.com","subject":"Re: [PATCH v2 0/5] %(trailers) improvements in pretty format","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-05T07:09:25Z","receivedAt":"2018-11-05T07:09:33Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nEric Sunshine writes:\n> If \"key\" is for including particular trailers, intuition might lead\n> people to think that \"nokey\" is for excluding certain trailers.\n> Perhaps a different name for \"nokey\", such as \"valueonly\" or\n> \"stripkey\", would be better.\n\nGood point. I guess \"valueonly\" would be preferred as it says what it\nshows, not what it hides.\n"},{"id":"362477","messageId":"875zxckk1g.fsf@0x63.nu","threadId":"49703","inReplyTo":"CAPig+cQeUxxvgNGVc_iZ4v0U77obFu6-q0QbtzTJdnEep8eq+Q@mail.gmail.com","subject":"Re: [PATCH v2 2/5] pretty: allow showing specific trailers","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-05T08:26:32Z","receivedAt":"2018-11-05T08:26:39Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nEric Sunshine writes:\n> Should the code tolerate a trailing colon? (Genuine question; it's\n> easy to do and would be more user-friendly.)\n\nI would make sense to allow the trailing colon, it is easy enough to\njust strip that away when reading the argument.\n\nHowever I'm not sure how that would fit together with the possibility to\nlater lifting it to a regexp, hard to strip a trailing colon from a\nregexp in a generic way.\n\n\n> What happens if 'key=...' is specified multiple times?\n\nMy first thought was to simply disallow that. But that seemed hard to\nfit into current model where errors just means don't expand.\n\nI would guess that most useful and intuitive to user would be to handle\nmultiple key arguments by showing any of those keys.\n\n\n\n> Thinking further on the last two points, should be a regular expression?\n\nIt probably would make sense. I can see how the regexp '^.*-by$' would\nbe useful (but glob style matching would suffice in that case).\n\nAlso handling multi-matching with an alternation group would be elegant\n%(trailers:key=\"(A|B)\"). Except for the fact that the parser would need to\nunderstand some kind of quoting, which seems like an major undertaking.\n\nI guess having it as a regular exception would also mean that it needs\nto get some way to cache the re so it is compiled once, and not for each expansion.\n\n>\n>> + free(opts.filter_key);\n>\n> If I understand correctly, this is making a copy of so that it\n> will be NUL-terminated since the code added to trailer.c uses a simple\n> strcasecmp() to match it. Would it make sense to avoid the copy by\n> adding fields 'opts.filter_key' and 'opts.filter_key_len' and using\n> strncasecmp() instead? (Genuine question; not necessarily a request\n> for change.)\n\nI'm also not very happy about that copy.\n\nJust using strncasecmp would cause it to be prefix match, no?\n\nBut if changing matching semantics to handle multiple key options to\nsomething else I'm thinking opts.filter_key would be replaced with a\nopts.filter callback function, and that part would need to be rewritten\nanyway.\n\n>\n>> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\n>> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '\n>> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '\n>> + git log --no-walk --pretty=\"%(trailers:key=Acked-by)\" >actual &&\n>> + {\n>> + echo \"Acked-by: A U Thor \" &&\n>> + echo\n>> + } >expect &&\n>> + test_cmp expect actual\n>> +'\n>\n> I guess these new tests are modeled after one or two existing tests\n> which use a series of 'echo' statements\n\nI guess I could change it to \"--pretty=format:%(trailers:key=Acked-by)\"\nto get separator semantics and avoid that extra blank line, making it\nsimpler.\n"},{"id":"362478","messageId":"8736sflya8.fsf@0x63.nu","threadId":"49703","inReplyTo":"xmqqtvkwjn1k.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-05T08:32:47Z","receivedAt":"2018-11-05T08:32:50Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJunio C Hamano writes:\n> I do not think \"fundamental\" is the best name for this, but I agree\n> that it would be useful to split the helpers into one that is\n> \"constant across commits\" and the other one that is \"per commit\".\n\nAny suggestions for a better name?\n\nstandalone? simple? invariant? free?\n\n"},{"id":"362479","messageId":"CAPig+cTdAA-uPgi_viHhR8b17MgdM5RQ_7v-dWH-tr7BZa1Adw@mail.gmail.com","threadId":"49703","inReplyTo":"875zxckk1g.fsf@0x63.nu","subject":"Re: [PATCH v2 2/5] pretty: allow showing specific trailers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-05T09:00:34Z","receivedAt":"2018-11-05T09:00:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 5, 2018 at 3:26 AM Anders Waldenborg wrote:\n> Eric Sunshine writes:\n> > Should the code tolerate a trailing colon? (Genuine question; it's\n> > easy to do and would be more user-friendly.)\n>\n> I would make sense to allow the trailing colon, it is easy enough to\n> just strip that away when reading the argument.\n>\n> However I'm not sure how that would fit together with the possibility to\n> later lifting it to a regexp, hard to strip a trailing colon from a\n> regexp in a generic way.\n\nWhich is a good reason to think about these issues now, before being\nset in stone.\n\n> > What happens if 'key=...' is specified multiple times?\n>\n> My first thought was to simply disallow that. But that seemed hard to\n> fit into current model where errors just means don't expand.\n>\n> I would guess that most useful and intuitive to user would be to handle\n> multiple key arguments by showing any of those keys.\n\nAgreed.\n\n> > Thinking further on the last two points, should be a regular expression?\n>\n> It probably would make sense. I can see how the regexp '^.*-by$' would\n> be useful (but glob style matching would suffice in that case).\n>\n> Also handling multi-matching with an alternation group would be elegant\n> %(trailers:key=\"(A|B)\"). Except for the fact that the parser would need to\n> understand some kind of quoting, which seems like an major undertaking.\n\nMaybe, maybe not. As long as we're careful not to paint ourselves into\na corner, it might very well be okay to start with the current\nimplementation of matching the full key as a literal string and\n(perhaps much) later introduce regex as an alternate way to specify\nthe key. For instance, 'key=literal' and 'key=/regex/' can co-exist,\nand the extraction of the regex inside /.../ should not be especially\ndifficult.\n\n> I guess having it as a regular exception would also mean that it needs\n> to get some way to cache the re so it is compiled once, and not for each expansion.\n\nYes, that's something I brought up a few years ago during a GSoC\nproject; not regex specifically, but that this parsing of the format\nis happening repeatedly rather than just once. I had suggested to the\nGSoC student that the parsing could be done early, compiling the\nformat expression into a \"machine\" which could be applied repeatedly.\nIt's a larger job, of course, not necessarily something worth tackling\nfor your current needs.\n\n> > If I understand correctly, this is making a copy of so that it\n> > will be NUL-terminated since the code added to trailer.c uses a simple\n> > strcasecmp() to match it. Would it make sense to avoid the copy by\n> > adding fields 'opts.filter_key' and 'opts.filter_key_len' and using\n> > strncasecmp() instead? (Genuine question; not necessarily a request\n> > for change.)\n>\n> I'm also not very happy about that copy.\n> Just using strncasecmp would cause it to be prefix match, no?\n\nWell, you could retain full key match by checking for NUL explicitly\nwith something like this:\n\n !strncasecmp(tok.buf, opts->filter_key, opts->filter_key_len) &&\n !tok.buf[opts->filter_key_len]\n"},{"id":"362498","messageId":"871s7zl6xp.fsf@0x63.nu","threadId":"49703","inReplyTo":"xmqqpnvkjmtu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-05T18:24:14Z","receivedAt":"2018-11-05T18:24:21Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJunio C Hamano writes:\n> Anders Waldenborg writes:\n>\n>> @@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n>> \t\t\t\t\t\targ++;\n>>\n>> \t\t\t\t\topts.only_trailers = 1;\n>> +\t\t\t\t} else if (skip_prefix(arg, \"separator=\", &arg)) {\n>> +\t\t\t\t\tsize_t seplen = strcspn(arg, \",)\");\n>> +\t\t\t\t\tstrbuf_reset(&sepbuf);\n>> +\t\t\t\t\tchar *fmt = xstrndup(arg, seplen);\n>> +\t\t\t\t\tstrbuf_expand(&sepbuf, fmt, format_fundamental, NULL);\n>\n> This somehow feels akin to using end-user supplied param to printf(3)\n> as its format argument e.g.\n>\n> \tint main(int ac, char *av) {\n> \t\tprintf(av[1]);\n> \t\treturn 0;\n> \t}\n>\n> which is not a good idea. Is there a mechanism with which we can\n> ensure that the separator= specification will never come from\n> potentially malicious sources (e.g. not used to show things on webpage\n> allowing random folks who access he site to supply custom format)?\n\nI can't see a case where this could add anything that isn't already\npossible.\n\nAFAICU strbuf_expand doesn't suffer from the worst things that printf(3)\nsuffers from wrt untrusted format string (i.e no printf style %n which\ncan write to memory, and no vaargs on stack which allows leaking random\nstuff).\n\nThe separator option is part of the full format string. If a malicious\nuser can specify that, they can't really do anything new, as the\nseparator only can expand %n and %xNN, which they already can do in the\nfull string.\n\nBut maybe I'm missing something?\n"},{"id":"362542","messageId":"xmqqr2fzgeqn.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"8736sflya8.fsf@0x63.nu","subject":"Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-06T01:46:08Z","receivedAt":"2018-11-06T01:46:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> Junio C Hamano writes:\n>> I do not think \"fundamental\" is the best name for this, but I agree\n>> that it would be useful to split the helpers into one that is\n>> \"constant across commits\" and the other one that is \"per commit\".\n>\n> Any suggestions for a better name?\n>\n> standalone? simple? invariant? free?\n\nIf these are like %n for LF or %09 for HT, perhaps they are\nconstants or \"literals\".\n"},{"id":"362543","messageId":"xmqqmuqngen7.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"871s7zl6xp.fsf@0x63.nu","subject":"Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-06T01:48:12Z","receivedAt":"2018-11-06T01:48:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> AFAICU strbuf_expand doesn't suffer from the worst things that printf(3)\n> suffers from wrt untrusted format string (i.e no printf style %n which\n> can write to memory, and no vaargs on stack which allows leaking random\n> stuff).\n>\n> The separator option is part of the full format string. If a malicious\n> user can specify that, they can't really do anything new, as the\n> separator only can expand %n and %xNN, which they already can do in the\n> full string.\n>\n> But maybe I'm missing something?\n\nI just wanted to make sure somebody thought it through (and hoped\nthat that somebody might be you). I do not offhand see a readily\nusable exploit vector myself.\n"},{"id":"363568","messageId":"20181118114427.1397-6-anders@0x63.nu","threadId":"49703","inReplyTo":"20181118114427.1397-1-anders@0x63.nu","subject":"[PATCH v3 5/5] pretty: add support for separator option in %(trailers)","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-18T11:44:27Z","receivedAt":"2018-11-18T11:45:26Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"By default trailer lines are terminated by linebreaks ('\\n'). By\nspecifying the new 'separator' option they will instead be separated by\nuser provided string and have separator semantics rather than terminator\nsemantics. The separator string can contain the literal formatting codes\n%n and %xNN allowing it to be things that are otherwise hard to type as\n%x00, or comma and end-parenthesis which would break parsing.\n\nE.g:\n $ git log --pretty='%(trailers:key=Reviewed-by,valueonly,separator=%x00)'\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 13 +++++++++---\n pretty.c | 15 +++++++++++++\n t/t4205-log-pretty-formats.sh | 36 ++++++++++++++++++++++++++++++++\n trailer.c | 15 +++++++++++--\n trailer.h | 1 +\n 5 files changed, 75 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 8cc8c3f9f..30e238338 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -218,9 +218,16 @@ endif::git-rev-list[]\n is given multiple times only last one is used.\n ** 'valueonly': skip over the key part of the trailer and only show\n the its value part.\n- ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer\n- lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows\n- trailer lines with key `Reviewed-by`.\n+ ** 'separator=': specifying an alternative separator than the\n+ default line feed character. SEP may can contain the literal\n+ formatting codes %n and %xNN allowing it to contain characters\n+ that are hard to type such as %x00, or comma and end-parenthesis\n+ which would break parsing. If option is given multiple times only\n+ the last one is used.\n+ ** Examples: `%(trailers:only,unfold,separator=%x00)` unfolds and\n+ shows all trailer lines separated by NUL character,\n+ `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer\n+ lines with key `Reviewed-by`.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex 819c5c50a..5b22a7237 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1318,6 +1318,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n \t\tstruct format_trailer_match_data filter_data;\n+\t\tstruct strbuf sepbuf = STRBUF_INIT;\n \t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n@@ -1348,6 +1349,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\targ = end;\n \t\t\t\t\tif (*arg == ',')\n \t\t\t\t\t\targ++;\n+\t\t\t\t} else if (skip_prefix(arg, \"separator=\", &arg)) {\n+\t\t\t\t\tsize_t seplen = strcspn(arg, \",)\");\n+\t\t\t\t\tchar *fmt;\n+\n+\t\t\t\t\tstrbuf_reset(&sepbuf);\n+\t\t\t\t\tfmt = xstrndup(arg, seplen);\n+\t\t\t\t\tstrbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);\n+\t\t\t\t\tfree(fmt);\n+\t\t\t\t\topts.separator = &sepbuf;\n+\n+\t\t\t\t\targ += seplen;\n+\t\t\t\t\tif (*arg == ',')\n+\t\t\t\t\t\targ++;\n \t\t\t\t} else\n \t\t\t\t\tbreak;\n \t\t\t}\n@@ -1356,6 +1370,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n \t\t\tret = arg - placeholder + 1;\n \t\t}\n+\t\tstrbuf_release(&sepbuf);\n \t\treturn ret;\n \t}\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 095208d6b..562b56dda 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -640,6 +640,42 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:separator) changes separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n+\tprintf \"XSigned-off-by: A U Thor \\0Acked-by: A U Thor \\0[ v2 updated patch description ]\\0Signed-off-by: A U Thor X\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tImportant fix\n+\n+\tThe fix is explained here\n+\n+\tCloses: #1234\n+\tEOF\n+\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tAnother fix\n+\n+\tThe fix is explained here\n+\n+\tCloses: #567\n+\tCloses: #890\n+\tEOF\n+\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tDoes not close any tickets\n+\tEOF\n+\n+\tgit log --pretty=\"%s% (trailers:separator=%x2c%x20,key=Closes,valueonly)\" HEAD~3.. >actual &&\n+\ttest_write_lines \\\n+\t\t\"Does not close any tickets\" \\\n+\t\t\"Another fix #567, #890\" \\\n+\t\t\"Important fix #1234\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex 662c7ff03..85cd2e52e 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\tconst struct trailer_info *info,\n \t\t\t\tconst struct process_trailer_options *opts)\n {\n+\tsize_t origlen = out->len;\n \tsize_t i;\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n-\tif (!opts->only_trailers && !opts->unfold) {\n+\tif (!opts->only_trailers && !opts->unfold && !opts->separator) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1150,16 +1151,26 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tif (!opts->filter || opts->filter(&tok, opts->filter_data)) {\n \t\t\t\tif (opts->unfold)\n \t\t\t\t\tunfold_value(&val);\n+\n+\t\t\t\tif (opts->separator && out->len != origlen)\n+\t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n \t\t\t\t\tstrbuf_addf(out, \"%s: \", tok.buf);\n \t\t\t\tstrbuf_addbuf(out, &val);\n-\t\t\t\tstrbuf_addch(out, '\\n');\n+\t\t\t\tif (!opts->separator)\n+\t\t\t\t\tstrbuf_addch(out, '\\n');\n \t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\n \n \t\t} else if (!opts->only_trailers) {\n+\t\t\tif (opts->separator && out->len != origlen) {\n+\t\t\t\tstrbuf_addbuf(out, opts->separator);\n+\t\t\t}\n \t\t\tstrbuf_addstr(out, trailer);\n+\t\t\tif (opts->separator) {\n+\t\t\t\tstrbuf_rtrim(out);\n+\t\t\t}\n \t\t}\n \t}\n \ndiff --git a/trailer.h b/trailer.h\nindex 06d417fe9..203acf4ee 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -73,6 +73,7 @@ struct process_trailer_options {\n \tint unfold;\n \tint no_divider;\n \tint value_only;\n+\tconst struct strbuf *separator;\n \tint (*filter)(const struct strbuf *, void *);\n \tvoid *filter_data;\n };\n-- \n2.17.1\n\n"},{"id":"363569","messageId":"20181118114427.1397-3-anders@0x63.nu","threadId":"49703","inReplyTo":"20181118114427.1397-1-anders@0x63.nu","subject":"[PATCH v3 2/5] pretty: allow showing specific trailers","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-18T11:44:24Z","receivedAt":"2018-11-18T11:45:27Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"Adds a new \"key=X\" option to \"%(trailers)\" which will cause it to only\nprint trailers lines which match the specified key.\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 17 +++++++++------\n pretty.c | 31 ++++++++++++++++++++++++++-\n t/t4205-log-pretty-formats.sh | 36 ++++++++++++++++++++++++++++++++\n trailer.c | 8 ++++---\n trailer.h | 2 ++\n 5 files changed, 84 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 417b638cd..75c2e838d 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -207,13 +207,18 @@ endif::git-rev-list[]\n than given and there are spaces on its left, use those spaces\n - '%><()', '%><|()': similar to '%<()', '%<|()'\n respectively, but padding both sides (i.e. the text is centered)\n-- %(trailers[:options]): display the trailers of the body as interpreted\n+- '%(trailers[:options])': display the trailers of the body as interpreted\n by linkgit:git-interpret-trailers[1]. The `trailers` string may be\n- followed by a colon and zero or more comma-separated options. If the\n- `only` option is given, omit non-trailer lines from the trailer block.\n- If the `unfold` option is given, behave as if interpret-trailer's\n- `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do\n- both.\n+ followed by a colon and zero or more comma-separated options:\n+ ** 'only': omit non-trailer lines from the trailer block.\n+ ** 'unfold': make it behave as if interpret-trailer's `--unfold`\n+ option was given.\n+ ** 'key=': only show trailers with specified key. Matching is\n+ done case-insensitively and trailing colon is optional. If option\n+ is given multiple times only last one is used.\n+ ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer\n+ lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows\n+ trailer lines with key `Reviewed-by`.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex aa03d5b23..aaedc8447 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1074,6 +1074,17 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,\n \treturn 0;\n }\n \n+struct format_trailer_match_data {\n+\tconst char *trailer;\n+\tsize_t trailer_len;\n+};\n+\n+static int format_trailer_match_cb(const struct strbuf *sb, void *ud)\n+{\n+\tstruct format_trailer_match_data *data = ud;\n+\treturn data->trailer_len == sb->len && !strncasecmp (data->trailer, sb->buf, sb->len);\n+}\n+\n static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\tconst char *placeholder,\n \t\t\t\tvoid *context)\n@@ -1312,6 +1323,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\t\tstruct format_trailer_match_data filter_data;\n \t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n@@ -1323,7 +1335,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\topts.only_trailers = 1;\n \t\t\t\telse if (match_placeholder_arg(arg, \"unfold\", &arg))\n \t\t\t\t\topts.unfold = 1;\n-\t\t\t\telse\n+\t\t\t\telse if (skip_prefix(arg, \"key=\", &arg)) {\n+\t\t\t\t\tconst char *end = arg + strcspn(arg, \",)\");\n+\n+\t\t\t\t\tfilter_data.trailer = arg;\n+\t\t\t\t\tfilter_data.trailer_len = end - arg;\n+\n+\t\t\t\t\tif (filter_data.trailer_len &&\n+\t\t\t\t\t filter_data.trailer[filter_data.trailer_len - 1] == ':')\n+\t\t\t\t\t\tfilter_data.trailer_len--;\n+\n+\t\t\t\t\topts.filter = format_trailer_match_cb;\n+\t\t\t\t\topts.filter_data = &filter_data;\n+\t\t\t\t\topts.only_trailers = 1;\n+\n+\t\t\t\t\targ = end;\n+\t\t\t\t\tif (*arg == ',')\n+\t\t\t\t\t\targ++;\n+\t\t\t\t} else\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 978a8a66f..aba7ba206 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -598,6 +598,42 @@ test_expect_success ':only and :unfold work together' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by)\" >actual &&\n+\techo \"Acked-by: A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=AcKed-bY)\" >actual &&\n+\techo \"Acked-by: A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo:) trailing colon also works' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by:)\" >actual &&\n+\techo \"Acked-by: A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=nonexistant) becomes empty' '\n+\tgit log --no-walk --pretty=\"x%(trailers:key=Nacked-by)x\" >actual &&\n+\techo \"xx\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Signed-Off-by)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Signed-Off-by,unfold)\" >actual &&\n+\tunfold expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex 0796f326b..97c8f2762 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tstruct strbuf val = STRBUF_INIT;\n \n \t\t\tparse_trailer(&tok, &val, NULL, trailer, separator_pos);\n-\t\t\tif (opts->unfold)\n-\t\t\t\tunfold_value(&val);\n+\t\t\tif (!opts->filter || opts->filter(&tok, opts->filter_data)) {\n+\t\t\t\tif (opts->unfold)\n+\t\t\t\t\tunfold_value(&val);\n \n-\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\n \ndiff --git a/trailer.h b/trailer.h\nindex b99773964..5255b676d 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -72,6 +72,8 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tint (*filter)(const struct strbuf *, void *);\n+\tvoid *filter_data;\n };\n \n #define PROCESS_TRAILER_OPTIONS_INIT {0}\n-- \n2.17.1\n\n"},{"id":"363570","messageId":"20181118114427.1397-4-anders@0x63.nu","threadId":"49703","inReplyTo":"20181118114427.1397-1-anders@0x63.nu","subject":"[PATCH v3 3/5] pretty: add support for \"valueonly\" option in %(trailers)","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-18T11:44:25Z","receivedAt":"2018-11-18T11:45:35Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"With the new \"key=\" option to %(trailers) it often makes little sense to\nshow the key, as it by definition already is know which trailer is\nprinted there. This new \"valueonly\" option makes it omit the key when\nprinting trailers.\n\nE.g.:\n $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,valueonly)' aaaa88182\nwill show:\n > upload-pack: fix broken if/else chain in config callback\n > Jeff King \n > Junio C Hamano \n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 2 ++\n pretty.c | 2 ++\n t/t4205-log-pretty-formats.sh | 6 ++++++\n trailer.c | 6 ++++--\n trailer.h | 1 +\n 5 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 75c2e838d..8cc8c3f9f 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -216,6 +216,8 @@ endif::git-rev-list[]\n ** 'key=': only show trailers with specified key. Matching is\n done case-insensitively and trailing colon is optional. If option\n is given multiple times only last one is used.\n+ ** 'valueonly': skip over the key part of the trailer and only show\n+ the its value part.\n ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer\n lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows\n trailer lines with key `Reviewed-by`.\ndiff --git a/pretty.c b/pretty.c\nindex aaedc8447..2e99f2418 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1335,6 +1335,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\topts.only_trailers = 1;\n \t\t\t\telse if (match_placeholder_arg(arg, \"unfold\", &arg))\n \t\t\t\t\topts.unfold = 1;\n+\t\t\t\telse if (match_placeholder_arg(arg, \"valueonly\", &arg))\n+\t\t\t\t\topts.value_only = 1;\n \t\t\t\telse if (skip_prefix(arg, \"key=\", &arg)) {\n \t\t\t\t\tconst char *end = arg + strcspn(arg, \",)\");\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex aba7ba206..095208d6b 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -634,6 +634,12 @@ test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,valueonly)\" >actual &&\n+\techo \"A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex 97c8f2762..662c7ff03 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tif (!opts->filter || opts->filter(&tok, opts->filter_data)) {\n \t\t\t\tif (opts->unfold)\n \t\t\t\t\tunfold_value(&val);\n-\n-\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t\tif (!opts->value_only)\n+\t\t\t\t\tstrbuf_addf(out, \"%s: \", tok.buf);\n+\t\t\t\tstrbuf_addbuf(out, &val);\n+\t\t\t\tstrbuf_addch(out, '\\n');\n \t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\ndiff --git a/trailer.h b/trailer.h\nindex 5255b676d..06d417fe9 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -72,6 +72,7 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tint value_only;\n \tint (*filter)(const struct strbuf *, void *);\n \tvoid *filter_data;\n };\n-- \n2.17.1\n\n"},{"id":"363571","messageId":"20181118114427.1397-2-anders@0x63.nu","threadId":"49703","inReplyTo":"20181118114427.1397-1-anders@0x63.nu","subject":"[PATCH v3 1/5] pretty: single return path in %(trailers) handling","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-18T11:44:23Z","receivedAt":"2018-11-18T11:45:42Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"No functional change intended.\n\nThis change may not seem useful on its own, but upcoming commits will do\nmemory allocation in there, and a single return path makes deallocation\neasier.\n\nSigned-off-by: Anders Waldenborg \n---\n pretty.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex b83a3ecd2..aa03d5b23 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1312,6 +1312,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n \n@@ -1328,8 +1329,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t}\n \t\tif (*arg == ')') {\n \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n-\t\t\treturn arg - placeholder + 1;\n+\t\t\tret = arg - placeholder + 1;\n \t\t}\n+\t\treturn ret;\n \t}\n \n \treturn 0;\t/* unknown placeholder */\n-- \n2.17.1\n\n"},{"id":"363572","messageId":"20181118114427.1397-5-anders@0x63.nu","threadId":"49703","inReplyTo":"20181118114427.1397-1-anders@0x63.nu","subject":"[PATCH v3 4/5] strbuf: separate callback for strbuf_expand:ing literals","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-18T11:44:26Z","receivedAt":"2018-11-18T11:45:49Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"Expanding '%n' and '%xNN' is generic functionality, so extract that from\nthe pretty.c formatter into a callback that can be reused.\n\nNo functional change intended\n\nSigned-off-by: Anders Waldenborg \n---\n pretty.c | 16 +++++-----------\n strbuf.c | 21 +++++++++++++++++++++\n strbuf.h | 8 ++++++++\n 3 files changed, 34 insertions(+), 11 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 2e99f2418..819c5c50a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1094,9 +1094,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tconst char *msg = c->message;\n \tstruct commit_list *p;\n \tconst char *arg;\n-\tint ch;\n+\tsize_t res;\n \n \t/* these are independent of the commit */\n+\tres = strbuf_expand_literal_cb(sb, placeholder, NULL);\n+\tif (res)\n+\t\treturn res;\n+\n \tswitch (placeholder[0]) {\n \tcase 'C':\n \t\tif (starts_with(placeholder + 1, \"(auto)\")) {\n@@ -1115,16 +1119,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t */\n \t\t\treturn ret;\n \t\t}\n-\tcase 'n':\t\t/* newline */\n-\t\tstrbuf_addch(sb, '\\n');\n-\t\treturn 1;\n-\tcase 'x':\n-\t\t/* %x00 == NUL, %x0a == LF, etc. */\n-\t\tch = hex2chr(placeholder + 1);\n-\t\tif (ch < 0)\n-\t\t\treturn 0;\n-\t\tstrbuf_addch(sb, ch);\n-\t\treturn 3;\n \tcase 'w':\n \t\tif (placeholder[1] == '(') {\n \t\t\tunsigned long width = 0, indent1 = 0, indent2 = 0;\ndiff --git a/strbuf.c b/strbuf.c\nindex f6a6cf78b..78eecd29f 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -380,6 +380,27 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,\n \t}\n }\n \n+size_t strbuf_expand_literal_cb(struct strbuf *sb,\n+\t\t\t\tconst char *placeholder,\n+\t\t\t\tvoid *context)\n+{\n+\tint ch;\n+\n+\tswitch (placeholder[0]) {\n+\tcase 'n':\t\t/* newline */\n+\t\tstrbuf_addch(sb, '\\n');\n+\t\treturn 1;\n+\tcase 'x':\n+\t\t/* %x00 == NUL, %x0a == LF, etc. */\n+\t\tch = hex2chr(placeholder + 1);\n+\t\tif (ch < 0)\n+\t\t\treturn 0;\n+\t\tstrbuf_addch(sb, ch);\n+\t\treturn 3;\n+\t}\n+\treturn 0;\n+}\n+\n size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,\n \t\tvoid *context)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex fc40873b6..52e44c9ab 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -320,6 +320,14 @@ void strbuf_expand(struct strbuf *sb,\n \t\t expand_fn_t fn,\n \t\t void *context);\n \n+/**\n+ * Used as callback for `strbuf_expand` to only expand literals\n+ * (i.e. %n and %xNN). The context argument is ignored.\n+ */\n+size_t strbuf_expand_literal_cb(struct strbuf *sb,\n+\t\t\t\tconst char *placeholder,\n+\t\t\t\tvoid *context);\n+\n /**\n * Used as callback for `strbuf_expand()`, expects an array of\n * struct strbuf_expand_dict_entry as context, i.e. pairs of\n-- \n2.17.1\n\n"},{"id":"363573","messageId":"20181118114427.1397-1-anders@0x63.nu","threadId":"49703","inReplyTo":"20181104152232.20671-1-anders@0x63.nu","subject":"[PATCH v3 0/5] %(trailers) improvements in pretty format","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-18T11:44:22Z","receivedAt":"2018-11-18T11:45:58Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"Updated since v2:\n * Allow trailing colon in 'key=' argument\n * Clarify documentation on how matching is done\n * Rename option to \"valueonly\"\n * Make trailing matching a callback function\n * Avoid copying match string\n * Simplify generation of \"expected\" in tests\n * Rename function to strbuf_expand_literal_cb\n * cocci suggested fixes\n\n\n\nAnders Waldenborg (5):\n pretty: single return path in %(trailers) handling\n pretty: allow showing specific trailers\n pretty: add support for \"valueonly\" option in %(trailers)\n strbuf: separate callback for strbuf_expand:ing literals\n pretty: add support for separator option in %(trailers)\n\n Documentation/pretty-formats.txt | 26 ++++++++---\n pretty.c | 68 ++++++++++++++++++++++------\n strbuf.c | 21 +++++++++\n strbuf.h | 8 ++++\n t/t4205-log-pretty-formats.sh | 78 ++++++++++++++++++++++++++++++++\n trailer.c | 25 ++++++++--\n trailer.h | 4 ++\n 7 files changed, 206 insertions(+), 24 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"363700","messageId":"xmqq36rwb8v5.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"20181118114427.1397-3-anders@0x63.nu","subject":"Re: [PATCH v3 2/5] pretty: allow showing specific trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-20T05:45:02Z","receivedAt":"2018-11-20T05:45:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> + followed by a colon and zero or more comma-separated options:\n> + ** 'only': omit non-trailer lines from the trailer block.\n> + ** 'unfold': make it behave as if interpret-trailer's `--unfold`\n> + option was given.\n> + ** 'key=': only show trailers with specified key. Matching is\n> + done case-insensitively and trailing colon is optional. If option\n> + is given multiple times only last one is used.\n> + ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer\n> + lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows\n> + trailer lines with key `Reviewed-by`.\n\nThe last \"Examples\" item does not logically belong to the other\nthree, which bothers me a bit.\n\n> diff --git a/pretty.c b/pretty.c\n> index aa03d5b23..aaedc8447 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1074,6 +1074,17 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,\n> \treturn 0;\n> }\n> \n> +struct format_trailer_match_data {\n> +\tconst char *trailer;\n> +\tsize_t trailer_len;\n> +};\n> +\n> +static int format_trailer_match_cb(const struct strbuf *sb, void *ud)\n> +{\n> +\tstruct format_trailer_match_data *data = ud;\n> +\treturn data->trailer_len == sb->len && !strncasecmp (data->trailer, sb->buf, sb->len);\n> +}\n\n> \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n> \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n> +\t\tstruct format_trailer_match_data filter_data;\n> \t\tsize_t ret = 0;\n> \n> \t\topts.no_divider = 1;\n> @@ -1323,7 +1335,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> \t\t\t\t\topts.only_trailers = 1;\n> \t\t\t\telse if (match_placeholder_arg(arg, \"unfold\", &arg))\n> \t\t\t\t\topts.unfold = 1;\n> -\t\t\t\telse\n> +\t\t\t\telse if (skip_prefix(arg, \"key=\", &arg)) {\n> +\t\t\t\t\tconst char *end = arg + strcspn(arg, \",)\");\n> +\n> +\t\t\t\t\tfilter_data.trailer = arg;\n> +\t\t\t\t\tfilter_data.trailer_len = end - arg;\n> +\n> +\t\t\t\t\tif (filter_data.trailer_len &&\n> +\t\t\t\t\t filter_data.trailer[filter_data.trailer_len - 1] == ':')\n> +\t\t\t\t\t\tfilter_data.trailer_len--;\n> +\n> +\t\t\t\t\topts.filter = format_trailer_match_cb;\n> +\t\t\t\t\topts.filter_data = &filter_data;\n> +\t\t\t\t\topts.only_trailers = 1;\n> +\n> +\t\t\t\t\targ = end;\n> +\t\t\t\t\tif (*arg == ',')\n> +\t\t\t\t\t\targ++;\n> +\t\t\t\t} else\n> \t\t\t\t\tbreak;\n> \t\t\t}\n\nThis is part of a loop that is entered after seeing \"%(trailers:\"\nand existing one looks for 'unfold' and 'only' by using the\nmatch_placeholder_arg() helper, which\n\n - returns false if the keyword is not what is being sought after;\n - returns 1 if it finds the keyword, followed by ',' or ')', and\n updates the end pointer to point at either the closing ')' or one\n place after the ','.\n\nThe new part cannot directly reuse the same helper because it\nexpects some non-constant string after \"key=\", but shouldn't we be\nintroducing a similar helper with extended feature to help this part\nof the code to stay readable? Exposing the minute details of the\nlogic to parse \"key=,...\" while hiding the counterpart to\nparse \"(only|unfold),...\" makes the implementation of the above loop\nuneven and hard to follow.\n\nI wonder if a helper like this would help:\n\nstatic int match_placeholder(const char *to_parse, const char *keyword,\n\t\t\t const char **value, size_t *valuelen,\n\t\t\t const char **end)\n{\n\tconst char *p;\n\n\tif (!(skip_prefix(to_parse, keyword, &p)))\n\t\treturn 0;\n\n\tif (value && valuelen) {\n\t\t/* expect \"=\" */\n\t\tsize_t vlen;\n\t\tif (*p++ != '=')\n\t\t\treturn 1;\n\t\tvlen = strcspn(p, \",)\");\n\t\tif (!p[vlen])\n\t\t\treturn 0;\n\t\t*value = p;\n\t\t*valuelen = vlen;\n\t\tp = p + vlen;\n\t}\n\n\tif (*p == ',') {\n\t\t*end = p + 1;\n\t\treturn 1;\n\t}\n\tif (*p == ')') {\n\t\t*end = p;\n\t\treturn 1;\n\t}\n\treturn 0;\n}\n\nwhich would allow the existing one to become a thin wrapper\n\nstatic int match_placeholder_arg(const char *a, const char *b, const char **c)\n{\n\treturn match_placeholder(a, b, NULL, NULL, c);\n}\n\nor can simply be eliminated by updating its only two callsites.\n\nIn the version you wrote, it is not clear what would happen if the\nformat string ends with \"%(trailers:key=\" (no value, no comma, not\neven the closing paren). I do not think it would fall into infinite\nloop, but I do not think the code structure without the helper that\nmakes the loop's logic uniform would allow it to properly diagnose\nsuch a malformed string.\n"},{"id":"363701","messageId":"xmqqy39o9tmq.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"20181118114427.1397-3-anders@0x63.nu","subject":"Re: [PATCH v3 2/5] pretty: allow showing specific trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-20T05:59:25Z","receivedAt":"2018-11-20T05:59:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> + followed by a colon and zero or more comma-separated options:\n> + ** 'only': omit non-trailer lines from the trailer block.\n> + ** 'unfold': make it behave as if interpret-trailer's `--unfold`\n> + option was given.\n> + ** 'key=': only show trailers with specified key. Matching is\n> + done case-insensitively and trailing colon is optional. If option\n> + is given multiple times only last one is used.\n\nIt would be good to allow multiple keys, as\n\n\t%(trailers:key=signed-off-by,key=helped-by)\n\nand\n\n\t%(trailers:key=signed-off-by)%(trailers:key=helped-by)\n\nwould mean quite a different thing. The former can preserve the\norder of these sign-offs and helped-bys in the original, while the\nlatter would have to show all the sign-offs before showing the\nhelped-bys, and I am not convinced if the latter is the only valid\nuse case.\n\nAlso, use of 'key=' automatically turns on 'only' as described, and\nI tend to agree that it would a convenient default mode (i.e. when\npicking certain trailers only with this mechanism, it is likely that\nthe user is willing to use %(subject) etc. to fill in what was lost\nby the implicit use of 'only'), but at the same time, it makes me\nwonder if we need to have a way to countermand an 'only' (or\n'unfold' for that matter) that was given earlier, e.g.\n\n\t%(trailers:key=signed-off-by,only=no)\n\nThanks.\n"},{"id":"363709","messageId":"CAPig+cQru=h9tdyW9MDmhXgCWG5oNWrSKEzduv-sDHVprE5+Zg@mail.gmail.com","threadId":"49703","inReplyTo":"20181118114427.1397-4-anders@0x63.nu","subject":"Re: [PATCH v3 3/5] pretty: add support for \"valueonly\" option in %(trailers)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-20T08:14:18Z","receivedAt":"2018-11-20T08:14:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 18, 2018 at 6:45 AM Anders Waldenborg wrote:\n> With the new \"key=\" option to %(trailers) it often makes little sense to\n> show the key, as it by definition already is know which trailer is\n\ns/know/known/\n\n> printed there. This new \"valueonly\" option makes it omit the key when\n> printing trailers.\n>\n> Signed-off-by: Anders Waldenborg \n"},{"id":"363710","messageId":"CAPig+cRhnEO7suiCB4j_7c3NdRHWkPjY8mp0jU76KdOoM_hhPQ@mail.gmail.com","threadId":"49703","inReplyTo":"20181118114427.1397-6-anders@0x63.nu","subject":"Re: [PATCH v3 5/5] pretty: add support for separator option in %(trailers)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-11-20T08:25:42Z","receivedAt":"2018-11-20T08:25:55Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 18, 2018 at 6:45 AM Anders Waldenborg wrote:\n> By default trailer lines are terminated by linebreaks ('\\n'). By\n> specifying the new 'separator' option they will instead be separated by\n> user provided string and have separator semantics rather than terminator\n> semantics. The separator string can contain the literal formatting codes\n> %n and %xNN allowing it to be things that are otherwise hard to type as\n> %x00, or comma and end-parenthesis which would break parsing.\n>\n> Signed-off-by: Anders Waldenborg \n> ---\n> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\n> @@ -218,9 +218,16 @@ endif::git-rev-list[]\n> + ** 'separator=': specifying an alternative separator than the\n> + default line feed character. SEP may can contain the literal\n> + formatting codes %n and %xNN allowing it to contain characters\n> + that are hard to type such as %x00, or comma and end-parenthesis\n> + which would break parsing. If option is given multiple times only\n> + the last one is used.\n\nIt's not clear from this documentation what constitutes a valid .\nIs it restricted to a single character? Can it be an arbitrary string?\nIf a string, does it need to be quoted? Does it support backslash\nescaping?\n\nAlthough I was able to guess that %xNN allowed hex input of a 7- or\n8-bit value, I found myself wondering what I was supposed to replace\n'n' with in \"%n\". I didn't fathom that \"%n\" was meant to be typed\nliterally to specify a newline character.\n\n> + ** Examples: `%(trailers:only,unfold,separator=%x00)` unfolds and\n> + shows all trailer lines separated by NUL character,\n> + `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer\n> + lines with key `Reviewed-by`.\n"},{"id":"364044","messageId":"87a7lw7oct.fsf@0x63.nu","threadId":"49703","inReplyTo":"xmqqy39o9tmq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/5] pretty: allow showing specific trailers","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-25T23:02:10Z","receivedAt":"2018-11-25T23:02:28Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJunio C Hamano writes:\n> Also, use of 'key=' automatically turns on 'only' as described, and\n> I tend to agree that it would a convenient default mode (i.e. when\n> picking certain trailers only with this mechanism, it is likely that\n> the user is willing to use %(subject) etc. to fill in what was lost\n> by the implicit use of 'only'), but at the same time, it makes me\n> wonder if we need to have a way to countermand an 'only' (or\n> 'unfold' for that matter) that was given earlier, e.g.\n>\n> \t%(trailers:key=signed-off-by,only=no)\n>\n> Thanks.\n\nI'm not sure what that would mean. The non-trailer lines in the trailer\nblock doesn't match the key.\n\nTake this commit as an example:\n\n$ git show -s --pretty=format:'%(trailers)' b4d065df03049bacfbc40467b60b13e804b7d289\nHelped-by: Jeff King \n[jc: took idea and log message from peff]\nSigned-off-by: Junio C Hamano \n\nWith 'only' it shows:\n$ git show -s --pretty=format:'%(trailers:only)' b4d065df03049bacfbc40467b60b13e804b7d289\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n\nNow with a \"key=signed-off-by\" option I would imagine that as adding a\n\"| grep -i '^signed-off-by:'\" to the end. In both cases (with and\nwithout 'only') that would give the same result:\n\"Signed-off-by: Junio C Hamano \"\n\n\n anders\n"},{"id":"364049","messageId":"xmqqva4kzg23.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"87a7lw7oct.fsf@0x63.nu","subject":"Re: [PATCH v3 2/5] pretty: allow showing specific trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-26T03:13:56Z","receivedAt":"2018-11-26T03:14:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> Junio C Hamano writes:\n>> Also, use of 'key=' automatically turns on 'only' as described, and\n>> I tend to agree that it would a convenient default mode (i.e. when\n>> picking certain trailers only with this mechanism, it is likely that\n>> the user is willing to use %(subject) etc. to fill in what was lost\n>> by the implicit use of 'only'), but at the same time, it makes me\n>> wonder if we need to have a way to countermand an 'only' (or\n>> 'unfold' for that matter) that was given earlier, e.g.\n>>\n>> \t%(trailers:key=signed-off-by,only=no)\n>>\n>> Thanks.\n>\n> I'm not sure what that would mean. The non-trailer lines in the trailer\n> block doesn't match the key.\n\nI was confused by the \"only\" stuff.\n\nWhen you give a key (or two), they cannot possibly name non-trailer\nlines, so while it may be possible to ask \"oh, by the way, I also\nwant non-trailer lines in addition to signed-off-by and cc lines\",\nthe value of being able to make such a request is dubious.\n\nThe value is dubious, but logically it makes it more consistent with\nthe current %(trailers) that lack 'only', which is \"oh by the way, I\nalso want non-trailer lines in addition to the trailers with\nkeyword\", to allow a way to countermand the 'only' you flip on by\ndefault when keywords are given.\n"},{"id":"364063","messageId":"878t1g72ee.fsf@0x63.nu","threadId":"49703","inReplyTo":"xmqqva4kzg23.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 2/5] pretty: allow showing specific trailers","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-11-26T06:56:25Z","receivedAt":"2018-11-26T06:56:32Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJunio C Hamano writes:\n> I was confused by the \"only\" stuff.\n>\n> When you give a key (or two), they cannot possibly name non-trailer\n> lines, so while it may be possible to ask \"oh, by the way, I also\n> want non-trailer lines in addition to signed-off-by and cc lines\",\n> the value of being able to make such a request is dubious.\n>\n> The value is dubious, but logically it makes it more consistent with\n> the current %(trailers) that lack 'only', which is \"oh by the way, I\n> also want non-trailer lines in addition to the trailers with\n> keyword\", to allow a way to countermand the 'only' you flip on by\n> default when keywords are given.\n\n\nWould it feel less inconsistent if it did not set the 'only_trailers'\noption?\n\nNow that I look at it again setting 'only_trailers' is more of an\nimplementation trick/hack to make sure it doesn't take the fast-path in\nformat_trailer_info (and by documenting it it justifies that hack). If\ninstead the 'filter' option is checked in the relevant places there\nwould be no need to mix up 'only' with 'filter'.\n\nThat is, do you think something like this should be squashed in?\n\ndiff --git a/pretty.c b/pretty.c\nindex 302e67fa8..f45ccaf51 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1360,7 +1360,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n\n opts.filter = format_trailer_match_cb;\n opts.filter_data = &filter_list;\n- opts.only_trailers = 1;\n } else\n break;\n }\ndiff --git a/trailer.c b/trailer.c\nindex 97c8f2762..07ca2b2c6 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,\n size_t i;\n\n /* If we want the whole block untouched, we can take the fast path. */\n- if (!opts->only_trailers && !opts->unfold) {\n+ if (!opts->only_trailers && !opts->unfold && !opts->filter) {\n strbuf_add(out, info->trailer_start,\n info->trailer_end - info->trailer_start);\n return;\n@@ -1156,7 +1156,7 @@ static void format_trailer_info(struct strbuf *out,\n strbuf_release(&tok);\n strbuf_release(&val);\n\n- } else if (!opts->only_trailers) {\n+ } else if (!opts->only_trailers && !opts->filter) {\n strbuf_addstr(out, trailer);\n }\n }\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 7548e1d38..ea3cd5b28 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -228,9 +228,9 @@ endif::git-rev-list[]\n ** 'key=': only show trailers with specified key. Matching is done\n case-insensitively and trailing colon is optional. If option is\n given multiple times trailer lines matching any of the keys are\n- shown. Non-trailer lines in the trailer block are also hidden\n- (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)`\n- shows trailer lines with key `Reviewed-by`.\n+ shown. Non-trailer lines in the trailer block are also hidden.\n+ E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key\n+ `Reviewed-by`.\n ** 'only': omit non-trailer lines from the trailer block.\n ** 'unfold': make it behave as if interpret-trailer's `--unfold`\n option was given. E.g., `%(trailers:only,unfold)` unfolds and\n"},{"id":"364067","messageId":"xmqq4lc4wa0o.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"878t1g72ee.fsf@0x63.nu","subject":"Re: [PATCH v3 2/5] pretty: allow showing specific trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-26T07:52:39Z","receivedAt":"2018-11-26T07:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> Would it feel less inconsistent if it did not set the 'only_trailers'\n> option?\n\nIf %(trailers:key=...) did not automatically imply 'only', it would\nbe very consistent.\n\nBut as I already said, I think it would be less convenient, as I do\nsuspect that those who want specific keys would want to see only\nthose trailers with specific keys.\n\nAnd if we want that convinience, we'd probably want a way to\noptionally disable that 'only' bit when the user wants to.\n\nAnd...\n\n> --- a/Documentation/pretty-formats.txt\n> +++ b/Documentation/pretty-formats.txt\n> @@ -228,9 +228,9 @@ endif::git-rev-list[]\n> ** 'key=': only show trailers with specified key. Matching is done\n> case-insensitively and trailing colon is optional. If option is\n> given multiple times trailer lines matching any of the keys are\n> - shown. Non-trailer lines in the trailer block are also hidden\n> - (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)`\n> - shows trailer lines with key `Reviewed-by`.\n> + shown. Non-trailer lines in the trailer block are also hidden.\n> + E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key\n> + `Reviewed-by`.\n\n... I do not think this change reduces the puzzlement felt by\nreaders who would have wondered how that implied 'only' can be\ncountermanded with the old text. It just makes it look even less\nexplained to them.\n\nIf we assume that nobody would ever want to mix non-trailers when\nasking specific keywords, then \"them\" in the above paragraph would\nbecome an empty set, and we do not have to worry about them. I am\nnot sure if Git is still such a small project to allow us rely on\nsuch an assumption, though.\n\n> ** 'only': omit non-trailer lines from the trailer block.\n> ** 'unfold': make it behave as if interpret-trailer's `--unfold`\n> option was given. E.g., `%(trailers:only,unfold)` unfolds and\n\n"},{"id":"364807","messageId":"20181208163647.19538-1-anders@0x63.nu","threadId":"49703","inReplyTo":"20181028125025.30952-1-anders@0x63.nu","subject":"[PATCH v4 0/7] %(trailers) improvements in pretty format","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-12-08T16:36:40Z","receivedAt":"2018-12-08T16:37:17Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"Updated since v3:\n * multiple 'key=' matches any\n * allow overriding implicit 'only' when using key\n * minor grammar and spelling fixes\n * documentation restructuring\n * Helper functions for parsing options\n\nAnders Waldenborg (7):\n doc: group pretty-format.txt placeholders descriptions\n pretty: allow %(trailers) options with explicit value\n pretty: single return path in %(trailers) handling\n pretty: allow showing specific trailers\n pretty: add support for \"valueonly\" option in %(trailers)\n strbuf: separate callback for strbuf_expand:ing literals\n pretty: add support for separator option in %(trailers)\n\n Documentation/pretty-formats.txt | 260 ++++++++++++++++++-------------\n pretty.c | 104 ++++++++++---\n strbuf.c | 21 +++\n strbuf.h | 8 +\n t/t4205-log-pretty-formats.sh | 111 +++++++++++++\n trailer.c | 25 ++-\n trailer.h | 4 +\n 7 files changed, 400 insertions(+), 133 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"364808","messageId":"20181208163647.19538-3-anders@0x63.nu","threadId":"49703","inReplyTo":"20181208163647.19538-1-anders@0x63.nu","subject":"[PATCH v4 2/7] pretty: allow %(trailers) options with explicit value","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-12-08T16:36:42Z","receivedAt":"2018-12-08T16:37:46Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"In addition to old %(trailers:only) it is now allowed to write\n%(trailers:only=yes)\n\nBy itself this only gives (the not quite so useful) possibility to have\nusers change their mind in the middle of a formatting\nstring (%(trailers:only=true,only=false)). However, it gives users the\nopportunity to override defaults from future options.\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 14 ++++++++++----\n pretty.c | 32 +++++++++++++++++++++++++++-----\n t/t4205-log-pretty-formats.sh | 18 ++++++++++++++++++\n 3 files changed, 55 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 86d804fe97..d33b072eb2 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -225,10 +225,16 @@ endif::git-rev-list[]\n linkgit:git-interpret-trailers[1]. The\n `trailers` string may be followed by a colon\n and zero or more comma-separated options:\n-** 'only': omit non-trailer lines from the trailer block.\n-** 'unfold': make it behave as if interpret-trailer's `--unfold`\n- option was given. E.g., `%(trailers:only,unfold)` unfolds and\n- shows all trailer lines.\n+** 'only[=val]': select whether non-trailer lines from the trailer\n+ block should be included. The `only` keyword may optionally be\n+ followed by an equal sign and one of `true`, `on`, `yes` to omit or\n+ `false`, `off`, `no` to show the non-trailer lines. If option is\n+ given without value it is enabled. If given multiple times the last\n+ value is used.\n+** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`\n+ option was given. In same way as to for `only` it can be followed\n+ by an equal sign and explicit value. E.g.,\n+ `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex b83a3ecd23..26efdba73a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1074,6 +1074,31 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,\n \treturn 0;\n }\n \n+static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,\n+\t\t\t\t const char **end, int *val)\n+{\n+\tconst char *p;\n+\tif (!skip_prefix(to_parse, candidate, &p))\n+\t\treturn 0;\n+\n+\tif (match_placeholder_arg(p, \"=no\", end) ||\n+\t match_placeholder_arg(p, \"=off\", end) ||\n+\t match_placeholder_arg(p, \"=false\", end)) {\n+\t\t*val = 0;\n+\t\treturn 1;\n+\t}\n+\n+\tif (match_placeholder_arg(p, \"\", end) ||\n+\t match_placeholder_arg(p, \"=yes\", end) ||\n+\t match_placeholder_arg(p, \"=on\", end) ||\n+\t match_placeholder_arg(p, \"=true\", end)) {\n+\t\t*val = 1;\n+\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\tconst char *placeholder,\n \t\t\t\tvoid *context)\n@@ -1318,11 +1343,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tif (*arg == ':') {\n \t\t\targ++;\n \t\t\tfor (;;) {\n-\t\t\t\tif (match_placeholder_arg(arg, \"only\", &arg))\n-\t\t\t\t\topts.only_trailers = 1;\n-\t\t\t\telse if (match_placeholder_arg(arg, \"unfold\", &arg))\n-\t\t\t\t\topts.unfold = 1;\n-\t\t\t\telse\n+\t\t\t\tif (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n+\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 978a8a66ff..63730a4ec0 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:only=yes) shows only \"key: value\" trailers' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:only=yes)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:only=no) shows all trailers' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:only=no)\" >actual &&\n+\tcat trailers >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:only=no,only=true) shows only \"key: value\" trailers' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:only=yes)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(trailers:unfold) unfolds trailers' '\n \tgit log --no-walk --pretty=\"%(trailers:unfold)\" >actual &&\n \t{\n-- \n2.17.1\n\n"},{"id":"364809","messageId":"20181208163647.19538-2-anders@0x63.nu","threadId":"49703","inReplyTo":"20181208163647.19538-1-anders@0x63.nu","subject":"[PATCH v4 1/7] doc: group pretty-format.txt placeholders descriptions","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-12-08T16:36:41Z","receivedAt":"2018-12-08T16:37:48Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"The placeholders can be grouped into three kinds:\n * literals\n * affecting formatting of later placeholders\n * expanding to information in commit\n\nAlso change the list to a definition list (using '::')\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 235 ++++++++++++++++---------------\n 1 file changed, 125 insertions(+), 110 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 417b638cd8..86d804fe97 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -102,118 +102,133 @@ The title was >>t4119: test autocomputing -p for traditional diff input.<<\n +\n The placeholders are:\n \n-- '%H': commit hash\n-- '%h': abbreviated commit hash\n-- '%T': tree hash\n-- '%t': abbreviated tree hash\n-- '%P': parent hashes\n-- '%p': abbreviated parent hashes\n-- '%an': author name\n-- '%aN': author name (respecting .mailmap, see linkgit:git-shortlog[1]\n- or linkgit:git-blame[1])\n-- '%ae': author email\n-- '%aE': author email (respecting .mailmap, see\n- linkgit:git-shortlog[1] or linkgit:git-blame[1])\n-- '%ad': author date (format respects --date= option)\n-- '%aD': author date, RFC2822 style\n-- '%ar': author date, relative\n-- '%at': author date, UNIX timestamp\n-- '%ai': author date, ISO 8601-like format\n-- '%aI': author date, strict ISO 8601 format\n-- '%cn': committer name\n-- '%cN': committer name (respecting .mailmap, see\n- linkgit:git-shortlog[1] or linkgit:git-blame[1])\n-- '%ce': committer email\n-- '%cE': committer email (respecting .mailmap, see\n- linkgit:git-shortlog[1] or linkgit:git-blame[1])\n-- '%cd': committer date (format respects --date= option)\n-- '%cD': committer date, RFC2822 style\n-- '%cr': committer date, relative\n-- '%ct': committer date, UNIX timestamp\n-- '%ci': committer date, ISO 8601-like format\n-- '%cI': committer date, strict ISO 8601 format\n-- '%d': ref names, like the --decorate option of linkgit:git-log[1]\n-- '%D': ref names without the \" (\", \")\" wrapping.\n-- '%e': encoding\n-- '%s': subject\n-- '%f': sanitized subject line, suitable for a filename\n-- '%b': body\n-- '%B': raw body (unwrapped subject and body)\n+- Placeholders that expand to a single literal character:\n+'%n':: newline\n+'%%':: a raw '%'\n+'%x00':: print a byte from a hex code\n+\n+- Placeholders that affect formatting of later placeholders:\n+'%Cred':: switch color to red\n+'%Cgreen':: switch color to green\n+'%Cblue':: switch color to blue\n+'%Creset':: reset color\n+'%C(...)':: color specification, as described under Values in the\n+ \"CONFIGURATION FILE\" section of linkgit:git-config[1]. By\n+ default, colors are shown only when enabled for log output\n+ (by `color.diff`, `color.ui`, or `--color`, and respecting\n+ the `auto` settings of the former if we are going to a\n+ terminal). `%C(auto,...)` is accepted as a historical\n+ synonym for the default (e.g., `%C(auto,red)`). Specifying\n+ `%C(always,...) will show the colors even when color is\n+ not otherwise enabled (though consider just using\n+ `--color=always` to enable color for the whole output,\n+ including this format and anything else git might color).\n+ `auto` alone (i.e. `%C(auto)`) will turn on auto coloring\n+ on the next placeholders until the color is switched\n+ again.\n+'%m':: left (`<`), right (`>`) or boundary (`-`) mark\n+'%w([[,[,]]])':: switch line wrapping, like the -w option of\n+ linkgit:git-shortlog[1].\n+'%<([,trunc|ltrunc|mtrunc])':: make the next placeholder take at\n+ least N columns, padding spaces on\n+ the right if necessary. Optionally\n+ truncate at the beginning (ltrunc),\n+ the middle (mtrunc) or the end\n+ (trunc) if the output is longer than\n+ N columns. Note that truncating\n+ only works correctly with N >= 2.\n+'%<|()':: make the next placeholder take at least until Nth\n+ columns, padding spaces on the right if necessary\n+'%>()', '%>|()':: similar to '%<()', '%<|()' respectively,\n+ but padding spaces on the left\n+'%>>()', '%>>|()':: similar to '%>()', '%>|()'\n+ respectively, except that if the next\n+ placeholder takes more spaces than given and\n+ there are spaces on its left, use those\n+ spaces\n+'%><()', '%><|()':: similar to '%<()', '%<|()'\n+ respectively, but padding both sides\n+ (i.e. the text is centered)\n+\n+- Placeholders that expand to information extracted from the commit:\n+'%H':: commit hash\n+'%h':: abbreviated commit hash\n+'%T':: tree hash\n+'%t':: abbreviated tree hash\n+'%P':: parent hashes\n+'%p':: abbreviated parent hashes\n+'%an':: author name\n+'%aN':: author name (respecting .mailmap, see linkgit:git-shortlog[1]\n+ or linkgit:git-blame[1])\n+'%ae':: author email\n+'%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]\n+ or linkgit:git-blame[1])\n+'%ad':: author date (format respects --date= option)\n+'%aD':: author date, RFC2822 style\n+'%ar':: author date, relative\n+'%at':: author date, UNIX timestamp\n+'%ai':: author date, ISO 8601-like format\n+'%aI':: author date, strict ISO 8601 format\n+'%cn':: committer name\n+'%cN':: committer name (respecting .mailmap, see\n+ linkgit:git-shortlog[1] or linkgit:git-blame[1])\n+'%ce':: committer email\n+'%cE':: committer email (respecting .mailmap, see\n+ linkgit:git-shortlog[1] or linkgit:git-blame[1])\n+'%cd':: committer date (format respects --date= option)\n+'%cD':: committer date, RFC2822 style\n+'%cr':: committer date, relative\n+'%ct':: committer date, UNIX timestamp\n+'%ci':: committer date, ISO 8601-like format\n+'%cI':: committer date, strict ISO 8601 format\n+'%d':: ref names, like the --decorate option of linkgit:git-log[1]\n+'%D':: ref names without the \" (\", \")\" wrapping.\n+'%e':: encoding\n+'%s':: subject\n+'%f':: sanitized subject line, suitable for a filename\n+'%b':: body\n+'%B':: raw body (unwrapped subject and body)\n ifndef::git-rev-list[]\n-- '%N': commit notes\n+'%N':: commit notes\n endif::git-rev-list[]\n-- '%GG': raw verification message from GPG for a signed commit\n-- '%G?': show \"G\" for a good (valid) signature,\n- \"B\" for a bad signature,\n- \"U\" for a good signature with unknown validity,\n- \"X\" for a good signature that has expired,\n- \"Y\" for a good signature made by an expired key,\n- \"R\" for a good signature made by a revoked key,\n- \"E\" if the signature cannot be checked (e.g. missing key)\n- and \"N\" for no signature\n-- '%GS': show the name of the signer for a signed commit\n-- '%GK': show the key used to sign a signed commit\n-- '%GF': show the fingerprint of the key used to sign a signed commit\n-- '%GP': show the fingerprint of the primary key whose subkey was used\n- to sign a signed commit\n-- '%gD': reflog selector, e.g., `refs/stash@{1}` or\n- `refs/stash@{2 minutes ago`}; the format follows the rules described\n- for the `-g` option. The portion before the `@` is the refname as\n- given on the command line (so `git log -g refs/heads/master` would\n- yield `refs/heads/master@{0}`).\n-- '%gd': shortened reflog selector; same as `%gD`, but the refname\n- portion is shortened for human readability (so `refs/heads/master`\n- becomes just `master`).\n-- '%gn': reflog identity name\n-- '%gN': reflog identity name (respecting .mailmap, see\n- linkgit:git-shortlog[1] or linkgit:git-blame[1])\n-- '%ge': reflog identity email\n-- '%gE': reflog identity email (respecting .mailmap, see\n- linkgit:git-shortlog[1] or linkgit:git-blame[1])\n-- '%gs': reflog subject\n-- '%Cred': switch color to red\n-- '%Cgreen': switch color to green\n-- '%Cblue': switch color to blue\n-- '%Creset': reset color\n-- '%C(...)': color specification, as described under Values in the\n- \"CONFIGURATION FILE\" section of linkgit:git-config[1].\n- By default, colors are shown only when enabled for log output (by\n- `color.diff`, `color.ui`, or `--color`, and respecting the `auto`\n- settings of the former if we are going to a terminal). `%C(auto,...)`\n- is accepted as a historical synonym for the default (e.g.,\n- `%C(auto,red)`). Specifying `%C(always,...) will show the colors\n- even when color is not otherwise enabled (though consider\n- just using `--color=always` to enable color for the whole output,\n- including this format and anything else git might color). `auto`\n- alone (i.e. `%C(auto)`) will turn on auto coloring on the next\n- placeholders until the color is switched again.\n-- '%m': left (`<`), right (`>`) or boundary (`-`) mark\n-- '%n': newline\n-- '%%': a raw '%'\n-- '%x00': print a byte from a hex code\n-- '%w([[,[,]]])': switch line wrapping, like the -w option of\n- linkgit:git-shortlog[1].\n-- '%<([,trunc|ltrunc|mtrunc])': make the next placeholder take at\n- least N columns, padding spaces on the right if necessary.\n- Optionally truncate at the beginning (ltrunc), the middle (mtrunc)\n- or the end (trunc) if the output is longer than N columns.\n- Note that truncating only works correctly with N >= 2.\n-- '%<|()': make the next placeholder take at least until Nth\n- columns, padding spaces on the right if necessary\n-- '%>()', '%>|()': similar to '%<()', '%<|()'\n- respectively, but padding spaces on the left\n-- '%>>()', '%>>|()': similar to '%>()', '%>|()'\n- respectively, except that if the next placeholder takes more spaces\n- than given and there are spaces on its left, use those spaces\n-- '%><()', '%><|()': similar to '%<()', '%<|()'\n- respectively, but padding both sides (i.e. the text is centered)\n-- %(trailers[:options]): display the trailers of the body as interpreted\n- by linkgit:git-interpret-trailers[1]. The `trailers` string may be\n- followed by a colon and zero or more comma-separated options. If the\n- `only` option is given, omit non-trailer lines from the trailer block.\n- If the `unfold` option is given, behave as if interpret-trailer's\n- `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do\n- both.\n+'%GG':: raw verification message from GPG for a signed commit\n+'%G?':: show \"G\" for a good (valid) signature,\n+ \"B\" for a bad signature,\n+ \"U\" for a good signature with unknown validity,\n+ \"X\" for a good signature that has expired,\n+ \"Y\" for a good signature made by an expired key,\n+ \"R\" for a good signature made by a revoked key,\n+ \"E\" if the signature cannot be checked (e.g. missing key)\n+ and \"N\" for no signature\n+'%GS':: show the name of the signer for a signed commit\n+'%GK':: show the key used to sign a signed commit\n+'%GF':: show the fingerprint of the key used to sign a signed commit\n+'%GP':: show the fingerprint of the primary key whose subkey was used\n+ to sign a signed commit\n+'%gD':: reflog selector, e.g., `refs/stash@{1}` or `refs/stash@{2\n+ minutes ago`}; the format follows the rules described for the\n+ `-g` option. The portion before the `@` is the refname as\n+ given on the command line (so `git log -g refs/heads/master`\n+ would yield `refs/heads/master@{0}`).\n+'%gd':: shortened reflog selector; same as `%gD`, but the refname\n+ portion is shortened for human readability (so\n+ `refs/heads/master` becomes just `master`).\n+'%gn':: reflog identity name\n+'%gN':: reflog identity name (respecting .mailmap, see\n+ linkgit:git-shortlog[1] or linkgit:git-blame[1])\n+'%ge':: reflog identity email\n+'%gE':: reflog identity email (respecting .mailmap, see\n+ linkgit:git-shortlog[1] or linkgit:git-blame[1])\n+'%gs':: reflog subject\n+'%(trailers[:options])':: display the trailers of the body as\n+ interpreted by\n+ linkgit:git-interpret-trailers[1]. The\n+ `trailers` string may be followed by a colon\n+ and zero or more comma-separated options:\n+** 'only': omit non-trailer lines from the trailer block.\n+** 'unfold': make it behave as if interpret-trailer's `--unfold`\n+ option was given. E.g., `%(trailers:only,unfold)` unfolds and\n+ shows all trailer lines.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n-- \n2.17.1\n\n"},{"id":"364810","messageId":"20181208163647.19538-4-anders@0x63.nu","threadId":"49703","inReplyTo":"20181208163647.19538-1-anders@0x63.nu","subject":"[PATCH v4 3/7] pretty: single return path in %(trailers) handling","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-12-08T16:36:43Z","receivedAt":"2018-12-08T16:37:49Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"No functional change intended.\n\nThis change may not seem useful on its own, but upcoming commits will do\nmemory allocation in there, and a single return path makes deallocation\neasier.\n\nSigned-off-by: Anders Waldenborg \n---\n pretty.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 26efdba73a..044447e6c0 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1337,6 +1337,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n \n@@ -1350,8 +1351,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t}\n \t\tif (*arg == ')') {\n \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n-\t\t\treturn arg - placeholder + 1;\n+\t\t\tret = arg - placeholder + 1;\n \t\t}\n+\t\treturn ret;\n \t}\n \n \treturn 0;\t/* unknown placeholder */\n-- \n2.17.1\n\n"},{"id":"364811","messageId":"20181208163647.19538-6-anders@0x63.nu","threadId":"49703","inReplyTo":"20181208163647.19538-1-anders@0x63.nu","subject":"[PATCH v4 5/7] pretty: add support for \"valueonly\" option in %(trailers)","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-12-08T16:36:45Z","receivedAt":"2018-12-08T16:37:52Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"With the new \"key=\" option to %(trailers) it often makes little sense to\nshow the key, as it by definition already is knows which trailer is\nprinted there. This new \"valueonly\" option makes it omit the key when\nprinting trailers.\n\nE.g.:\n $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,valueonly)' aaaa88182\nwill show:\n > upload-pack: fix broken if/else chain in config callback\n > Jeff King \n > Junio C Hamano \n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 2 ++\n pretty.c | 3 ++-\n t/t4205-log-pretty-formats.sh | 6 ++++++\n trailer.c | 6 ++++--\n trailer.h | 1 +\n 5 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex d6add831c0..a920dd15b1 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -243,6 +243,8 @@ endif::git-rev-list[]\n option was given. In same way as to for `only` it can be followed\n by an equal sign and explicit value. E.g.,\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n+** 'valueonly[=val]': skip over the key part of the trailer line and only\n+ show the value part. Also this optionally allows explicit value.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex 541a553ccc..c508357606 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1383,7 +1383,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\topts.filter_data = &filter_list;\n \t\t\t\t\topts.only_trailers = 1;\n \t\t\t\t} else if (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n-\t\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n+\t\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold) &&\n+\t\t\t\t\t !match_placeholder_bool_arg(arg, \"valueonly\", &arg, &opts.value_only))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 54239290cf..22336c5485 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -667,6 +667,12 @@ test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes non\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,valueonly)\" >actual &&\n+\techo \"A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex d6da555cd7..d0d9e91631 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tif (!opts->filter || opts->filter(&tok, opts->filter_data)) {\n \t\t\t\tif (opts->unfold)\n \t\t\t\t\tunfold_value(&val);\n-\n-\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t\tif (!opts->value_only)\n+\t\t\t\t\tstrbuf_addf(out, \"%s: \", tok.buf);\n+\t\t\t\tstrbuf_addbuf(out, &val);\n+\t\t\t\tstrbuf_addch(out, '\\n');\n \t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\ndiff --git a/trailer.h b/trailer.h\nindex 5255b676de..06d417fe93 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -72,6 +72,7 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tint value_only;\n \tint (*filter)(const struct strbuf *, void *);\n \tvoid *filter_data;\n };\n-- \n2.17.1\n\n"},{"id":"364812","messageId":"20181208163647.19538-7-anders@0x63.nu","threadId":"49703","inReplyTo":"20181208163647.19538-1-anders@0x63.nu","subject":"[PATCH v4 6/7] strbuf: separate callback for strbuf_expand:ing literals","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-12-08T16:36:46Z","receivedAt":"2018-12-08T16:37:53Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"Expanding '%n' and '%xNN' is generic functionality, so extract that from\nthe pretty.c formatter into a callback that can be reused.\n\nNo functional change intended\n\nSigned-off-by: Anders Waldenborg \n---\n pretty.c | 16 +++++-----------\n strbuf.c | 21 +++++++++++++++++++++\n strbuf.h | 8 ++++++++\n 3 files changed, 34 insertions(+), 11 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex c508357606..50d0b5830d 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1133,9 +1133,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tconst char *msg = c->message;\n \tstruct commit_list *p;\n \tconst char *arg;\n-\tint ch;\n+\tsize_t res;\n \n \t/* these are independent of the commit */\n+\tres = strbuf_expand_literal_cb(sb, placeholder, NULL);\n+\tif (res)\n+\t\treturn res;\n+\n \tswitch (placeholder[0]) {\n \tcase 'C':\n \t\tif (starts_with(placeholder + 1, \"(auto)\")) {\n@@ -1154,16 +1158,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t */\n \t\t\treturn ret;\n \t\t}\n-\tcase 'n':\t\t/* newline */\n-\t\tstrbuf_addch(sb, '\\n');\n-\t\treturn 1;\n-\tcase 'x':\n-\t\t/* %x00 == NUL, %x0a == LF, etc. */\n-\t\tch = hex2chr(placeholder + 1);\n-\t\tif (ch < 0)\n-\t\t\treturn 0;\n-\t\tstrbuf_addch(sb, ch);\n-\t\treturn 3;\n \tcase 'w':\n \t\tif (placeholder[1] == '(') {\n \t\t\tunsigned long width = 0, indent1 = 0, indent2 = 0;\ndiff --git a/strbuf.c b/strbuf.c\nindex f6a6cf78b9..78eecd29f7 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -380,6 +380,27 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,\n \t}\n }\n \n+size_t strbuf_expand_literal_cb(struct strbuf *sb,\n+\t\t\t\tconst char *placeholder,\n+\t\t\t\tvoid *context)\n+{\n+\tint ch;\n+\n+\tswitch (placeholder[0]) {\n+\tcase 'n':\t\t/* newline */\n+\t\tstrbuf_addch(sb, '\\n');\n+\t\treturn 1;\n+\tcase 'x':\n+\t\t/* %x00 == NUL, %x0a == LF, etc. */\n+\t\tch = hex2chr(placeholder + 1);\n+\t\tif (ch < 0)\n+\t\t\treturn 0;\n+\t\tstrbuf_addch(sb, ch);\n+\t\treturn 3;\n+\t}\n+\treturn 0;\n+}\n+\n size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,\n \t\tvoid *context)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex fc40873b65..52e44c9ab8 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -320,6 +320,14 @@ void strbuf_expand(struct strbuf *sb,\n \t\t expand_fn_t fn,\n \t\t void *context);\n \n+/**\n+ * Used as callback for `strbuf_expand` to only expand literals\n+ * (i.e. %n and %xNN). The context argument is ignored.\n+ */\n+size_t strbuf_expand_literal_cb(struct strbuf *sb,\n+\t\t\t\tconst char *placeholder,\n+\t\t\t\tvoid *context);\n+\n /**\n * Used as callback for `strbuf_expand()`, expects an array of\n * struct strbuf_expand_dict_entry as context, i.e. pairs of\n-- \n2.17.1\n\n"},{"id":"364813","messageId":"20181208163647.19538-8-anders@0x63.nu","threadId":"49703","inReplyTo":"20181208163647.19538-1-anders@0x63.nu","subject":"[PATCH v4 7/7] pretty: add support for separator option in %(trailers)","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-12-08T16:36:47Z","receivedAt":"2018-12-08T16:37:54Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"By default trailer lines are terminated by linebreaks ('\\n'). By\nspecifying the new 'separator' option they will instead be separated by\nuser provided string and have separator semantics rather than terminator\nsemantics. The separator string can contain the literal formatting codes\n%n and %xNN allowing it to be things that are otherwise hard to type\nsuch as %x00, or comma and end-parenthesis which would break parsing.\n\nE.g:\n $ git log --pretty='%(trailers:key=Reviewed-by,valueonly,separator=%x00)'\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 9 ++++++++\n pretty.c | 10 +++++++++\n t/t4205-log-pretty-formats.sh | 36 ++++++++++++++++++++++++++++++++\n trailer.c | 15 +++++++++++--\n trailer.h | 1 +\n 5 files changed, 69 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex a920dd15b1..ce087dee80 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -239,6 +239,15 @@ endif::git-rev-list[]\n `false`, `off`, `no` to show the non-trailer lines. If option is\n given without value it is enabled. If given multiple times the last\n value is used.\n+** 'separator=': specify a separator inserted between trailer\n+ lines. When this option is not given each trailer line is\n+ terminated with a line feed character. The string SEP may contain\n+ the literal formatting codes described above. To use comma as\n+ separator one must use `%x2C` as it would otherwise be parsed as\n+ next option. If separator option is given multiple times only the\n+ last one is used. E.g., `%(trailers:key=Ticket,separator=%x2C )`\n+ shows all trailer lines whose key is \"Ticket\" separated by a comma\n+ and a space.\n ** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`\n option was given. In same way as to for `only` it can be followed\n by an equal sign and explicit value. E.g.,\ndiff --git a/pretty.c b/pretty.c\nindex 50d0b5830d..c7609493ee 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1357,6 +1357,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n \t\tstruct string_list filter_list = STRING_LIST_INIT_NODUP;\n+\t\tstruct strbuf sepbuf = STRBUF_INIT;\n \t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n@@ -1376,6 +1377,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\topts.filter = format_trailer_match_cb;\n \t\t\t\t\topts.filter_data = &filter_list;\n \t\t\t\t\topts.only_trailers = 1;\n+\t\t\t\t} else if (match_placeholder_arg_value(arg, \"separator\", &arg, &argval, &arglen)) {\n+\t\t\t\t\tchar *fmt;\n+\n+\t\t\t\t\tstrbuf_reset(&sepbuf);\n+\t\t\t\t\tfmt = xstrndup(argval, arglen);\n+\t\t\t\t\tstrbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);\n+\t\t\t\t\tfree(fmt);\n+\t\t\t\t\topts.separator = &sepbuf;\n \t\t\t\t} else if (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n \t\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold) &&\n \t\t\t\t\t !match_placeholder_bool_arg(arg, \"valueonly\", &arg, &opts.value_only))\n@@ -1387,6 +1396,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\tret = arg - placeholder + 1;\n \t\t}\n \t\tstring_list_clear (&filter_list, 0);\n+\t\tstrbuf_release(&sepbuf);\n \t\treturn ret;\n \t}\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 22336c5485..282369dac0 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -673,6 +673,42 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:separator) changes separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n+\tprintf \"XSigned-off-by: A U Thor \\0Acked-by: A U Thor \\0[ v2 updated patch description ]\\0Signed-off-by: A U Thor X\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tImportant fix\n+\n+\tThe fix is explained here\n+\n+\tCloses: #1234\n+\tEOF\n+\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tAnother fix\n+\n+\tThe fix is explained here\n+\n+\tCloses: #567\n+\tCloses: #890\n+\tEOF\n+\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tDoes not close any tickets\n+\tEOF\n+\n+\tgit log --pretty=\"%s% (trailers:separator=%x2c%x20,key=Closes,valueonly)\" HEAD~3.. >actual &&\n+\ttest_write_lines \\\n+\t\t\"Does not close any tickets\" \\\n+\t\t\"Another fix #567, #890\" \\\n+\t\t\"Important fix #1234\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex d0d9e91631..0c414f2fed 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\tconst struct trailer_info *info,\n \t\t\t\tconst struct process_trailer_options *opts)\n {\n+\tsize_t origlen = out->len;\n \tsize_t i;\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n-\tif (!opts->only_trailers && !opts->unfold && !opts->filter) {\n+\tif (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1150,16 +1151,26 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tif (!opts->filter || opts->filter(&tok, opts->filter_data)) {\n \t\t\t\tif (opts->unfold)\n \t\t\t\t\tunfold_value(&val);\n+\n+\t\t\t\tif (opts->separator && out->len != origlen)\n+\t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n \t\t\t\t\tstrbuf_addf(out, \"%s: \", tok.buf);\n \t\t\t\tstrbuf_addbuf(out, &val);\n-\t\t\t\tstrbuf_addch(out, '\\n');\n+\t\t\t\tif (!opts->separator)\n+\t\t\t\t\tstrbuf_addch(out, '\\n');\n \t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\n \n \t\t} else if (!opts->only_trailers) {\n+\t\t\tif (opts->separator && out->len != origlen) {\n+\t\t\t\tstrbuf_addbuf(out, opts->separator);\n+\t\t\t}\n \t\t\tstrbuf_addstr(out, trailer);\n+\t\t\tif (opts->separator) {\n+\t\t\t\tstrbuf_rtrim(out);\n+\t\t\t}\n \t\t}\n \t}\n \ndiff --git a/trailer.h b/trailer.h\nindex 06d417fe93..203acf4ee1 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -73,6 +73,7 @@ struct process_trailer_options {\n \tint unfold;\n \tint no_divider;\n \tint value_only;\n+\tconst struct strbuf *separator;\n \tint (*filter)(const struct strbuf *, void *);\n \tvoid *filter_data;\n };\n-- \n2.17.1\n\n"},{"id":"364814","messageId":"20181208163647.19538-5-anders@0x63.nu","threadId":"49703","inReplyTo":"20181208163647.19538-1-anders@0x63.nu","subject":"[PATCH v4 4/7] pretty: allow showing specific trailers","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-12-08T16:36:44Z","receivedAt":"2018-12-08T16:37:56Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"Adds a new \"key=X\" option to \"%(trailers)\" which will cause it to only\nprint trailer lines which match any of the specified keys.\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 8 +++++\n pretty.c | 47 ++++++++++++++++++++++++++---\n t/t4205-log-pretty-formats.sh | 51 ++++++++++++++++++++++++++++++++\n trailer.c | 10 ++++---\n trailer.h | 2 ++\n 5 files changed, 110 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex d33b072eb2..d6add831c0 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -225,6 +225,14 @@ endif::git-rev-list[]\n linkgit:git-interpret-trailers[1]. The\n `trailers` string may be followed by a colon\n and zero or more comma-separated options:\n+** 'key=': only show trailers with specified key. Matching is done\n+ case-insensitively and trailing colon is optional. If option is\n+ given multiple times trailer lines matching any of the keys are\n+ shown. This option automatically enables the `only` option so that\n+ non-trailer lines in the trailer block are hidden. If that is not\n+ desired it can be disabled with `only=false`. E.g.,\n+ `%(trailers:key=Reviewed-by)` shows trailer lines with key\n+ `Reviewed-by`.\n ** 'only[=val]': select whether non-trailer lines from the trailer\n block should be included. The `only` keyword may optionally be\n followed by an equal sign and one of `true`, `on`, `yes` to omit or\ndiff --git a/pretty.c b/pretty.c\nindex 044447e6c0..541a553ccc 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1056,13 +1056,20 @@ static size_t parse_padding_placeholder(struct strbuf *sb,\n \treturn 0;\n }\n \n-static int match_placeholder_arg(const char *to_parse, const char *candidate,\n-\t\t\t\t const char **end)\n+static int match_placeholder_arg_value(const char *to_parse, const char *candidate,\n+\t\t\t\t const char **end, const char **valuestart, size_t *valuelen)\n {\n \tconst char *p;\n \n \tif (!(skip_prefix(to_parse, candidate, &p)))\n \t\treturn 0;\n+\tif (valuestart) {\n+\t\tif (*p != '=')\n+\t\t\treturn 0;\n+\t\t*valuestart = p + 1;\n+\t\t*valuelen = strcspn(*valuestart, \",)\");\n+\t\tp = *valuestart + *valuelen;\n+\t}\n \tif (*p == ',') {\n \t\t*end = p + 1;\n \t\treturn 1;\n@@ -1074,6 +1081,12 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,\n \treturn 0;\n }\n \n+static int match_placeholder_arg(const char *to_parse, const char *candidate,\n+\t\t\t\t const char **end)\n+{\n+\treturn match_placeholder_arg_value(to_parse, candidate, end, NULL, NULL);\n+}\n+\n static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,\n \t\t\t\t const char **end, int *val)\n {\n@@ -1095,7 +1108,19 @@ static int match_placeholder_bool_arg(const char *to_parse, const char *candidat\n \t\t*val = 1;\n \t\treturn 1;\n \t}\n+\treturn 0;\n+}\n \n+static int format_trailer_match_cb(const struct strbuf *key, void *ud)\n+{\n+\tconst struct string_list *list = ud;\n+\tconst struct string_list_item *item;\n+\n+\tfor_each_string_list_item (item, list) {\n+\t\tif (key->len == (uintptr_t)item->util &&\n+\t\t !strncasecmp (item->string, key->buf, key->len))\n+\t\t\treturn 1;\n+\t}\n \treturn 0;\n }\n \n@@ -1337,6 +1362,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\t\tstruct string_list filter_list = STRING_LIST_INIT_NODUP;\n \t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n@@ -1344,8 +1370,20 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tif (*arg == ':') {\n \t\t\targ++;\n \t\t\tfor (;;) {\n-\t\t\t\tif (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n-\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n+\t\t\t\tconst char *argval;\n+\t\t\t\tsize_t arglen;\n+\n+\t\t\t\tif (match_placeholder_arg_value(arg, \"key\", &arg, &argval, &arglen)) {\n+\t\t\t\t\tuintptr_t len = arglen;\n+\t\t\t\t\tif (len && argval[len - 1] == ':')\n+\t\t\t\t\t\tlen--;\n+\t\t\t\t\tstring_list_append(&filter_list, argval)->util = (char *)len;\n+\n+\t\t\t\t\topts.filter = format_trailer_match_cb;\n+\t\t\t\t\topts.filter_data = &filter_list;\n+\t\t\t\t\topts.only_trailers = 1;\n+\t\t\t\t} else if (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n+\t\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\n@@ -1353,6 +1391,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n \t\t\tret = arg - placeholder + 1;\n \t\t}\n+\t\tstring_list_clear (&filter_list, 0);\n \t\treturn ret;\n \t}\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 63730a4ec0..54239290cf 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -616,6 +616,57 @@ test_expect_success ':only and :unfold work together' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by)\" >actual &&\n+\techo \"Acked-by: A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=AcKed-bY)\" >actual &&\n+\techo \"Acked-by: A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo:) trailing colon also works' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by:)\" >actual &&\n+\techo \"Acked-by: A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo) multiple keys' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by:,key=Signed-off-By)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=nonexistant) becomes empty' '\n+\tgit log --no-walk --pretty=\"x%(trailers:key=Nacked-by)x\" >actual &&\n+\techo \"xx\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Signed-Off-by)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Signed-Off-by,unfold)\" >actual &&\n+\tunfold expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,only=no)\" >actual &&\n+\t{\n+\t\techo \"Acked-by: A U Thor \" &&\n+\t\tgrep patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex 0796f326b3..d6da555cd7 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,\n \tsize_t i;\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n-\tif (!opts->only_trailers && !opts->unfold) {\n+\tif (!opts->only_trailers && !opts->unfold && !opts->filter) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tstruct strbuf val = STRBUF_INIT;\n \n \t\t\tparse_trailer(&tok, &val, NULL, trailer, separator_pos);\n-\t\t\tif (opts->unfold)\n-\t\t\t\tunfold_value(&val);\n+\t\t\tif (!opts->filter || opts->filter(&tok, opts->filter_data)) {\n+\t\t\t\tif (opts->unfold)\n+\t\t\t\t\tunfold_value(&val);\n \n-\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\n \ndiff --git a/trailer.h b/trailer.h\nindex b997739649..5255b676de 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -72,6 +72,8 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tint (*filter)(const struct strbuf *, void *);\n+\tvoid *filter_data;\n };\n \n #define PROCESS_TRAILER_OPTIONS_INIT {0}\n-- \n2.17.1\n\n"},{"id":"364911","messageId":"xmqqa7ldkbwr.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"20181208163647.19538-3-anders@0x63.nu","subject":"Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-10T08:45:40Z","receivedAt":"2018-12-10T08:45:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> In addition to old %(trailers:only) it is now allowed to write\n> %(trailers:only=yes)\n\ns/$/. Similarly the unfold option can take a boolean./\n\n> By itself this only gives (the not quite so useful) possibility to have\n> users change their mind in the middle of a formatting\n> string (%(trailers:only=true,only=false)). However, it gives users the\n> opportunity to override defaults from future options.\n\nMakes sense.\n\n> +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`\n> + option was given. In same way as to for `only` it can be followed\n> + by an equal sign and explicit value. E.g.,\n> + `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n\n> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,\n> +\t\t\t\t const char **end, int *val)\n> +{\n> +\tconst char *p;\n> +\tif (!skip_prefix(to_parse, candidate, &p))\n> +\t\treturn 0;\n> +\n> +\tif (match_placeholder_arg(p, \"=no\", end) ||\n> +\t match_placeholder_arg(p, \"=off\", end) ||\n> +\t match_placeholder_arg(p, \"=false\", end)) {\n> +\t\t*val = 0;\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tif (match_placeholder_arg(p, \"\", end) ||\n> +\t match_placeholder_arg(p, \"=yes\", end) ||\n> +\t match_placeholder_arg(p, \"=on\", end) ||\n> +\t match_placeholder_arg(p, \"=true\", end)) {\n> +\t\t*val = 1;\n> +\t\treturn 1;\n> +\t}\n\nHmph. Is there a possibility to arrenge the code so that we do not\nhave to maintain these variants of true/false representations here,\nwhen we should already have one in config.c?\n\nThe match_placeholder_arg() function is a bit too limiting as it can\nonly recognize the value that we know about for a thing like this.\nInstead, perhaps we can cut what follows \"=\" syntactically, looking\nfor either NUL, ',', or ')', and then call git_parse_maybe_bool() on\nit. That way, we can handle %(trailers:only=bogo) more sensibly,\nno? Syntactically we can recognize that the user wanted to give\n'bogo' as the value to 'only', and say \"'bogo' is not a boolean\" if\nwe did so.\n\n> +\treturn 0;\n> +}\n> +\n"},{"id":"364913","messageId":"xmqqy38xiwv2.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"20181208163647.19538-5-anders@0x63.nu","subject":"Re: [PATCH v4 4/7] pretty: allow showing specific trailers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-10T08:56:01Z","receivedAt":"2018-12-10T08:56:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> -static int match_placeholder_arg(const char *to_parse, const char *candidate,\n> -\t\t\t\t const char **end)\n> +static int match_placeholder_arg_value(const char *to_parse, const char *candidate,\n> +\t\t\t\t const char **end, const char **valuestart, size_t *valuelen)\n> {\n> \tconst char *p;\n> \n> \tif (!(skip_prefix(to_parse, candidate, &p)))\n> \t\treturn 0;\n> +\tif (valuestart) {\n> +\t\tif (*p != '=')\n> +\t\t\treturn 0;\n> +\t\t*valuestart = p + 1;\n> +\t\t*valuelen = strcspn(*valuestart, \",)\");\n> +\t\tp = *valuestart + *valuelen;\n> +\t}\n> \tif (*p == ',') {\n> \t\t*end = p + 1;\n> \t\treturn 1;\n> @@ -1074,6 +1081,12 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,\n> \treturn 0;\n> }\n> \n> +static int match_placeholder_arg(const char *to_parse, const char *candidate,\n> +\t\t\t\t const char **end)\n> +{\n> +\treturn match_placeholder_arg_value(to_parse, candidate, end, NULL, NULL);\n> +}\n> +\n\nOK. The unified parsing of boolean value I mentioned on an earlier\nstep can naturally be done using martch_placeholder_arg_value(), I\nthink, in match_placeholder_bool_arg().\n\n> +static int format_trailer_match_cb(const struct strbuf *key, void *ud)\n> +{\n> +\tconst struct string_list *list = ud;\n> +\tconst struct string_list_item *item;\n> +\n> +\tfor_each_string_list_item (item, list) {\n> +\t\tif (key->len == (uintptr_t)item->util &&\n> +\t\t !strncasecmp (item->string, key->buf, key->len))\n\nRemove SP after strncasecmp.\n\nWe won't have too many elements in this string list, so O(N*M)\nsearch like this one would be OK.\n\n> +\t\t\treturn 1;\n> +\t}\n> \treturn 0;\n> }\n> \n> @@ -1337,6 +1362,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> \n> \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n> \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n> +\t\tstruct string_list filter_list = STRING_LIST_INIT_NODUP;\n> \t\tsize_t ret = 0;\n> \n> \t\topts.no_divider = 1;\n> @@ -1344,8 +1370,20 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> \t\tif (*arg == ':') {\n> \t\t\targ++;\n> \t\t\tfor (;;) {\n> -\t\t\t\tif (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n> -\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n> +\t\t\t\tconst char *argval;\n> +\t\t\t\tsize_t arglen;\n> +\n> +\t\t\t\tif (match_placeholder_arg_value(arg, \"key\", &arg, &argval, &arglen)) {\n> +\t\t\t\t\tuintptr_t len = arglen;\n> +\t\t\t\t\tif (len && argval[len - 1] == ':')\n> +\t\t\t\t\t\tlen--;\n> +\t\t\t\t\tstring_list_append(&filter_list, argval)->util = (char *)len;\n> +\n> +\t\t\t\t\topts.filter = format_trailer_match_cb;\n> +\t\t\t\t\topts.filter_data = &filter_list;\n> +\t\t\t\t\topts.only_trailers = 1;\n> +\t\t\t\t} else if (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n> +\t\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n> \t\t\t\t\tbreak;\n> \t\t\t}\n> \t\t}\n> @@ -1353,6 +1391,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n> \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n> \t\t\tret = arg - placeholder + 1;\n> \t\t}\n> +\t\tstring_list_clear (&filter_list, 0);\n\nRemove SP after string_list_clear.\n\n> \t\treturn ret;\n> \t}\n> \n"},{"id":"365554","messageId":"87o99iwmjn.fsf@0x63.nu","threadId":"49703","inReplyTo":"xmqqa7ldkbwr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2018-12-18T21:30:04Z","receivedAt":"2018-12-18T21:30:24Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJunio C Hamano writes:\n> That way, we can handle %(trailers:only=bogo) more sensibly,\n> no? Syntactically we can recognize that the user wanted to give\n> 'bogo' as the value to 'only', and say \"'bogo' is not a boolean\" if\n> we did so.\n\nI agree that proper error reporting for the pretty formatting strings\nwould be great. But that would depart from the current extremely crude\nerror handling where incorrect formatting placeholders are just left\nunexpanded. How would such change in error handling be done safely, wrt\nbackwards compatibility changes?\n\nTo get good diagnostics for incorrect formatting strings I think the way\nforward is to have the formatting strings parsed once into some kind of\nAST or machine (as also mentioned by Jeff) that is just executed many\ntimes, instead of parsed each time like today.\n\n anders\n"},{"id":"367883","messageId":"20190128213337.24752-1-anders@0x63.nu","threadId":"49703","inReplyTo":"20181028125025.30952-1-anders@0x63.nu","subject":"[PATCH v5 0/7] %(trailers) improvements in pretty format","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-28T21:33:30Z","receivedAt":"2019-01-28T21:51:08Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"Updates since v4:\n * Coding style fixes\n * Reuse git_parse_maybe_bool for bool parsing\n\nAnders Waldenborg (7):\n doc: group pretty-format.txt placeholders descriptions\n pretty: Allow %(trailers) options with explicit value\n pretty: single return path in %(trailers) handling\n pretty: allow showing specific trailers\n pretty: add support for \"valueonly\" option in %(trailers)\n strbuf: separate callback for strbuf_expand:ing literals\n pretty: add support for separator option in %(trailers)\n\n Documentation/pretty-formats.txt | 260 ++++++++++++++++++-------------\n pretty.c | 113 +++++++++++---\n strbuf.c | 21 +++\n strbuf.h | 8 +\n t/t4205-log-pretty-formats.sh | 117 ++++++++++++++\n trailer.c | 25 ++-\n trailer.h | 4 +\n 7 files changed, 415 insertions(+), 133 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"367884","messageId":"20190128213337.24752-3-anders@0x63.nu","threadId":"49703","inReplyTo":"20190128213337.24752-1-anders@0x63.nu","subject":"[PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-28T21:33:32Z","receivedAt":"2019-01-28T21:51:10Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"In addition to old %(trailers:only) it is now allowed to write\n%(trailers:only=yes)\n\nBy itself this only gives (the not quite so useful) possibility to have\nusers change their mind in the middle of a formatting\nstring (%(trailers:only=true,only=false)). However, it gives users the\nopportunity to override defaults from future options.\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 14 ++++++---\n pretty.c | 52 +++++++++++++++++++++++++++-----\n t/t4205-log-pretty-formats.sh | 18 +++++++++++\n 3 files changed, 73 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 86d804fe97..d33b072eb2 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -225,10 +225,16 @@ endif::git-rev-list[]\n linkgit:git-interpret-trailers[1]. The\n `trailers` string may be followed by a colon\n and zero or more comma-separated options:\n-** 'only': omit non-trailer lines from the trailer block.\n-** 'unfold': make it behave as if interpret-trailer's `--unfold`\n- option was given. E.g., `%(trailers:only,unfold)` unfolds and\n- shows all trailer lines.\n+** 'only[=val]': select whether non-trailer lines from the trailer\n+ block should be included. The `only` keyword may optionally be\n+ followed by an equal sign and one of `true`, `on`, `yes` to omit or\n+ `false`, `off`, `no` to show the non-trailer lines. If option is\n+ given without value it is enabled. If given multiple times the last\n+ value is used.\n+** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`\n+ option was given. In same way as to for `only` it can be followed\n+ by an equal sign and explicit value. E.g.,\n+ `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex b83a3ecd23..b8d71a57c9 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1056,13 +1056,25 @@ static size_t parse_padding_placeholder(struct strbuf *sb,\n \treturn 0;\n }\n \n-static int match_placeholder_arg(const char *to_parse, const char *candidate,\n-\t\t\t\t const char **end)\n+static int match_placeholder_arg_value(const char *to_parse, const char *candidate,\n+\t\t\t\t const char **end, const char **valuestart, size_t *valuelen)\n {\n \tconst char *p;\n \n \tif (!(skip_prefix(to_parse, candidate, &p)))\n \t\treturn 0;\n+\tif (valuestart) {\n+\t\tif (*p == '=') {\n+\t\t\t*valuestart = p + 1;\n+\t\t\t*valuelen = strcspn(*valuestart, \",)\");\n+\t\t\tp = *valuestart + *valuelen;\n+\t\t} else {\n+\t\t\tif (*p != ',' && *p != ')')\n+\t\t\t\treturn 0;\n+\t\t\t*valuestart = NULL;\n+\t\t\t*valuelen = 0;\n+\t\t}\n+\t}\n \tif (*p == ',') {\n \t\t*end = p + 1;\n \t\treturn 1;\n@@ -1074,6 +1086,35 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,\n \treturn 0;\n }\n \n+static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,\n+\t\t\t\t const char **end, int *val)\n+{\n+\tchar buf[8];\n+\tconst char *strval;\n+\tsize_t len;\n+\tint v;\n+\n+\tif (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len))\n+\t\treturn 0;\n+\n+\tif (!strval) {\n+\t\t*val = 1;\n+\t\treturn 1;\n+\t}\n+\n+\tstrlcpy(buf, strval, sizeof(buf));\n+\tif (len < sizeof(buf))\n+\t\tbuf[len] = 0;\n+\n+\tv = git_parse_maybe_bool(buf);\n+\tif (v == -1)\n+\t\treturn 0;\n+\n+\t*val = v;\n+\n+\treturn 1;\n+}\n+\n static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\tconst char *placeholder,\n \t\t\t\tvoid *context)\n@@ -1318,11 +1359,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tif (*arg == ':') {\n \t\t\targ++;\n \t\t\tfor (;;) {\n-\t\t\t\tif (match_placeholder_arg(arg, \"only\", &arg))\n-\t\t\t\t\topts.only_trailers = 1;\n-\t\t\t\telse if (match_placeholder_arg(arg, \"unfold\", &arg))\n-\t\t\t\t\topts.unfold = 1;\n-\t\t\t\telse\n+\t\t\t\tif (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n+\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 978a8a66ff..63730a4ec0 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:only=yes) shows only \"key: value\" trailers' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:only=yes)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:only=no) shows all trailers' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:only=no)\" >actual &&\n+\tcat trailers >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:only=no,only=true) shows only \"key: value\" trailers' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:only=yes)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(trailers:unfold) unfolds trailers' '\n \tgit log --no-walk --pretty=\"%(trailers:unfold)\" >actual &&\n \t{\n-- \n2.17.1\n\n"},{"id":"367885","messageId":"20190128213337.24752-6-anders@0x63.nu","threadId":"49703","inReplyTo":"20190128213337.24752-1-anders@0x63.nu","subject":"[PATCH v5 5/7] pretty: add support for \"valueonly\" option in %(trailers)","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-28T21:33:35Z","receivedAt":"2019-01-28T21:51:13Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"With the new \"key=\" option to %(trailers) it often makes little sense to\nshow the key, as it by definition already is knows which trailer is\nprinted there. This new \"valueonly\" option makes it omit the key when\nprinting trailers.\n\nE.g.:\n $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,valueonly)' aaaa88182\nwill show:\n > upload-pack: fix broken if/else chain in config callback\n > Jeff King \n > Junio C Hamano \n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 2 ++\n pretty.c | 3 ++-\n t/t4205-log-pretty-formats.sh | 6 ++++++\n trailer.c | 6 ++++--\n trailer.h | 1 +\n 5 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex d6add831c0..a920dd15b1 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -243,6 +243,8 @@ endif::git-rev-list[]\n option was given. In same way as to for `only` it can be followed\n by an equal sign and explicit value. E.g.,\n `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n+** 'valueonly[=val]': skip over the key part of the trailer line and only\n+ show the value part. Also this optionally allows explicit value.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex d3dd2d6254..ed25845c98 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1391,7 +1391,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\topts.filter_data = &filter_list;\n \t\t\t\t\topts.only_trailers = 1;\n \t\t\t\t} else if (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n-\t\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n+\t\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold) &&\n+\t\t\t\t\t !match_placeholder_bool_arg(arg, \"valueonly\", &arg, &opts.value_only))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex d87201afbe..1ad6834781 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -673,6 +673,12 @@ test_expect_success '%(trailers:key) without value is error' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,valueonly)\" >actual &&\n+\techo \"A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex d6da555cd7..d0d9e91631 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tif (!opts->filter || opts->filter(&tok, opts->filter_data)) {\n \t\t\t\tif (opts->unfold)\n \t\t\t\t\tunfold_value(&val);\n-\n-\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t\tif (!opts->value_only)\n+\t\t\t\t\tstrbuf_addf(out, \"%s: \", tok.buf);\n+\t\t\t\tstrbuf_addbuf(out, &val);\n+\t\t\t\tstrbuf_addch(out, '\\n');\n \t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\ndiff --git a/trailer.h b/trailer.h\nindex 5255b676de..06d417fe93 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -72,6 +72,7 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tint value_only;\n \tint (*filter)(const struct strbuf *, void *);\n \tvoid *filter_data;\n };\n-- \n2.17.1\n\n"},{"id":"367886","messageId":"20190128213337.24752-8-anders@0x63.nu","threadId":"49703","inReplyTo":"20190128213337.24752-1-anders@0x63.nu","subject":"[PATCH v5 7/7] pretty: add support for separator option in %(trailers)","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-28T21:33:37Z","receivedAt":"2019-01-28T21:51:15Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"By default trailer lines are terminated by linebreaks ('\\n'). By\nspecifying the new 'separator' option they will instead be separated by\nuser provided string and have separator semantics rather than terminator\nsemantics. The separator string can contain the literal formatting codes\n%n and %xNN allowing it to be things that are otherwise hard to type\nsuch as %x00, or comma and end-parenthesis which would break parsing.\n\nE.g:\n $ git log --pretty='%(trailers:key=Reviewed-by,valueonly,separator=%x00)'\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 9 ++++++++\n pretty.c | 10 +++++++++\n t/t4205-log-pretty-formats.sh | 36 ++++++++++++++++++++++++++++++++\n trailer.c | 15 +++++++++++--\n trailer.h | 1 +\n 5 files changed, 69 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex a920dd15b1..ce087dee80 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -239,6 +239,15 @@ endif::git-rev-list[]\n `false`, `off`, `no` to show the non-trailer lines. If option is\n given without value it is enabled. If given multiple times the last\n value is used.\n+** 'separator=': specify a separator inserted between trailer\n+ lines. When this option is not given each trailer line is\n+ terminated with a line feed character. The string SEP may contain\n+ the literal formatting codes described above. To use comma as\n+ separator one must use `%x2C` as it would otherwise be parsed as\n+ next option. If separator option is given multiple times only the\n+ last one is used. E.g., `%(trailers:key=Ticket,separator=%x2C )`\n+ shows all trailer lines whose key is \"Ticket\" separated by a comma\n+ and a space.\n ** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`\n option was given. In same way as to for `only` it can be followed\n by an equal sign and explicit value. E.g.,\ndiff --git a/pretty.c b/pretty.c\nindex 7baa4c1c26..99b66ccf5a 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1361,6 +1361,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n \t\tstruct string_list filter_list = STRING_LIST_INIT_NODUP;\n+\t\tstruct strbuf sepbuf = STRBUF_INIT;\n \t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n@@ -1384,6 +1385,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\t\topts.filter = format_trailer_match_cb;\n \t\t\t\t\topts.filter_data = &filter_list;\n \t\t\t\t\topts.only_trailers = 1;\n+\t\t\t\t} else if (match_placeholder_arg_value(arg, \"separator\", &arg, &argval, &arglen)) {\n+\t\t\t\t\tchar *fmt;\n+\n+\t\t\t\t\tstrbuf_reset(&sepbuf);\n+\t\t\t\t\tfmt = xstrndup(argval, arglen);\n+\t\t\t\t\tstrbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL);\n+\t\t\t\t\tfree(fmt);\n+\t\t\t\t\topts.separator = &sepbuf;\n \t\t\t\t} else if (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n \t\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold) &&\n \t\t\t\t\t !match_placeholder_bool_arg(arg, \"valueonly\", &arg, &opts.value_only))\n@@ -1396,6 +1405,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t}\n \ttrailer_out:\n \t\tstring_list_clear(&filter_list, 0);\n+\t\tstrbuf_release(&sepbuf);\n \t\treturn ret;\n \t}\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 1ad6834781..99f50fa401 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -679,6 +679,42 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:separator) changes separator' '\n+\tgit log --no-walk --pretty=format:\"X%(trailers:separator=%x00,unfold)X\" >actual &&\n+\tprintf \"XSigned-off-by: A U Thor \\0Acked-by: A U Thor \\0[ v2 updated patch description ]\\0Signed-off-by: A U Thor X\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' '\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tImportant fix\n+\n+\tThe fix is explained here\n+\n+\tCloses: #1234\n+\tEOF\n+\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tAnother fix\n+\n+\tThe fix is explained here\n+\n+\tCloses: #567\n+\tCloses: #890\n+\tEOF\n+\n+\tgit commit --allow-empty -F - <<-\\EOF &&\n+\tDoes not close any tickets\n+\tEOF\n+\n+\tgit log --pretty=\"%s% (trailers:separator=%x2c%x20,key=Closes,valueonly)\" HEAD~3.. >actual &&\n+\ttest_write_lines \\\n+\t\t\"Does not close any tickets\" \\\n+\t\t\"Another fix #567, #890\" \\\n+\t\t\"Important fix #1234\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex d0d9e91631..0c414f2fed 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\t\tconst struct trailer_info *info,\n \t\t\t\tconst struct process_trailer_options *opts)\n {\n+\tsize_t origlen = out->len;\n \tsize_t i;\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n-\tif (!opts->only_trailers && !opts->unfold && !opts->filter) {\n+\tif (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1150,16 +1151,26 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tif (!opts->filter || opts->filter(&tok, opts->filter_data)) {\n \t\t\t\tif (opts->unfold)\n \t\t\t\t\tunfold_value(&val);\n+\n+\t\t\t\tif (opts->separator && out->len != origlen)\n+\t\t\t\t\tstrbuf_addbuf(out, opts->separator);\n \t\t\t\tif (!opts->value_only)\n \t\t\t\t\tstrbuf_addf(out, \"%s: \", tok.buf);\n \t\t\t\tstrbuf_addbuf(out, &val);\n-\t\t\t\tstrbuf_addch(out, '\\n');\n+\t\t\t\tif (!opts->separator)\n+\t\t\t\t\tstrbuf_addch(out, '\\n');\n \t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\n \n \t\t} else if (!opts->only_trailers) {\n+\t\t\tif (opts->separator && out->len != origlen) {\n+\t\t\t\tstrbuf_addbuf(out, opts->separator);\n+\t\t\t}\n \t\t\tstrbuf_addstr(out, trailer);\n+\t\t\tif (opts->separator) {\n+\t\t\t\tstrbuf_rtrim(out);\n+\t\t\t}\n \t\t}\n \t}\n \ndiff --git a/trailer.h b/trailer.h\nindex 06d417fe93..203acf4ee1 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -73,6 +73,7 @@ struct process_trailer_options {\n \tint unfold;\n \tint no_divider;\n \tint value_only;\n+\tconst struct strbuf *separator;\n \tint (*filter)(const struct strbuf *, void *);\n \tvoid *filter_data;\n };\n-- \n2.17.1\n\n"},{"id":"367887","messageId":"20190128213337.24752-2-anders@0x63.nu","threadId":"49703","inReplyTo":"20190128213337.24752-1-anders@0x63.nu","subject":"[PATCH v5 1/7] doc: group pretty-format.txt placeholders descriptions","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-28T21:33:31Z","receivedAt":"2019-01-28T21:51:19Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"The placeholders can be grouped into three kinds:\n * literals\n * affecting formatting of later placeholders\n * expanding to information in commit\n\nAlso change the list to a definition list (using '::')\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 235 ++++++++++++++++---------------\n 1 file changed, 125 insertions(+), 110 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 417b638cd8..86d804fe97 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -102,118 +102,133 @@ The title was >>t4119: test autocomputing -p for traditional diff input.<<\n +\n The placeholders are:\n \n-- '%H': commit hash\n-- '%h': abbreviated commit hash\n-- '%T': tree hash\n-- '%t': abbreviated tree hash\n-- '%P': parent hashes\n-- '%p': abbreviated parent hashes\n-- '%an': author name\n-- '%aN': author name (respecting .mailmap, see linkgit:git-shortlog[1]\n- or linkgit:git-blame[1])\n-- '%ae': author email\n-- '%aE': author email (respecting .mailmap, see\n- linkgit:git-shortlog[1] or linkgit:git-blame[1])\n-- '%ad': author date (format respects --date= option)\n-- '%aD': author date, RFC2822 style\n-- '%ar': author date, relative\n-- '%at': author date, UNIX timestamp\n-- '%ai': author date, ISO 8601-like format\n-- '%aI': author date, strict ISO 8601 format\n-- '%cn': committer name\n-- '%cN': committer name (respecting .mailmap, see\n- linkgit:git-shortlog[1] or linkgit:git-blame[1])\n-- '%ce': committer email\n-- '%cE': committer email (respecting .mailmap, see\n- linkgit:git-shortlog[1] or linkgit:git-blame[1])\n-- '%cd': committer date (format respects --date= option)\n-- '%cD': committer date, RFC2822 style\n-- '%cr': committer date, relative\n-- '%ct': committer date, UNIX timestamp\n-- '%ci': committer date, ISO 8601-like format\n-- '%cI': committer date, strict ISO 8601 format\n-- '%d': ref names, like the --decorate option of linkgit:git-log[1]\n-- '%D': ref names without the \" (\", \")\" wrapping.\n-- '%e': encoding\n-- '%s': subject\n-- '%f': sanitized subject line, suitable for a filename\n-- '%b': body\n-- '%B': raw body (unwrapped subject and body)\n+- Placeholders that expand to a single literal character:\n+'%n':: newline\n+'%%':: a raw '%'\n+'%x00':: print a byte from a hex code\n+\n+- Placeholders that affect formatting of later placeholders:\n+'%Cred':: switch color to red\n+'%Cgreen':: switch color to green\n+'%Cblue':: switch color to blue\n+'%Creset':: reset color\n+'%C(...)':: color specification, as described under Values in the\n+ \"CONFIGURATION FILE\" section of linkgit:git-config[1]. By\n+ default, colors are shown only when enabled for log output\n+ (by `color.diff`, `color.ui`, or `--color`, and respecting\n+ the `auto` settings of the former if we are going to a\n+ terminal). `%C(auto,...)` is accepted as a historical\n+ synonym for the default (e.g., `%C(auto,red)`). Specifying\n+ `%C(always,...) will show the colors even when color is\n+ not otherwise enabled (though consider just using\n+ `--color=always` to enable color for the whole output,\n+ including this format and anything else git might color).\n+ `auto` alone (i.e. `%C(auto)`) will turn on auto coloring\n+ on the next placeholders until the color is switched\n+ again.\n+'%m':: left (`<`), right (`>`) or boundary (`-`) mark\n+'%w([[,[,]]])':: switch line wrapping, like the -w option of\n+ linkgit:git-shortlog[1].\n+'%<([,trunc|ltrunc|mtrunc])':: make the next placeholder take at\n+ least N columns, padding spaces on\n+ the right if necessary. Optionally\n+ truncate at the beginning (ltrunc),\n+ the middle (mtrunc) or the end\n+ (trunc) if the output is longer than\n+ N columns. Note that truncating\n+ only works correctly with N >= 2.\n+'%<|()':: make the next placeholder take at least until Nth\n+ columns, padding spaces on the right if necessary\n+'%>()', '%>|()':: similar to '%<()', '%<|()' respectively,\n+ but padding spaces on the left\n+'%>>()', '%>>|()':: similar to '%>()', '%>|()'\n+ respectively, except that if the next\n+ placeholder takes more spaces than given and\n+ there are spaces on its left, use those\n+ spaces\n+'%><()', '%><|()':: similar to '%<()', '%<|()'\n+ respectively, but padding both sides\n+ (i.e. the text is centered)\n+\n+- Placeholders that expand to information extracted from the commit:\n+'%H':: commit hash\n+'%h':: abbreviated commit hash\n+'%T':: tree hash\n+'%t':: abbreviated tree hash\n+'%P':: parent hashes\n+'%p':: abbreviated parent hashes\n+'%an':: author name\n+'%aN':: author name (respecting .mailmap, see linkgit:git-shortlog[1]\n+ or linkgit:git-blame[1])\n+'%ae':: author email\n+'%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1]\n+ or linkgit:git-blame[1])\n+'%ad':: author date (format respects --date= option)\n+'%aD':: author date, RFC2822 style\n+'%ar':: author date, relative\n+'%at':: author date, UNIX timestamp\n+'%ai':: author date, ISO 8601-like format\n+'%aI':: author date, strict ISO 8601 format\n+'%cn':: committer name\n+'%cN':: committer name (respecting .mailmap, see\n+ linkgit:git-shortlog[1] or linkgit:git-blame[1])\n+'%ce':: committer email\n+'%cE':: committer email (respecting .mailmap, see\n+ linkgit:git-shortlog[1] or linkgit:git-blame[1])\n+'%cd':: committer date (format respects --date= option)\n+'%cD':: committer date, RFC2822 style\n+'%cr':: committer date, relative\n+'%ct':: committer date, UNIX timestamp\n+'%ci':: committer date, ISO 8601-like format\n+'%cI':: committer date, strict ISO 8601 format\n+'%d':: ref names, like the --decorate option of linkgit:git-log[1]\n+'%D':: ref names without the \" (\", \")\" wrapping.\n+'%e':: encoding\n+'%s':: subject\n+'%f':: sanitized subject line, suitable for a filename\n+'%b':: body\n+'%B':: raw body (unwrapped subject and body)\n ifndef::git-rev-list[]\n-- '%N': commit notes\n+'%N':: commit notes\n endif::git-rev-list[]\n-- '%GG': raw verification message from GPG for a signed commit\n-- '%G?': show \"G\" for a good (valid) signature,\n- \"B\" for a bad signature,\n- \"U\" for a good signature with unknown validity,\n- \"X\" for a good signature that has expired,\n- \"Y\" for a good signature made by an expired key,\n- \"R\" for a good signature made by a revoked key,\n- \"E\" if the signature cannot be checked (e.g. missing key)\n- and \"N\" for no signature\n-- '%GS': show the name of the signer for a signed commit\n-- '%GK': show the key used to sign a signed commit\n-- '%GF': show the fingerprint of the key used to sign a signed commit\n-- '%GP': show the fingerprint of the primary key whose subkey was used\n- to sign a signed commit\n-- '%gD': reflog selector, e.g., `refs/stash@{1}` or\n- `refs/stash@{2 minutes ago`}; the format follows the rules described\n- for the `-g` option. The portion before the `@` is the refname as\n- given on the command line (so `git log -g refs/heads/master` would\n- yield `refs/heads/master@{0}`).\n-- '%gd': shortened reflog selector; same as `%gD`, but the refname\n- portion is shortened for human readability (so `refs/heads/master`\n- becomes just `master`).\n-- '%gn': reflog identity name\n-- '%gN': reflog identity name (respecting .mailmap, see\n- linkgit:git-shortlog[1] or linkgit:git-blame[1])\n-- '%ge': reflog identity email\n-- '%gE': reflog identity email (respecting .mailmap, see\n- linkgit:git-shortlog[1] or linkgit:git-blame[1])\n-- '%gs': reflog subject\n-- '%Cred': switch color to red\n-- '%Cgreen': switch color to green\n-- '%Cblue': switch color to blue\n-- '%Creset': reset color\n-- '%C(...)': color specification, as described under Values in the\n- \"CONFIGURATION FILE\" section of linkgit:git-config[1].\n- By default, colors are shown only when enabled for log output (by\n- `color.diff`, `color.ui`, or `--color`, and respecting the `auto`\n- settings of the former if we are going to a terminal). `%C(auto,...)`\n- is accepted as a historical synonym for the default (e.g.,\n- `%C(auto,red)`). Specifying `%C(always,...) will show the colors\n- even when color is not otherwise enabled (though consider\n- just using `--color=always` to enable color for the whole output,\n- including this format and anything else git might color). `auto`\n- alone (i.e. `%C(auto)`) will turn on auto coloring on the next\n- placeholders until the color is switched again.\n-- '%m': left (`<`), right (`>`) or boundary (`-`) mark\n-- '%n': newline\n-- '%%': a raw '%'\n-- '%x00': print a byte from a hex code\n-- '%w([[,[,]]])': switch line wrapping, like the -w option of\n- linkgit:git-shortlog[1].\n-- '%<([,trunc|ltrunc|mtrunc])': make the next placeholder take at\n- least N columns, padding spaces on the right if necessary.\n- Optionally truncate at the beginning (ltrunc), the middle (mtrunc)\n- or the end (trunc) if the output is longer than N columns.\n- Note that truncating only works correctly with N >= 2.\n-- '%<|()': make the next placeholder take at least until Nth\n- columns, padding spaces on the right if necessary\n-- '%>()', '%>|()': similar to '%<()', '%<|()'\n- respectively, but padding spaces on the left\n-- '%>>()', '%>>|()': similar to '%>()', '%>|()'\n- respectively, except that if the next placeholder takes more spaces\n- than given and there are spaces on its left, use those spaces\n-- '%><()', '%><|()': similar to '%<()', '%<|()'\n- respectively, but padding both sides (i.e. the text is centered)\n-- %(trailers[:options]): display the trailers of the body as interpreted\n- by linkgit:git-interpret-trailers[1]. The `trailers` string may be\n- followed by a colon and zero or more comma-separated options. If the\n- `only` option is given, omit non-trailer lines from the trailer block.\n- If the `unfold` option is given, behave as if interpret-trailer's\n- `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do\n- both.\n+'%GG':: raw verification message from GPG for a signed commit\n+'%G?':: show \"G\" for a good (valid) signature,\n+ \"B\" for a bad signature,\n+ \"U\" for a good signature with unknown validity,\n+ \"X\" for a good signature that has expired,\n+ \"Y\" for a good signature made by an expired key,\n+ \"R\" for a good signature made by a revoked key,\n+ \"E\" if the signature cannot be checked (e.g. missing key)\n+ and \"N\" for no signature\n+'%GS':: show the name of the signer for a signed commit\n+'%GK':: show the key used to sign a signed commit\n+'%GF':: show the fingerprint of the key used to sign a signed commit\n+'%GP':: show the fingerprint of the primary key whose subkey was used\n+ to sign a signed commit\n+'%gD':: reflog selector, e.g., `refs/stash@{1}` or `refs/stash@{2\n+ minutes ago`}; the format follows the rules described for the\n+ `-g` option. The portion before the `@` is the refname as\n+ given on the command line (so `git log -g refs/heads/master`\n+ would yield `refs/heads/master@{0}`).\n+'%gd':: shortened reflog selector; same as `%gD`, but the refname\n+ portion is shortened for human readability (so\n+ `refs/heads/master` becomes just `master`).\n+'%gn':: reflog identity name\n+'%gN':: reflog identity name (respecting .mailmap, see\n+ linkgit:git-shortlog[1] or linkgit:git-blame[1])\n+'%ge':: reflog identity email\n+'%gE':: reflog identity email (respecting .mailmap, see\n+ linkgit:git-shortlog[1] or linkgit:git-blame[1])\n+'%gs':: reflog subject\n+'%(trailers[:options])':: display the trailers of the body as\n+ interpreted by\n+ linkgit:git-interpret-trailers[1]. The\n+ `trailers` string may be followed by a colon\n+ and zero or more comma-separated options:\n+** 'only': omit non-trailer lines from the trailer block.\n+** 'unfold': make it behave as if interpret-trailer's `--unfold`\n+ option was given. E.g., `%(trailers:only,unfold)` unfolds and\n+ shows all trailer lines.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\n-- \n2.17.1\n\n"},{"id":"367888","messageId":"20190128213337.24752-7-anders@0x63.nu","threadId":"49703","inReplyTo":"20190128213337.24752-1-anders@0x63.nu","subject":"[PATCH v5 6/7] strbuf: separate callback for strbuf_expand:ing literals","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-28T21:33:36Z","receivedAt":"2019-01-28T21:51:21Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"Expanding '%n' and '%xNN' is generic functionality, so extract that from\nthe pretty.c formatter into a callback that can be reused.\n\nNo functional change intended\n\nSigned-off-by: Anders Waldenborg \n---\n pretty.c | 16 +++++-----------\n strbuf.c | 21 +++++++++++++++++++++\n strbuf.h | 8 ++++++++\n 3 files changed, 34 insertions(+), 11 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex ed25845c98..7baa4c1c26 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1137,9 +1137,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tconst char *msg = c->message;\n \tstruct commit_list *p;\n \tconst char *arg;\n-\tint ch;\n+\tsize_t res;\n \n \t/* these are independent of the commit */\n+\tres = strbuf_expand_literal_cb(sb, placeholder, NULL);\n+\tif (res)\n+\t\treturn res;\n+\n \tswitch (placeholder[0]) {\n \tcase 'C':\n \t\tif (starts_with(placeholder + 1, \"(auto)\")) {\n@@ -1158,16 +1162,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t */\n \t\t\treturn ret;\n \t\t}\n-\tcase 'n':\t\t/* newline */\n-\t\tstrbuf_addch(sb, '\\n');\n-\t\treturn 1;\n-\tcase 'x':\n-\t\t/* %x00 == NUL, %x0a == LF, etc. */\n-\t\tch = hex2chr(placeholder + 1);\n-\t\tif (ch < 0)\n-\t\t\treturn 0;\n-\t\tstrbuf_addch(sb, ch);\n-\t\treturn 3;\n \tcase 'w':\n \t\tif (placeholder[1] == '(') {\n \t\t\tunsigned long width = 0, indent1 = 0, indent2 = 0;\ndiff --git a/strbuf.c b/strbuf.c\nindex f6a6cf78b9..78eecd29f7 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -380,6 +380,27 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,\n \t}\n }\n \n+size_t strbuf_expand_literal_cb(struct strbuf *sb,\n+\t\t\t\tconst char *placeholder,\n+\t\t\t\tvoid *context)\n+{\n+\tint ch;\n+\n+\tswitch (placeholder[0]) {\n+\tcase 'n':\t\t/* newline */\n+\t\tstrbuf_addch(sb, '\\n');\n+\t\treturn 1;\n+\tcase 'x':\n+\t\t/* %x00 == NUL, %x0a == LF, etc. */\n+\t\tch = hex2chr(placeholder + 1);\n+\t\tif (ch < 0)\n+\t\t\treturn 0;\n+\t\tstrbuf_addch(sb, ch);\n+\t\treturn 3;\n+\t}\n+\treturn 0;\n+}\n+\n size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,\n \t\tvoid *context)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex fc40873b65..52e44c9ab8 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -320,6 +320,14 @@ void strbuf_expand(struct strbuf *sb,\n \t\t expand_fn_t fn,\n \t\t void *context);\n \n+/**\n+ * Used as callback for `strbuf_expand` to only expand literals\n+ * (i.e. %n and %xNN). The context argument is ignored.\n+ */\n+size_t strbuf_expand_literal_cb(struct strbuf *sb,\n+\t\t\t\tconst char *placeholder,\n+\t\t\t\tvoid *context);\n+\n /**\n * Used as callback for `strbuf_expand()`, expects an array of\n * struct strbuf_expand_dict_entry as context, i.e. pairs of\n-- \n2.17.1\n\n"},{"id":"367889","messageId":"20190128213337.24752-5-anders@0x63.nu","threadId":"49703","inReplyTo":"20190128213337.24752-1-anders@0x63.nu","subject":"[PATCH v5 4/7] pretty: allow showing specific trailers","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-28T21:33:34Z","receivedAt":"2019-01-28T21:51:24Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"Adds a new \"key=X\" option to \"%(trailers)\" which will cause it to only\nprint trailer lines which match any of the specified keys.\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 8 +++++\n pretty.c | 36 ++++++++++++++++++--\n t/t4205-log-pretty-formats.sh | 57 ++++++++++++++++++++++++++++++++\n trailer.c | 10 +++---\n trailer.h | 2 ++\n 5 files changed, 107 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex d33b072eb2..d6add831c0 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -225,6 +225,14 @@ endif::git-rev-list[]\n linkgit:git-interpret-trailers[1]. The\n `trailers` string may be followed by a colon\n and zero or more comma-separated options:\n+** 'key=': only show trailers with specified key. Matching is done\n+ case-insensitively and trailing colon is optional. If option is\n+ given multiple times trailer lines matching any of the keys are\n+ shown. This option automatically enables the `only` option so that\n+ non-trailer lines in the trailer block are hidden. If that is not\n+ desired it can be disabled with `only=false`. E.g.,\n+ `%(trailers:key=Reviewed-by)` shows trailer lines with key\n+ `Reviewed-by`.\n ** 'only[=val]': select whether non-trailer lines from the trailer\n block should be included. The `only` keyword may optionally be\n followed by an equal sign and one of `true`, `on`, `yes` to omit or\ndiff --git a/pretty.c b/pretty.c\nindex 65a1b9bd82..d3dd2d6254 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1115,6 +1115,19 @@ static int match_placeholder_bool_arg(const char *to_parse, const char *candidat\n \treturn 1;\n }\n \n+static int format_trailer_match_cb(const struct strbuf *key, void *ud)\n+{\n+\tconst struct string_list *list = ud;\n+\tconst struct string_list_item *item;\n+\n+\tfor_each_string_list_item (item, list) {\n+\t\tif (key->len == (uintptr_t)item->util &&\n+\t\t !strncasecmp(item->string, key->buf, key->len))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\tconst char *placeholder,\n \t\t\t\tvoid *context)\n@@ -1353,6 +1366,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\t\tstruct string_list filter_list = STRING_LIST_INIT_NODUP;\n \t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n@@ -1360,8 +1374,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tif (*arg == ':') {\n \t\t\targ++;\n \t\t\tfor (;;) {\n-\t\t\t\tif (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n-\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n+\t\t\t\tconst char *argval;\n+\t\t\t\tsize_t arglen;\n+\n+\t\t\t\tif (match_placeholder_arg_value(arg, \"key\", &arg, &argval, &arglen)) {\n+\t\t\t\t\tuintptr_t len = arglen;\n+\n+\t\t\t\t\tif (!argval)\n+\t\t\t\t\t\tgoto trailer_out;\n+\n+\t\t\t\t\tif (len && argval[len - 1] == ':')\n+\t\t\t\t\t\tlen--;\n+\t\t\t\t\tstring_list_append(&filter_list, argval)->util = (char *)len;\n+\n+\t\t\t\t\topts.filter = format_trailer_match_cb;\n+\t\t\t\t\topts.filter_data = &filter_list;\n+\t\t\t\t\topts.only_trailers = 1;\n+\t\t\t\t} else if (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n+\t\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\n@@ -1369,6 +1399,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n \t\t\tret = arg - placeholder + 1;\n \t\t}\n+\ttrailer_out:\n+\t\tstring_list_clear(&filter_list, 0);\n \t\treturn ret;\n \t}\n \ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 63730a4ec0..d87201afbe 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -616,6 +616,63 @@ test_expect_success ':only and :unfold work together' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by)\" >actual &&\n+\techo \"Acked-by: A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=AcKed-bY)\" >actual &&\n+\techo \"Acked-by: A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo:) trailing colon also works' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by:)\" >actual &&\n+\techo \"Acked-by: A U Thor \" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo) multiple keys' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by:,key=Signed-off-By)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=nonexistant) becomes empty' '\n+\tgit log --no-walk --pretty=\"x%(trailers:key=Nacked-by)x\" >actual &&\n+\techo \"xx\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Signed-Off-by)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Signed-Off-by,unfold)\" >actual &&\n+\tunfold expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' '\n+\tgit log --no-walk --pretty=\"format:%(trailers:key=Acked-by,only=no)\" >actual &&\n+\t{\n+\t\techo \"Acked-by: A U Thor \" &&\n+\t\tgrep patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:key) without value is error' '\n+\tgit log --no-walk --pretty=\"tformat:%(trailers:key)\" >actual &&\n+\techo \"%(trailers:key)\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'trailer parsing not fooled by --- line' '\n \tgit commit --allow-empty -F - <<-\\EOF &&\n \tthis is the subject\ndiff --git a/trailer.c b/trailer.c\nindex 0796f326b3..d6da555cd7 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out,\n \tsize_t i;\n \n \t/* If we want the whole block untouched, we can take the fast path. */\n-\tif (!opts->only_trailers && !opts->unfold) {\n+\tif (!opts->only_trailers && !opts->unfold && !opts->filter) {\n \t\tstrbuf_add(out, info->trailer_start,\n \t\t\t info->trailer_end - info->trailer_start);\n \t\treturn;\n@@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out,\n \t\t\tstruct strbuf val = STRBUF_INIT;\n \n \t\t\tparse_trailer(&tok, &val, NULL, trailer, separator_pos);\n-\t\t\tif (opts->unfold)\n-\t\t\t\tunfold_value(&val);\n+\t\t\tif (!opts->filter || opts->filter(&tok, opts->filter_data)) {\n+\t\t\t\tif (opts->unfold)\n+\t\t\t\t\tunfold_value(&val);\n \n-\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t\tstrbuf_addf(out, \"%s: %s\\n\", tok.buf, val.buf);\n+\t\t\t}\n \t\t\tstrbuf_release(&tok);\n \t\t\tstrbuf_release(&val);\n \ndiff --git a/trailer.h b/trailer.h\nindex b997739649..5255b676de 100644\n--- a/trailer.h\n+++ b/trailer.h\n@@ -72,6 +72,8 @@ struct process_trailer_options {\n \tint only_input;\n \tint unfold;\n \tint no_divider;\n+\tint (*filter)(const struct strbuf *, void *);\n+\tvoid *filter_data;\n };\n \n #define PROCESS_TRAILER_OPTIONS_INIT {0}\n-- \n2.17.1\n\n"},{"id":"367890","messageId":"20190128213337.24752-4-anders@0x63.nu","threadId":"49703","inReplyTo":"20190128213337.24752-1-anders@0x63.nu","subject":"[PATCH v5 3/7] pretty: single return path in %(trailers) handling","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-28T21:33:33Z","receivedAt":"2019-01-28T21:51:26Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"No functional change intended.\n\nThis change may not seem useful on its own, but upcoming commits will do\nmemory allocation in there, and a single return path makes deallocation\neasier.\n\nSigned-off-by: Anders Waldenborg \n---\n pretty.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex b8d71a57c9..65a1b9bd82 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1353,6 +1353,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \n \tif (skip_prefix(placeholder, \"(trailers\", &arg)) {\n \t\tstruct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;\n+\t\tsize_t ret = 0;\n \n \t\topts.no_divider = 1;\n \n@@ -1366,8 +1367,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t}\n \t\tif (*arg == ')') {\n \t\t\tformat_trailers_from_commit(sb, msg + c->subject_off, &opts);\n-\t\t\treturn arg - placeholder + 1;\n+\t\t\tret = arg - placeholder + 1;\n \t\t}\n+\t\treturn ret;\n \t}\n \n \treturn 0;\t/* unknown placeholder */\n-- \n2.17.1\n\n"},{"id":"367896","messageId":"xmqq8sz49zm1.fsf@gitster-ct.c.googlers.com","threadId":"49703","inReplyTo":"20190128213337.24752-3-anders@0x63.nu","subject":"Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-28T22:38:46Z","receivedAt":"2019-01-28T22:38:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Anders Waldenborg writes:\n\n> Subject: Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value\n\nStyle: s/pretty: Allow/pretty: allow/ (haven't I said this often enough?)\n\n> +** 'only[=val]': select whether non-trailer lines from the trailer\n> + block should be included. The `only` keyword may optionally be\n> + followed by an equal sign and one of `true`, `on`, `yes` to omit or\n> + `false`, `off`, `no` to show the non-trailer lines. If option is\n> + given without value it is enabled. If given multiple times the last\n> + value is used.\n> +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`\n> + option was given. In same way as to for `only` it can be followed\n> + by an equal sign and explicit value. E.g.,\n> + `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n\nSounds sensible.\n\n> diff --git a/pretty.c b/pretty.c\n> index b83a3ecd23..b8d71a57c9 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1056,13 +1056,25 @@ static size_t parse_padding_placeholder(struct strbuf *sb,\n> \treturn 0;\n> }\n> \n> -static int match_placeholder_arg(const char *to_parse, const char *candidate,\n> -\t\t\t\t const char **end)\n> +static int match_placeholder_arg_value(const char *to_parse, const char *candidate,\n> +\t\t\t\t const char **end, const char **valuestart, size_t *valuelen)\n\nAn overlong line here.\n\n> ...\n> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,\n> +\t\t\t\t const char **end, int *val)\n> +{\n> +\tchar buf[8];\n> +\tconst char *strval;\n> +\tsize_t len;\n> +\tint v;\n> +\n> +\tif (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len))\n> +\t\treturn 0;\n> +\n> +\tif (!strval) {\n> +\t\t*val = 1;\n> +\t\treturn 1;\n> +\t}\n> +\n> +\tstrlcpy(buf, strval, sizeof(buf));\n> +\tif (len < sizeof(buf))\n> +\t\tbuf[len] = 0;\n\nDoesn't strlcpy() terminate buf[len] if len is short enough?\nEven if the strval is longer than buf[], strlcpy() would truncate\nand make sure buf[] is NUL terminated, no?\n\n> +\tv = git_parse_maybe_bool(buf);\n\nWhy?\n\nThis function would simply be buggy and incapable of parsing a\nrepresentation of a boolean value that is longer than 8 bytes (if\nsuch a representation exists), so chomping an overlong string at the\nend and feeding it to git_parse_maybe_bool() is a nonsense, isn't\nit?\n\nIn this particular case, strlcpy() is inviting a buggy programming.\nIf there were a 7-letter representation of falsehood, strval may be\nthat 7-letter thing, in which case you would want to feed it to\ngit_parse_maybe_bool() to receive \"false\" from it, or strval may\nhave that 7-letter thing followed by a 'x' (so as a token, that is\nnot a correctly spelled falsehood), but strlcpy() would chomp and\nshow the same 7-letter falsehood to git_parse_maybe_bool(). That\nrobs you from an opportunity to diagnose such a bogus input as an\nerror.\n\nInstead of using \"char buf[8]\", just using a strbuf and avoidng\nstrlcpy() would make the code much better, I would think.\n\n"},{"id":"367932","messageId":"87tvhsklpb.fsf@0x63.nu","threadId":"49703","inReplyTo":"xmqq8sz49zm1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-29T06:45:06Z","receivedAt":"2019-01-29T06:45:33Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJunio C Hamano writes:\n\n>> ...\n>> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,\n>> +\t\t\t\t const char **end, int *val)\n>> +{\n>> +\tchar buf[8];\n>> +\tconst char *strval;\n>> +\tsize_t len;\n>> +\tint v;\n>> +\n>> +\tif (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len))\n>> +\t\treturn 0;\n>> +\n>> +\tif (!strval) {\n>> +\t\t*val = 1;\n>> +\t\treturn 1;\n>> +\t}\n>> +\n>> +\tstrlcpy(buf, strval, sizeof(buf));\n>> +\tif (len < sizeof(buf))\n>> +\t\tbuf[len] = 0;\n>\n> Doesn't strlcpy() terminate buf[len] if len is short enough?\n> Even if the strval is longer than buf[], strlcpy() would truncate\n> and make sure buf[] is NUL terminated, no?\n\nYes, but no. strval is not NUL-terminated at len. E.g strval would point\nto \"false,something=true\". `buf[len] = 0` makes sure it becomes \"false\".\n\n> Instead of using \"char buf[8]\", just using a strbuf and avoidng\n> strlcpy() would make the code much better, I would think.\n\nYes, taking the heap allocation hit would most likely make the intent\nclearer.\n"},{"id":"367933","messageId":"87r2cwklgj.fsf@0x63.nu","threadId":"49703","inReplyTo":"xmqq8sz49zm1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v5 2/7 update] pretty: allow %(trailers) options with explicit value","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-29T06:49:00Z","receivedAt":"2019-01-29T06:49:04Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nIn addition to old %(trailers:only) it is now allowed to write\n%(trailers:only=yes)\n\nBy itself this only gives (the not quite so useful) possibility to have\nusers change their mind in the middle of a formatting\nstring (%(trailers:only=true,only=false)). However, it gives users the\nopportunity to override defaults from future options.\n\nSigned-off-by: Anders Waldenborg \n---\n Documentation/pretty-formats.txt | 14 ++++++---\n pretty.c | 52 +++++++++++++++++++++++++++-----\n t/t4205-log-pretty-formats.sh | 18 +++++++++++\n 3 files changed, 73 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt\nindex 86d804fe97..d33b072eb2 100644\n--- a/Documentation/pretty-formats.txt\n+++ b/Documentation/pretty-formats.txt\n@@ -225,10 +225,16 @@ endif::git-rev-list[]\n linkgit:git-interpret-trailers[1]. The\n `trailers` string may be followed by a colon\n and zero or more comma-separated options:\n-** 'only': omit non-trailer lines from the trailer block.\n-** 'unfold': make it behave as if interpret-trailer's `--unfold`\n- option was given. E.g., `%(trailers:only,unfold)` unfolds and\n- shows all trailer lines.\n+** 'only[=val]': select whether non-trailer lines from the trailer\n+ block should be included. The `only` keyword may optionally be\n+ followed by an equal sign and one of `true`, `on`, `yes` to omit or\n+ `false`, `off`, `no` to show the non-trailer lines. If option is\n+ given without value it is enabled. If given multiple times the last\n+ value is used.\n+** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold`\n+ option was given. In same way as to for `only` it can be followed\n+ by an equal sign and explicit value. E.g.,\n+ `%(trailers:only,unfold=true)` unfolds and shows all trailer lines.\n \n NOTE: Some placeholders may depend on other options given to the\n revision traversal engine. For example, the `%g*` reflog options will\ndiff --git a/pretty.c b/pretty.c\nindex b83a3ecd23..4dfbd38cf6 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1056,13 +1056,26 @@ static size_t parse_padding_placeholder(struct strbuf *sb,\n \treturn 0;\n }\n \n-static int match_placeholder_arg(const char *to_parse, const char *candidate,\n-\t\t\t\t const char **end)\n+static int match_placeholder_arg_value(const char *to_parse, const char *candidate,\n+\t\t\t\t const char **end, const char **valuestart,\n+\t\t\t\t size_t *valuelen)\n {\n \tconst char *p;\n \n \tif (!(skip_prefix(to_parse, candidate, &p)))\n \t\treturn 0;\n+\tif (valuestart) {\n+\t\tif (*p == '=') {\n+\t\t\t*valuestart = p + 1;\n+\t\t\t*valuelen = strcspn(*valuestart, \",)\");\n+\t\t\tp = *valuestart + *valuelen;\n+\t\t} else {\n+\t\t\tif (*p != ',' && *p != ')')\n+\t\t\t\treturn 0;\n+\t\t\t*valuestart = NULL;\n+\t\t\t*valuelen = 0;\n+\t\t}\n+\t}\n \tif (*p == ',') {\n \t\t*end = p + 1;\n \t\treturn 1;\n@@ -1074,6 +1087,34 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate,\n \treturn 0;\n }\n \n+static int match_placeholder_bool_arg(const char *to_parse, const char *candidate,\n+\t\t\t\t const char **end, int *val)\n+{\n+\tconst char *argval;\n+\tchar *strval;\n+\tsize_t arglen;\n+\tint v;\n+\n+\tif (!match_placeholder_arg_value(to_parse, candidate, end, &argval, &arglen))\n+\t\treturn 0;\n+\n+\tif (!argval) {\n+\t\t*val = 1;\n+\t\treturn 1;\n+\t}\n+\n+\tstrval = xstrndup(argval, arglen);\n+\tv = git_parse_maybe_bool(strval);\n+\tfree(strval);\n+\n+\tif (v == -1)\n+\t\treturn 0;\n+\n+\t*val = v;\n+\n+\treturn 1;\n+}\n+\n static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\t\tconst char *placeholder,\n \t\t\t\tvoid *context)\n@@ -1318,11 +1359,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\tif (*arg == ':') {\n \t\t\targ++;\n \t\t\tfor (;;) {\n-\t\t\t\tif (match_placeholder_arg(arg, \"only\", &arg))\n-\t\t\t\t\topts.only_trailers = 1;\n-\t\t\t\telse if (match_placeholder_arg(arg, \"unfold\", &arg))\n-\t\t\t\t\topts.unfold = 1;\n-\t\t\t\telse\n+\t\t\t\tif (!match_placeholder_bool_arg(arg, \"only\", &arg, &opts.only_trailers) &&\n+\t\t\t\t !match_placeholder_bool_arg(arg, \"unfold\", &arg, &opts.unfold))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 978a8a66ff..63730a4ec0 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only \"key: value\" trailers' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '%(trailers:only=yes) shows only \"key: value\" trailers' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:only=yes)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:only=no) shows all trailers' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:only=no)\" >actual &&\n+\tcat trailers >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '%(trailers:only=no,only=true) shows only \"key: value\" trailers' '\n+\tgit log --no-walk --pretty=format:\"%(trailers:only=yes)\" >actual &&\n+\tgrep -v patch.description expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '%(trailers:unfold) unfolds trailers' '\n \tgit log --no-walk --pretty=\"%(trailers:unfold)\" >actual &&\n \t{\n-- \n2.17.1\n\n"},{"id":"368015","messageId":"20190129165523.GA7349@sigill.intra.peff.net","threadId":"49703","inReplyTo":"87o99iwmjn.fsf@0x63.nu","subject":"Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-29T16:55:23Z","receivedAt":"2019-01-29T16:55:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 18, 2018 at 10:30:04PM +0100, Anders Waldenborg wrote:\n\n> \n> Junio C Hamano writes:\n> > That way, we can handle %(trailers:only=bogo) more sensibly,\n> > no? Syntactically we can recognize that the user wanted to give\n> > 'bogo' as the value to 'only', and say \"'bogo' is not a boolean\" if\n> > we did so.\n> \n> I agree that proper error reporting for the pretty formatting strings\n> would be great. But that would depart from the current extremely crude\n> error handling where incorrect formatting placeholders are just left\n> unexpanded. How would such change in error handling be done safely, wrt\n> backwards compatibility changes?\n\nI think we'd want to move in the direction of enforcing valid\nexpressions for %(foo) placeholders. There's some small value in leaving\n%X alone if we do not understand \"X\" (not to mention the backwards\n%compatibility you mentioned), but I think %() is a pretty\ndeliberate indication that a placeholder was meant there.\n\nWe already do this for ref-filter expansions:\n\n $ git for-each-ref --format='%(foo)'\n fatal: unknown field name: foo\n\nWe don't for \"--pretty\" formats, but I do wonder if anybody would be\nreally mad (after all, we have declared ourselves free to add new\nplaceholders, so such formats are not future-proof).\n\n-Peff\n"},{"id":"368016","messageId":"20190129165715.GB7349@sigill.intra.peff.net","threadId":"49703","inReplyTo":"87tvhsklpb.fsf@0x63.nu","subject":"Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-29T16:57:16Z","receivedAt":"2019-01-29T16:57:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 29, 2019 at 07:45:06AM +0100, Anders Waldenborg wrote:\n\n> > Instead of using \"char buf[8]\", just using a strbuf and avoidng\n> > strlcpy() would make the code much better, I would think.\n> \n> Yes, taking the heap allocation hit would most likely make the intent\n> clearer.\n\nIf you can reuse the same struct and strbuf_reset() it each time, then\nthat amortizes the cost of the heap (to basically once per program run,\ninstead of once per commit).\n\n-Peff\n"},{"id":"368047","messageId":"87pnsfkvk1.fsf@0x63.nu","threadId":"49703","inReplyTo":"20190129165523.GA7349@sigill.intra.peff.net","subject":"Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-29T21:23:10Z","receivedAt":"2019-01-29T21:23:17Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nJeff King writes:\n> There's some small value in leaving\n> %X alone if we do not understand \"X\" (not to mention the backwards\n> %compatibility you mentioned), but I think %() is a pretty\n> deliberate indication that a placeholder was meant there.\n\nGood point.\n\n> We already do this for ref-filter expansions:\n>\n> $ git for-each-ref --format='%(foo)'\n> fatal: unknown field name: foo\n>\n> We don't for \"--pretty\" formats, but I do wonder if anybody would be\n> really mad (after all, we have declared ourselves free to add new\n> placeholders, so such formats are not future-proof).\n\nOh my. I wasn't aware that there was a totally separate string\ninterpolation implementation used for ref filters. That one has\nseparated parsing, making it more amenable to good error handling.\nI wonder if that could be generalized and reused for pretty formats.\n\nHowever I doubt I will have time to dig deeper into that in near time.\n"},{"id":"368256","messageId":"87o97wll60.fsf@0x63.nu","threadId":"49703","inReplyTo":"CAL21Bmmx=EO+R2t+KviNekDhU3fc0wjCcmUmbzLa14bb0PAmHA@mail.gmail.com","subject":"Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value","fromName":"Anders Waldenborg","fromEmail":"anders@0x63.nu","sentAt":"2019-01-31T18:46:47Z","receivedAt":"2019-01-31T18:47:06Z","isPatch":true,"sender":{"key":"anders@0x63.nu","avatar":"https://avatars.githubusercontent.com/u/1566016?v=4"},"body":"\nОля Тележная writes:\n>> Oh my. I wasn't aware that there was a totally separate string\n>> interpolation implementation used for ref filters. That one has\n>> separated parsing, making it more amenable to good error handling.\n>> I wonder if that could be generalized and reused for pretty formats.\n>>\n>> However I doubt I will have time to dig deeper into that in near time.\n>>\n>\n> Sorry, I haven't read your patch in details. If you will be at Git Merge\n> tomorrow, you could ask me any questions, I can explain how for-each-ref\n> formatting works amd maybe even give you some ideas how to use its logic in\n> pretty, I was thinking about it a bit.\n\nNo, unfortunately I'm not at Git Merge.\n\nI think I got how the formatting work. But if you have ideas on how to\nreuse the logic I'm all ears.\n\n"},{"id":"368400","messageId":"CAL21BmnU2aTT_8iqejurgKeHXk-kmmGK1tmXLcVh7G12rwRPOw@mail.gmail.com","threadId":"49703","inReplyTo":"87o97wll60.fsf@0x63.nu","subject":"Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2019-02-02T09:14:13Z","receivedAt":"2019-02-02T09:14:28Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"чт, 31 янв. 2019 г. в 21:47, Anders Waldenborg :\n>\n>\n> Оля Тележная writes:\n> >> Oh my. I wasn't aware that there was a totally separate string\n> >> interpolation implementation used for ref filters. That one has\n> >> separated parsing, making it more amenable to good error handling.\n> >> I wonder if that could be generalized and reused for pretty formats.\n> >>\n> >> However I doubt I will have time to dig deeper into that in near time.\n> >>\n> >\n> > Sorry, I haven't read your patch in details. If you will be at Git Merge\n> > tomorrow, you could ask me any questions, I can explain how for-each-ref\n> > formatting works amd maybe even give you some ideas how to use its logic in\n> > pretty, I was thinking about it a bit.\n>\n> No, unfortunately I'm not at Git Merge.\n>\n> I think I got how the formatting work. But if you have ideas on how to\n> reuse the logic I'm all ears.\n\n Maybe my advices will be not suitable: I know how ref-filter works,\nbut I know only a few about pretty system. The main point is that we\nneed to save backward compatibility, and that means that we can't just\ndrop pretty logic and start using ref-filter one there. I guess it's\nnot so hard to make like translation table between pretty commands and\nref-filter one. For example, 'short' in pretty means 'commit\n%(objectname)%0aAuthor: %(author)' in ref-filter. So if I wanted to\nchange pretty logic, I would add support of all ref-filter commands\nand make translation table for backward compatibility. Hope it was\nhelpful.\n\n\n\n>\n"}]}