* [PATCH] pretty: Add %(trailer:X) to display single trailer @ 2018-10-28 12:50 Anders Waldenborg 2018-10-29 4:49 ` Junio C Hamano ` (4 more replies) 0 siblings, 5 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-10-28 12:50 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg This new format placeholder allows displaying only a single trailer. The formatting done is similar to what is done for --decorate/%d using parentheses and comma separation. It's intended use is for things like ticket references in trailers. So with a commit with a message like: > Some good commit > > Ticket: XYZ-123 running: $ git log --pretty="%H %s% (trailer:Ticket)" will give: > 123456789a Some good commit (Ticket: XYZ-123) Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 4 ++++ pretty.c | 16 +++++++++++++ t/t4205-log-pretty-formats.sh | 40 ++++++++++++++++++++++++++++++++ trailer.c | 18 ++++++++++++-- trailer.h | 1 + 5 files changed, 77 insertions(+), 2 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 6109ef09aa..a46d0c0717 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -211,6 +211,10 @@ endif::git-rev-list[] If the `unfold` option is given, behave as if interpret-trailer's `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do both. +- %(trailer:<t>): display the specified trailer in parentheses (like + %d does for refnames). If there are multiple entries of that trailer + they are shown comma separated. If there are no matching trailers + nothing is displayed. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index 8ca29e9281..61ae34ced4 100644 --- a/pretty.c +++ b/pretty.c @@ -1324,6 +1324,22 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ } } + if (skip_prefix(placeholder, "(trailer:", &arg)) { + struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; + opts.no_divider = 1; + opts.only_trailers = 1; + opts.unfold = 1; + + const char *end = strchr(arg, ')'); + if (!end) + return 0; + + opts.filter_trailer = xstrndup(arg, end - arg); + format_trailers_from_commit(sb, msg + c->subject_off, &opts); + free(opts.filter_trailer); + return end - placeholder + 1; + } + return 0; /* unknown placeholder */ } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 978a8a66ff..e929f820e7 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -598,6 +598,46 @@ test_expect_success ':only and :unfold work together' ' test_cmp expect actual ' +test_expect_success 'pretty format %(trailer:foo) shows that trailer' ' + git log --no-walk --pretty="%(trailer:Acked-By)" >actual && + { + echo "(Acked-By: A U Thor <author@example.com>)" + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailer:nonexistant) becomes empty' ' + git log --no-walk --pretty="x%(trailer:Nacked-By)x" >actual && + { + echo "xx" + } >expect && + test_cmp expect actual +' + +test_expect_success '% (trailer:nonexistant) with space becomes empty' ' + git log --no-walk --pretty="x% (trailer:Nacked-By)x" >actual && + { + echo "xx" + } >expect && + test_cmp expect actual +' + +test_expect_success '% (trailer:foo) with space adds space before' ' + git log --no-walk --pretty="x% (trailer:Acked-By)x" >actual && + { + echo "x (Acked-By: A U Thor <author@example.com>)x" + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailer:foo) with multiple lines becomes comma separated and unwrapped' ' + git log --no-walk --pretty="%(trailer:Signed-Off-By)" >actual && + { + echo "(Signed-Off-By: A U Thor <author@example.com>, A U Thor <author@example.com>)" + } >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index 0796f326b3..d337bca8dd 100644 --- a/trailer.c +++ b/trailer.c @@ -1138,6 +1138,7 @@ static void format_trailer_info(struct strbuf *out, return; } + int printed_first = 0; for (i = 0; i < info->trailer_nr; i++) { char *trailer = info->trailers[i]; ssize_t separator_pos = find_separator(trailer, separators); @@ -1150,7 +1151,19 @@ static void format_trailer_info(struct strbuf *out, if (opts->unfold) unfold_value(&val); - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + if (opts->filter_trailer) { + if (!strcasecmp (tok.buf, opts->filter_trailer)) { + if (!printed_first) { + strbuf_addf(out, "(%s: ", opts->filter_trailer); + printed_first = 1; + } else { + strbuf_addstr(out, ", "); + } + strbuf_addstr(out, val.buf); + } + } else { + strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + } strbuf_release(&tok); strbuf_release(&val); @@ -1158,7 +1171,8 @@ static void format_trailer_info(struct strbuf *out, strbuf_addstr(out, trailer); } } - + if (printed_first) + strbuf_addstr(out, ")"); } void format_trailers_from_commit(struct strbuf *out, const char *msg, diff --git a/trailer.h b/trailer.h index b997739649..852c79d449 100644 --- a/trailer.h +++ b/trailer.h @@ -72,6 +72,7 @@ struct process_trailer_options { int only_input; int unfold; int no_divider; + char *filter_trailer; }; #define PROCESS_TRAILER_OPTIONS_INIT {0} -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH] pretty: Add %(trailer:X) to display single trailer 2018-10-28 12:50 [PATCH] pretty: Add %(trailer:X) to display single trailer Anders Waldenborg @ 2018-10-29 4:49 ` Junio C Hamano 2018-10-29 14:14 ` Jeff King ` (3 subsequent siblings) 4 siblings, 0 replies; 69+ messages in thread From: Junio C Hamano @ 2018-10-29 4:49 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King Anders Waldenborg <anders@0x63.nu> writes: > This new format placeholder allows displaying only a single > trailer. The formatting done is similar to what is done for > --decorate/%d using parentheses and comma separation. > > It's intended use is for things like ticket references in trailers. > > So with a commit with a message like: > > > Some good commit > > > > Ticket: XYZ-123 > > running: > > $ git log --pretty="%H %s% (trailer:Ticket)" > > will give: > > > 123456789a Some good commit (Ticket: XYZ-123) Sounds useful, but a few questions off the top of my head are: - How would this work together with existing %(trailers:...)? - Can't this be made to a new option, in addition to existing 'only' and 'unfold', to existing %(trailer:...)? If not, what are the missing pieces that we need to add in order to make that possible? The latter is especially true as from the surface, it smell like that the whole reason why this patch introduces a new placeholder with confusingly simliar name is because the patch did not bother to think of a way to make it fit there as an enhancement of it. > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt > index 6109ef09aa..a46d0c0717 100644 > --- a/Documentation/pretty-formats.txt > +++ b/Documentation/pretty-formats.txt > @@ -211,6 +211,10 @@ endif::git-rev-list[] > If the `unfold` option is given, behave as if interpret-trailer's > `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do > both. > +- %(trailer:<t>): display the specified trailer in parentheses (like > + %d does for refnames). If there are multiple entries of that trailer > + they are shown comma separated. If there are no matching trailers > + nothing is displayed. As this list is sorted roughly alphabetically for short ones, I think it is better to keep that order for the longer ones that begin with "%(". This should be instead inserted before the description for the existing "%(trailers[:options])". Assuming that we want this %(trailer) separate from %(trailers), that is, of course. > diff --git a/pretty.c b/pretty.c > index 8ca29e9281..61ae34ced4 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1324,6 +1324,22 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > } > } > > + if (skip_prefix(placeholder, "(trailer:", &arg)) { > + struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; > + opts.no_divider = 1; > + opts.only_trailers = 1; > + opts.unfold = 1; This makes me suspect that it would be very nice if this is implemented as a new "option" to the existing "%(trailers[:option])" thing. It does mostly identical thing as the existing code. > + const char *end = strchr(arg, ')'); Avoid decl-after-statement. > + if (!end) > + return 0; > + > + opts.filter_trailer = xstrndup(arg, end - arg); > + format_trailers_from_commit(sb, msg + c->subject_off, &opts); > + free(opts.filter_trailer); > + return end - placeholder + 1; > + } > + > return 0; /* unknown placeholder */ > } > > diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh > index 978a8a66ff..e929f820e7 100755 > --- a/t/t4205-log-pretty-formats.sh > +++ b/t/t4205-log-pretty-formats.sh > @@ -598,6 +598,46 @@ test_expect_success ':only and :unfold work together' ' > test_cmp expect actual > ' > > +test_expect_success 'pretty format %(trailer:foo) shows that trailer' ' > + git log --no-walk --pretty="%(trailer:Acked-By)" >actual && > + { > + echo "(Acked-By: A U Thor <author@example.com>)" > + } >expect && > + test_cmp expect actual > +' > + > +test_expect_success '%(trailer:nonexistant) becomes empty' ' > + git log --no-walk --pretty="x%(trailer:Nacked-By)x" >actual && > + { > + echo "xx" > + } >expect && > + test_cmp expect actual > +' > + > +test_expect_success '% (trailer:nonexistant) with space becomes empty' ' > + git log --no-walk --pretty="x% (trailer:Nacked-By)x" >actual && > + { > + echo "xx" > + } >expect && > + test_cmp expect actual > +' > + > +test_expect_success '% (trailer:foo) with space adds space before' ' > + git log --no-walk --pretty="x% (trailer:Acked-By)x" >actual && > + { > + echo "x (Acked-By: A U Thor <author@example.com>)x" > + } >expect && > + test_cmp expect actual > +' These are both good positive-negative pairs of tests. > +test_expect_success '%(trailer:foo) with multiple lines becomes comma separated and unwrapped' ' > + git log --no-walk --pretty="%(trailer:Signed-Off-By)" >actual && > + { > + echo "(Signed-Off-By: A U Thor <author@example.com>, A U Thor <author@example.com>)" > + } >expect && > + test_cmp expect actual > +' This also tells me that it is a bad design to add this as a separate new feature that takes the trailer key as an end-user suppied value. There is no way to extend this to other needs, such as "do similar thing as %(trailer:foo) does by default, but do not unwrap; give two or more 'Signed-off-by:' separately)". I wonder why something like %(trailers:comma,token=foo) were not considered. %(trailers:only,token=foo,token=bar) might even be a good way to grab only Foo: and Bar: trailers in the order they appear in the original commit, filtering out all the other trailers and non-trailer text in the log message. > diff --git a/trailer.c b/trailer.c > index 0796f326b3..d337bca8dd 100644 > --- a/trailer.c > +++ b/trailer.c > @@ -1138,6 +1138,7 @@ static void format_trailer_info(struct strbuf *out, > return; > } > > + int printed_first = 0; decl-afer-stmt. > for (i = 0; i < info->trailer_nr; i++) { > char *trailer = info->trailers[i]; > ssize_t separator_pos = find_separator(trailer, separators); > @@ -1150,7 +1151,19 @@ static void format_trailer_info(struct strbuf *out, > if (opts->unfold) > unfold_value(&val); > > - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); > + if (opts->filter_trailer) { > + if (!strcasecmp (tok.buf, opts->filter_trailer)) { > + if (!printed_first) { > + strbuf_addf(out, "(%s: ", opts->filter_trailer); > + printed_first = 1; > + } else { > + strbuf_addstr(out, ", "); > + } > + strbuf_addstr(out, val.buf); > + } > + } else { > + strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); > + } > strbuf_release(&tok); > strbuf_release(&val); > > @@ -1158,7 +1171,8 @@ static void format_trailer_info(struct strbuf *out, > strbuf_addstr(out, trailer); > } > } > - > + if (printed_first) > + strbuf_addstr(out, ")"); > } > > void format_trailers_from_commit(struct strbuf *out, const char *msg, > diff --git a/trailer.h b/trailer.h > index b997739649..852c79d449 100644 > --- a/trailer.h > +++ b/trailer.h > @@ -72,6 +72,7 @@ struct process_trailer_options { > int only_input; > int unfold; > int no_divider; > + char *filter_trailer; > }; > > #define PROCESS_TRAILER_OPTIONS_INIT {0} ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH] pretty: Add %(trailer:X) to display single trailer 2018-10-28 12:50 [PATCH] pretty: Add %(trailer:X) to display single trailer Anders Waldenborg 2018-10-29 4:49 ` Junio C Hamano @ 2018-10-29 14:14 ` Jeff King 2018-10-29 17:05 ` Anders Waldenborg 2018-11-04 15:22 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Anders Waldenborg ` (2 subsequent siblings) 4 siblings, 1 reply; 69+ messages in thread From: Jeff King @ 2018-10-29 14:14 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Junio C Hamano On Sun, Oct 28, 2018 at 01:50:25PM +0100, Anders Waldenborg wrote: > This new format placeholder allows displaying only a single > trailer. The formatting done is similar to what is done for > --decorate/%d using parentheses and comma separation. Displaying a single trailer makes sense as a goal. It was one of the things I considered when working on %(trailers), actually, but I ended up needing something a bit more flexible (hence the ability to dump the trailers in a parse-able format, where I feed them to another script). But your ticket example makes sense for just ordinary log displays. Junio's review already covered my biggest question, which is why not something like "%(trailers:key=ticket)". And likewise making things like comma-separation options. But my second question is whether we want to provide something more flexible than the always-parentheses that "%d" provides. That has been a problem in the past when people want to format the decoration in some other way. We have formatting magic for "if this thing is non-empty, then show this prefix" in the for-each-ref formatter, but I'm not sure that we do in the commit pretty-printer beyond "% ". I wonder if we could/should add a a placeholder for "if this thing is non-empty, put in a space and enclose it in parentheses". > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt > index 6109ef09aa..a46d0c0717 100644 > --- a/Documentation/pretty-formats.txt > +++ b/Documentation/pretty-formats.txt > @@ -211,6 +211,10 @@ endif::git-rev-list[] > If the `unfold` option is given, behave as if interpret-trailer's > `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do > both. > +- %(trailer:<t>): display the specified trailer in parentheses (like > + %d does for refnames). If there are multiple entries of that trailer > + they are shown comma separated. If there are no matching trailers > + nothing is displayed. It might be worth specifying how this match is done. I'm thinking specifically of whether it's case-sensitive, but I wonder if there should be any allowance for other normalization (e.g., allowing a regex to match "coauthored-by" and "co-authored-by" or something). -Peff ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH] pretty: Add %(trailer:X) to display single trailer 2018-10-29 14:14 ` Jeff King @ 2018-10-29 17:05 ` Anders Waldenborg 2018-10-31 20:27 ` Jeff King 0 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-10-29 17:05 UTC (permalink / raw) To: peff; +Cc: Anders Waldenborg, git, gitster On Mon, Oct 29, 2018 at 3:14 PM Jeff King <peff@peff.net> wrote: > Junio's review already covered my biggest question, which is why not > something like "%(trailers:key=ticket)". And likewise making things like > comma-separation options. Jeff, Junio, thanks! Your questions pretty much matches what I (and a colleague I discussed this with before posting) was concerned about. My first try actually had it as an option to "trailers". But it got a bit messy with the argument parsing, and the fact that there was a fast path making it work when only specified. I did not want to spend lot of time reworking fixing that before I had some feedback, so I went for a smallest possible patch to float the idea with (a patch is worth a 1000 words). I'll start by reworking my patch to handle %(trailers:key=X) (I'll assume keys never contain ')' or ','), and ignore any formatting until the way forward there is decided (see below). > But my second question is whether we want to provide something more > flexible than the always-parentheses that "%d" provides. That has been a > problem in the past when people want to format the decoration in some > other way. Maybe just like +/-/space can be used directly after %, a () pair can be allowed.. E.g "%d" would just be an alias for "%()D", and for trailers it would be something like "%()(trailers:key=foo)" There is another special cased placeholder %f (sanitized subject line, suitable for a filename). Which also could be changed to be a format specifiier, allowing sanitize any thing, e.g "%!an" for sanitized author name. Is even the linebreak to commaseparation a generic thing? "% ,()(trailers:key=Ticket)" it starts go look a bit silly. Then there are the padding modifiers. %<() %<|(). They operate on next placeholder. "%<(10)%s" Is that a better syntax? "%()%(trailers:key=Ticket,comma)" I can also imagine moving all these modifiers into a generic modifier syntax in brackets (and keeping old for backwards compat) %[lpad=10,ltrunc=10]s == %<(10,trunc)%s %[nonempty-prefix="%n"]GS == %+GS %[nonempty-prefix=" (",nonempty-suffix=")"]D == %d Which would mean something like this for tickets thing: %[nonempty-prefix=" (Tickets:",nonempty-suffix=")",commaseparatelines](trailers:key=Ticket,nokey) which is kinda verbose. > We have formatting magic for "if this thing is non-empty, then show this > prefix" in the for-each-ref formatter, but I'm not sure that we do in > the commit pretty-printer beyond "% ". I wonder if we could/should add a > a placeholder for "if this thing is non-empty, put in a space and > enclose it in parentheses". Would there be any interest in consolidating those formatters? Even though they are totally separate beasts today. I think having all attributes available on long form (e.g "%(authorname)") in addition to existing short forms in pretty-formatter would make sense. anders ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH] pretty: Add %(trailer:X) to display single trailer 2018-10-29 17:05 ` Anders Waldenborg @ 2018-10-31 20:27 ` Jeff King 2018-10-31 23:01 ` Anders Waldenborg 0 siblings, 1 reply; 69+ messages in thread From: Jeff King @ 2018-10-31 20:27 UTC (permalink / raw) To: Anders Waldenborg; +Cc: Olga Telezhnaya, git, gitster On Mon, Oct 29, 2018 at 06:05:34PM +0100, Anders Waldenborg wrote: > I'll start by reworking my patch to handle %(trailers:key=X) (I'll > assume keys never contain ')' or ','), and ignore any formatting until > the way forward there is decided (see below). IMHO that is probably an acceptable tradeoff. We haven't really made any rules for quoting arbitrary values in other %() sequences. I think it's something we may want to have eventually, but as long as the rule for now is "you can't do that", I think it would be OK to loosen it later. > > But my second question is whether we want to provide something more > > flexible than the always-parentheses that "%d" provides. That has been a > > problem in the past when people want to format the decoration in some > > other way. > > Maybe just like +/-/space can be used directly after %, a () pair can > be allowed.. E.g "%d" would just be an alias for "%()D", and for > trailers it would be something like "%()(trailers:key=foo)" Yeah, I was thinking that "(" was taken as a special character, but I guess immediately followed by ")" it is easy to parse left-to-right with no ambiguity. Would it include the leading space, too? It would be nice if it could be combined with "% " in an orthogonal way. I guess in theory "% ()D" would work, but it may need some tweaks to the parsing. > There is another special cased placeholder %f (sanitized subject line, > suitable for a filename). Which also could be changed to be a format > specifiier, allowing sanitize any thing, e.g "%!an" for sanitized > author name. Yeah, I agree we should be able to sanitize anything. It's not strictly related to your patch, though, so you may or may not want to go down this rabbit hole. :) > Is even the linebreak to commaseparation a generic thing? > "% ,()(trailers:key=Ticket)" it starts go look a bit silly. In theory, yeah. I agree it's getting a bit magical. > Then there are the padding modifiers. %<() %<|(). They operate on next > placeholder. "%<(10)%s" Is that a better syntax? > "%()%(trailers:key=Ticket,comma)" > > I can also imagine moving all these modifiers into a generic modifier > syntax in brackets (and keeping old for backwards compat) > %[lpad=10,ltrunc=10]s == %<(10,trunc)%s > %[nonempty-prefix="%n"]GS == %+GS > %[nonempty-prefix=" (",nonempty-suffix=")"]D == %d > Which would mean something like this for tickets thing: > %[nonempty-prefix=" (Tickets:",nonempty-suffix=")",commaseparatelines](trailers:key=Ticket,nokey) > which is kinda verbose. Yes. I had dreams of eventually stuffing all of those as options into the placeholders themselves. So "%s" would eventually have a long-form of "%(subject)", and in that syntax it could be: %(subject:lpad=10,filename) or something. I'm not completely opposed to: %[lpad=10,filename]%(subject) which keeps the "formatting" arguments out of the regular placeholders. On the other hand, if the rule were not "this affects the next placeholder" but had a true ending mark, then we could make a real parse-tree out of it, and format chunks of placeholders. E.g.: %(format:lpad=30,filename)%(subject) %(authordate)%(end) would pad and format the whole string with two placeholders. I know that going down this road eventually involves reinventing XML, but I think having an actual tree structure may not be an unreasonable thing to shoot for. I dunno. You certainly don't need to solve all of these issues for what you want to do. My main concern for now is to avoid introducing new syntax that we'll be stuck with forever, even though it may later become redundant (or worse, create parsing ambiguities). > > We have formatting magic for "if this thing is non-empty, then show this > > prefix" in the for-each-ref formatter, but I'm not sure that we do in > > the commit pretty-printer beyond "% ". I wonder if we could/should add a > > a placeholder for "if this thing is non-empty, put in a space and > > enclose it in parentheses". > > Would there be any interest in consolidating those formatters? Even > though they are totally separate beasts today. I think having all > attributes available on long form (e.g "%(authorname)") in addition to > existing short forms in pretty-formatter would make sense. Yes, there's great interest. :) The formats are not mutually incompatible at this point, so we should be able to come up with a unified language that maintains backwards compatibility. One of the tricky parts is that some of the formatters have more information than others (for-each-ref has a ref, which may resolve to any object type; cat-file has objects only; --pretty has only commits). This was the subject of last year's Outreachy work. There's still a ways to go, but you can find some of the previous discussions and work by searching for Olga's work in the archive: https://public-inbox.org/git/?q=olga+telezhnaya I've also cc'd her here, as she's still been doing some work since then. -Peff ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH] pretty: Add %(trailer:X) to display single trailer 2018-10-31 20:27 ` Jeff King @ 2018-10-31 23:01 ` Anders Waldenborg 2018-11-01 18:42 ` Jeff King 0 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-10-31 23:01 UTC (permalink / raw) To: Jeff King; +Cc: Olga Telezhnaya, git, gitster Jeff King writes: > On the other hand, if the rule were not "this affects the next > placeholder" but had a true ending mark, then we could make a real > parse-tree out of it, and format chunks of placeholders. E.g.: > > %(format:lpad=30,filename)%(subject) %(authordate)%(end) > > would pad and format the whole string with two placeholders. I know that > going down this road eventually involves reinventing XML, but I think > having an actual tree structure may not be an unreasonable thing to > shoot for. Yes. I'm thinking that with [] for formatting specifiers and () for placeholders, {} would be available for nesting. E.g: %[lpad=30,mangle]{%(subject) %ad%} > My main concern for now is to avoid introducing new > syntax that we'll be stuck with forever, even though it may later become > redundant (or worse, create parsing ambiguities). Agreed. I'm planning to work on the initial "trailer:key=" part later this week. Maybe I can play around with different formatting options and see how it affects the parser. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH] pretty: Add %(trailer:X) to display single trailer 2018-10-31 23:01 ` Anders Waldenborg @ 2018-11-01 18:42 ` Jeff King 0 siblings, 0 replies; 69+ messages in thread From: Jeff King @ 2018-11-01 18:42 UTC (permalink / raw) To: Anders Waldenborg; +Cc: Olga Telezhnaya, git, gitster On Thu, Nov 01, 2018 at 12:01:28AM +0100, Anders Waldenborg wrote: > Jeff King writes: > > > On the other hand, if the rule were not "this affects the next > > placeholder" but had a true ending mark, then we could make a real > > parse-tree out of it, and format chunks of placeholders. E.g.: > > > > %(format:lpad=30,filename)%(subject) %(authordate)%(end) > > > > would pad and format the whole string with two placeholders. I know that > > going down this road eventually involves reinventing XML, but I think > > having an actual tree structure may not be an unreasonable thing to > > shoot for. > > Yes. I'm thinking that with [] for formatting specifiers and () for > placeholders, {} would be available for nesting. E.g: > > %[lpad=30,mangle]{%(subject) %ad%} Hmm. That's kind of ugly, but probably not really any uglier than any of the things I showed. And it has the advantage that we could implement %[] now, and later extend it (well, I guess we'd want to make sure that "%[lpad=30]{foo}" does not treat the curly braces literally, since we'd eventually make them syntactically significant). > I'm planning to work on the initial "trailer:key=" part later this > week. Maybe I can play around with different formatting options and see > how it affects the parser. Great! Thanks for working on this. -Peff ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 0/5] %(trailers) improvements in pretty format 2018-10-28 12:50 [PATCH] pretty: Add %(trailer:X) to display single trailer Anders Waldenborg 2018-10-29 4:49 ` Junio C Hamano 2018-10-29 14:14 ` Jeff King @ 2018-11-04 15:22 ` Anders Waldenborg 2018-11-04 15:22 ` [PATCH v2 1/5] pretty: single return path in %(trailers) handling Anders Waldenborg ` (6 more replies) 2018-12-08 16:36 ` [PATCH v4 0/7] %(trailers) improvements in pretty format Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 0/7] %(trailers) improvements in pretty format Anders Waldenborg 4 siblings, 7 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-11-04 15:22 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg This adds support for three new options to %(trailers): * key -- show only trailers with specified key * nokey -- don't show key part of trailers * separator -- allow specifying custom separator between trailers Anders Waldenborg (5): pretty: single return path in %(trailers) handling pretty: allow showing specific trailers pretty: add support for "nokey" option in %(trailers) pretty: extract fundamental placeholders to separate function pretty: add support for separator option in %(trailers) Documentation/pretty-formats.txt | 17 +++++--- pretty.c | 71 ++++++++++++++++++++++++++------ t/t4205-log-pretty-formats.sh | 60 +++++++++++++++++++++++++++ trailer.c | 28 ++++++++++--- trailer.h | 3 ++ 5 files changed, 156 insertions(+), 23 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 1/5] pretty: single return path in %(trailers) handling 2018-11-04 15:22 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Anders Waldenborg @ 2018-11-04 15:22 ` Anders Waldenborg 2018-11-04 15:22 ` [PATCH v2 2/5] pretty: allow showing specific trailers Anders Waldenborg ` (5 subsequent siblings) 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-11-04 15:22 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg No functional change intended. This change may not seem useful on its own, but upcoming commits will do memory allocation in there, and a single return path makes deallocation easier. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- pretty.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pretty.c b/pretty.c index b83a3ecd2..aa03d5b23 100644 --- a/pretty.c +++ b/pretty.c @@ -1312,6 +1312,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; + size_t ret = 0; opts.no_divider = 1; @@ -1328,8 +1329,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); - return arg - placeholder + 1; + ret = arg - placeholder + 1; } + return ret; } return 0; /* unknown placeholder */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 2/5] pretty: allow showing specific trailers 2018-11-04 15:22 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Anders Waldenborg 2018-11-04 15:22 ` [PATCH v2 1/5] pretty: single return path in %(trailers) handling Anders Waldenborg @ 2018-11-04 15:22 ` Anders Waldenborg 2018-11-04 18:14 ` Eric Sunshine 2018-11-05 5:14 ` Junio C Hamano 2018-11-04 15:22 ` [PATCH v2 3/5] pretty: add support for "nokey" option in %(trailers) Anders Waldenborg ` (4 subsequent siblings) 6 siblings, 2 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-11-04 15:22 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg Adds a new "key=X" option to "%(trailers)" which will cause it to only print trailers lines which matches the specified key. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 13 +++++---- pretty.c | 15 ++++++++++- t/t4205-log-pretty-formats.sh | 45 ++++++++++++++++++++++++++++++++ trailer.c | 8 +++--- trailer.h | 1 + 5 files changed, 73 insertions(+), 9 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 417b638cd..8326fc45e 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -209,11 +209,14 @@ endif::git-rev-list[] respectively, but padding both sides (i.e. the text is centered) - %(trailers[:options]): display the trailers of the body as interpreted by linkgit:git-interpret-trailers[1]. The `trailers` string may be - followed by a colon and zero or more comma-separated options. If the - `only` option is given, omit non-trailer lines from the trailer block. - If the `unfold` option is given, behave as if interpret-trailer's - `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do - both. + followed by a colon and zero or more comma-separated options. The + allowed options are `only` which omits non-trailer lines from the + trailer block, `unfold` to make it behave as if interpret-trailer's + `--unfold` option was given, and `key=T` to only show trailers with + specified key (matching is done + case-insensitively). E.g. `%(trailers:only,unfold)` unfolds and + shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)` + unfolds and shows trailer lines with key `Reviewed-by`. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index aa03d5b23..cdca9dce2 100644 --- a/pretty.c +++ b/pretty.c @@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ opts.only_trailers = 1; else if (match_placeholder_arg(arg, "unfold", &arg)) opts.unfold = 1; - else + else if (skip_prefix(arg, "key=", &arg)) { + const char *end = arg + strcspn(arg, ",)"); + + if (opts.filter_key) + free(opts.filter_key); + + opts.filter_key = xstrndup(arg, end - arg); + arg = end; + if (*arg == ',') + arg++; + + opts.only_trailers = 1; + } else break; } } @@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ format_trailers_from_commit(sb, msg + c->subject_off, &opts); ret = arg - placeholder + 1; } + free(opts.filter_key); return ret; } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 978a8a66f..0f5207242 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' ' test_cmp expect actual ' +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' ' + git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual && + { + echo "Acked-by: A U Thor <author@example.com>" && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' ' + git log --no-walk --pretty="%(trailers:key=AcKed-bY)" >actual && + { + echo "Acked-by: A U Thor <author@example.com>" && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=nonexistant) becomes empty' ' + git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual && + { + echo "xx" + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' ' + git log --no-walk --pretty="%(trailers:key=Signed-Off-by)" >actual && + { + grep -v patch.description <trailers | grep -v Acked-by && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=foo,unfold) properly unfolds' ' + git log --no-walk --pretty="%(trailers:key=Signed-Off-by,unfold)" >actual && + { + echo "Signed-off-by: A U Thor <author@example.com>" && + echo "Signed-off-by: A U Thor <author@example.com>" && + echo + } >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index 0796f326b..cbbb553e4 100644 --- a/trailer.c +++ b/trailer.c @@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out, struct strbuf val = STRBUF_INIT; parse_trailer(&tok, &val, NULL, trailer, separator_pos); - if (opts->unfold) - unfold_value(&val); + if (!opts->filter_key || !strcasecmp (tok.buf, opts->filter_key)) { + if (opts->unfold) + unfold_value(&val); - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + } strbuf_release(&tok); strbuf_release(&val); diff --git a/trailer.h b/trailer.h index b99773964..d052d02ae 100644 --- a/trailer.h +++ b/trailer.h @@ -72,6 +72,7 @@ struct process_trailer_options { int only_input; int unfold; int no_divider; + char *filter_key; }; #define PROCESS_TRAILER_OPTIONS_INIT {0} -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v2 2/5] pretty: allow showing specific trailers 2018-11-04 15:22 ` [PATCH v2 2/5] pretty: allow showing specific trailers Anders Waldenborg @ 2018-11-04 18:14 ` Eric Sunshine 2018-11-05 3:48 ` Junio C Hamano 2018-11-05 8:26 ` Anders Waldenborg 2018-11-05 5:14 ` Junio C Hamano 1 sibling, 2 replies; 69+ messages in thread From: Eric Sunshine @ 2018-11-04 18:14 UTC (permalink / raw) To: anders; +Cc: Git List, Junio C Hamano, Jeff King, Olga Telezhnaya On Sun, Nov 4, 2018 at 10:24 AM Anders Waldenborg <anders@0x63.nu> wrote: > Adds a new "key=X" option to "%(trailers)" which will cause it to only > print trailers lines which matches the specified key. > > Signed-off-by: Anders Waldenborg <anders@0x63.nu> > --- > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt > @@ -209,11 +209,14 @@ endif::git-rev-list[] > - %(trailers[:options]): display the trailers of the body as interpreted > by linkgit:git-interpret-trailers[1]. The `trailers` string may be > + followed by a colon and zero or more comma-separated options. The > + allowed options are `only` which omits non-trailer lines from the > + trailer block, `unfold` to make it behave as if interpret-trailer's > + `--unfold` option was given, and `key=T` to only show trailers with > + specified key (matching is done > + case-insensitively). Does the user have to include the colon when specifying <val> of 'key=<val>'? I can see from peeking at the implementation that the colon must not be used, but this should be documented. Should the code tolerate a trailing colon? (Genuine question; it's easy to do and would be more user-friendly.) Does 'key=<val>', do a full or partial match on trailers? And, if partial, is the match anchored at the start or can it match anywhere in the trailer key? I see from the implementation that it does a full match, but this behavior should be documented. What happens if 'key=...' is specified multiple times? Are the multiple keys conjunctive? Disjunctive? Last-wins? I can see from the implementation that it is last-wins, but this behavior should be documented. (I wonder how painful it will be for people who want to match multiple keys. This doesn't have to be answered yet, as the behavior can always be loosened later to allow multiple-key matching since the current syntax does not disallow such expansion.) Thinking further on the last two points, should <val> be a regular expression? > + shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)` > + unfolds and shows trailer lines with key `Reviewed-by`. > diff --git a/pretty.c b/pretty.c > @@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > + opts.filter_key = xstrndup(arg, end - arg); > @@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > format_trailers_from_commit(sb, msg + c->subject_off, &opts); > } > + free(opts.filter_key); If I understand correctly, this is making a copy of <val> so that it will be NUL-terminated since the code added to trailer.c uses a simple strcasecmp() to match it. Would it make sense to avoid the copy by adding fields 'opts.filter_key' and 'opts.filter_key_len' and using strncasecmp() instead? (Genuine question; not necessarily a request for change.) > diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh > @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' ' > +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' ' > + git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual && > + { > + echo "Acked-by: A U Thor <author@example.com>" && > + echo > + } >expect && > + test_cmp expect actual > +' I guess these new tests are modeled after one or two existing tests which use a series of 'echo' statements, but an alternative would be: cat <<-\EOF >expect && Acked-by: A U Thor <author@example.com> EOF or, even: test_write_lines "Acked-by: A U Thor <author@example.com>" "" && though, that's probably less readable. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 2/5] pretty: allow showing specific trailers 2018-11-04 18:14 ` Eric Sunshine @ 2018-11-05 3:48 ` Junio C Hamano 2018-11-05 3:52 ` Eric Sunshine 2018-11-05 8:26 ` Anders Waldenborg 1 sibling, 1 reply; 69+ messages in thread From: Junio C Hamano @ 2018-11-05 3:48 UTC (permalink / raw) To: Eric Sunshine; +Cc: anders, Git List, Jeff King, Olga Telezhnaya Eric Sunshine <sunshine@sunshineco.com> writes: > Does the user have to include the colon when specifying <val> of > 'key=<val>'? I can see from peeking at the implementation that the > colon must not be used, but this should be documented. Should the code > tolerate a trailing colon? (Genuine question; it's easy to do and > would be more user-friendly.) > > Does 'key=<val>', do a full or partial match on trailers? And, if > partial, is the match anchored at the start or can it match anywhere > in the trailer key? I see from the implementation that it does a full > match, but this behavior should be documented. > > What happens if 'key=...' is specified multiple times? Are the > multiple keys conjunctive? Disjunctive? Last-wins? I can see from the > implementation that it is last-wins, but this behavior should be > documented. (I wonder how painful it will be for people who want to > match multiple keys. This doesn't have to be answered yet, as the > behavior can always be loosened later to allow multiple-key matching > since the current syntax does not disallow such expansion.) > > Thinking further on the last two points, should <val> be a regular expression? Another thing that needs to be clarified in the document would be case sensitivity. People sometimes spell "Signed-Off-By:" by mistake (or is it by malice?). I do suspect that the parser should just make a list of sought-after keys, not doing "last-one-wins", as that won't be very difficult to do and makes what happens when given multiple keys trivially obvious. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 2/5] pretty: allow showing specific trailers 2018-11-05 3:48 ` Junio C Hamano @ 2018-11-05 3:52 ` Eric Sunshine 0 siblings, 0 replies; 69+ messages in thread From: Eric Sunshine @ 2018-11-05 3:52 UTC (permalink / raw) To: Junio C Hamano; +Cc: anders, Git List, Jeff King, Olga Telezhnaya On Sun, Nov 4, 2018 at 10:48 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > Does the user have to include the colon when specifying <val> of > > 'key=<val>'? > > Does 'key=<val>', do a full or partial match on trailers? > > What happens if 'key=...' is specified multiple times? > > Thinking further on the last two points, should <val> be a regular expression? > > Another thing that needs to be clarified in the document would be > case sensitivity. People sometimes spell "Signed-Off-By:" by > mistake (or is it by malice?). The documentation does say parenthetically "(matching is done case-insensitively)", so I think that's already covered. Or did you have something else in mind? ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 2/5] pretty: allow showing specific trailers 2018-11-04 18:14 ` Eric Sunshine 2018-11-05 3:48 ` Junio C Hamano @ 2018-11-05 8:26 ` Anders Waldenborg 2018-11-05 9:00 ` Eric Sunshine 1 sibling, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-11-05 8:26 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Jeff King, Olga Telezhnaya Eric Sunshine writes: > Should the code tolerate a trailing colon? (Genuine question; it's > easy to do and would be more user-friendly.) I would make sense to allow the trailing colon, it is easy enough to just strip that away when reading the argument. However I'm not sure how that would fit together with the possibility to later lifting it to a regexp, hard to strip a trailing colon from a regexp in a generic way. > What happens if 'key=...' is specified multiple times? My first thought was to simply disallow that. But that seemed hard to fit into current model where errors just means don't expand. I would guess that most useful and intuitive to user would be to handle multiple key arguments by showing any of those keys. > Thinking further on the last two points, should <val> be a regular expression? It probably would make sense. I can see how the regexp '^.*-by$' would be useful (but glob style matching would suffice in that case). Also handling multi-matching with an alternation group would be elegant %(trailers:key="(A|B)"). Except for the fact that the parser would need to understand some kind of quoting, which seems like an major undertaking. I guess having it as a regular exception would also mean that it needs to get some way to cache the re so it is compiled once, and not for each expansion. > >> + free(opts.filter_key); > > If I understand correctly, this is making a copy of <val> so that it > will be NUL-terminated since the code added to trailer.c uses a simple > strcasecmp() to match it. Would it make sense to avoid the copy by > adding fields 'opts.filter_key' and 'opts.filter_key_len' and using > strncasecmp() instead? (Genuine question; not necessarily a request > for change.) I'm also not very happy about that copy. Just using strncasecmp would cause it to be prefix match, no? But if changing matching semantics to handle multiple key options to something else I'm thinking opts.filter_key would be replaced with a opts.filter callback function, and that part would need to be rewritten anyway. > >> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh >> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' ' >> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' ' >> + git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual && >> + { >> + echo "Acked-by: A U Thor <author@example.com>" && >> + echo >> + } >expect && >> + test_cmp expect actual >> +' > > I guess these new tests are modeled after one or two existing tests > which use a series of 'echo' statements I guess I could change it to "--pretty=format:%(trailers:key=Acked-by)" to get separator semantics and avoid that extra blank line, making it simpler. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 2/5] pretty: allow showing specific trailers 2018-11-05 8:26 ` Anders Waldenborg @ 2018-11-05 9:00 ` Eric Sunshine 0 siblings, 0 replies; 69+ messages in thread From: Eric Sunshine @ 2018-11-05 9:00 UTC (permalink / raw) To: anders; +Cc: Git List, Junio C Hamano, Jeff King, Olga Telezhnaya On Mon, Nov 5, 2018 at 3:26 AM Anders Waldenborg <anders@0x63.nu> wrote: > Eric Sunshine writes: > > Should the code tolerate a trailing colon? (Genuine question; it's > > easy to do and would be more user-friendly.) > > I would make sense to allow the trailing colon, it is easy enough to > just strip that away when reading the argument. > > However I'm not sure how that would fit together with the possibility to > later lifting it to a regexp, hard to strip a trailing colon from a > regexp in a generic way. Which is a good reason to think about these issues now, before being set in stone. > > What happens if 'key=...' is specified multiple times? > > My first thought was to simply disallow that. But that seemed hard to > fit into current model where errors just means don't expand. > > I would guess that most useful and intuitive to user would be to handle > multiple key arguments by showing any of those keys. Agreed. > > Thinking further on the last two points, should <val> be a regular expression? > > It probably would make sense. I can see how the regexp '^.*-by$' would > be useful (but glob style matching would suffice in that case). > > Also handling multi-matching with an alternation group would be elegant > %(trailers:key="(A|B)"). Except for the fact that the parser would need to > understand some kind of quoting, which seems like an major undertaking. Maybe, maybe not. As long as we're careful not to paint ourselves into a corner, it might very well be okay to start with the current implementation of matching the full key as a literal string and (perhaps much) later introduce regex as an alternate way to specify the key. For instance, 'key=literal' and 'key=/regex/' can co-exist, and the extraction of the regex inside /.../ should not be especially difficult. > I guess having it as a regular exception would also mean that it needs > to get some way to cache the re so it is compiled once, and not for each expansion. Yes, that's something I brought up a few years ago during a GSoC project; not regex specifically, but that this parsing of the format is happening repeatedly rather than just once. I had suggested to the GSoC student that the parsing could be done early, compiling the format expression into a "machine" which could be applied repeatedly. It's a larger job, of course, not necessarily something worth tackling for your current needs. > > If I understand correctly, this is making a copy of <val> so that it > > will be NUL-terminated since the code added to trailer.c uses a simple > > strcasecmp() to match it. Would it make sense to avoid the copy by > > adding fields 'opts.filter_key' and 'opts.filter_key_len' and using > > strncasecmp() instead? (Genuine question; not necessarily a request > > for change.) > > I'm also not very happy about that copy. > Just using strncasecmp would cause it to be prefix match, no? Well, you could retain full key match by checking for NUL explicitly with something like this: !strncasecmp(tok.buf, opts->filter_key, opts->filter_key_len) && !tok.buf[opts->filter_key_len] ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 2/5] pretty: allow showing specific trailers 2018-11-04 15:22 ` [PATCH v2 2/5] pretty: allow showing specific trailers Anders Waldenborg 2018-11-04 18:14 ` Eric Sunshine @ 2018-11-05 5:14 ` Junio C Hamano 1 sibling, 0 replies; 69+ messages in thread From: Junio C Hamano @ 2018-11-05 5:14 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King, Olga Telezhnaya Anders Waldenborg <anders@0x63.nu> writes: > + else if (skip_prefix(arg, "key=", &arg)) { > + const char *end = arg + strcspn(arg, ",)"); > + > + if (opts.filter_key) > + free(opts.filter_key); Call the free() unconditionally, cf. contrib/coccinelle/free.cocci. ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 3/5] pretty: add support for "nokey" option in %(trailers) 2018-11-04 15:22 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Anders Waldenborg 2018-11-04 15:22 ` [PATCH v2 1/5] pretty: single return path in %(trailers) handling Anders Waldenborg 2018-11-04 15:22 ` [PATCH v2 2/5] pretty: allow showing specific trailers Anders Waldenborg @ 2018-11-04 15:22 ` Anders Waldenborg 2018-11-04 15:22 ` [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function Anders Waldenborg ` (3 subsequent siblings) 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-11-04 15:22 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg With the new "key=" option to %trailers it often makes little sense to show the key, as it by definition already is know which trailer is printed there. This new "nokey" option makes it omit key trailer key when printing trailers. E.g.: $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,nokey)' aaaa88182 will show: > upload-pack: fix broken if/else chain in config callback > Jeff King <peff@peff.net> > Junio C Hamano <gitster@pobox.com> Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 11 ++++++----- pretty.c | 2 ++ t/t4205-log-pretty-formats.sh | 9 +++++++++ trailer.c | 6 ++++-- trailer.h | 1 + 5 files changed, 22 insertions(+), 7 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 8326fc45e..e115e355d 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -212,11 +212,12 @@ endif::git-rev-list[] followed by a colon and zero or more comma-separated options. The allowed options are `only` which omits non-trailer lines from the trailer block, `unfold` to make it behave as if interpret-trailer's - `--unfold` option was given, and `key=T` to only show trailers with - specified key (matching is done - case-insensitively). E.g. `%(trailers:only,unfold)` unfolds and - shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)` - unfolds and shows trailer lines with key `Reviewed-by`. + `--unfold` option was given, `key=T` to only show trailers with + specified key (matching is done case-insensitively), and `nokey` + which makes it skip over the key part of the trailer and only show + value. E.g. `%(trailers:only,unfold)` unfolds and shows all trailer + lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows + trailer lines with key `Reviewed-by`. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index cdca9dce2..f87ba4f18 100644 --- a/pretty.c +++ b/pretty.c @@ -1323,6 +1323,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ opts.only_trailers = 1; else if (match_placeholder_arg(arg, "unfold", &arg)) opts.unfold = 1; + else if (match_placeholder_arg(arg, "nokey", &arg)) + opts.no_key = 1; else if (skip_prefix(arg, "key=", &arg)) { const char *end = arg + strcspn(arg, ",)"); diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 0f5207242..e7de3b18a 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -643,6 +643,15 @@ test_expect_success '%(trailers:key=foo,unfold) properly unfolds' ' test_cmp expect actual ' +test_expect_success '%(trailers:key=foo,nokey) shows only value' ' + git log --no-walk --pretty="%(trailers:key=Acked-by,nokey)" >actual && + { + echo "A U Thor <author@example.com>" && + echo + } >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index cbbb553e4..4f19c34cb 100644 --- a/trailer.c +++ b/trailer.c @@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out, if (!opts->filter_key || !strcasecmp (tok.buf, opts->filter_key)) { if (opts->unfold) unfold_value(&val); - - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + if (opts->no_key) + strbuf_addf(out, "%s\n", val.buf); + else + strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); } strbuf_release(&tok); strbuf_release(&val); diff --git a/trailer.h b/trailer.h index d052d02ae..83de87ee9 100644 --- a/trailer.h +++ b/trailer.h @@ -72,6 +72,7 @@ struct process_trailer_options { int only_input; int unfold; int no_divider; + int no_key; char *filter_key; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function 2018-11-04 15:22 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Anders Waldenborg ` (2 preceding siblings ...) 2018-11-04 15:22 ` [PATCH v2 3/5] pretty: add support for "nokey" option in %(trailers) Anders Waldenborg @ 2018-11-04 15:22 ` Anders Waldenborg 2018-11-05 2:06 ` Junio C Hamano 2018-11-04 15:22 ` [PATCH v2 5/5] pretty: add support for separator option in %(trailers) Anders Waldenborg ` (2 subsequent siblings) 6 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-11-04 15:22 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg No functional change intended Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- pretty.c | 37 ++++++++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 11 deletions(-) diff --git a/pretty.c b/pretty.c index f87ba4f18..9fdddce9d 100644 --- a/pretty.c +++ b/pretty.c @@ -1074,6 +1074,27 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate, return 0; } +static size_t format_fundamental(struct strbuf *sb, /* in UTF-8 */ + const char *placeholder, + void *context) +{ + int ch; + + switch (placeholder[0]) { + case 'n': /* newline */ + strbuf_addch(sb, '\n'); + return 1; + case 'x': + /* %x00 == NUL, %x0a == LF, etc. */ + ch = hex2chr(placeholder + 1); + if (ch < 0) + return 0; + strbuf_addch(sb, ch); + return 3; + } + return 0; +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1083,9 +1104,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *msg = c->message; struct commit_list *p; const char *arg; - int ch; + size_t res; /* these are independent of the commit */ + res = format_fundamental(sb, placeholder, NULL); + if (res) + return res; + switch (placeholder[0]) { case 'C': if (starts_with(placeholder + 1, "(auto)")) { @@ -1104,16 +1129,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ */ return ret; } - case 'n': /* newline */ - strbuf_addch(sb, '\n'); - return 1; - case 'x': - /* %x00 == NUL, %x0a == LF, etc. */ - ch = hex2chr(placeholder + 1); - if (ch < 0) - return 0; - strbuf_addch(sb, ch); - return 3; case 'w': if (placeholder[1] == '(') { unsigned long width = 0, indent1 = 0, indent2 = 0; -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function 2018-11-04 15:22 ` [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function Anders Waldenborg @ 2018-11-05 2:06 ` Junio C Hamano 2018-11-05 8:32 ` Anders Waldenborg 0 siblings, 1 reply; 69+ messages in thread From: Junio C Hamano @ 2018-11-05 2:06 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King, Olga Telezhnaya Anders Waldenborg <anders@0x63.nu> writes: > No functional change intended > > Signed-off-by: Anders Waldenborg <anders@0x63.nu> > --- > pretty.c | 37 ++++++++++++++++++++++++++----------- > 1 file changed, 26 insertions(+), 11 deletions(-) I do not think "fundamental" is the best name for this, but I agree that it would be useful to split the helpers into one that is "constant across commits" and the other one that is "per commit". > diff --git a/pretty.c b/pretty.c > index f87ba4f18..9fdddce9d 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1074,6 +1074,27 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate, > return 0; > } > > +static size_t format_fundamental(struct strbuf *sb, /* in UTF-8 */ > + const char *placeholder, > + void *context) > +{ > + int ch; > + > + switch (placeholder[0]) { > + case 'n': /* newline */ > + strbuf_addch(sb, '\n'); > + return 1; > + case 'x': > + /* %x00 == NUL, %x0a == LF, etc. */ > + ch = hex2chr(placeholder + 1); > + if (ch < 0) > + return 0; > + strbuf_addch(sb, ch); > + return 3; > + } > + return 0; > +} > + > static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > const char *placeholder, > void *context) > @@ -1083,9 +1104,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > const char *msg = c->message; > struct commit_list *p; > const char *arg; > - int ch; > + size_t res; > > /* these are independent of the commit */ > + res = format_fundamental(sb, placeholder, NULL); > + if (res) > + return res; > + > switch (placeholder[0]) { > case 'C': > if (starts_with(placeholder + 1, "(auto)")) { > @@ -1104,16 +1129,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > */ > return ret; > } > - case 'n': /* newline */ > - strbuf_addch(sb, '\n'); > - return 1; > - case 'x': > - /* %x00 == NUL, %x0a == LF, etc. */ > - ch = hex2chr(placeholder + 1); > - if (ch < 0) > - return 0; > - strbuf_addch(sb, ch); > - return 3; > case 'w': > if (placeholder[1] == '(') { > unsigned long width = 0, indent1 = 0, indent2 = 0; ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function 2018-11-05 2:06 ` Junio C Hamano @ 2018-11-05 8:32 ` Anders Waldenborg 2018-11-06 1:46 ` Junio C Hamano 0 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-11-05 8:32 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Olga Telezhnaya Junio C Hamano writes: > I do not think "fundamental" is the best name for this, but I agree > that it would be useful to split the helpers into one that is > "constant across commits" and the other one that is "per commit". Any suggestions for a better name? standalone? simple? invariant? free? ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function 2018-11-05 8:32 ` Anders Waldenborg @ 2018-11-06 1:46 ` Junio C Hamano 0 siblings, 0 replies; 69+ messages in thread From: Junio C Hamano @ 2018-11-06 1:46 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King, Olga Telezhnaya Anders Waldenborg <anders@0x63.nu> writes: > Junio C Hamano writes: >> I do not think "fundamental" is the best name for this, but I agree >> that it would be useful to split the helpers into one that is >> "constant across commits" and the other one that is "per commit". > > Any suggestions for a better name? > > standalone? simple? invariant? free? If these are like %n for LF or %09 for HT, perhaps they are constants or "literals". ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v2 5/5] pretty: add support for separator option in %(trailers) 2018-11-04 15:22 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Anders Waldenborg ` (3 preceding siblings ...) 2018-11-04 15:22 ` [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function Anders Waldenborg @ 2018-11-04 15:22 ` Anders Waldenborg 2018-11-05 2:10 ` Junio C Hamano 2018-11-05 5:18 ` Junio C Hamano 2018-11-04 17:40 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Eric Sunshine 2018-11-18 11:44 ` [PATCH v3 " Anders Waldenborg 6 siblings, 2 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-11-04 15:22 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg By default trailer lines are terminated by linebreaks ('\n'). By specifying the new 'separator' option they will instead be separated by user provided string and have separator semantics rather than terminator semantics. The separator string can contain the fundamental formatting codes %n and %xNN allowing it to be things that are otherwise hard to type as %x00, or command and end-parenthesis which would break parsing. E.g: $ git log --pretty='%(trailers:key=Reviewed-by,nokey,separator=%x00)' Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 13 ++++++++----- pretty.c | 13 +++++++++++++ t/t4205-log-pretty-formats.sh | 6 ++++++ trailer.c | 20 +++++++++++++++++--- trailer.h | 1 + 5 files changed, 45 insertions(+), 8 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index e115e355d..3312850e6 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -213,11 +213,14 @@ endif::git-rev-list[] allowed options are `only` which omits non-trailer lines from the trailer block, `unfold` to make it behave as if interpret-trailer's `--unfold` option was given, `key=T` to only show trailers with - specified key (matching is done case-insensitively), and `nokey` - which makes it skip over the key part of the trailer and only show - value. E.g. `%(trailers:only,unfold)` unfolds and shows all trailer - lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows - trailer lines with key `Reviewed-by`. + specified key (matching is done case-insensitively), `nokey` which + makes it skip over the key part of the trailer and only show value + and `separator` which allows specifying an alternative separator + than the default line + break. E.g. `%(trailers:only,unfold,separator=%x00)` unfolds and + shows all trailer lines separated by NUL character, + `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer lines + with key `Reviewed-by`. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index 9fdddce9d..f73a2b0dc 100644 --- a/pretty.c +++ b/pretty.c @@ -1327,6 +1327,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; + struct strbuf sepbuf = STRBUF_INIT; size_t ret = 0; opts.no_divider = 1; @@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ arg++; opts.only_trailers = 1; + } else if (skip_prefix(arg, "separator=", &arg)) { + size_t seplen = strcspn(arg, ",)"); + strbuf_reset(&sepbuf); + char *fmt = xstrndup(arg, seplen); + strbuf_expand(&sepbuf, fmt, format_fundamental, NULL); + free(fmt); + opts.separator = &sepbuf; + + arg += seplen; + if (*arg == ',') + arg++; } else break; } @@ -1360,6 +1372,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ format_trailers_from_commit(sb, msg + c->subject_off, &opts); ret = arg - placeholder + 1; } + strbuf_release (&sepbuf); free(opts.filter_key); return ret; } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index e7de3b18a..71218d22e 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -652,6 +652,12 @@ test_expect_success '%(trailers:key=foo,nokey) shows only value' ' test_cmp expect actual ' +test_expect_success 'pretty format %(trailers:separator) changes separator' ' + git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual && + printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index 4f19c34cb..a79e4e36a 100644 --- a/trailer.c +++ b/trailer.c @@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out, const struct trailer_info *info, const struct process_trailer_options *opts) { + int first_printed = 0; size_t i; /* If we want the whole block untouched, we can take the fast path. */ - if (!opts->only_trailers && !opts->unfold) { + if (!opts->only_trailers && !opts->unfold && !opts->separator) { strbuf_add(out, info->trailer_start, info->trailer_end - info->trailer_start); return; @@ -1150,16 +1151,29 @@ static void format_trailer_info(struct strbuf *out, if (!opts->filter_key || !strcasecmp (tok.buf, opts->filter_key)) { if (opts->unfold) unfold_value(&val); + if (opts->separator && first_printed) + strbuf_addbuf(out, opts->separator); if (opts->no_key) - strbuf_addf(out, "%s\n", val.buf); + strbuf_addf(out, "%s", val.buf); else - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + strbuf_addf(out, "%s: %s", tok.buf, val.buf); + if (!opts->separator) + strbuf_addch(out, '\n'); + + first_printed = 1; } strbuf_release(&tok); strbuf_release(&val); } else if (!opts->only_trailers) { + if (opts->separator && first_printed) { + strbuf_addbuf(out, opts->separator); + } strbuf_addstr(out, trailer); + if (opts->separator) { + strbuf_rtrim(out); + } + first_printed = 1; } } diff --git a/trailer.h b/trailer.h index 83de87ee9..0e9d89660 100644 --- a/trailer.h +++ b/trailer.h @@ -74,6 +74,7 @@ struct process_trailer_options { int no_divider; int no_key; char *filter_key; + const struct strbuf *separator; }; #define PROCESS_TRAILER_OPTIONS_INIT {0} -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers) 2018-11-04 15:22 ` [PATCH v2 5/5] pretty: add support for separator option in %(trailers) Anders Waldenborg @ 2018-11-05 2:10 ` Junio C Hamano 2018-11-05 18:24 ` Anders Waldenborg 2018-11-05 5:18 ` Junio C Hamano 1 sibling, 1 reply; 69+ messages in thread From: Junio C Hamano @ 2018-11-05 2:10 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King, Olga Telezhnaya Anders Waldenborg <anders@0x63.nu> writes: > @@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > arg++; > > opts.only_trailers = 1; > + } else if (skip_prefix(arg, "separator=", &arg)) { > + size_t seplen = strcspn(arg, ",)"); > + strbuf_reset(&sepbuf); > + char *fmt = xstrndup(arg, seplen); > + strbuf_expand(&sepbuf, fmt, format_fundamental, NULL); This somehow feels akin to using end-user supplied param to printf(3) as its format argument e.g. int main(int ac, char *av) { printf(av[1]); return 0; } which is not a good idea. Is there a mechanism with which we can ensure that the separator=<what> specification will never come from potentially malicious sources (e.g. not used to show things on webpage allowing random folks who access he site to supply custom format)? ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers) 2018-11-05 2:10 ` Junio C Hamano @ 2018-11-05 18:24 ` Anders Waldenborg 2018-11-06 1:48 ` Junio C Hamano 0 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-11-05 18:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Olga Telezhnaya Junio C Hamano writes: > Anders Waldenborg <anders@0x63.nu> writes: > >> @@ -1352,6 +1353,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ >> arg++; >> >> opts.only_trailers = 1; >> + } else if (skip_prefix(arg, "separator=", &arg)) { >> + size_t seplen = strcspn(arg, ",)"); >> + strbuf_reset(&sepbuf); >> + char *fmt = xstrndup(arg, seplen); >> + strbuf_expand(&sepbuf, fmt, format_fundamental, NULL); > > This somehow feels akin to using end-user supplied param to printf(3) > as its format argument e.g. > > int main(int ac, char *av) { > printf(av[1]); > return 0; > } > > which is not a good idea. Is there a mechanism with which we can > ensure that the separator=<what> specification will never come from > potentially malicious sources (e.g. not used to show things on webpage > allowing random folks who access he site to supply custom format)? I can't see a case where this could add anything that isn't already possible. AFAICU strbuf_expand doesn't suffer from the worst things that printf(3) suffers from wrt untrusted format string (i.e no printf style %n which can write to memory, and no vaargs on stack which allows leaking random stuff). The separator option is part of the full format string. If a malicious user can specify that, they can't really do anything new, as the separator only can expand %n and %xNN, which they already can do in the full string. But maybe I'm missing something? ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers) 2018-11-05 18:24 ` Anders Waldenborg @ 2018-11-06 1:48 ` Junio C Hamano 0 siblings, 0 replies; 69+ messages in thread From: Junio C Hamano @ 2018-11-06 1:48 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King, Olga Telezhnaya Anders Waldenborg <anders@0x63.nu> writes: > AFAICU strbuf_expand doesn't suffer from the worst things that printf(3) > suffers from wrt untrusted format string (i.e no printf style %n which > can write to memory, and no vaargs on stack which allows leaking random > stuff). > > The separator option is part of the full format string. If a malicious > user can specify that, they can't really do anything new, as the > separator only can expand %n and %xNN, which they already can do in the > full string. > > But maybe I'm missing something? I just wanted to make sure somebody thought it through (and hoped that that somebody might be you). I do not offhand see a readily usable exploit vector myself. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 5/5] pretty: add support for separator option in %(trailers) 2018-11-04 15:22 ` [PATCH v2 5/5] pretty: add support for separator option in %(trailers) Anders Waldenborg 2018-11-05 2:10 ` Junio C Hamano @ 2018-11-05 5:18 ` Junio C Hamano 1 sibling, 0 replies; 69+ messages in thread From: Junio C Hamano @ 2018-11-05 5:18 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King, Olga Telezhnaya Anders Waldenborg <anders@0x63.nu> writes: > + if (opts->separator && first_printed) > + strbuf_addbuf(out, opts->separator); > if (opts->no_key) > - strbuf_addf(out, "%s\n", val.buf); > + strbuf_addf(out, "%s", val.buf); Avoid addf with "%s" alone as a formatter; instead say strbuf_addstr(out, val.buf); cf. contrib/coccinelle/strbuf.cocci ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 0/5] %(trailers) improvements in pretty format 2018-11-04 15:22 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Anders Waldenborg ` (4 preceding siblings ...) 2018-11-04 15:22 ` [PATCH v2 5/5] pretty: add support for separator option in %(trailers) Anders Waldenborg @ 2018-11-04 17:40 ` Eric Sunshine 2018-11-05 7:09 ` Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 " Anders Waldenborg 6 siblings, 1 reply; 69+ messages in thread From: Eric Sunshine @ 2018-11-04 17:40 UTC (permalink / raw) To: anders; +Cc: Git List, Junio C Hamano, Jeff King, Olga Telezhnaya On Sun, Nov 4, 2018 at 10:23 AM Anders Waldenborg <anders@0x63.nu> wrote: > This adds support for three new options to %(trailers): > * key -- show only trailers with specified key > * nokey -- don't show key part of trailers > * separator -- allow specifying custom separator between trailers If "key" is for including particular trailers, intuition might lead people to think that "nokey" is for excluding certain trailers. Perhaps a different name for "nokey", such as "valueonly" or "stripkey", would be better. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v2 0/5] %(trailers) improvements in pretty format 2018-11-04 17:40 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Eric Sunshine @ 2018-11-05 7:09 ` Anders Waldenborg 0 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-11-05 7:09 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Junio C Hamano, Jeff King, Olga Telezhnaya Eric Sunshine writes: > If "key" is for including particular trailers, intuition might lead > people to think that "nokey" is for excluding certain trailers. > Perhaps a different name for "nokey", such as "valueonly" or > "stripkey", would be better. Good point. I guess "valueonly" would be preferred as it says what it shows, not what it hides. ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 0/5] %(trailers) improvements in pretty format 2018-11-04 15:22 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Anders Waldenborg ` (5 preceding siblings ...) 2018-11-04 17:40 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Eric Sunshine @ 2018-11-18 11:44 ` Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 1/5] pretty: single return path in %(trailers) handling Anders Waldenborg ` (4 more replies) 6 siblings, 5 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-11-18 11:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg Updated since v2: * Allow trailing colon in 'key=' argument * Clarify documentation on how matching is done * Rename option to "valueonly" * Make trailing matching a callback function * Avoid copying match string * Simplify generation of "expected" in tests * Rename function to strbuf_expand_literal_cb * cocci suggested fixes Anders Waldenborg (5): pretty: single return path in %(trailers) handling pretty: allow showing specific trailers pretty: add support for "valueonly" option in %(trailers) strbuf: separate callback for strbuf_expand:ing literals pretty: add support for separator option in %(trailers) Documentation/pretty-formats.txt | 26 ++++++++--- pretty.c | 68 ++++++++++++++++++++++------ strbuf.c | 21 +++++++++ strbuf.h | 8 ++++ t/t4205-log-pretty-formats.sh | 78 ++++++++++++++++++++++++++++++++ trailer.c | 25 ++++++++-- trailer.h | 4 ++ 7 files changed, 206 insertions(+), 24 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 1/5] pretty: single return path in %(trailers) handling 2018-11-18 11:44 ` [PATCH v3 " Anders Waldenborg @ 2018-11-18 11:44 ` Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 2/5] pretty: allow showing specific trailers Anders Waldenborg ` (3 subsequent siblings) 4 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-11-18 11:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg No functional change intended. This change may not seem useful on its own, but upcoming commits will do memory allocation in there, and a single return path makes deallocation easier. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- pretty.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pretty.c b/pretty.c index b83a3ecd2..aa03d5b23 100644 --- a/pretty.c +++ b/pretty.c @@ -1312,6 +1312,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; + size_t ret = 0; opts.no_divider = 1; @@ -1328,8 +1329,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); - return arg - placeholder + 1; + ret = arg - placeholder + 1; } + return ret; } return 0; /* unknown placeholder */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 2/5] pretty: allow showing specific trailers 2018-11-18 11:44 ` [PATCH v3 " Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 1/5] pretty: single return path in %(trailers) handling Anders Waldenborg @ 2018-11-18 11:44 ` Anders Waldenborg 2018-11-20 5:45 ` Junio C Hamano 2018-11-20 5:59 ` Junio C Hamano 2018-11-18 11:44 ` [PATCH v3 3/5] pretty: add support for "valueonly" option in %(trailers) Anders Waldenborg ` (2 subsequent siblings) 4 siblings, 2 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-11-18 11:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg Adds a new "key=X" option to "%(trailers)" which will cause it to only print trailers lines which match the specified key. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 17 +++++++++------ pretty.c | 31 ++++++++++++++++++++++++++- t/t4205-log-pretty-formats.sh | 36 ++++++++++++++++++++++++++++++++ trailer.c | 8 ++++--- trailer.h | 2 ++ 5 files changed, 84 insertions(+), 10 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 417b638cd..75c2e838d 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -207,13 +207,18 @@ endif::git-rev-list[] than given and there are spaces on its left, use those spaces - '%><(<N>)', '%><|(<N>)': similar to '%<(<N>)', '%<|(<N>)' respectively, but padding both sides (i.e. the text is centered) -- %(trailers[:options]): display the trailers of the body as interpreted +- '%(trailers[:options])': display the trailers of the body as interpreted by linkgit:git-interpret-trailers[1]. The `trailers` string may be - followed by a colon and zero or more comma-separated options. If the - `only` option is given, omit non-trailer lines from the trailer block. - If the `unfold` option is given, behave as if interpret-trailer's - `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do - both. + followed by a colon and zero or more comma-separated options: + ** 'only': omit non-trailer lines from the trailer block. + ** 'unfold': make it behave as if interpret-trailer's `--unfold` + option was given. + ** 'key=<K>': only show trailers with specified key. Matching is + done case-insensitively and trailing colon is optional. If option + is given multiple times only last one is used. + ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer + lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows + trailer lines with key `Reviewed-by`. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index aa03d5b23..aaedc8447 100644 --- a/pretty.c +++ b/pretty.c @@ -1074,6 +1074,17 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate, return 0; } +struct format_trailer_match_data { + const char *trailer; + size_t trailer_len; +}; + +static int format_trailer_match_cb(const struct strbuf *sb, void *ud) +{ + struct format_trailer_match_data *data = ud; + return data->trailer_len == sb->len && !strncasecmp (data->trailer, sb->buf, sb->len); +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1312,6 +1323,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; + struct format_trailer_match_data filter_data; size_t ret = 0; opts.no_divider = 1; @@ -1323,7 +1335,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ opts.only_trailers = 1; else if (match_placeholder_arg(arg, "unfold", &arg)) opts.unfold = 1; - else + else if (skip_prefix(arg, "key=", &arg)) { + const char *end = arg + strcspn(arg, ",)"); + + filter_data.trailer = arg; + filter_data.trailer_len = end - arg; + + if (filter_data.trailer_len && + filter_data.trailer[filter_data.trailer_len - 1] == ':') + filter_data.trailer_len--; + + opts.filter = format_trailer_match_cb; + opts.filter_data = &filter_data; + opts.only_trailers = 1; + + arg = end; + if (*arg == ',') + arg++; + } else break; } } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 978a8a66f..aba7ba206 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -598,6 +598,42 @@ test_expect_success ':only and :unfold work together' ' test_cmp expect actual ' +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by)" >actual && + echo "Acked-by: A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' ' + git log --no-walk --pretty="format:%(trailers:key=AcKed-bY)" >actual && + echo "Acked-by: A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo:) trailing colon also works' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by:)" >actual && + echo "Acked-by: A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=nonexistant) becomes empty' ' + git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual && + echo "xx" >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' ' + git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by)" >actual && + grep -v patch.description <trailers | grep -v Acked-by >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=foo,unfold) properly unfolds' ' + git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by,unfold)" >actual && + unfold <trailers | grep Signed-off-by >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index 0796f326b..97c8f2762 100644 --- a/trailer.c +++ b/trailer.c @@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out, struct strbuf val = STRBUF_INIT; parse_trailer(&tok, &val, NULL, trailer, separator_pos); - if (opts->unfold) - unfold_value(&val); + if (!opts->filter || opts->filter(&tok, opts->filter_data)) { + if (opts->unfold) + unfold_value(&val); - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + } strbuf_release(&tok); strbuf_release(&val); diff --git a/trailer.h b/trailer.h index b99773964..5255b676d 100644 --- a/trailer.h +++ b/trailer.h @@ -72,6 +72,8 @@ struct process_trailer_options { int only_input; int unfold; int no_divider; + int (*filter)(const struct strbuf *, void *); + void *filter_data; }; #define PROCESS_TRAILER_OPTIONS_INIT {0} -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v3 2/5] pretty: allow showing specific trailers 2018-11-18 11:44 ` [PATCH v3 2/5] pretty: allow showing specific trailers Anders Waldenborg @ 2018-11-20 5:45 ` Junio C Hamano 2018-11-20 5:59 ` Junio C Hamano 1 sibling, 0 replies; 69+ messages in thread From: Junio C Hamano @ 2018-11-20 5:45 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King Anders Waldenborg <anders@0x63.nu> writes: > + followed by a colon and zero or more comma-separated options: > + ** 'only': omit non-trailer lines from the trailer block. > + ** 'unfold': make it behave as if interpret-trailer's `--unfold` > + option was given. > + ** 'key=<K>': only show trailers with specified key. Matching is > + done case-insensitively and trailing colon is optional. If option > + is given multiple times only last one is used. > + ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer > + lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows > + trailer lines with key `Reviewed-by`. The last "Examples" item does not logically belong to the other three, which bothers me a bit. > diff --git a/pretty.c b/pretty.c > index aa03d5b23..aaedc8447 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1074,6 +1074,17 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate, > return 0; > } > > +struct format_trailer_match_data { > + const char *trailer; > + size_t trailer_len; > +}; > + > +static int format_trailer_match_cb(const struct strbuf *sb, void *ud) > +{ > + struct format_trailer_match_data *data = ud; > + return data->trailer_len == sb->len && !strncasecmp (data->trailer, sb->buf, sb->len); > +} > if (skip_prefix(placeholder, "(trailers", &arg)) { > struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; > + struct format_trailer_match_data filter_data; > size_t ret = 0; > > opts.no_divider = 1; > @@ -1323,7 +1335,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > opts.only_trailers = 1; > else if (match_placeholder_arg(arg, "unfold", &arg)) > opts.unfold = 1; > - else > + else if (skip_prefix(arg, "key=", &arg)) { > + const char *end = arg + strcspn(arg, ",)"); > + > + filter_data.trailer = arg; > + filter_data.trailer_len = end - arg; > + > + if (filter_data.trailer_len && > + filter_data.trailer[filter_data.trailer_len - 1] == ':') > + filter_data.trailer_len--; > + > + opts.filter = format_trailer_match_cb; > + opts.filter_data = &filter_data; > + opts.only_trailers = 1; > + > + arg = end; > + if (*arg == ',') > + arg++; > + } else > break; > } This is part of a loop that is entered after seeing "%(trailers:" and existing one looks for 'unfold' and 'only' by using the match_placeholder_arg() helper, which - returns false if the keyword is not what is being sought after; - returns 1 if it finds the keyword, followed by ',' or ')', and updates the end pointer to point at either the closing ')' or one place after the ','. The new part cannot directly reuse the same helper because it expects some non-constant string after "key=", but shouldn't we be introducing a similar helper with extended feature to help this part of the code to stay readable? Exposing the minute details of the logic to parse "key=<value>,..." while hiding the counterpart to parse "(only|unfold),..." makes the implementation of the above loop uneven and hard to follow. I wonder if a helper like this would help: static int match_placeholder(const char *to_parse, const char *keyword, const char **value, size_t *valuelen, const char **end) { const char *p; if (!(skip_prefix(to_parse, keyword, &p))) return 0; if (value && valuelen) { /* expect "<keyword>=<value>" */ size_t vlen; if (*p++ != '=') return 1; vlen = strcspn(p, ",)"); if (!p[vlen]) return 0; *value = p; *valuelen = vlen; p = p + vlen; } if (*p == ',') { *end = p + 1; return 1; } if (*p == ')') { *end = p; return 1; } return 0; } which would allow the existing one to become a thin wrapper static int match_placeholder_arg(const char *a, const char *b, const char **c) { return match_placeholder(a, b, NULL, NULL, c); } or can simply be eliminated by updating its only two callsites. In the version you wrote, it is not clear what would happen if the format string ends with "%(trailers:key=" (no value, no comma, not even the closing paren). I do not think it would fall into infinite loop, but I do not think the code structure without the helper that makes the loop's logic uniform would allow it to properly diagnose such a malformed string. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v3 2/5] pretty: allow showing specific trailers 2018-11-18 11:44 ` [PATCH v3 2/5] pretty: allow showing specific trailers Anders Waldenborg 2018-11-20 5:45 ` Junio C Hamano @ 2018-11-20 5:59 ` Junio C Hamano 2018-11-25 23:02 ` Anders Waldenborg 1 sibling, 1 reply; 69+ messages in thread From: Junio C Hamano @ 2018-11-20 5:59 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King Anders Waldenborg <anders@0x63.nu> writes: > + followed by a colon and zero or more comma-separated options: > + ** 'only': omit non-trailer lines from the trailer block. > + ** 'unfold': make it behave as if interpret-trailer's `--unfold` > + option was given. > + ** 'key=<K>': only show trailers with specified key. Matching is > + done case-insensitively and trailing colon is optional. If option > + is given multiple times only last one is used. It would be good to allow multiple keys, as %(trailers:key=signed-off-by,key=helped-by) and %(trailers:key=signed-off-by)%(trailers:key=helped-by) would mean quite a different thing. The former can preserve the order of these sign-offs and helped-bys in the original, while the latter would have to show all the sign-offs before showing the helped-bys, and I am not convinced if the latter is the only valid use case. Also, use of 'key=' automatically turns on 'only' as described, and I tend to agree that it would a convenient default mode (i.e. when picking certain trailers only with this mechanism, it is likely that the user is willing to use %(subject) etc. to fill in what was lost by the implicit use of 'only'), but at the same time, it makes me wonder if we need to have a way to countermand an 'only' (or 'unfold' for that matter) that was given earlier, e.g. %(trailers:key=signed-off-by,only=no) Thanks. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v3 2/5] pretty: allow showing specific trailers 2018-11-20 5:59 ` Junio C Hamano @ 2018-11-25 23:02 ` Anders Waldenborg 2018-11-26 3:13 ` Junio C Hamano 0 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-11-25 23:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King Junio C Hamano writes: > Also, use of 'key=' automatically turns on 'only' as described, and > I tend to agree that it would a convenient default mode (i.e. when > picking certain trailers only with this mechanism, it is likely that > the user is willing to use %(subject) etc. to fill in what was lost > by the implicit use of 'only'), but at the same time, it makes me > wonder if we need to have a way to countermand an 'only' (or > 'unfold' for that matter) that was given earlier, e.g. > > %(trailers:key=signed-off-by,only=no) > > Thanks. I'm not sure what that would mean. The non-trailer lines in the trailer block doesn't match the key. Take this commit as an example: $ git show -s --pretty=format:'%(trailers)' b4d065df03049bacfbc40467b60b13e804b7d289 Helped-by: Jeff King <peff@peff.net> [jc: took idea and log message from peff] Signed-off-by: Junio C Hamano <gitster@pobox.com> With 'only' it shows: $ git show -s --pretty=format:'%(trailers:only)' b4d065df03049bacfbc40467b60b13e804b7d289 Helped-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> Now with a "key=signed-off-by" option I would imagine that as adding a "| grep -i '^signed-off-by:'" to the end. In both cases (with and without 'only') that would give the same result: "Signed-off-by: Junio C Hamano <gitster@pobox.com>" anders ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v3 2/5] pretty: allow showing specific trailers 2018-11-25 23:02 ` Anders Waldenborg @ 2018-11-26 3:13 ` Junio C Hamano 2018-11-26 6:56 ` Anders Waldenborg 0 siblings, 1 reply; 69+ messages in thread From: Junio C Hamano @ 2018-11-26 3:13 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King Anders Waldenborg <anders@0x63.nu> writes: > Junio C Hamano writes: >> Also, use of 'key=' automatically turns on 'only' as described, and >> I tend to agree that it would a convenient default mode (i.e. when >> picking certain trailers only with this mechanism, it is likely that >> the user is willing to use %(subject) etc. to fill in what was lost >> by the implicit use of 'only'), but at the same time, it makes me >> wonder if we need to have a way to countermand an 'only' (or >> 'unfold' for that matter) that was given earlier, e.g. >> >> %(trailers:key=signed-off-by,only=no) >> >> Thanks. > > I'm not sure what that would mean. The non-trailer lines in the trailer > block doesn't match the key. I was confused by the "only" stuff. When you give a key (or two), they cannot possibly name non-trailer lines, so while it may be possible to ask "oh, by the way, I also want non-trailer lines in addition to signed-off-by and cc lines", the value of being able to make such a request is dubious. The value is dubious, but logically it makes it more consistent with the current %(trailers) that lack 'only', which is "oh by the way, I also want non-trailer lines in addition to the trailers with keyword", to allow a way to countermand the 'only' you flip on by default when keywords are given. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v3 2/5] pretty: allow showing specific trailers 2018-11-26 3:13 ` Junio C Hamano @ 2018-11-26 6:56 ` Anders Waldenborg 2018-11-26 7:52 ` Junio C Hamano 0 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-11-26 6:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King Junio C Hamano writes: > I was confused by the "only" stuff. > > When you give a key (or two), they cannot possibly name non-trailer > lines, so while it may be possible to ask "oh, by the way, I also > want non-trailer lines in addition to signed-off-by and cc lines", > the value of being able to make such a request is dubious. > > The value is dubious, but logically it makes it more consistent with > the current %(trailers) that lack 'only', which is "oh by the way, I > also want non-trailer lines in addition to the trailers with > keyword", to allow a way to countermand the 'only' you flip on by > default when keywords are given. Would it feel less inconsistent if it did not set the 'only_trailers' option? Now that I look at it again setting 'only_trailers' is more of an implementation trick/hack to make sure it doesn't take the fast-path in format_trailer_info (and by documenting it it justifies that hack). If instead the 'filter' option is checked in the relevant places there would be no need to mix up 'only' with 'filter'. That is, do you think something like this should be squashed in? diff --git a/pretty.c b/pretty.c index 302e67fa8..f45ccaf51 100644 --- a/pretty.c +++ b/pretty.c @@ -1360,7 +1360,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ opts.filter = format_trailer_match_cb; opts.filter_data = &filter_list; - opts.only_trailers = 1; } else break; } diff --git a/trailer.c b/trailer.c index 97c8f2762..07ca2b2c6 100644 --- a/trailer.c +++ b/trailer.c @@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out, size_t i; /* If we want the whole block untouched, we can take the fast path. */ - if (!opts->only_trailers && !opts->unfold) { + if (!opts->only_trailers && !opts->unfold && !opts->filter) { strbuf_add(out, info->trailer_start, info->trailer_end - info->trailer_start); return; @@ -1156,7 +1156,7 @@ static void format_trailer_info(struct strbuf *out, strbuf_release(&tok); strbuf_release(&val); - } else if (!opts->only_trailers) { + } else if (!opts->only_trailers && !opts->filter) { strbuf_addstr(out, trailer); } } diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 7548e1d38..ea3cd5b28 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -228,9 +228,9 @@ endif::git-rev-list[] ** 'key=<K>': only show trailers with specified key. Matching is done case-insensitively and trailing colon is optional. If option is given multiple times trailer lines matching any of the keys are - shown. Non-trailer lines in the trailer block are also hidden - (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)` - shows trailer lines with key `Reviewed-by`. + shown. Non-trailer lines in the trailer block are also hidden. + E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key + `Reviewed-by`. ** 'only': omit non-trailer lines from the trailer block. ** 'unfold': make it behave as if interpret-trailer's `--unfold` option was given. E.g., `%(trailers:only,unfold)` unfolds and ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v3 2/5] pretty: allow showing specific trailers 2018-11-26 6:56 ` Anders Waldenborg @ 2018-11-26 7:52 ` Junio C Hamano 0 siblings, 0 replies; 69+ messages in thread From: Junio C Hamano @ 2018-11-26 7:52 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King Anders Waldenborg <anders@0x63.nu> writes: > Would it feel less inconsistent if it did not set the 'only_trailers' > option? If %(trailers:key=...) did not automatically imply 'only', it would be very consistent. But as I already said, I think it would be less convenient, as I do suspect that those who want specific keys would want to see only those trailers with specific keys. And if we want that convinience, we'd probably want a way to optionally disable that 'only' bit when the user wants to. And... > --- a/Documentation/pretty-formats.txt > +++ b/Documentation/pretty-formats.txt > @@ -228,9 +228,9 @@ endif::git-rev-list[] > ** 'key=<K>': only show trailers with specified key. Matching is done > case-insensitively and trailing colon is optional. If option is > given multiple times trailer lines matching any of the keys are > - shown. Non-trailer lines in the trailer block are also hidden > - (i.e. 'key' implies 'only'). E.g., `%(trailers:key=Reviewed-by)` > - shows trailer lines with key `Reviewed-by`. > + shown. Non-trailer lines in the trailer block are also hidden. > + E.g., `%(trailers:key=Reviewed-by)` shows trailer lines with key > + `Reviewed-by`. ... I do not think this change reduces the puzzlement felt by readers who would have wondered how that implied 'only' can be countermanded with the old text. It just makes it look even less explained to them. If we assume that nobody would ever want to mix non-trailers when asking specific keywords, then "them" in the above paragraph would become an empty set, and we do not have to worry about them. I am not sure if Git is still such a small project to allow us rely on such an assumption, though. > ** 'only': omit non-trailer lines from the trailer block. > ** 'unfold': make it behave as if interpret-trailer's `--unfold` > option was given. E.g., `%(trailers:only,unfold)` unfolds and ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 3/5] pretty: add support for "valueonly" option in %(trailers) 2018-11-18 11:44 ` [PATCH v3 " Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 1/5] pretty: single return path in %(trailers) handling Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 2/5] pretty: allow showing specific trailers Anders Waldenborg @ 2018-11-18 11:44 ` Anders Waldenborg 2018-11-20 8:14 ` Eric Sunshine 2018-11-18 11:44 ` [PATCH v3 4/5] strbuf: separate callback for strbuf_expand:ing literals Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 5/5] pretty: add support for separator option in %(trailers) Anders Waldenborg 4 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-11-18 11:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg With the new "key=" option to %(trailers) it often makes little sense to show the key, as it by definition already is know which trailer is printed there. This new "valueonly" option makes it omit the key when printing trailers. E.g.: $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,valueonly)' aaaa88182 will show: > upload-pack: fix broken if/else chain in config callback > Jeff King <peff@peff.net> > Junio C Hamano <gitster@pobox.com> Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 2 ++ pretty.c | 2 ++ t/t4205-log-pretty-formats.sh | 6 ++++++ trailer.c | 6 ++++-- trailer.h | 1 + 5 files changed, 15 insertions(+), 2 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 75c2e838d..8cc8c3f9f 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -216,6 +216,8 @@ endif::git-rev-list[] ** 'key=<K>': only show trailers with specified key. Matching is done case-insensitively and trailing colon is optional. If option is given multiple times only last one is used. + ** 'valueonly': skip over the key part of the trailer and only show + the its value part. ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer lines with key `Reviewed-by`. diff --git a/pretty.c b/pretty.c index aaedc8447..2e99f2418 100644 --- a/pretty.c +++ b/pretty.c @@ -1335,6 +1335,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ opts.only_trailers = 1; else if (match_placeholder_arg(arg, "unfold", &arg)) opts.unfold = 1; + else if (match_placeholder_arg(arg, "valueonly", &arg)) + opts.value_only = 1; else if (skip_prefix(arg, "key=", &arg)) { const char *end = arg + strcspn(arg, ",)"); diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index aba7ba206..095208d6b 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -634,6 +634,12 @@ test_expect_success '%(trailers:key=foo,unfold) properly unfolds' ' test_cmp expect actual ' +test_expect_success '%(trailers:key=foo,valueonly) shows only value' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual && + echo "A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index 97c8f2762..662c7ff03 100644 --- a/trailer.c +++ b/trailer.c @@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out, if (!opts->filter || opts->filter(&tok, opts->filter_data)) { if (opts->unfold) unfold_value(&val); - - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + if (!opts->value_only) + strbuf_addf(out, "%s: ", tok.buf); + strbuf_addbuf(out, &val); + strbuf_addch(out, '\n'); } strbuf_release(&tok); strbuf_release(&val); diff --git a/trailer.h b/trailer.h index 5255b676d..06d417fe9 100644 --- a/trailer.h +++ b/trailer.h @@ -72,6 +72,7 @@ struct process_trailer_options { int only_input; int unfold; int no_divider; + int value_only; int (*filter)(const struct strbuf *, void *); void *filter_data; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v3 3/5] pretty: add support for "valueonly" option in %(trailers) 2018-11-18 11:44 ` [PATCH v3 3/5] pretty: add support for "valueonly" option in %(trailers) Anders Waldenborg @ 2018-11-20 8:14 ` Eric Sunshine 0 siblings, 0 replies; 69+ messages in thread From: Eric Sunshine @ 2018-11-20 8:14 UTC (permalink / raw) To: anders; +Cc: Git List, Junio C Hamano, Jeff King On Sun, Nov 18, 2018 at 6:45 AM Anders Waldenborg <anders@0x63.nu> wrote: > With the new "key=" option to %(trailers) it often makes little sense to > show the key, as it by definition already is know which trailer is s/know/known/ > printed there. This new "valueonly" option makes it omit the key when > printing trailers. > > Signed-off-by: Anders Waldenborg <anders@0x63.nu> ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v3 4/5] strbuf: separate callback for strbuf_expand:ing literals 2018-11-18 11:44 ` [PATCH v3 " Anders Waldenborg ` (2 preceding siblings ...) 2018-11-18 11:44 ` [PATCH v3 3/5] pretty: add support for "valueonly" option in %(trailers) Anders Waldenborg @ 2018-11-18 11:44 ` Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 5/5] pretty: add support for separator option in %(trailers) Anders Waldenborg 4 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-11-18 11:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg Expanding '%n' and '%xNN' is generic functionality, so extract that from the pretty.c formatter into a callback that can be reused. No functional change intended Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- pretty.c | 16 +++++----------- strbuf.c | 21 +++++++++++++++++++++ strbuf.h | 8 ++++++++ 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/pretty.c b/pretty.c index 2e99f2418..819c5c50a 100644 --- a/pretty.c +++ b/pretty.c @@ -1094,9 +1094,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *msg = c->message; struct commit_list *p; const char *arg; - int ch; + size_t res; /* these are independent of the commit */ + res = strbuf_expand_literal_cb(sb, placeholder, NULL); + if (res) + return res; + switch (placeholder[0]) { case 'C': if (starts_with(placeholder + 1, "(auto)")) { @@ -1115,16 +1119,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ */ return ret; } - case 'n': /* newline */ - strbuf_addch(sb, '\n'); - return 1; - case 'x': - /* %x00 == NUL, %x0a == LF, etc. */ - ch = hex2chr(placeholder + 1); - if (ch < 0) - return 0; - strbuf_addch(sb, ch); - return 3; case 'w': if (placeholder[1] == '(') { unsigned long width = 0, indent1 = 0, indent2 = 0; diff --git a/strbuf.c b/strbuf.c index f6a6cf78b..78eecd29f 100644 --- a/strbuf.c +++ b/strbuf.c @@ -380,6 +380,27 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, } } +size_t strbuf_expand_literal_cb(struct strbuf *sb, + const char *placeholder, + void *context) +{ + int ch; + + switch (placeholder[0]) { + case 'n': /* newline */ + strbuf_addch(sb, '\n'); + return 1; + case 'x': + /* %x00 == NUL, %x0a == LF, etc. */ + ch = hex2chr(placeholder + 1); + if (ch < 0) + return 0; + strbuf_addch(sb, ch); + return 3; + } + return 0; +} + size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context) { diff --git a/strbuf.h b/strbuf.h index fc40873b6..52e44c9ab 100644 --- a/strbuf.h +++ b/strbuf.h @@ -320,6 +320,14 @@ void strbuf_expand(struct strbuf *sb, expand_fn_t fn, void *context); +/** + * Used as callback for `strbuf_expand` to only expand literals + * (i.e. %n and %xNN). The context argument is ignored. + */ +size_t strbuf_expand_literal_cb(struct strbuf *sb, + const char *placeholder, + void *context); + /** * Used as callback for `strbuf_expand()`, expects an array of * struct strbuf_expand_dict_entry as context, i.e. pairs of -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v3 5/5] pretty: add support for separator option in %(trailers) 2018-11-18 11:44 ` [PATCH v3 " Anders Waldenborg ` (3 preceding siblings ...) 2018-11-18 11:44 ` [PATCH v3 4/5] strbuf: separate callback for strbuf_expand:ing literals Anders Waldenborg @ 2018-11-18 11:44 ` Anders Waldenborg 2018-11-20 8:25 ` Eric Sunshine 4 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-11-18 11:44 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg By default trailer lines are terminated by linebreaks ('\n'). By specifying the new 'separator' option they will instead be separated by user provided string and have separator semantics rather than terminator semantics. The separator string can contain the literal formatting codes %n and %xNN allowing it to be things that are otherwise hard to type as %x00, or comma and end-parenthesis which would break parsing. E.g: $ git log --pretty='%(trailers:key=Reviewed-by,valueonly,separator=%x00)' Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 13 +++++++++--- pretty.c | 15 +++++++++++++ t/t4205-log-pretty-formats.sh | 36 ++++++++++++++++++++++++++++++++ trailer.c | 15 +++++++++++-- trailer.h | 1 + 5 files changed, 75 insertions(+), 5 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 8cc8c3f9f..30e238338 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -218,9 +218,16 @@ endif::git-rev-list[] is given multiple times only last one is used. ** 'valueonly': skip over the key part of the trailer and only show the its value part. - ** Examples: `%(trailers:only,unfold)` unfolds and shows all trailer - lines, `%(trailers:key=Reviewed-by,unfold)` unfolds and shows - trailer lines with key `Reviewed-by`. + ** 'separator=<SEP>': specifying an alternative separator than the + default line feed character. SEP may can contain the literal + formatting codes %n and %xNN allowing it to contain characters + that are hard to type such as %x00, or comma and end-parenthesis + which would break parsing. If option is given multiple times only + the last one is used. + ** Examples: `%(trailers:only,unfold,separator=%x00)` unfolds and + shows all trailer lines separated by NUL character, + `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer + lines with key `Reviewed-by`. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index 819c5c50a..5b22a7237 100644 --- a/pretty.c +++ b/pretty.c @@ -1318,6 +1318,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; struct format_trailer_match_data filter_data; + struct strbuf sepbuf = STRBUF_INIT; size_t ret = 0; opts.no_divider = 1; @@ -1348,6 +1349,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ arg = end; if (*arg == ',') arg++; + } else if (skip_prefix(arg, "separator=", &arg)) { + size_t seplen = strcspn(arg, ",)"); + char *fmt; + + strbuf_reset(&sepbuf); + fmt = xstrndup(arg, seplen); + strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL); + free(fmt); + opts.separator = &sepbuf; + + arg += seplen; + if (*arg == ',') + arg++; } else break; } @@ -1356,6 +1370,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ format_trailers_from_commit(sb, msg + c->subject_off, &opts); ret = arg - placeholder + 1; } + strbuf_release(&sepbuf); return ret; } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 095208d6b..562b56dda 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -640,6 +640,42 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' ' test_cmp expect actual ' +test_expect_success 'pretty format %(trailers:separator) changes separator' ' + git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual && + printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' ' + git commit --allow-empty -F - <<-\EOF && + Important fix + + The fix is explained here + + Closes: #1234 + EOF + + git commit --allow-empty -F - <<-\EOF && + Another fix + + The fix is explained here + + Closes: #567 + Closes: #890 + EOF + + git commit --allow-empty -F - <<-\EOF && + Does not close any tickets + EOF + + git log --pretty="%s% (trailers:separator=%x2c%x20,key=Closes,valueonly)" HEAD~3.. >actual && + test_write_lines \ + "Does not close any tickets" \ + "Another fix #567, #890" \ + "Important fix #1234" >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index 662c7ff03..85cd2e52e 100644 --- a/trailer.c +++ b/trailer.c @@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out, const struct trailer_info *info, const struct process_trailer_options *opts) { + size_t origlen = out->len; size_t i; /* If we want the whole block untouched, we can take the fast path. */ - if (!opts->only_trailers && !opts->unfold) { + if (!opts->only_trailers && !opts->unfold && !opts->separator) { strbuf_add(out, info->trailer_start, info->trailer_end - info->trailer_start); return; @@ -1150,16 +1151,26 @@ static void format_trailer_info(struct strbuf *out, if (!opts->filter || opts->filter(&tok, opts->filter_data)) { if (opts->unfold) unfold_value(&val); + + if (opts->separator && out->len != origlen) + strbuf_addbuf(out, opts->separator); if (!opts->value_only) strbuf_addf(out, "%s: ", tok.buf); strbuf_addbuf(out, &val); - strbuf_addch(out, '\n'); + if (!opts->separator) + strbuf_addch(out, '\n'); } strbuf_release(&tok); strbuf_release(&val); } else if (!opts->only_trailers) { + if (opts->separator && out->len != origlen) { + strbuf_addbuf(out, opts->separator); + } strbuf_addstr(out, trailer); + if (opts->separator) { + strbuf_rtrim(out); + } } } diff --git a/trailer.h b/trailer.h index 06d417fe9..203acf4ee 100644 --- a/trailer.h +++ b/trailer.h @@ -73,6 +73,7 @@ struct process_trailer_options { int unfold; int no_divider; int value_only; + const struct strbuf *separator; int (*filter)(const struct strbuf *, void *); void *filter_data; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v3 5/5] pretty: add support for separator option in %(trailers) 2018-11-18 11:44 ` [PATCH v3 5/5] pretty: add support for separator option in %(trailers) Anders Waldenborg @ 2018-11-20 8:25 ` Eric Sunshine 0 siblings, 0 replies; 69+ messages in thread From: Eric Sunshine @ 2018-11-20 8:25 UTC (permalink / raw) To: anders; +Cc: Git List, Junio C Hamano, Jeff King On Sun, Nov 18, 2018 at 6:45 AM Anders Waldenborg <anders@0x63.nu> wrote: > By default trailer lines are terminated by linebreaks ('\n'). By > specifying the new 'separator' option they will instead be separated by > user provided string and have separator semantics rather than terminator > semantics. The separator string can contain the literal formatting codes > %n and %xNN allowing it to be things that are otherwise hard to type as > %x00, or comma and end-parenthesis which would break parsing. > > Signed-off-by: Anders Waldenborg <anders@0x63.nu> > --- > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt > @@ -218,9 +218,16 @@ endif::git-rev-list[] > + ** 'separator=<SEP>': specifying an alternative separator than the > + default line feed character. SEP may can contain the literal > + formatting codes %n and %xNN allowing it to contain characters > + that are hard to type such as %x00, or comma and end-parenthesis > + which would break parsing. If option is given multiple times only > + the last one is used. It's not clear from this documentation what constitutes a valid <SEP>. Is it restricted to a single character? Can it be an arbitrary string? If a string, does it need to be quoted? Does it support backslash escaping? Although I was able to guess that %xNN allowed hex input of a 7- or 8-bit value, I found myself wondering what I was supposed to replace 'n' with in "%n". I didn't fathom that "%n" was meant to be typed literally to specify a newline character. > + ** Examples: `%(trailers:only,unfold,separator=%x00)` unfolds and > + shows all trailer lines separated by NUL character, > + `%(trailers:key=Reviewed-by,unfold)` unfolds and shows trailer > + lines with key `Reviewed-by`. ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v4 0/7] %(trailers) improvements in pretty format 2018-10-28 12:50 [PATCH] pretty: Add %(trailer:X) to display single trailer Anders Waldenborg ` (2 preceding siblings ...) 2018-11-04 15:22 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Anders Waldenborg @ 2018-12-08 16:36 ` Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 1/7] doc: group pretty-format.txt placeholders descriptions Anders Waldenborg ` (6 more replies) 2019-01-28 21:33 ` [PATCH v5 0/7] %(trailers) improvements in pretty format Anders Waldenborg 4 siblings, 7 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-12-08 16:36 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg Updated since v3: * multiple 'key=' matches any * allow overriding implicit 'only' when using key * minor grammar and spelling fixes * documentation restructuring * Helper functions for parsing options Anders Waldenborg (7): doc: group pretty-format.txt placeholders descriptions pretty: allow %(trailers) options with explicit value pretty: single return path in %(trailers) handling pretty: allow showing specific trailers pretty: add support for "valueonly" option in %(trailers) strbuf: separate callback for strbuf_expand:ing literals pretty: add support for separator option in %(trailers) Documentation/pretty-formats.txt | 260 ++++++++++++++++++------------- pretty.c | 104 ++++++++++--- strbuf.c | 21 +++ strbuf.h | 8 + t/t4205-log-pretty-formats.sh | 111 +++++++++++++ trailer.c | 25 ++- trailer.h | 4 + 7 files changed, 400 insertions(+), 133 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v4 1/7] doc: group pretty-format.txt placeholders descriptions 2018-12-08 16:36 ` [PATCH v4 0/7] %(trailers) improvements in pretty format Anders Waldenborg @ 2018-12-08 16:36 ` Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value Anders Waldenborg ` (5 subsequent siblings) 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-12-08 16:36 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg The placeholders can be grouped into three kinds: * literals * affecting formatting of later placeholders * expanding to information in commit Also change the list to a definition list (using '::') Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 235 ++++++++++++++++--------------- 1 file changed, 125 insertions(+), 110 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 417b638cd8..86d804fe97 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -102,118 +102,133 @@ The title was >>t4119: test autocomputing -p<n> for traditional diff input.<< + The placeholders are: -- '%H': commit hash -- '%h': abbreviated commit hash -- '%T': tree hash -- '%t': abbreviated tree hash -- '%P': parent hashes -- '%p': abbreviated parent hashes -- '%an': author name -- '%aN': author name (respecting .mailmap, see linkgit:git-shortlog[1] - or linkgit:git-blame[1]) -- '%ae': author email -- '%aE': author email (respecting .mailmap, see - linkgit:git-shortlog[1] or linkgit:git-blame[1]) -- '%ad': author date (format respects --date= option) -- '%aD': author date, RFC2822 style -- '%ar': author date, relative -- '%at': author date, UNIX timestamp -- '%ai': author date, ISO 8601-like format -- '%aI': author date, strict ISO 8601 format -- '%cn': committer name -- '%cN': committer name (respecting .mailmap, see - linkgit:git-shortlog[1] or linkgit:git-blame[1]) -- '%ce': committer email -- '%cE': committer email (respecting .mailmap, see - linkgit:git-shortlog[1] or linkgit:git-blame[1]) -- '%cd': committer date (format respects --date= option) -- '%cD': committer date, RFC2822 style -- '%cr': committer date, relative -- '%ct': committer date, UNIX timestamp -- '%ci': committer date, ISO 8601-like format -- '%cI': committer date, strict ISO 8601 format -- '%d': ref names, like the --decorate option of linkgit:git-log[1] -- '%D': ref names without the " (", ")" wrapping. -- '%e': encoding -- '%s': subject -- '%f': sanitized subject line, suitable for a filename -- '%b': body -- '%B': raw body (unwrapped subject and body) +- Placeholders that expand to a single literal character: +'%n':: newline +'%%':: a raw '%' +'%x00':: print a byte from a hex code + +- Placeholders that affect formatting of later placeholders: +'%Cred':: switch color to red +'%Cgreen':: switch color to green +'%Cblue':: switch color to blue +'%Creset':: reset color +'%C(...)':: color specification, as described under Values in the + "CONFIGURATION FILE" section of linkgit:git-config[1]. By + default, colors are shown only when enabled for log output + (by `color.diff`, `color.ui`, or `--color`, and respecting + the `auto` settings of the former if we are going to a + terminal). `%C(auto,...)` is accepted as a historical + synonym for the default (e.g., `%C(auto,red)`). Specifying + `%C(always,...) will show the colors even when color is + not otherwise enabled (though consider just using + `--color=always` to enable color for the whole output, + including this format and anything else git might color). + `auto` alone (i.e. `%C(auto)`) will turn on auto coloring + on the next placeholders until the color is switched + again. +'%m':: left (`<`), right (`>`) or boundary (`-`) mark +'%w([<w>[,<i1>[,<i2>]]])':: switch line wrapping, like the -w option of + linkgit:git-shortlog[1]. +'%<(<N>[,trunc|ltrunc|mtrunc])':: make the next placeholder take at + least N columns, padding spaces on + the right if necessary. Optionally + truncate at the beginning (ltrunc), + the middle (mtrunc) or the end + (trunc) if the output is longer than + N columns. Note that truncating + only works correctly with N >= 2. +'%<|(<N>)':: make the next placeholder take at least until Nth + columns, padding spaces on the right if necessary +'%>(<N>)', '%>|(<N>)':: similar to '%<(<N>)', '%<|(<N>)' respectively, + but padding spaces on the left +'%>>(<N>)', '%>>|(<N>)':: similar to '%>(<N>)', '%>|(<N>)' + respectively, except that if the next + placeholder takes more spaces than given and + there are spaces on its left, use those + spaces +'%><(<N>)', '%><|(<N>)':: similar to '%<(<N>)', '%<|(<N>)' + respectively, but padding both sides + (i.e. the text is centered) + +- Placeholders that expand to information extracted from the commit: +'%H':: commit hash +'%h':: abbreviated commit hash +'%T':: tree hash +'%t':: abbreviated tree hash +'%P':: parent hashes +'%p':: abbreviated parent hashes +'%an':: author name +'%aN':: author name (respecting .mailmap, see linkgit:git-shortlog[1] + or linkgit:git-blame[1]) +'%ae':: author email +'%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1] + or linkgit:git-blame[1]) +'%ad':: author date (format respects --date= option) +'%aD':: author date, RFC2822 style +'%ar':: author date, relative +'%at':: author date, UNIX timestamp +'%ai':: author date, ISO 8601-like format +'%aI':: author date, strict ISO 8601 format +'%cn':: committer name +'%cN':: committer name (respecting .mailmap, see + linkgit:git-shortlog[1] or linkgit:git-blame[1]) +'%ce':: committer email +'%cE':: committer email (respecting .mailmap, see + linkgit:git-shortlog[1] or linkgit:git-blame[1]) +'%cd':: committer date (format respects --date= option) +'%cD':: committer date, RFC2822 style +'%cr':: committer date, relative +'%ct':: committer date, UNIX timestamp +'%ci':: committer date, ISO 8601-like format +'%cI':: committer date, strict ISO 8601 format +'%d':: ref names, like the --decorate option of linkgit:git-log[1] +'%D':: ref names without the " (", ")" wrapping. +'%e':: encoding +'%s':: subject +'%f':: sanitized subject line, suitable for a filename +'%b':: body +'%B':: raw body (unwrapped subject and body) ifndef::git-rev-list[] -- '%N': commit notes +'%N':: commit notes endif::git-rev-list[] -- '%GG': raw verification message from GPG for a signed commit -- '%G?': show "G" for a good (valid) signature, - "B" for a bad signature, - "U" for a good signature with unknown validity, - "X" for a good signature that has expired, - "Y" for a good signature made by an expired key, - "R" for a good signature made by a revoked key, - "E" if the signature cannot be checked (e.g. missing key) - and "N" for no signature -- '%GS': show the name of the signer for a signed commit -- '%GK': show the key used to sign a signed commit -- '%GF': show the fingerprint of the key used to sign a signed commit -- '%GP': show the fingerprint of the primary key whose subkey was used - to sign a signed commit -- '%gD': reflog selector, e.g., `refs/stash@{1}` or - `refs/stash@{2 minutes ago`}; the format follows the rules described - for the `-g` option. The portion before the `@` is the refname as - given on the command line (so `git log -g refs/heads/master` would - yield `refs/heads/master@{0}`). -- '%gd': shortened reflog selector; same as `%gD`, but the refname - portion is shortened for human readability (so `refs/heads/master` - becomes just `master`). -- '%gn': reflog identity name -- '%gN': reflog identity name (respecting .mailmap, see - linkgit:git-shortlog[1] or linkgit:git-blame[1]) -- '%ge': reflog identity email -- '%gE': reflog identity email (respecting .mailmap, see - linkgit:git-shortlog[1] or linkgit:git-blame[1]) -- '%gs': reflog subject -- '%Cred': switch color to red -- '%Cgreen': switch color to green -- '%Cblue': switch color to blue -- '%Creset': reset color -- '%C(...)': color specification, as described under Values in the - "CONFIGURATION FILE" section of linkgit:git-config[1]. - By default, colors are shown only when enabled for log output (by - `color.diff`, `color.ui`, or `--color`, and respecting the `auto` - settings of the former if we are going to a terminal). `%C(auto,...)` - is accepted as a historical synonym for the default (e.g., - `%C(auto,red)`). Specifying `%C(always,...) will show the colors - even when color is not otherwise enabled (though consider - just using `--color=always` to enable color for the whole output, - including this format and anything else git might color). `auto` - alone (i.e. `%C(auto)`) will turn on auto coloring on the next - placeholders until the color is switched again. -- '%m': left (`<`), right (`>`) or boundary (`-`) mark -- '%n': newline -- '%%': a raw '%' -- '%x00': print a byte from a hex code -- '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of - linkgit:git-shortlog[1]. -- '%<(<N>[,trunc|ltrunc|mtrunc])': make the next placeholder take at - least N columns, padding spaces on the right if necessary. - Optionally truncate at the beginning (ltrunc), the middle (mtrunc) - or the end (trunc) if the output is longer than N columns. - Note that truncating only works correctly with N >= 2. -- '%<|(<N>)': make the next placeholder take at least until Nth - columns, padding spaces on the right if necessary -- '%>(<N>)', '%>|(<N>)': similar to '%<(<N>)', '%<|(<N>)' - respectively, but padding spaces on the left -- '%>>(<N>)', '%>>|(<N>)': similar to '%>(<N>)', '%>|(<N>)' - respectively, except that if the next placeholder takes more spaces - than given and there are spaces on its left, use those spaces -- '%><(<N>)', '%><|(<N>)': similar to '%<(<N>)', '%<|(<N>)' - respectively, but padding both sides (i.e. the text is centered) -- %(trailers[:options]): display the trailers of the body as interpreted - by linkgit:git-interpret-trailers[1]. The `trailers` string may be - followed by a colon and zero or more comma-separated options. If the - `only` option is given, omit non-trailer lines from the trailer block. - If the `unfold` option is given, behave as if interpret-trailer's - `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do - both. +'%GG':: raw verification message from GPG for a signed commit +'%G?':: show "G" for a good (valid) signature, + "B" for a bad signature, + "U" for a good signature with unknown validity, + "X" for a good signature that has expired, + "Y" for a good signature made by an expired key, + "R" for a good signature made by a revoked key, + "E" if the signature cannot be checked (e.g. missing key) + and "N" for no signature +'%GS':: show the name of the signer for a signed commit +'%GK':: show the key used to sign a signed commit +'%GF':: show the fingerprint of the key used to sign a signed commit +'%GP':: show the fingerprint of the primary key whose subkey was used + to sign a signed commit +'%gD':: reflog selector, e.g., `refs/stash@{1}` or `refs/stash@{2 + minutes ago`}; the format follows the rules described for the + `-g` option. The portion before the `@` is the refname as + given on the command line (so `git log -g refs/heads/master` + would yield `refs/heads/master@{0}`). +'%gd':: shortened reflog selector; same as `%gD`, but the refname + portion is shortened for human readability (so + `refs/heads/master` becomes just `master`). +'%gn':: reflog identity name +'%gN':: reflog identity name (respecting .mailmap, see + linkgit:git-shortlog[1] or linkgit:git-blame[1]) +'%ge':: reflog identity email +'%gE':: reflog identity email (respecting .mailmap, see + linkgit:git-shortlog[1] or linkgit:git-blame[1]) +'%gs':: reflog subject +'%(trailers[:options])':: display the trailers of the body as + interpreted by + linkgit:git-interpret-trailers[1]. The + `trailers` string may be followed by a colon + and zero or more comma-separated options: +** 'only': omit non-trailer lines from the trailer block. +** 'unfold': make it behave as if interpret-trailer's `--unfold` + option was given. E.g., `%(trailers:only,unfold)` unfolds and + shows all trailer lines. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value 2018-12-08 16:36 ` [PATCH v4 0/7] %(trailers) improvements in pretty format Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 1/7] doc: group pretty-format.txt placeholders descriptions Anders Waldenborg @ 2018-12-08 16:36 ` Anders Waldenborg 2018-12-10 8:45 ` Junio C Hamano 2018-12-08 16:36 ` [PATCH v4 3/7] pretty: single return path in %(trailers) handling Anders Waldenborg ` (4 subsequent siblings) 6 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-12-08 16:36 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg In addition to old %(trailers:only) it is now allowed to write %(trailers:only=yes) By itself this only gives (the not quite so useful) possibility to have users change their mind in the middle of a formatting string (%(trailers:only=true,only=false)). However, it gives users the opportunity to override defaults from future options. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 14 ++++++++++---- pretty.c | 32 +++++++++++++++++++++++++++----- t/t4205-log-pretty-formats.sh | 18 ++++++++++++++++++ 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 86d804fe97..d33b072eb2 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -225,10 +225,16 @@ endif::git-rev-list[] linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by a colon and zero or more comma-separated options: -** 'only': omit non-trailer lines from the trailer block. -** 'unfold': make it behave as if interpret-trailer's `--unfold` - option was given. E.g., `%(trailers:only,unfold)` unfolds and - shows all trailer lines. +** 'only[=val]': select whether non-trailer lines from the trailer + block should be included. The `only` keyword may optionally be + followed by an equal sign and one of `true`, `on`, `yes` to omit or + `false`, `off`, `no` to show the non-trailer lines. If option is + given without value it is enabled. If given multiple times the last + value is used. +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold` + option was given. In same way as to for `only` it can be followed + by an equal sign and explicit value. E.g., + `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index b83a3ecd23..26efdba73a 100644 --- a/pretty.c +++ b/pretty.c @@ -1074,6 +1074,31 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate, return 0; } +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, + const char **end, int *val) +{ + const char *p; + if (!skip_prefix(to_parse, candidate, &p)) + return 0; + + if (match_placeholder_arg(p, "=no", end) || + match_placeholder_arg(p, "=off", end) || + match_placeholder_arg(p, "=false", end)) { + *val = 0; + return 1; + } + + if (match_placeholder_arg(p, "", end) || + match_placeholder_arg(p, "=yes", end) || + match_placeholder_arg(p, "=on", end) || + match_placeholder_arg(p, "=true", end)) { + *val = 1; + return 1; + } + + return 0; +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1318,11 +1343,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (*arg == ':') { arg++; for (;;) { - if (match_placeholder_arg(arg, "only", &arg)) - opts.only_trailers = 1; - else if (match_placeholder_arg(arg, "unfold", &arg)) - opts.unfold = 1; - else + if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && + !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) break; } } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 978a8a66ff..63730a4ec0 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only "key: value" trailers' ' test_cmp expect actual ' +test_expect_success '%(trailers:only=yes) shows only "key: value" trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual && + grep -v patch.description <trailers >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only=no) shows all trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=no)" >actual && + cat trailers >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual && + grep -v patch.description <trailers >expect && + test_cmp expect actual +' + test_expect_success '%(trailers:unfold) unfolds trailers' ' git log --no-walk --pretty="%(trailers:unfold)" >actual && { -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value 2018-12-08 16:36 ` [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value Anders Waldenborg @ 2018-12-10 8:45 ` Junio C Hamano 2018-12-18 21:30 ` Anders Waldenborg 0 siblings, 1 reply; 69+ messages in thread From: Junio C Hamano @ 2018-12-10 8:45 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King, Olga Telezhnaya Anders Waldenborg <anders@0x63.nu> writes: > In addition to old %(trailers:only) it is now allowed to write > %(trailers:only=yes) s/$/. Similarly the unfold option can take a boolean./ > By itself this only gives (the not quite so useful) possibility to have > users change their mind in the middle of a formatting > string (%(trailers:only=true,only=false)). However, it gives users the > opportunity to override defaults from future options. Makes sense. > +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold` > + option was given. In same way as to for `only` it can be followed > + by an equal sign and explicit value. E.g., > + `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. > +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, > + const char **end, int *val) > +{ > + const char *p; > + if (!skip_prefix(to_parse, candidate, &p)) > + return 0; > + > + if (match_placeholder_arg(p, "=no", end) || > + match_placeholder_arg(p, "=off", end) || > + match_placeholder_arg(p, "=false", end)) { > + *val = 0; > + return 1; > + } > + > + if (match_placeholder_arg(p, "", end) || > + match_placeholder_arg(p, "=yes", end) || > + match_placeholder_arg(p, "=on", end) || > + match_placeholder_arg(p, "=true", end)) { > + *val = 1; > + return 1; > + } Hmph. Is there a possibility to arrenge the code so that we do not have to maintain these variants of true/false representations here, when we should already have one in config.c? The match_placeholder_arg() function is a bit too limiting as it can only recognize the value that we know about for a thing like this. Instead, perhaps we can cut what follows "=" syntactically, looking for either NUL, ',', or ')', and then call git_parse_maybe_bool() on it. That way, we can handle %(trailers:only=bogo) more sensibly, no? Syntactically we can recognize that the user wanted to give 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if we did so. > + return 0; > +} > + ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value 2018-12-10 8:45 ` Junio C Hamano @ 2018-12-18 21:30 ` Anders Waldenborg 2019-01-29 16:55 ` Jeff King 0 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-12-18 21:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Olga Telezhnaya Junio C Hamano writes: > That way, we can handle %(trailers:only=bogo) more sensibly, > no? Syntactically we can recognize that the user wanted to give > 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if > we did so. I agree that proper error reporting for the pretty formatting strings would be great. But that would depart from the current extremely crude error handling where incorrect formatting placeholders are just left unexpanded. How would such change in error handling be done safely, wrt backwards compatibility changes? To get good diagnostics for incorrect formatting strings I think the way forward is to have the formatting strings parsed once into some kind of AST or machine (as also mentioned by Jeff) that is just executed many times, instead of parsed each time like today. anders ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value 2018-12-18 21:30 ` Anders Waldenborg @ 2019-01-29 16:55 ` Jeff King 2019-01-29 21:23 ` Anders Waldenborg 0 siblings, 1 reply; 69+ messages in thread From: Jeff King @ 2019-01-29 16:55 UTC (permalink / raw) To: Anders Waldenborg; +Cc: Junio C Hamano, git, Olga Telezhnaya On Tue, Dec 18, 2018 at 10:30:04PM +0100, Anders Waldenborg wrote: > > Junio C Hamano writes: > > That way, we can handle %(trailers:only=bogo) more sensibly, > > no? Syntactically we can recognize that the user wanted to give > > 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if > > we did so. > > I agree that proper error reporting for the pretty formatting strings > would be great. But that would depart from the current extremely crude > error handling where incorrect formatting placeholders are just left > unexpanded. How would such change in error handling be done safely, wrt > backwards compatibility changes? I think we'd want to move in the direction of enforcing valid expressions for %(foo) placeholders. There's some small value in leaving %X alone if we do not understand "X" (not to mention the backwards %compatibility you mentioned), but I think %() is a pretty deliberate indication that a placeholder was meant there. We already do this for ref-filter expansions: $ git for-each-ref --format='%(foo)' fatal: unknown field name: foo We don't for "--pretty" formats, but I do wonder if anybody would be really mad (after all, we have declared ourselves free to add new placeholders, so such formats are not future-proof). -Peff ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value 2019-01-29 16:55 ` Jeff King @ 2019-01-29 21:23 ` Anders Waldenborg [not found] ` <CAL21Bmmx=EO+R2t+KviNekDhU3fc0wjCcmUmbzLa14bb0PAmHA@mail.gmail.com> 0 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2019-01-29 21:23 UTC (permalink / raw) To: Jeff King; +Cc: Anders Waldenborg, Junio C Hamano, git, Olga Telezhnaya Jeff King writes: > There's some small value in leaving > %X alone if we do not understand "X" (not to mention the backwards > %compatibility you mentioned), but I think %() is a pretty > deliberate indication that a placeholder was meant there. Good point. > We already do this for ref-filter expansions: > > $ git for-each-ref --format='%(foo)' > fatal: unknown field name: foo > > We don't for "--pretty" formats, but I do wonder if anybody would be > really mad (after all, we have declared ourselves free to add new > placeholders, so such formats are not future-proof). Oh my. I wasn't aware that there was a totally separate string interpolation implementation used for ref filters. That one has separated parsing, making it more amenable to good error handling. I wonder if that could be generalized and reused for pretty formats. However I doubt I will have time to dig deeper into that in near time. ^ permalink raw reply [flat|nested] 69+ messages in thread
[parent not found: <CAL21Bmmx=EO+R2t+KviNekDhU3fc0wjCcmUmbzLa14bb0PAmHA@mail.gmail.com>]
* Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value [not found] ` <CAL21Bmmx=EO+R2t+KviNekDhU3fc0wjCcmUmbzLa14bb0PAmHA@mail.gmail.com> @ 2019-01-31 18:46 ` Anders Waldenborg 2019-02-02 9:14 ` Оля Тележная 0 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2019-01-31 18:46 UTC (permalink / raw) To: Оля Тележная Cc: Jeff King, Junio C Hamano, git Оля Тележная writes: >> Oh my. I wasn't aware that there was a totally separate string >> interpolation implementation used for ref filters. That one has >> separated parsing, making it more amenable to good error handling. >> I wonder if that could be generalized and reused for pretty formats. >> >> However I doubt I will have time to dig deeper into that in near time. >> > > Sorry, I haven't read your patch in details. If you will be at Git Merge > tomorrow, you could ask me any questions, I can explain how for-each-ref > formatting works amd maybe even give you some ideas how to use its logic in > pretty, I was thinking about it a bit. No, unfortunately I'm not at Git Merge. I think I got how the formatting work. But if you have ideas on how to reuse the logic I'm all ears. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value 2019-01-31 18:46 ` Anders Waldenborg @ 2019-02-02 9:14 ` Оля Тележная 0 siblings, 0 replies; 69+ messages in thread From: Оля Тележная @ 2019-02-02 9:14 UTC (permalink / raw) To: Anders Waldenborg; +Cc: Jeff King, Junio C Hamano, git чт, 31 янв. 2019 г. в 21:47, Anders Waldenborg <anders@0x63.nu>: > > > Оля Тележная writes: > >> Oh my. I wasn't aware that there was a totally separate string > >> interpolation implementation used for ref filters. That one has > >> separated parsing, making it more amenable to good error handling. > >> I wonder if that could be generalized and reused for pretty formats. > >> > >> However I doubt I will have time to dig deeper into that in near time. > >> > > > > Sorry, I haven't read your patch in details. If you will be at Git Merge > > tomorrow, you could ask me any questions, I can explain how for-each-ref > > formatting works amd maybe even give you some ideas how to use its logic in > > pretty, I was thinking about it a bit. > > No, unfortunately I'm not at Git Merge. > > I think I got how the formatting work. But if you have ideas on how to > reuse the logic I'm all ears. Maybe my advices will be not suitable: I know how ref-filter works, but I know only a few about pretty system. The main point is that we need to save backward compatibility, and that means that we can't just drop pretty logic and start using ref-filter one there. I guess it's not so hard to make like translation table between pretty commands and ref-filter one. For example, 'short' in pretty means 'commit %(objectname)%0aAuthor: %(author)' in ref-filter. So if I wanted to change pretty logic, I would add support of all ref-filter commands and make translation table for backward compatibility. Hope it was helpful. > ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v4 3/7] pretty: single return path in %(trailers) handling 2018-12-08 16:36 ` [PATCH v4 0/7] %(trailers) improvements in pretty format Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 1/7] doc: group pretty-format.txt placeholders descriptions Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value Anders Waldenborg @ 2018-12-08 16:36 ` Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 4/7] pretty: allow showing specific trailers Anders Waldenborg ` (3 subsequent siblings) 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-12-08 16:36 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg No functional change intended. This change may not seem useful on its own, but upcoming commits will do memory allocation in there, and a single return path makes deallocation easier. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- pretty.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pretty.c b/pretty.c index 26efdba73a..044447e6c0 100644 --- a/pretty.c +++ b/pretty.c @@ -1337,6 +1337,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; + size_t ret = 0; opts.no_divider = 1; @@ -1350,8 +1351,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); - return arg - placeholder + 1; + ret = arg - placeholder + 1; } + return ret; } return 0; /* unknown placeholder */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v4 4/7] pretty: allow showing specific trailers 2018-12-08 16:36 ` [PATCH v4 0/7] %(trailers) improvements in pretty format Anders Waldenborg ` (2 preceding siblings ...) 2018-12-08 16:36 ` [PATCH v4 3/7] pretty: single return path in %(trailers) handling Anders Waldenborg @ 2018-12-08 16:36 ` Anders Waldenborg 2018-12-10 8:56 ` Junio C Hamano 2018-12-08 16:36 ` [PATCH v4 5/7] pretty: add support for "valueonly" option in %(trailers) Anders Waldenborg ` (2 subsequent siblings) 6 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2018-12-08 16:36 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg Adds a new "key=X" option to "%(trailers)" which will cause it to only print trailer lines which match any of the specified keys. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 8 +++++ pretty.c | 47 ++++++++++++++++++++++++++--- t/t4205-log-pretty-formats.sh | 51 ++++++++++++++++++++++++++++++++ trailer.c | 10 ++++--- trailer.h | 2 ++ 5 files changed, 110 insertions(+), 8 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index d33b072eb2..d6add831c0 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -225,6 +225,14 @@ endif::git-rev-list[] linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by a colon and zero or more comma-separated options: +** 'key=<K>': only show trailers with specified key. Matching is done + case-insensitively and trailing colon is optional. If option is + given multiple times trailer lines matching any of the keys are + shown. This option automatically enables the `only` option so that + non-trailer lines in the trailer block are hidden. If that is not + desired it can be disabled with `only=false`. E.g., + `%(trailers:key=Reviewed-by)` shows trailer lines with key + `Reviewed-by`. ** 'only[=val]': select whether non-trailer lines from the trailer block should be included. The `only` keyword may optionally be followed by an equal sign and one of `true`, `on`, `yes` to omit or diff --git a/pretty.c b/pretty.c index 044447e6c0..541a553ccc 100644 --- a/pretty.c +++ b/pretty.c @@ -1056,13 +1056,20 @@ static size_t parse_padding_placeholder(struct strbuf *sb, return 0; } -static int match_placeholder_arg(const char *to_parse, const char *candidate, - const char **end) +static int match_placeholder_arg_value(const char *to_parse, const char *candidate, + const char **end, const char **valuestart, size_t *valuelen) { const char *p; if (!(skip_prefix(to_parse, candidate, &p))) return 0; + if (valuestart) { + if (*p != '=') + return 0; + *valuestart = p + 1; + *valuelen = strcspn(*valuestart, ",)"); + p = *valuestart + *valuelen; + } if (*p == ',') { *end = p + 1; return 1; @@ -1074,6 +1081,12 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate, return 0; } +static int match_placeholder_arg(const char *to_parse, const char *candidate, + const char **end) +{ + return match_placeholder_arg_value(to_parse, candidate, end, NULL, NULL); +} + static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, const char **end, int *val) { @@ -1095,7 +1108,19 @@ static int match_placeholder_bool_arg(const char *to_parse, const char *candidat *val = 1; return 1; } + return 0; +} +static int format_trailer_match_cb(const struct strbuf *key, void *ud) +{ + const struct string_list *list = ud; + const struct string_list_item *item; + + for_each_string_list_item (item, list) { + if (key->len == (uintptr_t)item->util && + !strncasecmp (item->string, key->buf, key->len)) + return 1; + } return 0; } @@ -1337,6 +1362,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; + struct string_list filter_list = STRING_LIST_INIT_NODUP; size_t ret = 0; opts.no_divider = 1; @@ -1344,8 +1370,20 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (*arg == ':') { arg++; for (;;) { - if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && - !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) + const char *argval; + size_t arglen; + + if (match_placeholder_arg_value(arg, "key", &arg, &argval, &arglen)) { + uintptr_t len = arglen; + if (len && argval[len - 1] == ':') + len--; + string_list_append(&filter_list, argval)->util = (char *)len; + + opts.filter = format_trailer_match_cb; + opts.filter_data = &filter_list; + opts.only_trailers = 1; + } else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && + !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) break; } } @@ -1353,6 +1391,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ format_trailers_from_commit(sb, msg + c->subject_off, &opts); ret = arg - placeholder + 1; } + string_list_clear (&filter_list, 0); return ret; } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 63730a4ec0..54239290cf 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -616,6 +616,57 @@ test_expect_success ':only and :unfold work together' ' test_cmp expect actual ' +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by)" >actual && + echo "Acked-by: A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' ' + git log --no-walk --pretty="format:%(trailers:key=AcKed-bY)" >actual && + echo "Acked-by: A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo:) trailing colon also works' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by:)" >actual && + echo "Acked-by: A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo) multiple keys' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by:,key=Signed-off-By)" >actual && + grep -v patch.description <trailers >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=nonexistant) becomes empty' ' + git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual && + echo "xx" >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' ' + git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by)" >actual && + grep -v patch.description <trailers | grep -v Acked-by >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=foo,unfold) properly unfolds' ' + git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by,unfold)" >actual && + unfold <trailers | grep Signed-off-by >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by,only=no)" >actual && + { + echo "Acked-by: A U Thor <author@example.com>" && + grep patch.description <trailers + } >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index 0796f326b3..d6da555cd7 100644 --- a/trailer.c +++ b/trailer.c @@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out, size_t i; /* If we want the whole block untouched, we can take the fast path. */ - if (!opts->only_trailers && !opts->unfold) { + if (!opts->only_trailers && !opts->unfold && !opts->filter) { strbuf_add(out, info->trailer_start, info->trailer_end - info->trailer_start); return; @@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out, struct strbuf val = STRBUF_INIT; parse_trailer(&tok, &val, NULL, trailer, separator_pos); - if (opts->unfold) - unfold_value(&val); + if (!opts->filter || opts->filter(&tok, opts->filter_data)) { + if (opts->unfold) + unfold_value(&val); - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + } strbuf_release(&tok); strbuf_release(&val); diff --git a/trailer.h b/trailer.h index b997739649..5255b676de 100644 --- a/trailer.h +++ b/trailer.h @@ -72,6 +72,8 @@ struct process_trailer_options { int only_input; int unfold; int no_divider; + int (*filter)(const struct strbuf *, void *); + void *filter_data; }; #define PROCESS_TRAILER_OPTIONS_INIT {0} -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v4 4/7] pretty: allow showing specific trailers 2018-12-08 16:36 ` [PATCH v4 4/7] pretty: allow showing specific trailers Anders Waldenborg @ 2018-12-10 8:56 ` Junio C Hamano 0 siblings, 0 replies; 69+ messages in thread From: Junio C Hamano @ 2018-12-10 8:56 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King, Olga Telezhnaya Anders Waldenborg <anders@0x63.nu> writes: > -static int match_placeholder_arg(const char *to_parse, const char *candidate, > - const char **end) > +static int match_placeholder_arg_value(const char *to_parse, const char *candidate, > + const char **end, const char **valuestart, size_t *valuelen) > { > const char *p; > > if (!(skip_prefix(to_parse, candidate, &p))) > return 0; > + if (valuestart) { > + if (*p != '=') > + return 0; > + *valuestart = p + 1; > + *valuelen = strcspn(*valuestart, ",)"); > + p = *valuestart + *valuelen; > + } > if (*p == ',') { > *end = p + 1; > return 1; > @@ -1074,6 +1081,12 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate, > return 0; > } > > +static int match_placeholder_arg(const char *to_parse, const char *candidate, > + const char **end) > +{ > + return match_placeholder_arg_value(to_parse, candidate, end, NULL, NULL); > +} > + OK. The unified parsing of boolean value I mentioned on an earlier step can naturally be done using martch_placeholder_arg_value(), I think, in match_placeholder_bool_arg(). > +static int format_trailer_match_cb(const struct strbuf *key, void *ud) > +{ > + const struct string_list *list = ud; > + const struct string_list_item *item; > + > + for_each_string_list_item (item, list) { > + if (key->len == (uintptr_t)item->util && > + !strncasecmp (item->string, key->buf, key->len)) Remove SP after strncasecmp. We won't have too many elements in this string list, so O(N*M) search like this one would be OK. > + return 1; > + } > return 0; > } > > @@ -1337,6 +1362,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > > if (skip_prefix(placeholder, "(trailers", &arg)) { > struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; > + struct string_list filter_list = STRING_LIST_INIT_NODUP; > size_t ret = 0; > > opts.no_divider = 1; > @@ -1344,8 +1370,20 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > if (*arg == ':') { > arg++; > for (;;) { > - if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && > - !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) > + const char *argval; > + size_t arglen; > + > + if (match_placeholder_arg_value(arg, "key", &arg, &argval, &arglen)) { > + uintptr_t len = arglen; > + if (len && argval[len - 1] == ':') > + len--; > + string_list_append(&filter_list, argval)->util = (char *)len; > + > + opts.filter = format_trailer_match_cb; > + opts.filter_data = &filter_list; > + opts.only_trailers = 1; > + } else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && > + !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) > break; > } > } > @@ -1353,6 +1391,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > format_trailers_from_commit(sb, msg + c->subject_off, &opts); > ret = arg - placeholder + 1; > } > + string_list_clear (&filter_list, 0); Remove SP after string_list_clear. > return ret; > } > ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v4 5/7] pretty: add support for "valueonly" option in %(trailers) 2018-12-08 16:36 ` [PATCH v4 0/7] %(trailers) improvements in pretty format Anders Waldenborg ` (3 preceding siblings ...) 2018-12-08 16:36 ` [PATCH v4 4/7] pretty: allow showing specific trailers Anders Waldenborg @ 2018-12-08 16:36 ` Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 6/7] strbuf: separate callback for strbuf_expand:ing literals Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 7/7] pretty: add support for separator option in %(trailers) Anders Waldenborg 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-12-08 16:36 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg With the new "key=" option to %(trailers) it often makes little sense to show the key, as it by definition already is knows which trailer is printed there. This new "valueonly" option makes it omit the key when printing trailers. E.g.: $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,valueonly)' aaaa88182 will show: > upload-pack: fix broken if/else chain in config callback > Jeff King <peff@peff.net> > Junio C Hamano <gitster@pobox.com> Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 2 ++ pretty.c | 3 ++- t/t4205-log-pretty-formats.sh | 6 ++++++ trailer.c | 6 ++++-- trailer.h | 1 + 5 files changed, 15 insertions(+), 3 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index d6add831c0..a920dd15b1 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -243,6 +243,8 @@ endif::git-rev-list[] option was given. In same way as to for `only` it can be followed by an equal sign and explicit value. E.g., `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. +** 'valueonly[=val]': skip over the key part of the trailer line and only + show the value part. Also this optionally allows explicit value. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index 541a553ccc..c508357606 100644 --- a/pretty.c +++ b/pretty.c @@ -1383,7 +1383,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ opts.filter_data = &filter_list; opts.only_trailers = 1; } else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && - !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) + !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) && + !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only)) break; } } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 54239290cf..22336c5485 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -667,6 +667,12 @@ test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes non test_cmp expect actual ' +test_expect_success '%(trailers:key=foo,valueonly) shows only value' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual && + echo "A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index d6da555cd7..d0d9e91631 100644 --- a/trailer.c +++ b/trailer.c @@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out, if (!opts->filter || opts->filter(&tok, opts->filter_data)) { if (opts->unfold) unfold_value(&val); - - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + if (!opts->value_only) + strbuf_addf(out, "%s: ", tok.buf); + strbuf_addbuf(out, &val); + strbuf_addch(out, '\n'); } strbuf_release(&tok); strbuf_release(&val); diff --git a/trailer.h b/trailer.h index 5255b676de..06d417fe93 100644 --- a/trailer.h +++ b/trailer.h @@ -72,6 +72,7 @@ struct process_trailer_options { int only_input; int unfold; int no_divider; + int value_only; int (*filter)(const struct strbuf *, void *); void *filter_data; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v4 6/7] strbuf: separate callback for strbuf_expand:ing literals 2018-12-08 16:36 ` [PATCH v4 0/7] %(trailers) improvements in pretty format Anders Waldenborg ` (4 preceding siblings ...) 2018-12-08 16:36 ` [PATCH v4 5/7] pretty: add support for "valueonly" option in %(trailers) Anders Waldenborg @ 2018-12-08 16:36 ` Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 7/7] pretty: add support for separator option in %(trailers) Anders Waldenborg 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-12-08 16:36 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg Expanding '%n' and '%xNN' is generic functionality, so extract that from the pretty.c formatter into a callback that can be reused. No functional change intended Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- pretty.c | 16 +++++----------- strbuf.c | 21 +++++++++++++++++++++ strbuf.h | 8 ++++++++ 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/pretty.c b/pretty.c index c508357606..50d0b5830d 100644 --- a/pretty.c +++ b/pretty.c @@ -1133,9 +1133,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *msg = c->message; struct commit_list *p; const char *arg; - int ch; + size_t res; /* these are independent of the commit */ + res = strbuf_expand_literal_cb(sb, placeholder, NULL); + if (res) + return res; + switch (placeholder[0]) { case 'C': if (starts_with(placeholder + 1, "(auto)")) { @@ -1154,16 +1158,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ */ return ret; } - case 'n': /* newline */ - strbuf_addch(sb, '\n'); - return 1; - case 'x': - /* %x00 == NUL, %x0a == LF, etc. */ - ch = hex2chr(placeholder + 1); - if (ch < 0) - return 0; - strbuf_addch(sb, ch); - return 3; case 'w': if (placeholder[1] == '(') { unsigned long width = 0, indent1 = 0, indent2 = 0; diff --git a/strbuf.c b/strbuf.c index f6a6cf78b9..78eecd29f7 100644 --- a/strbuf.c +++ b/strbuf.c @@ -380,6 +380,27 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, } } +size_t strbuf_expand_literal_cb(struct strbuf *sb, + const char *placeholder, + void *context) +{ + int ch; + + switch (placeholder[0]) { + case 'n': /* newline */ + strbuf_addch(sb, '\n'); + return 1; + case 'x': + /* %x00 == NUL, %x0a == LF, etc. */ + ch = hex2chr(placeholder + 1); + if (ch < 0) + return 0; + strbuf_addch(sb, ch); + return 3; + } + return 0; +} + size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context) { diff --git a/strbuf.h b/strbuf.h index fc40873b65..52e44c9ab8 100644 --- a/strbuf.h +++ b/strbuf.h @@ -320,6 +320,14 @@ void strbuf_expand(struct strbuf *sb, expand_fn_t fn, void *context); +/** + * Used as callback for `strbuf_expand` to only expand literals + * (i.e. %n and %xNN). The context argument is ignored. + */ +size_t strbuf_expand_literal_cb(struct strbuf *sb, + const char *placeholder, + void *context); + /** * Used as callback for `strbuf_expand()`, expects an array of * struct strbuf_expand_dict_entry as context, i.e. pairs of -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v4 7/7] pretty: add support for separator option in %(trailers) 2018-12-08 16:36 ` [PATCH v4 0/7] %(trailers) improvements in pretty format Anders Waldenborg ` (5 preceding siblings ...) 2018-12-08 16:36 ` [PATCH v4 6/7] strbuf: separate callback for strbuf_expand:ing literals Anders Waldenborg @ 2018-12-08 16:36 ` Anders Waldenborg 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2018-12-08 16:36 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Olga Telezhnaya, Anders Waldenborg By default trailer lines are terminated by linebreaks ('\n'). By specifying the new 'separator' option they will instead be separated by user provided string and have separator semantics rather than terminator semantics. The separator string can contain the literal formatting codes %n and %xNN allowing it to be things that are otherwise hard to type such as %x00, or comma and end-parenthesis which would break parsing. E.g: $ git log --pretty='%(trailers:key=Reviewed-by,valueonly,separator=%x00)' Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 9 ++++++++ pretty.c | 10 +++++++++ t/t4205-log-pretty-formats.sh | 36 ++++++++++++++++++++++++++++++++ trailer.c | 15 +++++++++++-- trailer.h | 1 + 5 files changed, 69 insertions(+), 2 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index a920dd15b1..ce087dee80 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -239,6 +239,15 @@ endif::git-rev-list[] `false`, `off`, `no` to show the non-trailer lines. If option is given without value it is enabled. If given multiple times the last value is used. +** 'separator=<SEP>': specify a separator inserted between trailer + lines. When this option is not given each trailer line is + terminated with a line feed character. The string SEP may contain + the literal formatting codes described above. To use comma as + separator one must use `%x2C` as it would otherwise be parsed as + next option. If separator option is given multiple times only the + last one is used. E.g., `%(trailers:key=Ticket,separator=%x2C )` + shows all trailer lines whose key is "Ticket" separated by a comma + and a space. ** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold` option was given. In same way as to for `only` it can be followed by an equal sign and explicit value. E.g., diff --git a/pretty.c b/pretty.c index 50d0b5830d..c7609493ee 100644 --- a/pretty.c +++ b/pretty.c @@ -1357,6 +1357,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; struct string_list filter_list = STRING_LIST_INIT_NODUP; + struct strbuf sepbuf = STRBUF_INIT; size_t ret = 0; opts.no_divider = 1; @@ -1376,6 +1377,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ opts.filter = format_trailer_match_cb; opts.filter_data = &filter_list; opts.only_trailers = 1; + } else if (match_placeholder_arg_value(arg, "separator", &arg, &argval, &arglen)) { + char *fmt; + + strbuf_reset(&sepbuf); + fmt = xstrndup(argval, arglen); + strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL); + free(fmt); + opts.separator = &sepbuf; } else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) && !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only)) @@ -1387,6 +1396,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ ret = arg - placeholder + 1; } string_list_clear (&filter_list, 0); + strbuf_release(&sepbuf); return ret; } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 22336c5485..282369dac0 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -673,6 +673,42 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' ' test_cmp expect actual ' +test_expect_success 'pretty format %(trailers:separator) changes separator' ' + git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual && + printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' ' + git commit --allow-empty -F - <<-\EOF && + Important fix + + The fix is explained here + + Closes: #1234 + EOF + + git commit --allow-empty -F - <<-\EOF && + Another fix + + The fix is explained here + + Closes: #567 + Closes: #890 + EOF + + git commit --allow-empty -F - <<-\EOF && + Does not close any tickets + EOF + + git log --pretty="%s% (trailers:separator=%x2c%x20,key=Closes,valueonly)" HEAD~3.. >actual && + test_write_lines \ + "Does not close any tickets" \ + "Another fix #567, #890" \ + "Important fix #1234" >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index d0d9e91631..0c414f2fed 100644 --- a/trailer.c +++ b/trailer.c @@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out, const struct trailer_info *info, const struct process_trailer_options *opts) { + size_t origlen = out->len; size_t i; /* If we want the whole block untouched, we can take the fast path. */ - if (!opts->only_trailers && !opts->unfold && !opts->filter) { + if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) { strbuf_add(out, info->trailer_start, info->trailer_end - info->trailer_start); return; @@ -1150,16 +1151,26 @@ static void format_trailer_info(struct strbuf *out, if (!opts->filter || opts->filter(&tok, opts->filter_data)) { if (opts->unfold) unfold_value(&val); + + if (opts->separator && out->len != origlen) + strbuf_addbuf(out, opts->separator); if (!opts->value_only) strbuf_addf(out, "%s: ", tok.buf); strbuf_addbuf(out, &val); - strbuf_addch(out, '\n'); + if (!opts->separator) + strbuf_addch(out, '\n'); } strbuf_release(&tok); strbuf_release(&val); } else if (!opts->only_trailers) { + if (opts->separator && out->len != origlen) { + strbuf_addbuf(out, opts->separator); + } strbuf_addstr(out, trailer); + if (opts->separator) { + strbuf_rtrim(out); + } } } diff --git a/trailer.h b/trailer.h index 06d417fe93..203acf4ee1 100644 --- a/trailer.h +++ b/trailer.h @@ -73,6 +73,7 @@ struct process_trailer_options { int unfold; int no_divider; int value_only; + const struct strbuf *separator; int (*filter)(const struct strbuf *, void *); void *filter_data; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v5 0/7] %(trailers) improvements in pretty format 2018-10-28 12:50 [PATCH] pretty: Add %(trailer:X) to display single trailer Anders Waldenborg ` (3 preceding siblings ...) 2018-12-08 16:36 ` [PATCH v4 0/7] %(trailers) improvements in pretty format Anders Waldenborg @ 2019-01-28 21:33 ` Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 1/7] doc: group pretty-format.txt placeholders descriptions Anders Waldenborg ` (6 more replies) 4 siblings, 7 replies; 69+ messages in thread From: Anders Waldenborg @ 2019-01-28 21:33 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg Updates since v4: * Coding style fixes * Reuse git_parse_maybe_bool for bool parsing Anders Waldenborg (7): doc: group pretty-format.txt placeholders descriptions pretty: Allow %(trailers) options with explicit value pretty: single return path in %(trailers) handling pretty: allow showing specific trailers pretty: add support for "valueonly" option in %(trailers) strbuf: separate callback for strbuf_expand:ing literals pretty: add support for separator option in %(trailers) Documentation/pretty-formats.txt | 260 ++++++++++++++++++------------- pretty.c | 113 +++++++++++--- strbuf.c | 21 +++ strbuf.h | 8 + t/t4205-log-pretty-formats.sh | 117 ++++++++++++++ trailer.c | 25 ++- trailer.h | 4 + 7 files changed, 415 insertions(+), 133 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 69+ messages in thread
* [PATCH v5 1/7] doc: group pretty-format.txt placeholders descriptions 2019-01-28 21:33 ` [PATCH v5 0/7] %(trailers) improvements in pretty format Anders Waldenborg @ 2019-01-28 21:33 ` Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value Anders Waldenborg ` (5 subsequent siblings) 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2019-01-28 21:33 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg The placeholders can be grouped into three kinds: * literals * affecting formatting of later placeholders * expanding to information in commit Also change the list to a definition list (using '::') Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 235 ++++++++++++++++--------------- 1 file changed, 125 insertions(+), 110 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 417b638cd8..86d804fe97 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -102,118 +102,133 @@ The title was >>t4119: test autocomputing -p<n> for traditional diff input.<< + The placeholders are: -- '%H': commit hash -- '%h': abbreviated commit hash -- '%T': tree hash -- '%t': abbreviated tree hash -- '%P': parent hashes -- '%p': abbreviated parent hashes -- '%an': author name -- '%aN': author name (respecting .mailmap, see linkgit:git-shortlog[1] - or linkgit:git-blame[1]) -- '%ae': author email -- '%aE': author email (respecting .mailmap, see - linkgit:git-shortlog[1] or linkgit:git-blame[1]) -- '%ad': author date (format respects --date= option) -- '%aD': author date, RFC2822 style -- '%ar': author date, relative -- '%at': author date, UNIX timestamp -- '%ai': author date, ISO 8601-like format -- '%aI': author date, strict ISO 8601 format -- '%cn': committer name -- '%cN': committer name (respecting .mailmap, see - linkgit:git-shortlog[1] or linkgit:git-blame[1]) -- '%ce': committer email -- '%cE': committer email (respecting .mailmap, see - linkgit:git-shortlog[1] or linkgit:git-blame[1]) -- '%cd': committer date (format respects --date= option) -- '%cD': committer date, RFC2822 style -- '%cr': committer date, relative -- '%ct': committer date, UNIX timestamp -- '%ci': committer date, ISO 8601-like format -- '%cI': committer date, strict ISO 8601 format -- '%d': ref names, like the --decorate option of linkgit:git-log[1] -- '%D': ref names without the " (", ")" wrapping. -- '%e': encoding -- '%s': subject -- '%f': sanitized subject line, suitable for a filename -- '%b': body -- '%B': raw body (unwrapped subject and body) +- Placeholders that expand to a single literal character: +'%n':: newline +'%%':: a raw '%' +'%x00':: print a byte from a hex code + +- Placeholders that affect formatting of later placeholders: +'%Cred':: switch color to red +'%Cgreen':: switch color to green +'%Cblue':: switch color to blue +'%Creset':: reset color +'%C(...)':: color specification, as described under Values in the + "CONFIGURATION FILE" section of linkgit:git-config[1]. By + default, colors are shown only when enabled for log output + (by `color.diff`, `color.ui`, or `--color`, and respecting + the `auto` settings of the former if we are going to a + terminal). `%C(auto,...)` is accepted as a historical + synonym for the default (e.g., `%C(auto,red)`). Specifying + `%C(always,...) will show the colors even when color is + not otherwise enabled (though consider just using + `--color=always` to enable color for the whole output, + including this format and anything else git might color). + `auto` alone (i.e. `%C(auto)`) will turn on auto coloring + on the next placeholders until the color is switched + again. +'%m':: left (`<`), right (`>`) or boundary (`-`) mark +'%w([<w>[,<i1>[,<i2>]]])':: switch line wrapping, like the -w option of + linkgit:git-shortlog[1]. +'%<(<N>[,trunc|ltrunc|mtrunc])':: make the next placeholder take at + least N columns, padding spaces on + the right if necessary. Optionally + truncate at the beginning (ltrunc), + the middle (mtrunc) or the end + (trunc) if the output is longer than + N columns. Note that truncating + only works correctly with N >= 2. +'%<|(<N>)':: make the next placeholder take at least until Nth + columns, padding spaces on the right if necessary +'%>(<N>)', '%>|(<N>)':: similar to '%<(<N>)', '%<|(<N>)' respectively, + but padding spaces on the left +'%>>(<N>)', '%>>|(<N>)':: similar to '%>(<N>)', '%>|(<N>)' + respectively, except that if the next + placeholder takes more spaces than given and + there are spaces on its left, use those + spaces +'%><(<N>)', '%><|(<N>)':: similar to '%<(<N>)', '%<|(<N>)' + respectively, but padding both sides + (i.e. the text is centered) + +- Placeholders that expand to information extracted from the commit: +'%H':: commit hash +'%h':: abbreviated commit hash +'%T':: tree hash +'%t':: abbreviated tree hash +'%P':: parent hashes +'%p':: abbreviated parent hashes +'%an':: author name +'%aN':: author name (respecting .mailmap, see linkgit:git-shortlog[1] + or linkgit:git-blame[1]) +'%ae':: author email +'%aE':: author email (respecting .mailmap, see linkgit:git-shortlog[1] + or linkgit:git-blame[1]) +'%ad':: author date (format respects --date= option) +'%aD':: author date, RFC2822 style +'%ar':: author date, relative +'%at':: author date, UNIX timestamp +'%ai':: author date, ISO 8601-like format +'%aI':: author date, strict ISO 8601 format +'%cn':: committer name +'%cN':: committer name (respecting .mailmap, see + linkgit:git-shortlog[1] or linkgit:git-blame[1]) +'%ce':: committer email +'%cE':: committer email (respecting .mailmap, see + linkgit:git-shortlog[1] or linkgit:git-blame[1]) +'%cd':: committer date (format respects --date= option) +'%cD':: committer date, RFC2822 style +'%cr':: committer date, relative +'%ct':: committer date, UNIX timestamp +'%ci':: committer date, ISO 8601-like format +'%cI':: committer date, strict ISO 8601 format +'%d':: ref names, like the --decorate option of linkgit:git-log[1] +'%D':: ref names without the " (", ")" wrapping. +'%e':: encoding +'%s':: subject +'%f':: sanitized subject line, suitable for a filename +'%b':: body +'%B':: raw body (unwrapped subject and body) ifndef::git-rev-list[] -- '%N': commit notes +'%N':: commit notes endif::git-rev-list[] -- '%GG': raw verification message from GPG for a signed commit -- '%G?': show "G" for a good (valid) signature, - "B" for a bad signature, - "U" for a good signature with unknown validity, - "X" for a good signature that has expired, - "Y" for a good signature made by an expired key, - "R" for a good signature made by a revoked key, - "E" if the signature cannot be checked (e.g. missing key) - and "N" for no signature -- '%GS': show the name of the signer for a signed commit -- '%GK': show the key used to sign a signed commit -- '%GF': show the fingerprint of the key used to sign a signed commit -- '%GP': show the fingerprint of the primary key whose subkey was used - to sign a signed commit -- '%gD': reflog selector, e.g., `refs/stash@{1}` or - `refs/stash@{2 minutes ago`}; the format follows the rules described - for the `-g` option. The portion before the `@` is the refname as - given on the command line (so `git log -g refs/heads/master` would - yield `refs/heads/master@{0}`). -- '%gd': shortened reflog selector; same as `%gD`, but the refname - portion is shortened for human readability (so `refs/heads/master` - becomes just `master`). -- '%gn': reflog identity name -- '%gN': reflog identity name (respecting .mailmap, see - linkgit:git-shortlog[1] or linkgit:git-blame[1]) -- '%ge': reflog identity email -- '%gE': reflog identity email (respecting .mailmap, see - linkgit:git-shortlog[1] or linkgit:git-blame[1]) -- '%gs': reflog subject -- '%Cred': switch color to red -- '%Cgreen': switch color to green -- '%Cblue': switch color to blue -- '%Creset': reset color -- '%C(...)': color specification, as described under Values in the - "CONFIGURATION FILE" section of linkgit:git-config[1]. - By default, colors are shown only when enabled for log output (by - `color.diff`, `color.ui`, or `--color`, and respecting the `auto` - settings of the former if we are going to a terminal). `%C(auto,...)` - is accepted as a historical synonym for the default (e.g., - `%C(auto,red)`). Specifying `%C(always,...) will show the colors - even when color is not otherwise enabled (though consider - just using `--color=always` to enable color for the whole output, - including this format and anything else git might color). `auto` - alone (i.e. `%C(auto)`) will turn on auto coloring on the next - placeholders until the color is switched again. -- '%m': left (`<`), right (`>`) or boundary (`-`) mark -- '%n': newline -- '%%': a raw '%' -- '%x00': print a byte from a hex code -- '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of - linkgit:git-shortlog[1]. -- '%<(<N>[,trunc|ltrunc|mtrunc])': make the next placeholder take at - least N columns, padding spaces on the right if necessary. - Optionally truncate at the beginning (ltrunc), the middle (mtrunc) - or the end (trunc) if the output is longer than N columns. - Note that truncating only works correctly with N >= 2. -- '%<|(<N>)': make the next placeholder take at least until Nth - columns, padding spaces on the right if necessary -- '%>(<N>)', '%>|(<N>)': similar to '%<(<N>)', '%<|(<N>)' - respectively, but padding spaces on the left -- '%>>(<N>)', '%>>|(<N>)': similar to '%>(<N>)', '%>|(<N>)' - respectively, except that if the next placeholder takes more spaces - than given and there are spaces on its left, use those spaces -- '%><(<N>)', '%><|(<N>)': similar to '%<(<N>)', '%<|(<N>)' - respectively, but padding both sides (i.e. the text is centered) -- %(trailers[:options]): display the trailers of the body as interpreted - by linkgit:git-interpret-trailers[1]. The `trailers` string may be - followed by a colon and zero or more comma-separated options. If the - `only` option is given, omit non-trailer lines from the trailer block. - If the `unfold` option is given, behave as if interpret-trailer's - `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do - both. +'%GG':: raw verification message from GPG for a signed commit +'%G?':: show "G" for a good (valid) signature, + "B" for a bad signature, + "U" for a good signature with unknown validity, + "X" for a good signature that has expired, + "Y" for a good signature made by an expired key, + "R" for a good signature made by a revoked key, + "E" if the signature cannot be checked (e.g. missing key) + and "N" for no signature +'%GS':: show the name of the signer for a signed commit +'%GK':: show the key used to sign a signed commit +'%GF':: show the fingerprint of the key used to sign a signed commit +'%GP':: show the fingerprint of the primary key whose subkey was used + to sign a signed commit +'%gD':: reflog selector, e.g., `refs/stash@{1}` or `refs/stash@{2 + minutes ago`}; the format follows the rules described for the + `-g` option. The portion before the `@` is the refname as + given on the command line (so `git log -g refs/heads/master` + would yield `refs/heads/master@{0}`). +'%gd':: shortened reflog selector; same as `%gD`, but the refname + portion is shortened for human readability (so + `refs/heads/master` becomes just `master`). +'%gn':: reflog identity name +'%gN':: reflog identity name (respecting .mailmap, see + linkgit:git-shortlog[1] or linkgit:git-blame[1]) +'%ge':: reflog identity email +'%gE':: reflog identity email (respecting .mailmap, see + linkgit:git-shortlog[1] or linkgit:git-blame[1]) +'%gs':: reflog subject +'%(trailers[:options])':: display the trailers of the body as + interpreted by + linkgit:git-interpret-trailers[1]. The + `trailers` string may be followed by a colon + and zero or more comma-separated options: +** 'only': omit non-trailer lines from the trailer block. +** 'unfold': make it behave as if interpret-trailer's `--unfold` + option was given. E.g., `%(trailers:only,unfold)` unfolds and + shows all trailer lines. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value 2019-01-28 21:33 ` [PATCH v5 0/7] %(trailers) improvements in pretty format Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 1/7] doc: group pretty-format.txt placeholders descriptions Anders Waldenborg @ 2019-01-28 21:33 ` Anders Waldenborg 2019-01-28 22:38 ` Junio C Hamano 2019-01-28 21:33 ` [PATCH v5 3/7] pretty: single return path in %(trailers) handling Anders Waldenborg ` (4 subsequent siblings) 6 siblings, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2019-01-28 21:33 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg In addition to old %(trailers:only) it is now allowed to write %(trailers:only=yes) By itself this only gives (the not quite so useful) possibility to have users change their mind in the middle of a formatting string (%(trailers:only=true,only=false)). However, it gives users the opportunity to override defaults from future options. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 14 ++++++--- pretty.c | 52 +++++++++++++++++++++++++++----- t/t4205-log-pretty-formats.sh | 18 +++++++++++ 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 86d804fe97..d33b072eb2 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -225,10 +225,16 @@ endif::git-rev-list[] linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by a colon and zero or more comma-separated options: -** 'only': omit non-trailer lines from the trailer block. -** 'unfold': make it behave as if interpret-trailer's `--unfold` - option was given. E.g., `%(trailers:only,unfold)` unfolds and - shows all trailer lines. +** 'only[=val]': select whether non-trailer lines from the trailer + block should be included. The `only` keyword may optionally be + followed by an equal sign and one of `true`, `on`, `yes` to omit or + `false`, `off`, `no` to show the non-trailer lines. If option is + given without value it is enabled. If given multiple times the last + value is used. +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold` + option was given. In same way as to for `only` it can be followed + by an equal sign and explicit value. E.g., + `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index b83a3ecd23..b8d71a57c9 100644 --- a/pretty.c +++ b/pretty.c @@ -1056,13 +1056,25 @@ static size_t parse_padding_placeholder(struct strbuf *sb, return 0; } -static int match_placeholder_arg(const char *to_parse, const char *candidate, - const char **end) +static int match_placeholder_arg_value(const char *to_parse, const char *candidate, + const char **end, const char **valuestart, size_t *valuelen) { const char *p; if (!(skip_prefix(to_parse, candidate, &p))) return 0; + if (valuestart) { + if (*p == '=') { + *valuestart = p + 1; + *valuelen = strcspn(*valuestart, ",)"); + p = *valuestart + *valuelen; + } else { + if (*p != ',' && *p != ')') + return 0; + *valuestart = NULL; + *valuelen = 0; + } + } if (*p == ',') { *end = p + 1; return 1; @@ -1074,6 +1086,35 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate, return 0; } +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, + const char **end, int *val) +{ + char buf[8]; + const char *strval; + size_t len; + int v; + + if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len)) + return 0; + + if (!strval) { + *val = 1; + return 1; + } + + strlcpy(buf, strval, sizeof(buf)); + if (len < sizeof(buf)) + buf[len] = 0; + + v = git_parse_maybe_bool(buf); + if (v == -1) + return 0; + + *val = v; + + return 1; +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1318,11 +1359,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (*arg == ':') { arg++; for (;;) { - if (match_placeholder_arg(arg, "only", &arg)) - opts.only_trailers = 1; - else if (match_placeholder_arg(arg, "unfold", &arg)) - opts.unfold = 1; - else + if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && + !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) break; } } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 978a8a66ff..63730a4ec0 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only "key: value" trailers' ' test_cmp expect actual ' +test_expect_success '%(trailers:only=yes) shows only "key: value" trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual && + grep -v patch.description <trailers >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only=no) shows all trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=no)" >actual && + cat trailers >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual && + grep -v patch.description <trailers >expect && + test_cmp expect actual +' + test_expect_success '%(trailers:unfold) unfolds trailers' ' git log --no-walk --pretty="%(trailers:unfold)" >actual && { -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value 2019-01-28 21:33 ` [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value Anders Waldenborg @ 2019-01-28 22:38 ` Junio C Hamano 2019-01-29 6:45 ` Anders Waldenborg 2019-01-29 6:49 ` [PATCH v5 2/7 update] pretty: allow " Anders Waldenborg 0 siblings, 2 replies; 69+ messages in thread From: Junio C Hamano @ 2019-01-28 22:38 UTC (permalink / raw) To: Anders Waldenborg; +Cc: git, Jeff King Anders Waldenborg <anders@0x63.nu> writes: > Subject: Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value Style: s/pretty: Allow/pretty: allow/ (haven't I said this often enough?) > +** 'only[=val]': select whether non-trailer lines from the trailer > + block should be included. The `only` keyword may optionally be > + followed by an equal sign and one of `true`, `on`, `yes` to omit or > + `false`, `off`, `no` to show the non-trailer lines. If option is > + given without value it is enabled. If given multiple times the last > + value is used. > +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold` > + option was given. In same way as to for `only` it can be followed > + by an equal sign and explicit value. E.g., > + `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. Sounds sensible. > diff --git a/pretty.c b/pretty.c > index b83a3ecd23..b8d71a57c9 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1056,13 +1056,25 @@ static size_t parse_padding_placeholder(struct strbuf *sb, > return 0; > } > > -static int match_placeholder_arg(const char *to_parse, const char *candidate, > - const char **end) > +static int match_placeholder_arg_value(const char *to_parse, const char *candidate, > + const char **end, const char **valuestart, size_t *valuelen) An overlong line here. > ... > +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, > + const char **end, int *val) > +{ > + char buf[8]; > + const char *strval; > + size_t len; > + int v; > + > + if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len)) > + return 0; > + > + if (!strval) { > + *val = 1; > + return 1; > + } > + > + strlcpy(buf, strval, sizeof(buf)); > + if (len < sizeof(buf)) > + buf[len] = 0; Doesn't strlcpy() terminate buf[len] if len is short enough? Even if the strval is longer than buf[], strlcpy() would truncate and make sure buf[] is NUL terminated, no? > + v = git_parse_maybe_bool(buf); Why? This function would simply be buggy and incapable of parsing a representation of a boolean value that is longer than 8 bytes (if such a representation exists), so chomping an overlong string at the end and feeding it to git_parse_maybe_bool() is a nonsense, isn't it? In this particular case, strlcpy() is inviting a buggy programming. If there were a 7-letter representation of falsehood, strval may be that 7-letter thing, in which case you would want to feed it to git_parse_maybe_bool() to receive "false" from it, or strval may have that 7-letter thing followed by a 'x' (so as a token, that is not a correctly spelled falsehood), but strlcpy() would chomp and show the same 7-letter falsehood to git_parse_maybe_bool(). That robs you from an opportunity to diagnose such a bogus input as an error. Instead of using "char buf[8]", just using a strbuf and avoidng strlcpy() would make the code much better, I would think. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value 2019-01-28 22:38 ` Junio C Hamano @ 2019-01-29 6:45 ` Anders Waldenborg 2019-01-29 16:57 ` Jeff King 2019-01-29 6:49 ` [PATCH v5 2/7 update] pretty: allow " Anders Waldenborg 1 sibling, 1 reply; 69+ messages in thread From: Anders Waldenborg @ 2019-01-29 6:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Anders Waldenborg, git, Jeff King Junio C Hamano writes: >> ... >> +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, >> + const char **end, int *val) >> +{ >> + char buf[8]; >> + const char *strval; >> + size_t len; >> + int v; >> + >> + if (!match_placeholder_arg_value(to_parse, candidate, end, &strval, &len)) >> + return 0; >> + >> + if (!strval) { >> + *val = 1; >> + return 1; >> + } >> + >> + strlcpy(buf, strval, sizeof(buf)); >> + if (len < sizeof(buf)) >> + buf[len] = 0; > > Doesn't strlcpy() terminate buf[len] if len is short enough? > Even if the strval is longer than buf[], strlcpy() would truncate > and make sure buf[] is NUL terminated, no? Yes, but no. strval is not NUL-terminated at len. E.g strval would point to "false,something=true". `buf[len] = 0` makes sure it becomes "false". > Instead of using "char buf[8]", just using a strbuf and avoidng > strlcpy() would make the code much better, I would think. Yes, taking the heap allocation hit would most likely make the intent clearer. ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value 2019-01-29 6:45 ` Anders Waldenborg @ 2019-01-29 16:57 ` Jeff King 0 siblings, 0 replies; 69+ messages in thread From: Jeff King @ 2019-01-29 16:57 UTC (permalink / raw) To: Anders Waldenborg; +Cc: Junio C Hamano, git On Tue, Jan 29, 2019 at 07:45:06AM +0100, Anders Waldenborg wrote: > > Instead of using "char buf[8]", just using a strbuf and avoidng > > strlcpy() would make the code much better, I would think. > > Yes, taking the heap allocation hit would most likely make the intent > clearer. If you can reuse the same struct and strbuf_reset() it each time, then that amortizes the cost of the heap (to basically once per program run, instead of once per commit). -Peff ^ permalink raw reply [flat|nested] 69+ messages in thread
* Re: [PATCH v5 2/7 update] pretty: allow %(trailers) options with explicit value 2019-01-28 22:38 ` Junio C Hamano 2019-01-29 6:45 ` Anders Waldenborg @ 2019-01-29 6:49 ` Anders Waldenborg 1 sibling, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2019-01-29 6:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Anders Waldenborg, git, Jeff King In addition to old %(trailers:only) it is now allowed to write %(trailers:only=yes) By itself this only gives (the not quite so useful) possibility to have users change their mind in the middle of a formatting string (%(trailers:only=true,only=false)). However, it gives users the opportunity to override defaults from future options. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 14 ++++++--- pretty.c | 52 +++++++++++++++++++++++++++----- t/t4205-log-pretty-formats.sh | 18 +++++++++++ 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 86d804fe97..d33b072eb2 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -225,10 +225,16 @@ endif::git-rev-list[] linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by a colon and zero or more comma-separated options: -** 'only': omit non-trailer lines from the trailer block. -** 'unfold': make it behave as if interpret-trailer's `--unfold` - option was given. E.g., `%(trailers:only,unfold)` unfolds and - shows all trailer lines. +** 'only[=val]': select whether non-trailer lines from the trailer + block should be included. The `only` keyword may optionally be + followed by an equal sign and one of `true`, `on`, `yes` to omit or + `false`, `off`, `no` to show the non-trailer lines. If option is + given without value it is enabled. If given multiple times the last + value is used. +** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold` + option was given. In same way as to for `only` it can be followed + by an equal sign and explicit value. E.g., + `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index b83a3ecd23..4dfbd38cf6 100644 --- a/pretty.c +++ b/pretty.c @@ -1056,13 +1056,26 @@ static size_t parse_padding_placeholder(struct strbuf *sb, return 0; } -static int match_placeholder_arg(const char *to_parse, const char *candidate, - const char **end) +static int match_placeholder_arg_value(const char *to_parse, const char *candidate, + const char **end, const char **valuestart, + size_t *valuelen) { const char *p; if (!(skip_prefix(to_parse, candidate, &p))) return 0; + if (valuestart) { + if (*p == '=') { + *valuestart = p + 1; + *valuelen = strcspn(*valuestart, ",)"); + p = *valuestart + *valuelen; + } else { + if (*p != ',' && *p != ')') + return 0; + *valuestart = NULL; + *valuelen = 0; + } + } if (*p == ',') { *end = p + 1; return 1; @@ -1074,6 +1087,34 @@ static int match_placeholder_arg(const char *to_parse, const char *candidate, return 0; } +static int match_placeholder_bool_arg(const char *to_parse, const char *candidate, + const char **end, int *val) +{ + const char *argval; + char *strval; + size_t arglen; + int v; + + if (!match_placeholder_arg_value(to_parse, candidate, end, &argval, &arglen)) + return 0; + + if (!argval) { + *val = 1; + return 1; + } + + strval = xstrndup(argval, arglen); + v = git_parse_maybe_bool(strval); + free(strval); + + if (v == -1) + return 0; + + *val = v; + + return 1; +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1318,11 +1359,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (*arg == ':') { arg++; for (;;) { - if (match_placeholder_arg(arg, "only", &arg)) - opts.only_trailers = 1; - else if (match_placeholder_arg(arg, "unfold", &arg)) - opts.unfold = 1; - else + if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && + !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) break; } } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 978a8a66ff..63730a4ec0 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -578,6 +578,24 @@ test_expect_success '%(trailers:only) shows only "key: value" trailers' ' test_cmp expect actual ' +test_expect_success '%(trailers:only=yes) shows only "key: value" trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual && + grep -v patch.description <trailers >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only=no) shows all trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=no)" >actual && + cat trailers >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only=no,only=true) shows only "key: value" trailers' ' + git log --no-walk --pretty=format:"%(trailers:only=yes)" >actual && + grep -v patch.description <trailers >expect && + test_cmp expect actual +' + test_expect_success '%(trailers:unfold) unfolds trailers' ' git log --no-walk --pretty="%(trailers:unfold)" >actual && { -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v5 3/7] pretty: single return path in %(trailers) handling 2019-01-28 21:33 ` [PATCH v5 0/7] %(trailers) improvements in pretty format Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 1/7] doc: group pretty-format.txt placeholders descriptions Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value Anders Waldenborg @ 2019-01-28 21:33 ` Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 4/7] pretty: allow showing specific trailers Anders Waldenborg ` (3 subsequent siblings) 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2019-01-28 21:33 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg No functional change intended. This change may not seem useful on its own, but upcoming commits will do memory allocation in there, and a single return path makes deallocation easier. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- pretty.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pretty.c b/pretty.c index b8d71a57c9..65a1b9bd82 100644 --- a/pretty.c +++ b/pretty.c @@ -1353,6 +1353,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; + size_t ret = 0; opts.no_divider = 1; @@ -1366,8 +1367,9 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); - return arg - placeholder + 1; + ret = arg - placeholder + 1; } + return ret; } return 0; /* unknown placeholder */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v5 4/7] pretty: allow showing specific trailers 2019-01-28 21:33 ` [PATCH v5 0/7] %(trailers) improvements in pretty format Anders Waldenborg ` (2 preceding siblings ...) 2019-01-28 21:33 ` [PATCH v5 3/7] pretty: single return path in %(trailers) handling Anders Waldenborg @ 2019-01-28 21:33 ` Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 5/7] pretty: add support for "valueonly" option in %(trailers) Anders Waldenborg ` (2 subsequent siblings) 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2019-01-28 21:33 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg Adds a new "key=X" option to "%(trailers)" which will cause it to only print trailer lines which match any of the specified keys. Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 8 +++++ pretty.c | 36 ++++++++++++++++++-- t/t4205-log-pretty-formats.sh | 57 ++++++++++++++++++++++++++++++++ trailer.c | 10 +++--- trailer.h | 2 ++ 5 files changed, 107 insertions(+), 6 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index d33b072eb2..d6add831c0 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -225,6 +225,14 @@ endif::git-rev-list[] linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by a colon and zero or more comma-separated options: +** 'key=<K>': only show trailers with specified key. Matching is done + case-insensitively and trailing colon is optional. If option is + given multiple times trailer lines matching any of the keys are + shown. This option automatically enables the `only` option so that + non-trailer lines in the trailer block are hidden. If that is not + desired it can be disabled with `only=false`. E.g., + `%(trailers:key=Reviewed-by)` shows trailer lines with key + `Reviewed-by`. ** 'only[=val]': select whether non-trailer lines from the trailer block should be included. The `only` keyword may optionally be followed by an equal sign and one of `true`, `on`, `yes` to omit or diff --git a/pretty.c b/pretty.c index 65a1b9bd82..d3dd2d6254 100644 --- a/pretty.c +++ b/pretty.c @@ -1115,6 +1115,19 @@ static int match_placeholder_bool_arg(const char *to_parse, const char *candidat return 1; } +static int format_trailer_match_cb(const struct strbuf *key, void *ud) +{ + const struct string_list *list = ud; + const struct string_list_item *item; + + for_each_string_list_item (item, list) { + if (key->len == (uintptr_t)item->util && + !strncasecmp(item->string, key->buf, key->len)) + return 1; + } + return 0; +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1353,6 +1366,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; + struct string_list filter_list = STRING_LIST_INIT_NODUP; size_t ret = 0; opts.no_divider = 1; @@ -1360,8 +1374,24 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (*arg == ':') { arg++; for (;;) { - if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && - !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) + const char *argval; + size_t arglen; + + if (match_placeholder_arg_value(arg, "key", &arg, &argval, &arglen)) { + uintptr_t len = arglen; + + if (!argval) + goto trailer_out; + + if (len && argval[len - 1] == ':') + len--; + string_list_append(&filter_list, argval)->util = (char *)len; + + opts.filter = format_trailer_match_cb; + opts.filter_data = &filter_list; + opts.only_trailers = 1; + } else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && + !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) break; } } @@ -1369,6 +1399,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ format_trailers_from_commit(sb, msg + c->subject_off, &opts); ret = arg - placeholder + 1; } + trailer_out: + string_list_clear(&filter_list, 0); return ret; } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 63730a4ec0..d87201afbe 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -616,6 +616,63 @@ test_expect_success ':only and :unfold work together' ' test_cmp expect actual ' +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by)" >actual && + echo "Acked-by: A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' ' + git log --no-walk --pretty="format:%(trailers:key=AcKed-bY)" >actual && + echo "Acked-by: A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo:) trailing colon also works' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by:)" >actual && + echo "Acked-by: A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo) multiple keys' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by:,key=Signed-off-By)" >actual && + grep -v patch.description <trailers >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=nonexistant) becomes empty' ' + git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual && + echo "xx" >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=foo) handles multiple lines even if folded' ' + git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by)" >actual && + grep -v patch.description <trailers | grep -v Acked-by >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key=foo,unfold) properly unfolds' ' + git log --no-walk --pretty="format:%(trailers:key=Signed-Off-by,unfold)" >actual && + unfold <trailers | grep Signed-off-by >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers:key=foo,only=no) also includes nontrailer lines' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by,only=no)" >actual && + { + echo "Acked-by: A U Thor <author@example.com>" && + grep patch.description <trailers + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:key) without value is error' ' + git log --no-walk --pretty="tformat:%(trailers:key)" >actual && + echo "%(trailers:key)" >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index 0796f326b3..d6da555cd7 100644 --- a/trailer.c +++ b/trailer.c @@ -1132,7 +1132,7 @@ static void format_trailer_info(struct strbuf *out, size_t i; /* If we want the whole block untouched, we can take the fast path. */ - if (!opts->only_trailers && !opts->unfold) { + if (!opts->only_trailers && !opts->unfold && !opts->filter) { strbuf_add(out, info->trailer_start, info->trailer_end - info->trailer_start); return; @@ -1147,10 +1147,12 @@ static void format_trailer_info(struct strbuf *out, struct strbuf val = STRBUF_INIT; parse_trailer(&tok, &val, NULL, trailer, separator_pos); - if (opts->unfold) - unfold_value(&val); + if (!opts->filter || opts->filter(&tok, opts->filter_data)) { + if (opts->unfold) + unfold_value(&val); - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + } strbuf_release(&tok); strbuf_release(&val); diff --git a/trailer.h b/trailer.h index b997739649..5255b676de 100644 --- a/trailer.h +++ b/trailer.h @@ -72,6 +72,8 @@ struct process_trailer_options { int only_input; int unfold; int no_divider; + int (*filter)(const struct strbuf *, void *); + void *filter_data; }; #define PROCESS_TRAILER_OPTIONS_INIT {0} -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v5 5/7] pretty: add support for "valueonly" option in %(trailers) 2019-01-28 21:33 ` [PATCH v5 0/7] %(trailers) improvements in pretty format Anders Waldenborg ` (3 preceding siblings ...) 2019-01-28 21:33 ` [PATCH v5 4/7] pretty: allow showing specific trailers Anders Waldenborg @ 2019-01-28 21:33 ` Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 6/7] strbuf: separate callback for strbuf_expand:ing literals Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 7/7] pretty: add support for separator option in %(trailers) Anders Waldenborg 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2019-01-28 21:33 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg With the new "key=" option to %(trailers) it often makes little sense to show the key, as it by definition already is knows which trailer is printed there. This new "valueonly" option makes it omit the key when printing trailers. E.g.: $ git show -s --pretty='%s%n%(trailers:key=Signed-off-by,valueonly)' aaaa88182 will show: > upload-pack: fix broken if/else chain in config callback > Jeff King <peff@peff.net> > Junio C Hamano <gitster@pobox.com> Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 2 ++ pretty.c | 3 ++- t/t4205-log-pretty-formats.sh | 6 ++++++ trailer.c | 6 ++++-- trailer.h | 1 + 5 files changed, 15 insertions(+), 3 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index d6add831c0..a920dd15b1 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -243,6 +243,8 @@ endif::git-rev-list[] option was given. In same way as to for `only` it can be followed by an equal sign and explicit value. E.g., `%(trailers:only,unfold=true)` unfolds and shows all trailer lines. +** 'valueonly[=val]': skip over the key part of the trailer line and only + show the value part. Also this optionally allows explicit value. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/pretty.c b/pretty.c index d3dd2d6254..ed25845c98 100644 --- a/pretty.c +++ b/pretty.c @@ -1391,7 +1391,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ opts.filter_data = &filter_list; opts.only_trailers = 1; } else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && - !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold)) + !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) && + !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only)) break; } } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index d87201afbe..1ad6834781 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -673,6 +673,12 @@ test_expect_success '%(trailers:key) without value is error' ' test_cmp expect actual ' +test_expect_success '%(trailers:key=foo,valueonly) shows only value' ' + git log --no-walk --pretty="format:%(trailers:key=Acked-by,valueonly)" >actual && + echo "A U Thor <author@example.com>" >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index d6da555cd7..d0d9e91631 100644 --- a/trailer.c +++ b/trailer.c @@ -1150,8 +1150,10 @@ static void format_trailer_info(struct strbuf *out, if (!opts->filter || opts->filter(&tok, opts->filter_data)) { if (opts->unfold) unfold_value(&val); - - strbuf_addf(out, "%s: %s\n", tok.buf, val.buf); + if (!opts->value_only) + strbuf_addf(out, "%s: ", tok.buf); + strbuf_addbuf(out, &val); + strbuf_addch(out, '\n'); } strbuf_release(&tok); strbuf_release(&val); diff --git a/trailer.h b/trailer.h index 5255b676de..06d417fe93 100644 --- a/trailer.h +++ b/trailer.h @@ -72,6 +72,7 @@ struct process_trailer_options { int only_input; int unfold; int no_divider; + int value_only; int (*filter)(const struct strbuf *, void *); void *filter_data; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v5 6/7] strbuf: separate callback for strbuf_expand:ing literals 2019-01-28 21:33 ` [PATCH v5 0/7] %(trailers) improvements in pretty format Anders Waldenborg ` (4 preceding siblings ...) 2019-01-28 21:33 ` [PATCH v5 5/7] pretty: add support for "valueonly" option in %(trailers) Anders Waldenborg @ 2019-01-28 21:33 ` Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 7/7] pretty: add support for separator option in %(trailers) Anders Waldenborg 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2019-01-28 21:33 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg Expanding '%n' and '%xNN' is generic functionality, so extract that from the pretty.c formatter into a callback that can be reused. No functional change intended Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- pretty.c | 16 +++++----------- strbuf.c | 21 +++++++++++++++++++++ strbuf.h | 8 ++++++++ 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/pretty.c b/pretty.c index ed25845c98..7baa4c1c26 100644 --- a/pretty.c +++ b/pretty.c @@ -1137,9 +1137,13 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *msg = c->message; struct commit_list *p; const char *arg; - int ch; + size_t res; /* these are independent of the commit */ + res = strbuf_expand_literal_cb(sb, placeholder, NULL); + if (res) + return res; + switch (placeholder[0]) { case 'C': if (starts_with(placeholder + 1, "(auto)")) { @@ -1158,16 +1162,6 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ */ return ret; } - case 'n': /* newline */ - strbuf_addch(sb, '\n'); - return 1; - case 'x': - /* %x00 == NUL, %x0a == LF, etc. */ - ch = hex2chr(placeholder + 1); - if (ch < 0) - return 0; - strbuf_addch(sb, ch); - return 3; case 'w': if (placeholder[1] == '(') { unsigned long width = 0, indent1 = 0, indent2 = 0; diff --git a/strbuf.c b/strbuf.c index f6a6cf78b9..78eecd29f7 100644 --- a/strbuf.c +++ b/strbuf.c @@ -380,6 +380,27 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, } } +size_t strbuf_expand_literal_cb(struct strbuf *sb, + const char *placeholder, + void *context) +{ + int ch; + + switch (placeholder[0]) { + case 'n': /* newline */ + strbuf_addch(sb, '\n'); + return 1; + case 'x': + /* %x00 == NUL, %x0a == LF, etc. */ + ch = hex2chr(placeholder + 1); + if (ch < 0) + return 0; + strbuf_addch(sb, ch); + return 3; + } + return 0; +} + size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context) { diff --git a/strbuf.h b/strbuf.h index fc40873b65..52e44c9ab8 100644 --- a/strbuf.h +++ b/strbuf.h @@ -320,6 +320,14 @@ void strbuf_expand(struct strbuf *sb, expand_fn_t fn, void *context); +/** + * Used as callback for `strbuf_expand` to only expand literals + * (i.e. %n and %xNN). The context argument is ignored. + */ +size_t strbuf_expand_literal_cb(struct strbuf *sb, + const char *placeholder, + void *context); + /** * Used as callback for `strbuf_expand()`, expects an array of * struct strbuf_expand_dict_entry as context, i.e. pairs of -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
* [PATCH v5 7/7] pretty: add support for separator option in %(trailers) 2019-01-28 21:33 ` [PATCH v5 0/7] %(trailers) improvements in pretty format Anders Waldenborg ` (5 preceding siblings ...) 2019-01-28 21:33 ` [PATCH v5 6/7] strbuf: separate callback for strbuf_expand:ing literals Anders Waldenborg @ 2019-01-28 21:33 ` Anders Waldenborg 6 siblings, 0 replies; 69+ messages in thread From: Anders Waldenborg @ 2019-01-28 21:33 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Anders Waldenborg By default trailer lines are terminated by linebreaks ('\n'). By specifying the new 'separator' option they will instead be separated by user provided string and have separator semantics rather than terminator semantics. The separator string can contain the literal formatting codes %n and %xNN allowing it to be things that are otherwise hard to type such as %x00, or comma and end-parenthesis which would break parsing. E.g: $ git log --pretty='%(trailers:key=Reviewed-by,valueonly,separator=%x00)' Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- Documentation/pretty-formats.txt | 9 ++++++++ pretty.c | 10 +++++++++ t/t4205-log-pretty-formats.sh | 36 ++++++++++++++++++++++++++++++++ trailer.c | 15 +++++++++++-- trailer.h | 1 + 5 files changed, 69 insertions(+), 2 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index a920dd15b1..ce087dee80 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -239,6 +239,15 @@ endif::git-rev-list[] `false`, `off`, `no` to show the non-trailer lines. If option is given without value it is enabled. If given multiple times the last value is used. +** 'separator=<SEP>': specify a separator inserted between trailer + lines. When this option is not given each trailer line is + terminated with a line feed character. The string SEP may contain + the literal formatting codes described above. To use comma as + separator one must use `%x2C` as it would otherwise be parsed as + next option. If separator option is given multiple times only the + last one is used. E.g., `%(trailers:key=Ticket,separator=%x2C )` + shows all trailer lines whose key is "Ticket" separated by a comma + and a space. ** 'unfold[=val]': make it behave as if interpret-trailer's `--unfold` option was given. In same way as to for `only` it can be followed by an equal sign and explicit value. E.g., diff --git a/pretty.c b/pretty.c index 7baa4c1c26..99b66ccf5a 100644 --- a/pretty.c +++ b/pretty.c @@ -1361,6 +1361,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; struct string_list filter_list = STRING_LIST_INIT_NODUP; + struct strbuf sepbuf = STRBUF_INIT; size_t ret = 0; opts.no_divider = 1; @@ -1384,6 +1385,14 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ opts.filter = format_trailer_match_cb; opts.filter_data = &filter_list; opts.only_trailers = 1; + } else if (match_placeholder_arg_value(arg, "separator", &arg, &argval, &arglen)) { + char *fmt; + + strbuf_reset(&sepbuf); + fmt = xstrndup(argval, arglen); + strbuf_expand(&sepbuf, fmt, strbuf_expand_literal_cb, NULL); + free(fmt); + opts.separator = &sepbuf; } else if (!match_placeholder_bool_arg(arg, "only", &arg, &opts.only_trailers) && !match_placeholder_bool_arg(arg, "unfold", &arg, &opts.unfold) && !match_placeholder_bool_arg(arg, "valueonly", &arg, &opts.value_only)) @@ -1396,6 +1405,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ } trailer_out: string_list_clear(&filter_list, 0); + strbuf_release(&sepbuf); return ret; } diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 1ad6834781..99f50fa401 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -679,6 +679,42 @@ test_expect_success '%(trailers:key=foo,valueonly) shows only value' ' test_cmp expect actual ' +test_expect_success 'pretty format %(trailers:separator) changes separator' ' + git log --no-walk --pretty=format:"X%(trailers:separator=%x00,unfold)X" >actual && + printf "XSigned-off-by: A U Thor <author@example.com>\0Acked-by: A U Thor <author@example.com>\0[ v2 updated patch description ]\0Signed-off-by: A U Thor <author@example.com>X" >expect && + test_cmp expect actual +' + +test_expect_success 'pretty format %(trailers) combining separator/key/valueonly' ' + git commit --allow-empty -F - <<-\EOF && + Important fix + + The fix is explained here + + Closes: #1234 + EOF + + git commit --allow-empty -F - <<-\EOF && + Another fix + + The fix is explained here + + Closes: #567 + Closes: #890 + EOF + + git commit --allow-empty -F - <<-\EOF && + Does not close any tickets + EOF + + git log --pretty="%s% (trailers:separator=%x2c%x20,key=Closes,valueonly)" HEAD~3.. >actual && + test_write_lines \ + "Does not close any tickets" \ + "Another fix #567, #890" \ + "Important fix #1234" >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject diff --git a/trailer.c b/trailer.c index d0d9e91631..0c414f2fed 100644 --- a/trailer.c +++ b/trailer.c @@ -1129,10 +1129,11 @@ static void format_trailer_info(struct strbuf *out, const struct trailer_info *info, const struct process_trailer_options *opts) { + size_t origlen = out->len; size_t i; /* If we want the whole block untouched, we can take the fast path. */ - if (!opts->only_trailers && !opts->unfold && !opts->filter) { + if (!opts->only_trailers && !opts->unfold && !opts->filter && !opts->separator) { strbuf_add(out, info->trailer_start, info->trailer_end - info->trailer_start); return; @@ -1150,16 +1151,26 @@ static void format_trailer_info(struct strbuf *out, if (!opts->filter || opts->filter(&tok, opts->filter_data)) { if (opts->unfold) unfold_value(&val); + + if (opts->separator && out->len != origlen) + strbuf_addbuf(out, opts->separator); if (!opts->value_only) strbuf_addf(out, "%s: ", tok.buf); strbuf_addbuf(out, &val); - strbuf_addch(out, '\n'); + if (!opts->separator) + strbuf_addch(out, '\n'); } strbuf_release(&tok); strbuf_release(&val); } else if (!opts->only_trailers) { + if (opts->separator && out->len != origlen) { + strbuf_addbuf(out, opts->separator); + } strbuf_addstr(out, trailer); + if (opts->separator) { + strbuf_rtrim(out); + } } } diff --git a/trailer.h b/trailer.h index 06d417fe93..203acf4ee1 100644 --- a/trailer.h +++ b/trailer.h @@ -73,6 +73,7 @@ struct process_trailer_options { int unfold; int no_divider; int value_only; + const struct strbuf *separator; int (*filter)(const struct strbuf *, void *); void *filter_data; }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 69+ messages in thread
end of thread, other threads:[~2019-02-02 9:14 UTC | newest] Thread overview: 69+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-28 12:50 [PATCH] pretty: Add %(trailer:X) to display single trailer Anders Waldenborg 2018-10-29 4:49 ` Junio C Hamano 2018-10-29 14:14 ` Jeff King 2018-10-29 17:05 ` Anders Waldenborg 2018-10-31 20:27 ` Jeff King 2018-10-31 23:01 ` Anders Waldenborg 2018-11-01 18:42 ` Jeff King 2018-11-04 15:22 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Anders Waldenborg 2018-11-04 15:22 ` [PATCH v2 1/5] pretty: single return path in %(trailers) handling Anders Waldenborg 2018-11-04 15:22 ` [PATCH v2 2/5] pretty: allow showing specific trailers Anders Waldenborg 2018-11-04 18:14 ` Eric Sunshine 2018-11-05 3:48 ` Junio C Hamano 2018-11-05 3:52 ` Eric Sunshine 2018-11-05 8:26 ` Anders Waldenborg 2018-11-05 9:00 ` Eric Sunshine 2018-11-05 5:14 ` Junio C Hamano 2018-11-04 15:22 ` [PATCH v2 3/5] pretty: add support for "nokey" option in %(trailers) Anders Waldenborg 2018-11-04 15:22 ` [PATCH v2 4/5] pretty: extract fundamental placeholders to separate function Anders Waldenborg 2018-11-05 2:06 ` Junio C Hamano 2018-11-05 8:32 ` Anders Waldenborg 2018-11-06 1:46 ` Junio C Hamano 2018-11-04 15:22 ` [PATCH v2 5/5] pretty: add support for separator option in %(trailers) Anders Waldenborg 2018-11-05 2:10 ` Junio C Hamano 2018-11-05 18:24 ` Anders Waldenborg 2018-11-06 1:48 ` Junio C Hamano 2018-11-05 5:18 ` Junio C Hamano 2018-11-04 17:40 ` [PATCH v2 0/5] %(trailers) improvements in pretty format Eric Sunshine 2018-11-05 7:09 ` Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 " Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 1/5] pretty: single return path in %(trailers) handling Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 2/5] pretty: allow showing specific trailers Anders Waldenborg 2018-11-20 5:45 ` Junio C Hamano 2018-11-20 5:59 ` Junio C Hamano 2018-11-25 23:02 ` Anders Waldenborg 2018-11-26 3:13 ` Junio C Hamano 2018-11-26 6:56 ` Anders Waldenborg 2018-11-26 7:52 ` Junio C Hamano 2018-11-18 11:44 ` [PATCH v3 3/5] pretty: add support for "valueonly" option in %(trailers) Anders Waldenborg 2018-11-20 8:14 ` Eric Sunshine 2018-11-18 11:44 ` [PATCH v3 4/5] strbuf: separate callback for strbuf_expand:ing literals Anders Waldenborg 2018-11-18 11:44 ` [PATCH v3 5/5] pretty: add support for separator option in %(trailers) Anders Waldenborg 2018-11-20 8:25 ` Eric Sunshine 2018-12-08 16:36 ` [PATCH v4 0/7] %(trailers) improvements in pretty format Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 1/7] doc: group pretty-format.txt placeholders descriptions Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value Anders Waldenborg 2018-12-10 8:45 ` Junio C Hamano 2018-12-18 21:30 ` Anders Waldenborg 2019-01-29 16:55 ` Jeff King 2019-01-29 21:23 ` Anders Waldenborg [not found] ` <CAL21Bmmx=EO+R2t+KviNekDhU3fc0wjCcmUmbzLa14bb0PAmHA@mail.gmail.com> 2019-01-31 18:46 ` Anders Waldenborg 2019-02-02 9:14 ` Оля Тележная 2018-12-08 16:36 ` [PATCH v4 3/7] pretty: single return path in %(trailers) handling Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 4/7] pretty: allow showing specific trailers Anders Waldenborg 2018-12-10 8:56 ` Junio C Hamano 2018-12-08 16:36 ` [PATCH v4 5/7] pretty: add support for "valueonly" option in %(trailers) Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 6/7] strbuf: separate callback for strbuf_expand:ing literals Anders Waldenborg 2018-12-08 16:36 ` [PATCH v4 7/7] pretty: add support for separator option in %(trailers) Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 0/7] %(trailers) improvements in pretty format Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 1/7] doc: group pretty-format.txt placeholders descriptions Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 2/7] pretty: Allow %(trailers) options with explicit value Anders Waldenborg 2019-01-28 22:38 ` Junio C Hamano 2019-01-29 6:45 ` Anders Waldenborg 2019-01-29 16:57 ` Jeff King 2019-01-29 6:49 ` [PATCH v5 2/7 update] pretty: allow " Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 3/7] pretty: single return path in %(trailers) handling Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 4/7] pretty: allow showing specific trailers Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 5/7] pretty: add support for "valueonly" option in %(trailers) Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 6/7] strbuf: separate callback for strbuf_expand:ing literals Anders Waldenborg 2019-01-28 21:33 ` [PATCH v5 7/7] pretty: add support for separator option in %(trailers) Anders Waldenborg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).