* [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) @ 2017-09-30 6:22 Taylor Blau 2017-09-30 6:22 ` [PATCH 1/5] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (6 more replies) 0 siblings, 7 replies; 78+ messages in thread From: Taylor Blau @ 2017-09-30 6:22 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau Hi, Attached is a patch to extend Peff's recent work of adding parsing options to "--pretty=%(trailers)" by supporting those same options in git-for-each-ref(1). In summary, this patch adds correct behavior for the following options, when given to git-for-each-ref(1): * --format="%(trailers:only)" * --format="%(trailers:unfold,unfold)" * --format="%(contents:trailers:only)" * --format="%(contents:trailers:unfold,unfold)" I have changed the syntax for specifying multiple sub-arguments in %(contents:trailers) and %(trailers) atoms to be ","-delimited instead of %":"-delimited. This is consistent with similar atoms, and is described in %greater detail in "pretty.c: delimit "%(trailers)" arguments with ","". I am also new around here: this is my first patch that I am sending to the mailing list, so this process is entirely new to me. My current focus is helping maintain Git LFS [1] at GitHub. If I have made any mistakes in formatting these patches or sending them, please let me know :-). Thank you in advance. -- - Taylor [1]: https://git-lfs.github.com Taylor Blau (5): pretty.c: delimit "%(trailers)" arguments with "," t6300: refactor %(trailers) tests ref-filter.c: add trailer options to used_atom ref-filter.c: use trailer_opts to format trailers ref-filter.c: parse trailers arguments with %(contents) atom Documentation/git-for-each-ref.txt | 7 ++- pretty.c | 13 +++--- ref-filter.c | 38 +++++++++++----- t/t4205-log-pretty-formats.sh | 4 +- t/t6300-for-each-ref.sh | 88 +++++++++++++++++++++++++++++++++++++- 5 files changed, 129 insertions(+), 21 deletions(-) -- 2.14.1.145.gb3622a4ee ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH 1/5] pretty.c: delimit "%(trailers)" arguments with "," 2017-09-30 6:22 [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Taylor Blau @ 2017-09-30 6:22 ` Taylor Blau 2017-09-30 6:49 ` Jeff King 2017-09-30 6:22 ` [PATCH 2/5] t6300: refactor %(trailers) tests Taylor Blau ` (5 subsequent siblings) 6 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-09-30 6:22 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau In preparation for adding consistent "%(trailers)" atom options to `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in pretty.c to separate sub-arguments with a ",", instead of a ":". Multiple sub-arguments are given either as "%(trailers:unfold,only)" or "%(trailers:only,unfold)". This change disambiguates between "top-level" arguments, and arguments given to the trailers atom itself. It is consistent with the behavior of "%(upstream)" and "%(push)" atoms. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pretty.c | 13 ++++++++----- t/t4205-log-pretty-formats.sh | 4 ++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/pretty.c b/pretty.c index 0e23fe3c0..68f736912 100644 --- a/pretty.c +++ b/pretty.c @@ -1265,11 +1265,14 @@ 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; - while (*arg == ':') { - if (skip_prefix(arg, ":only", &arg)) - opts.only_trailers = 1; - else if (skip_prefix(arg, ":unfold", &arg)) - opts.unfold = 1; + if (skip_prefix(arg, ":", &arg)) { + while (*arg != ')') { + skip_prefix(arg, ",", &arg); + if (skip_prefix(arg, "only", &arg)) + opts.only_trailers = 1; + else if (skip_prefix(arg, "unfold", &arg)) + opts.unfold = 1; + } } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index ec5f53010..977472f53 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' ' ' test_expect_success ':only and :unfold work together' ' - git log --no-walk --pretty="%(trailers:only:unfold)" >actual && - git log --no-walk --pretty="%(trailers:unfold:only)" >reverse && + git log --no-walk --pretty="%(trailers:only,unfold)" >actual && + git log --no-walk --pretty="%(trailers:unfold,only)" >reverse && test_cmp actual reverse && { grep -v patch.description <trailers | unfold && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH 1/5] pretty.c: delimit "%(trailers)" arguments with "," 2017-09-30 6:22 ` [PATCH 1/5] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-09-30 6:49 ` Jeff King 0 siblings, 0 replies; 78+ messages in thread From: Jeff King @ 2017-09-30 6:49 UTC (permalink / raw) To: Taylor Blau; +Cc: git, gitster On Fri, Sep 29, 2017 at 11:22:34PM -0700, Taylor Blau wrote: > In preparation for adding consistent "%(trailers)" atom options to > `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in pretty.c > to separate sub-arguments with a ",", instead of a ":". > > Multiple sub-arguments are given either as "%(trailers:unfold,only)" or > "%(trailers:only,unfold)". > > This change disambiguates between "top-level" arguments, and arguments given to > the trailers atom itself. It is consistent with the behavior of "%(upstream)" > and "%(push)" atoms. Great. I hope we'll eventually unify the pretty.c and ref-filter.c syntaxes and implementations, so consistency is good. I'm sure there will be some hackery to keep backwards compatibility in all cases, but the fewer the better. The "every option gets its own colon" scheme was just something I invented, because there was no prior art in pretty.c. I didn't think to check for similar cases in ref-filter.c. We should be safe to change this unconditionally, without worrying about breaking compatibility. These options were added in 58311c66fd (pretty: support normalization options for %(trailers), 2017-08-15), which hasn't been in any released version yet. > --- > pretty.c | 13 ++++++++----- > t/t4205-log-pretty-formats.sh | 4 ++-- > 2 files changed, 10 insertions(+), 7 deletions(-) I think this needs an update to Documentation/pretty-formats.txt. Maybe: diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 6b4a12f028..40948d925b 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -199,11 +199,13 @@ 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): display the trailers of the body as interpreted by - linkgit:git-interpret-trailers[1]. 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. +- %(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. NOTE: Some placeholders may depend on other options given to the revision traversal engine. For example, the `%g*` reflog options will diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index ec5f530102..977472f539 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' ' ' test_expect_success ':only and :unfold work together' ' - git log --no-walk --pretty="%(trailers:only:unfold)" >actual && - git log --no-walk --pretty="%(trailers:unfold:only)" >reverse && + git log --no-walk --pretty="%(trailers:only,unfold)" >actual && + git log --no-walk --pretty="%(trailers:unfold,only)" >reverse && test_cmp actual reverse && { grep -v patch.description <trailers | unfold && > diff --git a/pretty.c b/pretty.c > index 0e23fe3c0..68f736912 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1265,11 +1265,14 @@ 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; > - while (*arg == ':') { > - if (skip_prefix(arg, ":only", &arg)) > - opts.only_trailers = 1; > - else if (skip_prefix(arg, ":unfold", &arg)) > - opts.unfold = 1; > + if (skip_prefix(arg, ":", &arg)) { > + while (*arg != ')') { > + skip_prefix(arg, ",", &arg); > + if (skip_prefix(arg, "only", &arg)) > + opts.only_trailers = 1; > + else if (skip_prefix(arg, "unfold", &arg)) > + opts.unfold = 1; > + } > } Do we always make forward progress with this loop condition? If arg doesn't start with a close-paren, we'll keep looping. But if it's not a comma or one of the option strings, then skip_prefix() won't move the pointer forward. So I think "%(trailers:foo)" would loop forever. The original would complain about that and even almost-right things like "%(trailers:only-the-lonely", though it's pretty subtle. The atom parsers in ref-filter.c use string_list_split() which is way more readable and obvious. But I don't think we can do that here. ref-filter has a separate parsing step, so it can afford to be slow. Here we're parsing the pretty format individually for each commit in "git log", and allocations would be noticeable. Here's what I came up with: static int match_placeholder_arg(const char *to_parse, const char *candidate, const char **end) { const char *p; if (!skip_prefix(to_parse, candidate, &p)) return 0; if (*p == ',') { *end = p + 1; return 1; } if (*p == ')') { *end = p; return 1; } return 0; } ... if (skip_prefix(placeholder, "(trailers", &arg)) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; 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 break; } } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); return arg - placeholder + 1; } } I hate the "for (;;)", but at least it's easy to see that each iteration either makes progress or breaks out of the loop. -Peff ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH 2/5] t6300: refactor %(trailers) tests 2017-09-30 6:22 [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-09-30 6:22 ` [PATCH 1/5] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-09-30 6:22 ` Taylor Blau 2017-09-30 7:01 ` Jeff King 2017-09-30 6:22 ` [PATCH 3/5] ref-filter.c: add trailer options to used_atom Taylor Blau ` (4 subsequent siblings) 6 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-09-30 6:22 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau We currently have one test for %(trailers) in `git-for-each-ref(1)`, through "%(contents:trailers)". In preparation for more, let's add a few things: - Move the commit creation step to its own test so that it can be re-used. - Add a non-trailer to the commit's trailers to test that non-trailers aren't shown using "%(trailers:only)". - Add a multi-line trailer to ensure that trailers are unfolded correctly using "%(trailers:unfold)". Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t6300-for-each-ref.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 834a9ed16..2a9fcf713 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -592,18 +592,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' ' cat >trailers <<EOF Reviewed-by: A U Thor <author@example.com> Signed-off-by: A U Thor <author@example.com> +[ v2 updated patch description ] +Acked-by: A U Thor + <author@example.com> EOF -test_expect_success 'basic atom: head contents:trailers' ' + +test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && git add two && - git commit -F - <<-EOF && + git commit -F - <<-EOF trailers: this commit message has trailers Some message contents $(cat trailers) EOF +' + +test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && # git for-each-ref ends with a blank line -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH 2/5] t6300: refactor %(trailers) tests 2017-09-30 6:22 ` [PATCH 2/5] t6300: refactor %(trailers) tests Taylor Blau @ 2017-09-30 7:01 ` Jeff King 0 siblings, 0 replies; 78+ messages in thread From: Jeff King @ 2017-09-30 7:01 UTC (permalink / raw) To: Taylor Blau; +Cc: git, gitster On Fri, Sep 29, 2017 at 11:22:35PM -0700, Taylor Blau wrote: > We currently have one test for %(trailers) in `git-for-each-ref(1)`, through > "%(contents:trailers)". In preparation for more, let's add a few things: > > - Move the commit creation step to its own test so that it can be re-used. > > - Add a non-trailer to the commit's trailers to test that non-trailers aren't > shown using "%(trailers:only)". > > - Add a multi-line trailer to ensure that trailers are unfolded correctly > using "%(trailers:unfold)". This is a minor nit, but since you invited formatting critique in your cover letter, I feel entitled. :) Consider wrapping your commit messages (and emails in general) at 72 characters, rather than 80. That lets them show well on an 80-column display even when indented by "git log" or by inline quoting in an email reply. I'm also of the opinion that while 80 characters is fine for code, it's a bit wide for English text. You can find various claims online[1] from people interested in typography that a line width of about 60-70 characters is pleasant for reading. [1] E.g., https://baymard.com/blog/line-length-readability > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > t/t6300-for-each-ref.sh | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) The patch itself looks fine. :) -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH 3/5] ref-filter.c: add trailer options to used_atom 2017-09-30 6:22 [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-09-30 6:22 ` [PATCH 1/5] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-09-30 6:22 ` [PATCH 2/5] t6300: refactor %(trailers) tests Taylor Blau @ 2017-09-30 6:22 ` Taylor Blau 2017-09-30 7:10 ` Jeff King 2017-09-30 6:22 ` [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers Taylor Blau ` (3 subsequent siblings) 6 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-09-30 6:22 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau In preparation for "%(trailers)" to take trailer parsing arguments, use Jeff's convenience structure for trailer processing options introduced in 8abc89800c. We will later populate this field from the arguments given to %(trailers), and then use the trailer_opts instance to format ref trailers correctly using `format_trailer_from_commit`. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- ref-filter.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ref-filter.c b/ref-filter.c index 467c0279c..84f14093c 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -82,6 +82,7 @@ static struct used_atom { } remote_ref; struct { enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option; + struct process_trailer_options trailer_opts; unsigned int nlines; } contents; struct { -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH 3/5] ref-filter.c: add trailer options to used_atom 2017-09-30 6:22 ` [PATCH 3/5] ref-filter.c: add trailer options to used_atom Taylor Blau @ 2017-09-30 7:10 ` Jeff King 0 siblings, 0 replies; 78+ messages in thread From: Jeff King @ 2017-09-30 7:10 UTC (permalink / raw) To: Taylor Blau; +Cc: git, gitster On Fri, Sep 29, 2017 at 11:22:36PM -0700, Taylor Blau wrote: > In preparation for "%(trailers)" to take trailer parsing arguments, use Jeff's > convenience structure for trailer processing options introduced in 8abc89800c. > > We will later populate this field from the arguments given to %(trailers), and > then use the trailer_opts instance to format ref trailers correctly using > `format_trailer_from_commit`. I think this should probably just be squashed in with the next patch, since this does nothing at all without adding a caller that uses it. > diff --git a/ref-filter.c b/ref-filter.c > index 467c0279c..84f14093c 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -82,6 +82,7 @@ static struct used_atom { > } remote_ref; > struct { > enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option; > + struct process_trailer_options trailer_opts; > unsigned int nlines; > } contents; This contents struct is odd. The used_atom struct can have many types, and has has a big union of type-specific data. And then we break down the "contents" type with a further enum, but don't actually put the type-specific data into a union (not just your patch, but already "nlines" is specific only to C_LINES). It's probably not worth caring about, though. The point of a union is to reduce the overall struct size, and here our type-specific data is fairly small. It would only change the overall struct size if it were larger than other parts of the union (and remote_ref, for example, is pretty big). -Peff PS As an aside, I find the whole %(contents:...) thing a bit unfortunate. I understand why the _implementation_ wants to group similar options together so that it can avoid parsing too much. But the user doesn't care about that. Just "%(trailers)" should be sufficient (as evidenced by the fact that we added it a separate alias). But none of that is new to your series. ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers 2017-09-30 6:22 [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Taylor Blau ` (2 preceding siblings ...) 2017-09-30 6:22 ` [PATCH 3/5] ref-filter.c: add trailer options to used_atom Taylor Blau @ 2017-09-30 6:22 ` Taylor Blau 2017-09-30 7:21 ` Jeff King 2017-10-01 9:00 ` Junio C Hamano 2017-09-30 6:22 ` [PATCH 5/5] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau ` (2 subsequent siblings) 6 siblings, 2 replies; 78+ messages in thread From: Taylor Blau @ 2017-09-30 6:22 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau In preparation to support additional sub-arguments given to the "%(trailers)" atom, use 'format_trailers_from_commit' in order to format trailers in the desired manner. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 6 +++++- ref-filter.c | 31 ++++++++++++++++++++--------- t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 03e187a10..b7325a25d 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -213,7 +213,11 @@ line is 'contents:body', where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'contents:trailers'. +are obtained as 'contents:trailers'. Non-trailer lines from the trailer block +can be omitted with 'trailers:only'. Whitespace-continuations can be removed +from trailers so that each trailer appears on a line by itself with its full +content with 'trailers:unfold'. Both can be used together as +'trailers:unfold,only'. For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). diff --git a/ref-filter.c b/ref-filter.c index 84f14093c..8573acbed 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -178,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg) static void trailers_atom_parser(struct used_atom *atom, const char *arg) { - if (arg) - die(_("%%(trailers) does not take arguments")); + struct string_list params = STRING_LIST_INIT_DUP; + int i; + + if (arg) { + string_list_split(¶ms, arg, ',', -1); + for (i = 0; i < params.nr; i++) { + const char *s = params.items[i].string; + if (!strcmp(s, "unfold")) + atom->u.contents.trailer_opts.unfold = 1; + else if (!strcmp(s, "only")) + atom->u.contents.trailer_opts.only_trailers = 1; + else + die(_("unknown %%(trailers) argument: %s"), s); + } + } atom->u.contents.option = C_TRAILERS; + string_list_clear(¶ms, 0); } static void contents_atom_parser(struct used_atom *atom, const char *arg) @@ -1035,7 +1049,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj name++; if (strcmp(name, "subject") && strcmp(name, "body") && - strcmp(name, "trailers") && + !starts_with(name, "trailers") && !starts_with(name, "contents")) continue; if (!subpos) @@ -1060,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines); v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_TRAILERS) { - struct trailer_info info; + struct strbuf s = STRBUF_INIT; - /* Search for trailer info */ - trailer_info_get(&info, subpos); - v->s = xmemdupz(info.trailer_start, - info.trailer_end - info.trailer_start); - trailer_info_release(&info); + /* Format the trailer info according to the trailer_opts given */ + format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts); + + v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_BARE) v->s = xstrdup(subpos); } diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 2a9fcf713..2bd0c5da7 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -597,6 +597,9 @@ Acked-by: A U Thor <author@example.com> EOF +unfold () { + perl -0pe 's/\n\s+/ /' +} test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && @@ -610,6 +613,43 @@ test_expect_success 'set up trailers for next test' ' EOF ' +test_expect_success '%(trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' + git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers 2017-09-30 6:22 ` [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers Taylor Blau @ 2017-09-30 7:21 ` Jeff King 2017-10-01 9:08 ` Junio C Hamano 2017-10-01 9:00 ` Junio C Hamano 1 sibling, 1 reply; 78+ messages in thread From: Jeff King @ 2017-09-30 7:21 UTC (permalink / raw) To: Taylor Blau; +Cc: git, gitster On Fri, Sep 29, 2017 at 11:22:37PM -0700, Taylor Blau wrote: > In preparation to support additional sub-arguments given to the "%(trailers)" > atom, use 'format_trailers_from_commit' in order to format trailers in the > desired manner. This isn't just in preparation, is it? It looks like the options are here (which I think is fine, but the commit message probably needs updated). > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > Documentation/git-for-each-ref.txt | 6 +++++- > ref-filter.c | 31 ++++++++++++++++++++--------- > t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 67 insertions(+), 10 deletions(-) This patch didn't apply for me on top of the others. I get: Applying: ref-filter.c: use trailer_opts to format trailers error: patch failed: ref-filter.c:178 error: ref-filter.c: patch does not apply Patch failed at 0004 ref-filter.c: use trailer_opts to format trailers And then with "am -3": Applying: ref-filter.c: use trailer_opts to format trailers error: sha1 information is lacking or useless (ref-filter.c). error: could not build fake ancestor Patch failed at 0004 ref-filter.c: use trailer_opts to format trailers Did it get corrupted in transit, or did you hand-edit it? > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > index 03e187a10..b7325a25d 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -213,7 +213,11 @@ line is 'contents:body', where body is all of the lines after the first > blank line. The optional GPG signature is `contents:signature`. The > first `N` lines of the message is obtained using `contents:lines=N`. > Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] > -are obtained as 'contents:trailers'. > +are obtained as 'contents:trailers'. Non-trailer lines from the trailer block > +can be omitted with 'trailers:only'. Whitespace-continuations can be removed > +from trailers so that each trailer appears on a line by itself with its full > +content with 'trailers:unfold'. Both can be used together as > +'trailers:unfold,only'. I know you copied the single-quote formatting from the existing line, but this may be a good opportunity to switch to backticks, which is what we usually prefer these days for literal phrases. > diff --git a/ref-filter.c b/ref-filter.c > index 84f14093c..8573acbed 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -178,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg) > > static void trailers_atom_parser(struct used_atom *atom, const char *arg) > { > - if (arg) > - die(_("%%(trailers) does not take arguments")); > + struct string_list params = STRING_LIST_INIT_DUP; > + int i; > + > + if (arg) { > + string_list_split(¶ms, arg, ',', -1); > + for (i = 0; i < params.nr; i++) { > + const char *s = params.items[i].string; > + if (!strcmp(s, "unfold")) > + atom->u.contents.trailer_opts.unfold = 1; > + else if (!strcmp(s, "only")) > + atom->u.contents.trailer_opts.only_trailers = 1; > + else > + die(_("unknown %%(trailers) argument: %s"), s); > + } > + } > atom->u.contents.option = C_TRAILERS; > + string_list_clear(¶ms, 0); > } This looks good (and so much nicer than the contortions from pretty.c). The error behavior matches what we currently do for %(align), which makes sense. The trailer_opts should be zero-initialized to start with due to us calling memset on the whole used_atom struct. > static void contents_atom_parser(struct used_atom *atom, const char *arg) > @@ -1035,7 +1049,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj > name++; > if (strcmp(name, "subject") && > strcmp(name, "body") && > - strcmp(name, "trailers") && > + !starts_with(name, "trailers") && > !starts_with(name, "contents")) > continue; > if (!subpos) > @@ -1060,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj > append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines); > v->s = strbuf_detach(&s, NULL); > } else if (atom->u.contents.option == C_TRAILERS) { > - struct trailer_info info; > + struct strbuf s = STRBUF_INIT; > > - /* Search for trailer info */ > - trailer_info_get(&info, subpos); > - v->s = xmemdupz(info.trailer_start, > - info.trailer_end - info.trailer_start); > - trailer_info_release(&info); > + /* Format the trailer info according to the trailer_opts given */ > + format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts); > + > + v->s = strbuf_detach(&s, NULL); And this all looks straightforward and correct. > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index 2a9fcf713..2bd0c5da7 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh The tests are basically an adaptation of the ones from 58311c66fd (pretty: support normalization options for %(trailers), 2017-08-15), which make sense. One thing I did notice: > @@ -610,6 +613,43 @@ test_expect_success 'set up trailers for next test' ' > EOF > ' > > +test_expect_success '%(trailers:unfold) unfolds trailers' ' > + git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual && > + { > + unfold <trailers > + echo > + } >expect && > + test_cmp expect actual > +' This looks like two-space indentation, when it should be a tab. > +test_expect_success '%(trailers:only) shows only "key: value" trailers' ' > + git for-each-ref --format="%(trailers:only)" refs/heads/master >actual && > + { > + grep -v patch.description <trailers && > + echo > + } >expect && > + test_cmp expect actual > +' > + > +test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' > + git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual && > + git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse && > + test_cmp actual reverse && > + { > + grep -v patch.description <trailers | unfold && > + echo > + } >expect && > + test_cmp expect actual > +' These ones are tabs. GOod. > +test_expect_success '%(trailers) rejects unknown trailers arguments' ' > + cat >expect <<-EOF && > + fatal: unknown %(trailers) argument: unsupported > + EOF > + test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && > + test_cmp expect actual > +' But this one is mixed. :) -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers 2017-09-30 7:21 ` Jeff King @ 2017-10-01 9:08 ` Junio C Hamano 0 siblings, 0 replies; 78+ messages in thread From: Junio C Hamano @ 2017-10-01 9:08 UTC (permalink / raw) To: Jeff King; +Cc: Taylor Blau, git Jeff King <peff@peff.net> writes: > This patch didn't apply for me on top of the others. I get: > > Applying: ref-filter.c: use trailer_opts to format trailers > error: patch failed: ref-filter.c:178 > error: ref-filter.c: patch does not apply > Patch failed at 0004 ref-filter.c: use trailer_opts to format trailers > > And then with "am -3": > > Applying: ref-filter.c: use trailer_opts to format trailers > error: sha1 information is lacking or useless (ref-filter.c). > error: could not build fake ancestor > Patch failed at 0004 ref-filter.c: use trailer_opts to format trailers > > Did it get corrupted in transit, or did you hand-edit it? And the corresponding patch in v3 seems to have the same issue. ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers 2017-09-30 6:22 ` [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-09-30 7:21 ` Jeff King @ 2017-10-01 9:00 ` Junio C Hamano 2017-10-02 5:05 ` Jeff King 1 sibling, 1 reply; 78+ messages in thread From: Junio C Hamano @ 2017-10-01 9:00 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Taylor Blau <me@ttaylorr.com> writes: > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index 2a9fcf713..2bd0c5da7 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -597,6 +597,9 @@ Acked-by: A U Thor > <author@example.com> > EOF > > +unfold () { > + perl -0pe 's/\n\s+/ /' > +} For the purpose of the current shape of the test, the above might be sufficient, but the lack of "/g" at the end means that the script will happily stop after unfolding just one line, which probably is not what you intended. ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers 2017-10-01 9:00 ` Junio C Hamano @ 2017-10-02 5:05 ` Jeff King 2017-10-02 5:11 ` Taylor Blau 0 siblings, 1 reply; 78+ messages in thread From: Jeff King @ 2017-10-02 5:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git On Sun, Oct 01, 2017 at 06:00:25PM +0900, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > > index 2a9fcf713..2bd0c5da7 100755 > > --- a/t/t6300-for-each-ref.sh > > +++ b/t/t6300-for-each-ref.sh > > @@ -597,6 +597,9 @@ Acked-by: A U Thor > > <author@example.com> > > EOF > > > > +unfold () { > > + perl -0pe 's/\n\s+/ /' > > +} > > For the purpose of the current shape of the test, the above might be > sufficient, but the lack of "/g" at the end means that the script > will happily stop after unfolding just one line, which probably is > not what you intended. This is indirectly my fault, since this was copied from my t4205 version. It might be worth fixing while we're thinking about it, as it's a potential trap for future changes. -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers 2017-10-02 5:05 ` Jeff King @ 2017-10-02 5:11 ` Taylor Blau 0 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:11 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git On Mon, Oct 02, 2017 at 01:05:07AM -0400, Jeff King wrote: > On Sun, Oct 01, 2017 at 06:00:25PM +0900, Junio C Hamano wrote: > > > Taylor Blau <me@ttaylorr.com> writes: > > > > > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > > > index 2a9fcf713..2bd0c5da7 100755 > > > --- a/t/t6300-for-each-ref.sh > > > +++ b/t/t6300-for-each-ref.sh > > > @@ -597,6 +597,9 @@ Acked-by: A U Thor > > > <author@example.com> > > > EOF > > > > > > +unfold () { > > > + perl -0pe 's/\n\s+/ /' > > > +} > > > > For the purpose of the current shape of the test, the above might be > > sufficient, but the lack of "/g" at the end means that the script > > will happily stop after unfolding just one line, which probably is > > not what you intended. > > This is indirectly my fault, since this was copied from my t4205 > version. It might be worth fixing while we're thinking about it, as it's > a potential trap for future changes. I agree. I'll include a fix for this as an additional commit in v6. -- - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH 5/5] ref-filter.c: parse trailers arguments with %(contents) atom 2017-09-30 6:22 [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Taylor Blau ` (3 preceding siblings ...) 2017-09-30 6:22 ` [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers Taylor Blau @ 2017-09-30 6:22 ` Taylor Blau 2017-09-30 7:36 ` Jeff King 2017-09-30 7:38 ` [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Jeff King 2017-09-30 18:41 ` [PATCH v2 0/6] " Taylor Blau 6 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-09-30 6:22 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The %(contents) atom takes a contents "field" as its argument. Since "trailers" is one of those fields, extend contents_atom_parser to parse "trailers"'s arguments when used through "%(contents)", like: %(contents:trailers:unfold,only) A caveat: trailers_atom_parser expects NULL when no arguments are given (see: `parse_ref_filter_atom`). To simulate this behavior without teaching trailers_atom_parser to accept strings with length zero, conditionally pass NULL to trailers_atom_parser if the arguments portion of the argument to %(contents) is empty. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 9 +++++---- ref-filter.c | 6 ++++-- t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index b7325a25d..0aaac8af9 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -214,10 +214,11 @@ blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] are obtained as 'contents:trailers'. Non-trailer lines from the trailer block -can be omitted with 'trailers:only'. Whitespace-continuations can be removed -from trailers so that each trailer appears on a line by itself with its full -content with 'trailers:unfold'. Both can be used together as -'trailers:unfold,only'. +can be omitted with 'trailers:only', or 'contents:trailers:only'. +Whitespace-continuations can be removed from trailers so that each trailer +appears on a line by itself with its full content with 'trailers:unfold' or +'contents:trailers:unfold'. Both can be used together as 'trailers:unfold,only', +or 'contents:trailers:unfold,only'. For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). diff --git a/ref-filter.c b/ref-filter.c index 8573acbed..a8d4a52bd 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -207,8 +207,10 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg) atom->u.contents.option = C_SIG; else if (!strcmp(arg, "subject")) atom->u.contents.option = C_SUB; - else if (!strcmp(arg, "trailers")) - atom->u.contents.option = C_TRAILERS; + else if (skip_prefix(arg, "trailers", &arg)) { + skip_prefix(arg, ":", &arg); + trailers_atom_parser(atom, strlen(arg) ? arg : NULL); + } else if (skip_prefix(arg, "lines=", &arg)) { atom->u.contents.option = C_LINES; if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 2bd0c5da7..d9b71589f 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' ' + git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + test_expect_success '%(trailers) rejects unknown trailers arguments' ' cat >expect <<-EOF && fatal: unknown %(trailers) argument: unsupported @@ -650,6 +679,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH 5/5] ref-filter.c: parse trailers arguments with %(contents) atom 2017-09-30 6:22 ` [PATCH 5/5] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau @ 2017-09-30 7:36 ` Jeff King 0 siblings, 0 replies; 78+ messages in thread From: Jeff King @ 2017-09-30 7:36 UTC (permalink / raw) To: Taylor Blau; +Cc: git, gitster On Fri, Sep 29, 2017 at 11:22:38PM -0700, Taylor Blau wrote: > The %(contents) atom takes a contents "field" as its argument. Since "trailers" > is one of those fields, extend contents_atom_parser to parse "trailers"'s > arguments when used through "%(contents)", like: > > %(contents:trailers:unfold,only) > > A caveat: trailers_atom_parser expects NULL when no arguments are given (see: > `parse_ref_filter_atom`). To simulate this behavior without teaching > trailers_atom_parser to accept strings with length zero, conditionally pass > NULL to trailers_atom_parser if the arguments portion of the argument to > %(contents) is empty. Yeah, this is a weird effect of trailers_atom_parser() doing double-duty to parse both "%(contents:trailers)" and "%(trailers)". Though I think trailers_atom_parser() does do the sensible thing with an empty string (there are no options, so nothing to parse). I.e., I'd expect the same thing out of: %(trailers:) and %(trailers) even though one gets a NULL "arg" field and the other gets an empty string. > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > index b7325a25d..0aaac8af9 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -214,10 +214,11 @@ blank line. The optional GPG signature is `contents:signature`. The > first `N` lines of the message is obtained using `contents:lines=N`. > Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] > are obtained as 'contents:trailers'. Non-trailer lines from the trailer block > -can be omitted with 'trailers:only'. Whitespace-continuations can be removed > -from trailers so that each trailer appears on a line by itself with its full > -content with 'trailers:unfold'. Both can be used together as > -'trailers:unfold,only'. > +can be omitted with 'trailers:only', or 'contents:trailers:only'. > +Whitespace-continuations can be removed from trailers so that each trailer > +appears on a line by itself with its full content with 'trailers:unfold' or > +'contents:trailers:unfold'. Both can be used together as 'trailers:unfold,only', > +or 'contents:trailers:unfold,only'. Rather than enumerate each, we might do better to just say explicitly "contents:trailers" and "trailers" are aliases of one another. It looks like we don't even document %(trailers) at all here. I'd actually be in favor of just declaring %(trailers) the official spelling, and calling "%(contents:trailers)" a historical alias. > diff --git a/ref-filter.c b/ref-filter.c > index 8573acbed..a8d4a52bd 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -207,8 +207,10 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg) > atom->u.contents.option = C_SIG; > else if (!strcmp(arg, "subject")) > atom->u.contents.option = C_SUB; > - else if (!strcmp(arg, "trailers")) > - atom->u.contents.option = C_TRAILERS; > + else if (skip_prefix(arg, "trailers", &arg)) { > + skip_prefix(arg, ":", &arg); > + trailers_atom_parser(atom, strlen(arg) ? arg : NULL); > + } We usually spell "is this an empty string?" as "*arg" rather than calling strlen(). However, I'm not sure we need to check. As I said above, I think trailers_atom_parser() does the sensible thing with an empty string. And if we _did_ want to distinguish between %(contents:trailers:) and %(contents:trailers) then I don't think this quite does it. It passes NULL for both cases. So if we really want to emulate how parse_ref_filter_atom calls it, we'd want: if (!skip_prefix(arg, ":", &arg)) arg = NULL; trailers_atom_parser(atom, arg); > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index 2bd0c5da7..d9b71589f 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' > test_cmp expect actual > ' > > +test_expect_success '%(contents:trailers:unfold) unfolds trailers' ' > + git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual && > + { This has the same spaces/tabs thing going on as the previous commit. -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) 2017-09-30 6:22 [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Taylor Blau ` (4 preceding siblings ...) 2017-09-30 6:22 ` [PATCH 5/5] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau @ 2017-09-30 7:38 ` Jeff King 2017-09-30 18:41 ` [PATCH v2 0/6] " Taylor Blau 6 siblings, 0 replies; 78+ messages in thread From: Jeff King @ 2017-09-30 7:38 UTC (permalink / raw) To: Taylor Blau; +Cc: git, gitster On Fri, Sep 29, 2017 at 11:22:33PM -0700, Taylor Blau wrote: > Attached is a patch to extend Peff's recent work of adding parsing options to > "--pretty=%(trailers)" by supporting those same options in git-for-each-ref(1). Thanks for working on this. The direction and general sketch of the implementation look right to me. I noted a few problems inline, so this isn't quite right to be picked up, but I think a v2 should get us pretty close, if not all the way there. -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1) 2017-09-30 6:22 [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Taylor Blau ` (5 preceding siblings ...) 2017-09-30 7:38 ` [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Jeff King @ 2017-09-30 18:41 ` Taylor Blau 2017-09-30 18:46 ` [PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-01 0:06 ` [PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 6 siblings, 2 replies; 78+ messages in thread From: Taylor Blau @ 2017-09-30 18:41 UTC (permalink / raw) To: git; +Cc: gitster, peff Hi, Attached is v2 of my patch-set "Support %(trailers) arguments in for-each-ref(1)". It includes the following changes since v1: * Accept "empty-sub-argument" lists, like %(contents:trailers:). * Fix incorrect tabs/spaces formatting in t6300. * Modern-ize code fencing in Documentation/git-for-each-ref.txt * Squash commit adding trailer_opts to used_atom structure. --- [1/6]: pretty.c: delimit "%(trailers)" arguments with "," [2/6]: t6300: refactor %(trailers) tests [3/6]: doc: 'trailers' is the preferred way to format trailers [4/6]: doc: use modern "`"-style code fencing [5/6]: ref-filter.c: use trailer_opts to format trailers [6/6]: ref-filter.c: parse trailers arguments with %(contents) atom Thanks in advance :-). -- - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with "," 2017-09-30 18:41 ` [PATCH v2 0/6] " Taylor Blau @ 2017-09-30 18:46 ` Taylor Blau 2017-09-30 18:46 ` [PATCH v2 2/6] t6300: refactor %(trailers) tests Taylor Blau ` (4 more replies) 2017-10-01 0:06 ` [PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 1 sibling, 5 replies; 78+ messages in thread From: Taylor Blau @ 2017-09-30 18:46 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau In preparation for adding consistent "%(trailers)" atom options to `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in pretty.c to separate sub-arguments with a ",", instead of a ":". Multiple sub-arguments are given either as "%(trailers:unfold,only)" or "%(trailers:only,unfold)". This change disambiguates between "top-level" arguments, and arguments given to the trailers atom itself. It is consistent with the behavior of "%(upstream)" and "%(push)" atoms. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pretty.c | 34 +++++++++++++++++++++++++++++----- t/t4205-log-pretty-formats.sh | 4 ++-- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/pretty.c b/pretty.c index 0e23fe3c0..eeb51746c 100644 --- a/pretty.c +++ b/pretty.c @@ -1036,6 +1036,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) +{ + const char *p; + if (!(skip_prefix(to_parse, candidate, &p))) + return 0; + if (*p == ',') { + *end = p + 1; + return 1; + } + if (*p == ')') { + *end = p; + return 1; + } + return 0; +} + + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1265,11 +1284,16 @@ 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; - while (*arg == ':') { - if (skip_prefix(arg, ":only", &arg)) - opts.only_trailers = 1; - else if (skip_prefix(arg, ":unfold", &arg)) - opts.unfold = 1; + 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 + break; + } } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index ec5f53010..977472f53 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' ' ' test_expect_success ':only and :unfold work together' ' - git log --no-walk --pretty="%(trailers:only:unfold)" >actual && - git log --no-walk --pretty="%(trailers:unfold:only)" >reverse && + git log --no-walk --pretty="%(trailers:only,unfold)" >actual && + git log --no-walk --pretty="%(trailers:unfold,only)" >reverse && test_cmp actual reverse && { grep -v patch.description <trailers | unfold && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v2 2/6] t6300: refactor %(trailers) tests 2017-09-30 18:46 ` [PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-09-30 18:46 ` Taylor Blau 2017-09-30 18:46 ` [PATCH v2 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau ` (3 subsequent siblings) 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-09-30 18:46 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau We currently have one test for %(trailers) in `git-for-each-ref(1)`, through "%(contents:trailers)". In preparation for more, let's add a few things: - Move the commit creation step to its own test so that it can be re-used. - Add a non-trailer to the commit's trailers to test that non-trailers aren't shown using "%(trailers:only)". - Add a multi-line trailer to ensure that trailers are unfolded correctly using "%(trailers:unfold)". Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t6300-for-each-ref.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 834a9ed16..2a9fcf713 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -592,18 +592,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' ' cat >trailers <<EOF Reviewed-by: A U Thor <author@example.com> Signed-off-by: A U Thor <author@example.com> +[ v2 updated patch description ] +Acked-by: A U Thor + <author@example.com> EOF -test_expect_success 'basic atom: head contents:trailers' ' + +test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && git add two && - git commit -F - <<-EOF && + git commit -F - <<-EOF trailers: this commit message has trailers Some message contents $(cat trailers) EOF +' + +test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && # git for-each-ref ends with a blank line -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v2 3/6] doc: 'trailers' is the preferred way to format trailers 2017-09-30 18:46 ` [PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-09-30 18:46 ` [PATCH v2 2/6] t6300: refactor %(trailers) tests Taylor Blau @ 2017-09-30 18:46 ` Taylor Blau 2017-09-30 18:46 ` [PATCH v2 4/6] doc: use modern "`"-style code fencing Taylor Blau ` (2 subsequent siblings) 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-09-30 18:46 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The documentation makes reference to 'contents:trailers' as an example to dig the trailers out of a commit. 'trailers' is an unmentioned alternative, which is treated as an alias of 'contents:trailers'. Since 'trailers' is easier to type, prefer that as the designated way to dig out trailers information. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 03e187a10..6b38d9a22 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -213,7 +213,8 @@ line is 'contents:body', where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'contents:trailers'. +are obtained as 'trailers' (or by using the historical alias +'contents:trailers'). For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v2 4/6] doc: use modern "`"-style code fencing 2017-09-30 18:46 ` [PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-09-30 18:46 ` [PATCH v2 2/6] t6300: refactor %(trailers) tests Taylor Blau 2017-09-30 18:46 ` [PATCH v2 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau @ 2017-09-30 18:46 ` Taylor Blau 2017-09-30 18:46 ` [PATCH v2 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-09-30 18:46 ` [PATCH v2 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-09-30 18:46 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau "'"- (single-quote) styled code fencing is no longer considered modern within the "Documentation/" subtree. In preparation for adding additional information to this section of git-for-each-ref(1)'s documentation, update old-style code fencing to use "`"-style fencing instead. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 6b38d9a22..b6492820b 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -209,12 +209,12 @@ and `date` to extract the named component. The complete message in a commit and tag object is `contents`. Its first line is `contents:subject`, where subject is the concatenation of all lines of the commit message up to the first blank line. The next -line is 'contents:body', where body is all of the lines after the first +line is `contents:body`, where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'trailers' (or by using the historical alias -'contents:trailers'). +are obtained as `trailers` (or by using the historical alias +`contents:trailers`). For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v2 5/6] ref-filter.c: use trailer_opts to format trailers 2017-09-30 18:46 ` [PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (2 preceding siblings ...) 2017-09-30 18:46 ` [PATCH v2 4/6] doc: use modern "`"-style code fencing Taylor Blau @ 2017-09-30 18:46 ` Taylor Blau 2017-09-30 18:46 ` [PATCH v2 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-09-30 18:46 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau Fill trailer_opts with "unfold" and "only" to match the sub-arguments given to the "%(trailers)" atom. Then, let's use the filled trailer_opts instance with 'format_trailers_from_commit' in order to format trailers in the desired manner. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 5 ++++- ref-filter.c | 32 +++++++++++++++++++++--------- t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index b6492820b..a1d964182 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -214,7 +214,10 @@ blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] are obtained as `trailers` (or by using the historical alias -`contents:trailers`). +`contents:trailers`). Non-trailer lines from the trailer block can be omitted +with `trailers:only`. Whitespace-continuations can be removed from trailers so +that each trailer appears on a line by itself with its full content with +`trailers:unfold`. Both can be used together as `trailers:unfold,only`. For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). diff --git a/ref-filter.c b/ref-filter.c index 467c0279c..8573acbed 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -82,6 +82,7 @@ static struct used_atom { } remote_ref; struct { enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option; + struct process_trailer_options trailer_opts; unsigned int nlines; } contents; struct { @@ -177,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg) static void trailers_atom_parser(struct used_atom *atom, const char *arg) { - if (arg) - die(_("%%(trailers) does not take arguments")); + struct string_list params = STRING_LIST_INIT_DUP; + int i; + + if (arg) { + string_list_split(¶ms, arg, ',', -1); + for (i = 0; i < params.nr; i++) { + const char *s = params.items[i].string; + if (!strcmp(s, "unfold")) + atom->u.contents.trailer_opts.unfold = 1; + else if (!strcmp(s, "only")) + atom->u.contents.trailer_opts.only_trailers = 1; + else + die(_("unknown %%(trailers) argument: %s"), s); + } + } atom->u.contents.option = C_TRAILERS; + string_list_clear(¶ms, 0); } static void contents_atom_parser(struct used_atom *atom, const char *arg) @@ -1034,7 +1049,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj name++; if (strcmp(name, "subject") && strcmp(name, "body") && - strcmp(name, "trailers") && + !starts_with(name, "trailers") && !starts_with(name, "contents")) continue; if (!subpos) @@ -1059,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines); v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_TRAILERS) { - struct trailer_info info; + struct strbuf s = STRBUF_INIT; - /* Search for trailer info */ - trailer_info_get(&info, subpos); - v->s = xmemdupz(info.trailer_start, - info.trailer_end - info.trailer_start); - trailer_info_release(&info); + /* Format the trailer info according to the trailer_opts given */ + format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts); + + v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_BARE) v->s = xstrdup(subpos); } diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 2a9fcf713..6538e7b90 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -597,6 +597,9 @@ Acked-by: A U Thor <author@example.com> EOF +unfold () { + perl -0pe 's/\n\s+/ /' +} test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && @@ -610,6 +613,43 @@ test_expect_success 'set up trailers for next test' ' EOF ' +test_expect_success '%(trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' + git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v2 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-09-30 18:46 ` [PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (3 preceding siblings ...) 2017-09-30 18:46 ` [PATCH v2 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau @ 2017-09-30 18:46 ` Taylor Blau 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-09-30 18:46 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The %(contents) atom takes a contents "field" as its argument. Since "trailers" is one of those fields, extend contents_atom_parser to parse "trailers"'s arguments when used through "%(contents)", like: %(contents:trailers:unfold,only) A caveat: trailers_atom_parser expects NULL when no arguments are given (see: `parse_ref_filter_atom`). To simulate this behavior without teaching trailers_atom_parser to accept strings with length zero, conditionally pass NULL to trailers_atom_parser if the arguments portion of the argument to %(contents) is empty. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- ref-filter.c | 6 ++++-- t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 8573acbed..f5bde79f3 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -207,8 +207,10 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg) atom->u.contents.option = C_SIG; else if (!strcmp(arg, "subject")) atom->u.contents.option = C_SUB; - else if (!strcmp(arg, "trailers")) - atom->u.contents.option = C_TRAILERS; + else if (skip_prefix(arg, "trailers", &arg)) { + skip_prefix(arg, ":", &arg); + trailers_atom_parser(atom, arg); + } else if (skip_prefix(arg, "lines=", &arg)) { atom->u.contents.option = C_LINES; if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 6538e7b90..8c960ec24 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' ' + git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + test_expect_success '%(trailers) rejects unknown trailers arguments' ' cat >expect <<-EOF && fatal: unknown %(trailers) argument: unsupported @@ -650,6 +679,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1) 2017-09-30 18:41 ` [PATCH v2 0/6] " Taylor Blau 2017-09-30 18:46 ` [PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-10-01 0:06 ` Taylor Blau 2017-10-01 0:10 ` [PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-01 16:17 ` [PATCH v4 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 1 sibling, 2 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-01 0:06 UTC (permalink / raw) To: git Hi, Attached is v3 of my patch-set "Support %(trailers) arguments in for-each-ref(1)". It includes all of the changes in V2: > * Accept "empty-sub-argument" lists, like %(contents:trailers:). > > * Fix incorrect tabs/spaces formatting in t6300. > > * Modern-ize code fencing in Documentation/git-for-each-ref.txt > > * Squash commit adding trailer_opts to used_atom structure. As well as fixing: * Incorrect indentation in pretty.c (specifically: "pretty.c: delimit "%(trailers)" arguments with ","") * Issues with --format="%(contents:trailers:)", where trailers_atom_parser did not correctly handle empty string ("") and NULL's the same way. Thanks in advance :-). -- - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with "," 2017-10-01 0:06 ` [PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau @ 2017-10-01 0:10 ` Taylor Blau 2017-10-01 0:10 ` [PATCH v3 2/6] t6300: refactor %(trailers) tests Taylor Blau ` (4 more replies) 2017-10-01 16:17 ` [PATCH v4 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 1 sibling, 5 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-01 0:10 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau In preparation for adding consistent "%(trailers)" atom options to `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in pretty.c to separate sub-arguments with a ",", instead of a ":". Multiple sub-arguments are given either as "%(trailers:unfold,only)" or "%(trailers:only,unfold)". This change disambiguates between "top-level" arguments, and arguments given to the trailers atom itself. It is consistent with the behavior of "%(upstream)" and "%(push)" atoms. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pretty.c | 34 +++++++++++++++++++++++++++++----- t/t4205-log-pretty-formats.sh | 4 ++-- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/pretty.c b/pretty.c index 0e23fe3c0..9f8d75a84 100644 --- a/pretty.c +++ b/pretty.c @@ -1036,6 +1036,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) +{ + const char *p; + if (!(skip_prefix(to_parse, candidate, &p))) + return 0; + if (*p == ',') { + *end = p + 1; + return 1; + } + if (*p == ')') { + *end = p; + return 1; + } + return 0; +} + + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1265,11 +1284,16 @@ 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; - while (*arg == ':') { - if (skip_prefix(arg, ":only", &arg)) - opts.only_trailers = 1; - else if (skip_prefix(arg, ":unfold", &arg)) - opts.unfold = 1; + 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 + break; + } } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index ec5f53010..977472f53 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' ' ' test_expect_success ':only and :unfold work together' ' - git log --no-walk --pretty="%(trailers:only:unfold)" >actual && - git log --no-walk --pretty="%(trailers:unfold:only)" >reverse && + git log --no-walk --pretty="%(trailers:only,unfold)" >actual && + git log --no-walk --pretty="%(trailers:unfold,only)" >reverse && test_cmp actual reverse && { grep -v patch.description <trailers | unfold && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v3 2/6] t6300: refactor %(trailers) tests 2017-10-01 0:10 ` [PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-10-01 0:10 ` Taylor Blau 2017-10-01 0:10 ` [PATCH v3 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau ` (3 subsequent siblings) 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-01 0:10 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau We currently have one test for %(trailers) in `git-for-each-ref(1)`, through "%(contents:trailers)". In preparation for more, let's add a few things: - Move the commit creation step to its own test so that it can be re-used. - Add a non-trailer to the commit's trailers to test that non-trailers aren't shown using "%(trailers:only)". - Add a multi-line trailer to ensure that trailers are unfolded correctly using "%(trailers:unfold)". Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t6300-for-each-ref.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 834a9ed16..2a9fcf713 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -592,18 +592,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' ' cat >trailers <<EOF Reviewed-by: A U Thor <author@example.com> Signed-off-by: A U Thor <author@example.com> +[ v2 updated patch description ] +Acked-by: A U Thor + <author@example.com> EOF -test_expect_success 'basic atom: head contents:trailers' ' + +test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && git add two && - git commit -F - <<-EOF && + git commit -F - <<-EOF trailers: this commit message has trailers Some message contents $(cat trailers) EOF +' + +test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && # git for-each-ref ends with a blank line -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v3 3/6] doc: 'trailers' is the preferred way to format trailers 2017-10-01 0:10 ` [PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-01 0:10 ` [PATCH v3 2/6] t6300: refactor %(trailers) tests Taylor Blau @ 2017-10-01 0:10 ` Taylor Blau 2017-10-01 0:10 ` [PATCH v3 4/6] doc: use modern "`"-style code fencing Taylor Blau ` (2 subsequent siblings) 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-01 0:10 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The documentation makes reference to 'contents:trailers' as an example to dig the trailers out of a commit. 'trailers' is an unmentioned alternative, which is treated as an alias of 'contents:trailers'. Since 'trailers' is easier to type, prefer that as the designated way to dig out trailers information. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 03e187a10..6b38d9a22 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -213,7 +213,8 @@ line is 'contents:body', where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'contents:trailers'. +are obtained as 'trailers' (or by using the historical alias +'contents:trailers'). For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v3 4/6] doc: use modern "`"-style code fencing 2017-10-01 0:10 ` [PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-01 0:10 ` [PATCH v3 2/6] t6300: refactor %(trailers) tests Taylor Blau 2017-10-01 0:10 ` [PATCH v3 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau @ 2017-10-01 0:10 ` Taylor Blau 2017-10-01 0:10 ` [PATCH v3 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-10-01 0:10 ` [PATCH v3 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-01 0:10 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau "'"- (single-quote) styled code fencing is no longer considered modern within the "Documentation/" subtree. In preparation for adding additional information to this section of git-for-each-ref(1)'s documentation, update old-style code fencing to use "`"-style fencing instead. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 6b38d9a22..b6492820b 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -209,12 +209,12 @@ and `date` to extract the named component. The complete message in a commit and tag object is `contents`. Its first line is `contents:subject`, where subject is the concatenation of all lines of the commit message up to the first blank line. The next -line is 'contents:body', where body is all of the lines after the first +line is `contents:body`, where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'trailers' (or by using the historical alias -'contents:trailers'). +are obtained as `trailers` (or by using the historical alias +`contents:trailers`). For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v3 5/6] ref-filter.c: use trailer_opts to format trailers 2017-10-01 0:10 ` [PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (2 preceding siblings ...) 2017-10-01 0:10 ` [PATCH v3 4/6] doc: use modern "`"-style code fencing Taylor Blau @ 2017-10-01 0:10 ` Taylor Blau 2017-10-01 0:10 ` [PATCH v3 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-01 0:10 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau Fill trailer_opts with "unfold" and "only" to match the sub-arguments given to the "%(trailers)" atom. Then, let's use the filled trailer_opts instance with 'format_trailers_from_commit' in order to format trailers in the desired manner. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 5 ++++- ref-filter.c | 32 +++++++++++++++++++++--------- t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index b6492820b..a1d964182 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -214,7 +214,10 @@ blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] are obtained as `trailers` (or by using the historical alias -`contents:trailers`). +`contents:trailers`). Non-trailer lines from the trailer block can be omitted +with `trailers:only`. Whitespace-continuations can be removed from trailers so +that each trailer appears on a line by itself with its full content with +`trailers:unfold`. Both can be used together as `trailers:unfold,only`. For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). diff --git a/ref-filter.c b/ref-filter.c index 467c0279c..8573acbed 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -82,6 +82,7 @@ static struct used_atom { } remote_ref; struct { enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option; + struct process_trailer_options trailer_opts; unsigned int nlines; } contents; struct { @@ -177,9 +178,23 @@ static void subject_atom_parser(struct used_atom *atom, const char *arg) static void trailers_atom_parser(struct used_atom *atom, const char *arg) { - if (arg) - die(_("%%(trailers) does not take arguments")); + struct string_list params = STRING_LIST_INIT_DUP; + int i; + + if (arg) { + string_list_split(¶ms, arg, ',', -1); + for (i = 0; i < params.nr; i++) { + const char *s = params.items[i].string; + if (!strcmp(s, "unfold")) + atom->u.contents.trailer_opts.unfold = 1; + else if (!strcmp(s, "only")) + atom->u.contents.trailer_opts.only_trailers = 1; + else + die(_("unknown %%(trailers) argument: %s"), s); + } + } atom->u.contents.option = C_TRAILERS; + string_list_clear(¶ms, 0); } static void contents_atom_parser(struct used_atom *atom, const char *arg) @@ -1034,7 +1049,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj name++; if (strcmp(name, "subject") && strcmp(name, "body") && - strcmp(name, "trailers") && + !starts_with(name, "trailers") && !starts_with(name, "contents")) continue; if (!subpos) @@ -1059,13 +1074,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines); v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_TRAILERS) { - struct trailer_info info; + struct strbuf s = STRBUF_INIT; - /* Search for trailer info */ - trailer_info_get(&info, subpos); - v->s = xmemdupz(info.trailer_start, - info.trailer_end - info.trailer_start); - trailer_info_release(&info); + /* Format the trailer info according to the trailer_opts given */ + format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts); + + v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_BARE) v->s = xstrdup(subpos); } diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 2a9fcf713..6538e7b90 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -597,6 +597,9 @@ Acked-by: A U Thor <author@example.com> EOF +unfold () { + perl -0pe 's/\n\s+/ /' +} test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && @@ -610,6 +613,43 @@ test_expect_success 'set up trailers for next test' ' EOF ' +test_expect_success '%(trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' + git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v3 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-01 0:10 ` [PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (3 preceding siblings ...) 2017-10-01 0:10 ` [PATCH v3 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau @ 2017-10-01 0:10 ` Taylor Blau 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-01 0:10 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The %(contents) atom takes a contents "field" as its argument. Since "trailers" is one of those fields, extend contents_atom_parser to parse "trailers"'s arguments when used through "%(contents)", like: %(contents:trailers:unfold,only) A caveat: trailers_atom_parser expects NULL when no arguments are given (see: `parse_ref_filter_atom`). To simulate this behavior without teaching trailers_atom_parser to accept strings with length zero, conditionally pass NULL to trailers_atom_parser if the arguments portion of the argument to %(contents) is empty. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- ref-filter.c | 6 ++++-- t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 8573acbed..b5747e969 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -207,8 +207,10 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg) atom->u.contents.option = C_SIG; else if (!strcmp(arg, "subject")) atom->u.contents.option = C_SUB; - else if (!strcmp(arg, "trailers")) - atom->u.contents.option = C_TRAILERS; + else if (skip_prefix(arg, "trailers", &arg)) { + skip_prefix(arg, ":", &arg); + trailers_atom_parser(atom, *arg ? NULL : arg); + } else if (skip_prefix(arg, "lines=", &arg)) { atom->u.contents.option = C_LINES; if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 6538e7b90..8c960ec24 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -642,6 +642,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' ' + git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + test_expect_success '%(trailers) rejects unknown trailers arguments' ' cat >expect <<-EOF && fatal: unknown %(trailers) argument: unsupported @@ -650,6 +679,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH v4 0/6] Support %(trailers) arguments in for-each-ref(1) 2017-10-01 0:06 ` [PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-10-01 0:10 ` [PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-10-01 16:17 ` Taylor Blau 2017-10-01 16:18 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-02 0:31 ` [PATCH v5 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 1 sibling, 2 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-01 16:17 UTC (permalink / raw) To: git; +Cc: gitster, peff Hi, Attached is the fourth revision of my patch-set "Support %(trailers) arguments in for-each-ref(1)". It includes the following changes since v3: * Teach unfold() to unfold multiple lines. * Rebase patches to apply on top of master. Thanks in advance, and thanks for all of your help thus far :-). - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," 2017-10-01 16:17 ` [PATCH v4 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau @ 2017-10-01 16:18 ` Taylor Blau 2017-10-01 16:18 ` [PATCH v4 2/6] t6300: refactor %(trailers) tests Taylor Blau ` (5 more replies) 2017-10-02 0:31 ` [PATCH v5 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 1 sibling, 6 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-01 16:18 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau In preparation for adding consistent "%(trailers)" atom options to `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in pretty.c to separate sub-arguments with a ",", instead of a ":". Multiple sub-arguments are given either as "%(trailers:unfold,only)" or "%(trailers:only,unfold)". This change disambiguates between "top-level" arguments, and arguments given to the trailers atom itself. It is consistent with the behavior of "%(upstream)" and "%(push)" atoms. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pretty.c | 34 +++++++++++++++++++++++++++++----- t/t4205-log-pretty-formats.sh | 4 ++-- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/pretty.c b/pretty.c index 94eab5c89..eec128bc1 100644 --- a/pretty.c +++ b/pretty.c @@ -1056,6 +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) +{ + const char *p; + if (!(skip_prefix(to_parse, candidate, &p))) + return 0; + if (*p == ',') { + *end = p + 1; + return 1; + } + if (*p == ')') { + *end = p; + return 1; + } + return 0; +} + + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1285,11 +1304,16 @@ 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; - while (*arg == ':') { - if (skip_prefix(arg, ":only", &arg)) - opts.only_trailers = 1; - else if (skip_prefix(arg, ":unfold", &arg)) - opts.unfold = 1; + 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 + break; + } } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index ec5f53010..977472f53 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' ' ' test_expect_success ':only and :unfold work together' ' - git log --no-walk --pretty="%(trailers:only:unfold)" >actual && - git log --no-walk --pretty="%(trailers:unfold:only)" >reverse && + git log --no-walk --pretty="%(trailers:only,unfold)" >actual && + git log --no-walk --pretty="%(trailers:unfold,only)" >reverse && test_cmp actual reverse && { grep -v patch.description <trailers | unfold && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v4 2/6] t6300: refactor %(trailers) tests 2017-10-01 16:18 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-10-01 16:18 ` Taylor Blau 2017-10-02 0:12 ` Junio C Hamano 2017-10-01 16:18 ` [PATCH v4 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau ` (4 subsequent siblings) 5 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-10-01 16:18 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau We currently have one test for %(trailers) in `git-for-each-ref(1)`, through "%(contents:trailers)". In preparation for more, let's add a few things: - Move the commit creation step to its own test so that it can be re-used. - Add a non-trailer to the commit's trailers to test that non-trailers aren't shown using "%(trailers:only)". - Add a multi-line trailer to ensure that trailers are unfolded correctly using "%(trailers:unfold)". Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t6300-for-each-ref.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 2274a4b73..39431908d 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -605,18 +605,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' ' cat >trailers <<EOF Reviewed-by: A U Thor <author@example.com> Signed-off-by: A U Thor <author@example.com> +[ v2 updated patch description ] +Acked-by: A U Thor + <author@example.com> EOF -test_expect_success 'basic atom: head contents:trailers' ' + +test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && git add two && - git commit -F - <<-EOF && + git commit -F - <<-EOF trailers: this commit message has trailers Some message contents $(cat trailers) EOF +' + +test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && # git for-each-ref ends with a blank line -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH v4 2/6] t6300: refactor %(trailers) tests 2017-10-01 16:18 ` [PATCH v4 2/6] t6300: refactor %(trailers) tests Taylor Blau @ 2017-10-02 0:12 ` Junio C Hamano 0 siblings, 0 replies; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 0:12 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Looks good to me, thanks. ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v4 3/6] doc: 'trailers' is the preferred way to format trailers 2017-10-01 16:18 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-01 16:18 ` [PATCH v4 2/6] t6300: refactor %(trailers) tests Taylor Blau @ 2017-10-01 16:18 ` Taylor Blau 2017-10-01 16:18 ` [PATCH v4 4/6] doc: use modern "`"-style code fencing Taylor Blau ` (3 subsequent siblings) 5 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-01 16:18 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The documentation makes reference to 'contents:trailers' as an example to dig the trailers out of a commit. 'trailers' is an unmentioned alternative, which is treated as an alias of 'contents:trailers'. Since 'trailers' is easier to type, prefer that as the designated way to dig out trailers information. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 66b4e0a40..323ce07de 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -217,7 +217,8 @@ line is 'contents:body', where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'contents:trailers'. +are obtained as 'trailers' (or by using the historical alias +'contents:trailers'). For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v4 4/6] doc: use modern "`"-style code fencing 2017-10-01 16:18 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-01 16:18 ` [PATCH v4 2/6] t6300: refactor %(trailers) tests Taylor Blau 2017-10-01 16:18 ` [PATCH v4 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau @ 2017-10-01 16:18 ` Taylor Blau 2017-10-01 23:55 ` Junio C Hamano 2017-10-01 16:18 ` [PATCH v4 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau ` (2 subsequent siblings) 5 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-10-01 16:18 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau "'"- (single-quote) styled code fencing is no longer considered modern within the "Documentation/" subtree. In preparation for adding additional information to this section of git-for-each-ref(1)'s documentation, update old-style code fencing to use "`"-style fencing instead. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 323ce07de..1279b9733 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -213,12 +213,12 @@ and `date` to extract the named component. The complete message in a commit and tag object is `contents`. Its first line is `contents:subject`, where subject is the concatenation of all lines of the commit message up to the first blank line. The next -line is 'contents:body', where body is all of the lines after the first +line is `contents:body`, where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'trailers' (or by using the historical alias -'contents:trailers'). +are obtained as `trailers` (or by using the historical alias +`contents:trailers`). For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH v4 4/6] doc: use modern "`"-style code fencing 2017-10-01 16:18 ` [PATCH v4 4/6] doc: use modern "`"-style code fencing Taylor Blau @ 2017-10-01 23:55 ` Junio C Hamano 2017-10-02 0:06 ` Taylor Blau 0 siblings, 1 reply; 78+ messages in thread From: Junio C Hamano @ 2017-10-01 23:55 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Taylor Blau <me@ttaylorr.com> writes: > "'"- (single-quote) styled code fencing is no longer considered modern > within the "Documentation/" subtree. > > In preparation for adding additional information to this section of > git-for-each-ref(1)'s documentation, update old-style code fencing to > use "`"-style fencing instead. Is this just me who wants to do s/fenc/quot/g? Unless somebody objects, I'd do so while queuing. Thanks. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > Documentation/git-for-each-ref.txt | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt > index 323ce07de..1279b9733 100644 > --- a/Documentation/git-for-each-ref.txt > +++ b/Documentation/git-for-each-ref.txt > @@ -213,12 +213,12 @@ and `date` to extract the named component. > The complete message in a commit and tag object is `contents`. > Its first line is `contents:subject`, where subject is the concatenation > of all lines of the commit message up to the first blank line. The next > -line is 'contents:body', where body is all of the lines after the first > +line is `contents:body`, where body is all of the lines after the first > blank line. The optional GPG signature is `contents:signature`. The > first `N` lines of the message is obtained using `contents:lines=N`. > Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] > -are obtained as 'trailers' (or by using the historical alias > -'contents:trailers'). > +are obtained as `trailers` (or by using the historical alias > +`contents:trailers`). > > For sorting purposes, fields with numeric values sort in numeric order > (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v4 4/6] doc: use modern "`"-style code fencing 2017-10-01 23:55 ` Junio C Hamano @ 2017-10-02 0:06 ` Taylor Blau 2017-10-02 1:35 ` Junio C Hamano 0 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-10-02 0:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, peff On Mon, Oct 02, 2017 at 08:55:53AM +0900, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > "'"- (single-quote) styled code fencing is no longer considered modern > > within the "Documentation/" subtree. > > > > In preparation for adding additional information to this section of > > git-for-each-ref(1)'s documentation, update old-style code fencing to > > use "`"-style fencing instead. > > Is this just me who wants to do s/fenc/quot/g? Unless somebody > objects, I'd do so while queuing. I don't object, I think that fencing is less appropriate than quoting. I couldn't find the term myself when writing this commit :-). I am happy to send out v5 of this patch series with this commit re-written, or you can change it while queuing. Whichever is easier for you. Thanks again. -- - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v4 4/6] doc: use modern "`"-style code fencing 2017-10-02 0:06 ` Taylor Blau @ 2017-10-02 1:35 ` Junio C Hamano 2017-10-02 4:53 ` Jeff King 0 siblings, 1 reply; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 1:35 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Taylor Blau <me@ttaylorr.com> writes: >> Is this just me who wants to do s/fenc/quot/g? Unless somebody >> objects, I'd do so while queuing. > > I don't object, I think that fencing is less appropriate than quoting. > I couldn't find the term myself when writing this commit :-). > > I am happy to send out v5 of this patch series with this commit > re-written, or you can change it while queuing. Whichever is easier for > you. Just FYI, here is what I ended up with. I do not think this is about "modern" vs "old style"; it is more about using the more appropriate mark-up and our desire has always been to use `literal` for things that users need to type literally. I notice that I didn't remove 'modern' from the title in the versoin below, though. Thanks. commit a184d836bcf1a557520095a90b22edf659ee88f2 Author: Taylor Blau <me@ttaylorr.com> Date: Sun Oct 1 09:18:50 2017 -0700 doc: use modern "`<literal>`"-style quoting for literal strings "'<string>'"-style quoting is not appropriate when quoting literal strings in the "Documentation/" subtree. In preparation for adding additional information to this section of git-for-each-ref(1)'s documentation, update them to use "`<literal>`" instead. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v4 4/6] doc: use modern "`"-style code fencing 2017-10-02 1:35 ` Junio C Hamano @ 2017-10-02 4:53 ` Jeff King 0 siblings, 0 replies; 78+ messages in thread From: Jeff King @ 2017-10-02 4:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git On Mon, Oct 02, 2017 at 10:35:45AM +0900, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > >> Is this just me who wants to do s/fenc/quot/g? Unless somebody > >> objects, I'd do so while queuing. > > > > I don't object, I think that fencing is less appropriate than quoting. > > I couldn't find the term myself when writing this commit :-). > > > > I am happy to send out v5 of this patch series with this commit > > re-written, or you can change it while queuing. Whichever is easier for > > you. > > Just FYI, here is what I ended up with. I do not think this is > about "modern" vs "old style"; it is more about using the more > appropriate mark-up and our desire has always been to use `literal` > for things that users need to type literally. I think it was my earlier comment that led to the modern/old notion. A lot of the old code does use single-quotes haphazardly. I'm not sure if it's "old style" or just "we were less careful" then. Either way, the patch looks good to me. I also think fencing isn't the right word; that word implies to me a multi-line block like --------------------------- this is fenced --------------------------- -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v4 5/6] ref-filter.c: use trailer_opts to format trailers 2017-10-01 16:18 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (2 preceding siblings ...) 2017-10-01 16:18 ` [PATCH v4 4/6] doc: use modern "`"-style code fencing Taylor Blau @ 2017-10-01 16:18 ` Taylor Blau 2017-10-02 0:13 ` Junio C Hamano 2017-10-01 16:18 ` [PATCH v4 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 2017-10-02 0:11 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Junio C Hamano 5 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-10-01 16:18 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau Fill trailer_opts with "unfold" and "only" to match the sub-arguments given to the "%(trailers)" atom. Then, let's use the filled trailer_opts instance with 'format_trailers_from_commit' in order to format trailers in the desired manner. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 5 ++++- ref-filter.c | 32 +++++++++++++++++++++--------- t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1279b9733..4a2c851e6 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -218,7 +218,10 @@ blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] are obtained as `trailers` (or by using the historical alias -`contents:trailers`). +`contents:trailers`). Non-trailer lines from the trailer block can be omitted +with `trailers:only`. Whitespace-continuations can be removed from trailers so +that each trailer appears on a line by itself with its full content with +`trailers:unfold`. Both can be used together as `trailers:unfold,only`. For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). diff --git a/ref-filter.c b/ref-filter.c index bc591f4f3..43ed10a5e 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -82,6 +82,7 @@ static struct used_atom { } remote_ref; struct { enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option; + struct process_trailer_options trailer_opts; unsigned int nlines; } contents; struct { @@ -182,9 +183,23 @@ static void subject_atom_parser(const struct ref_format *format, struct used_ato static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { - if (arg) - die(_("%%(trailers) does not take arguments")); + struct string_list params = STRING_LIST_INIT_DUP; + int i; + + if (arg) { + string_list_split(¶ms, arg, ',', -1); + for (i = 0; i < params.nr; i++) { + const char *s = params.items[i].string; + if (!strcmp(s, "unfold")) + atom->u.contents.trailer_opts.unfold = 1; + else if (!strcmp(s, "only")) + atom->u.contents.trailer_opts.only_trailers = 1; + else + die(_("unknown %%(trailers) argument: %s"), s); + } + } atom->u.contents.option = C_TRAILERS; + string_list_clear(¶ms, 0); } static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) @@ -1042,7 +1057,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj name++; if (strcmp(name, "subject") && strcmp(name, "body") && - strcmp(name, "trailers") && + !starts_with(name, "trailers") && !starts_with(name, "contents")) continue; if (!subpos) @@ -1067,13 +1082,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines); v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_TRAILERS) { - struct trailer_info info; + struct strbuf s = STRBUF_INIT; - /* Search for trailer info */ - trailer_info_get(&info, subpos); - v->s = xmemdupz(info.trailer_start, - info.trailer_end - info.trailer_start); - trailer_info_release(&info); + /* Format the trailer info according to the trailer_opts given */ + format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts); + + v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_BARE) v->s = xstrdup(subpos); } diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 39431908d..e4ce9c550 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -610,6 +610,9 @@ Acked-by: A U Thor <author@example.com> EOF +unfold () { + perl -0pe 's/\n\s+/ /g' +} test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && @@ -623,6 +626,43 @@ test_expect_success 'set up trailers for next test' ' EOF ' +test_expect_success '%(trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' + git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH v4 5/6] ref-filter.c: use trailer_opts to format trailers 2017-10-01 16:18 ` [PATCH v4 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau @ 2017-10-02 0:13 ` Junio C Hamano 0 siblings, 0 replies; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 0:13 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Taylor Blau <me@ttaylorr.com> writes: > +test_expect_success '%(trailers) rejects unknown trailers arguments' ' > + cat >expect <<-EOF && > + fatal: unknown %(trailers) argument: unsupported > + EOF > + test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && > + test_cmp expect actual I'll have to remember to fix these two lines up before queuing. Please yell at me if the version I'll push out later forgets to do so. Thanks. ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v4 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-01 16:18 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (3 preceding siblings ...) 2017-10-01 16:18 ` [PATCH v4 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau @ 2017-10-01 16:18 ` Taylor Blau 2017-10-02 0:19 ` Junio C Hamano 2017-10-02 0:11 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Junio C Hamano 5 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-10-01 16:18 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The %(contents) atom takes a contents "field" as its argument. Since "trailers" is one of those fields, extend contents_atom_parser to parse "trailers"'s arguments when used through "%(contents)", like: %(contents:trailers:unfold,only) A caveat: trailers_atom_parser expects NULL when no arguments are given (see: `parse_ref_filter_atom`). To simulate this behavior without teaching trailers_atom_parser to accept strings with length zero, conditionally pass NULL to trailers_atom_parser if the arguments portion of the argument to %(contents) is empty. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- ref-filter.c | 6 ++++-- t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 43ed10a5e..2c03c2bf5 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -212,8 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at atom->u.contents.option = C_SIG; else if (!strcmp(arg, "subject")) atom->u.contents.option = C_SUB; - else if (!strcmp(arg, "trailers")) - atom->u.contents.option = C_TRAILERS; + else if (skip_prefix(arg, "trailers", &arg)) { + skip_prefix(arg, ":", &arg); + trailers_atom_parser(atom, *arg ? NULL : arg); + } else if (skip_prefix(arg, "lines=", &arg)) { atom->u.contents.option = C_LINES; if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index e4ce9c550..ab95fe427 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -655,6 +655,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' ' + git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + test_expect_success '%(trailers) rejects unknown trailers arguments' ' cat >expect <<-EOF && fatal: unknown %(trailers) argument: unsupported @@ -663,6 +692,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH v4 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-01 16:18 ` [PATCH v4 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau @ 2017-10-02 0:19 ` Junio C Hamano 0 siblings, 0 replies; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 0:19 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Taylor Blau <me@ttaylorr.com> writes: > The %(contents) atom takes a contents "field" as its argument. Since > "trailers" is one of those fields, extend contents_atom_parser to parse > "trailers"'s arguments when used through "%(contents)", like: > > %(contents:trailers:unfold,only) > > A caveat: trailers_atom_parser expects NULL when no arguments are given > (see: `parse_ref_filter_atom`). To simulate this behavior without > teaching trailers_atom_parser to accept strings with length zero, > conditionally pass NULL to trailers_atom_parser if the arguments portion > of the argument to %(contents) is empty. I got an impression during the earlier rounds' review discussion that trailers_atom_parser() will happily accept an empty string and work correctly, so this paragraph and the conditional *arg ? NULL : arg were both unneeded. - arg == "" is not NULL, so we first do string_list_split() on ',' - which yields an empty list i.e. params.nr == 0 - we do not loop and leave atom->u.contents.trailer_opts untouched. - and we set u.contents.option to C_TRAILERS - and we clear ¶ms string list before we leave. which is exactly the same as what happens when arg == NULL. Sooo.... > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > ref-filter.c | 6 ++++-- > t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+), 2 deletions(-) > > diff --git a/ref-filter.c b/ref-filter.c > index 43ed10a5e..2c03c2bf5 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -212,8 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at > atom->u.contents.option = C_SIG; > else if (!strcmp(arg, "subject")) > atom->u.contents.option = C_SUB; > - else if (!strcmp(arg, "trailers")) > - atom->u.contents.option = C_TRAILERS; > + else if (skip_prefix(arg, "trailers", &arg)) { > + skip_prefix(arg, ":", &arg); > + trailers_atom_parser(atom, *arg ? NULL : arg); > + } > else if (skip_prefix(arg, "lines=", &arg)) { Merge these two lines i.e. } else if (skip_prefix(arg, "lines=", ...) { > +test_expect_success '%(contents:trailers) rejects unknown trailers arguments' ' > + cat >expect <<-EOF && > + fatal: unknown %(trailers) argument: unsupported > + EOF > + test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual && > + test_cmp expect actual Funny indentation. ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," 2017-10-01 16:18 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (4 preceding siblings ...) 2017-10-01 16:18 ` [PATCH v4 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau @ 2017-10-02 0:11 ` Junio C Hamano 2017-10-02 5:00 ` Jeff King 5 siblings, 1 reply; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 0:11 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Taylor Blau <me@ttaylorr.com> writes: > In preparation for adding consistent "%(trailers)" atom options to > `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in > pretty.c to separate sub-arguments with a ",", instead of a ":". > > Multiple sub-arguments are given either as "%(trailers:unfold,only)" or > "%(trailers:only,unfold)". > > This change disambiguates between "top-level" arguments, and arguments > given to the trailers atom itself. It is consistent with the behavior of > "%(upstream)" and "%(push)" atoms. Looks good. Ignore the remainder unless you are interested in the recent "make style" discussion. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > pretty.c | 34 +++++++++++++++++++++++++++++----- > t/t4205-log-pretty-formats.sh | 4 ++-- > 2 files changed, 31 insertions(+), 7 deletions(-) > > diff --git a/pretty.c b/pretty.c > index 94eab5c89..eec128bc1 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -1056,6 +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) "make style" wants to format this like so: static int match_placeholder_arg(const char *to_parse, const char *candidate, const char **end) I think I can live with either versions ;-) > +{ > + const char *p; > + if (!(skip_prefix(to_parse, candidate, &p))) > + return 0; > + if (*p == ',') { > + *end = p + 1; > + return 1; > + } > + if (*p == ')') { > + *end = p; > + return 1; > + } > + return 0; > +} > + > + "make style" seems to be unhappy to see double blank here, and I tend to agree. > static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > const char *placeholder, > void *context) > @@ -1285,11 +1304,16 @@ 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; > - while (*arg == ':') { > - if (skip_prefix(arg, ":only", &arg)) > - opts.only_trailers = 1; > - else if (skip_prefix(arg, ":unfold", &arg)) > - opts.unfold = 1; > + 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 > + break; > + } > } > if (*arg == ')') { > format_trailers_from_commit(sb, msg + c->subject_off, &opts); > diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh > index ec5f53010..977472f53 100755 > --- a/t/t4205-log-pretty-formats.sh > +++ b/t/t4205-log-pretty-formats.sh > @@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' ' > ' > > test_expect_success ':only and :unfold work together' ' > - git log --no-walk --pretty="%(trailers:only:unfold)" >actual && > - git log --no-walk --pretty="%(trailers:unfold:only)" >reverse && > + git log --no-walk --pretty="%(trailers:only,unfold)" >actual && > + git log --no-walk --pretty="%(trailers:unfold,only)" >reverse && > test_cmp actual reverse && > { > grep -v patch.description <trailers | unfold && ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," 2017-10-02 0:11 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Junio C Hamano @ 2017-10-02 5:00 ` Jeff King 0 siblings, 0 replies; 78+ messages in thread From: Jeff King @ 2017-10-02 5:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git On Mon, Oct 02, 2017 at 09:11:50AM +0900, Junio C Hamano wrote: > > diff --git a/pretty.c b/pretty.c > > index 94eab5c89..eec128bc1 100644 > > --- a/pretty.c > > +++ b/pretty.c > > @@ -1056,6 +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) > > "make style" wants to format this like so: > > static int match_placeholder_arg(const char *to_parse, const char *candidate, > const char **end) > > I think I can live with either versions ;-) I can live with either, too, though I do prefer the 3-line form (which is unsurprising since it comes from what I wrote in an earlier thread). I don't think it conveys a huge amount of meaning here, but there are definitely times when I would tweak the wrapping of long argument lists to convey meaning. E.g.: void some_function(struct foo *foo, const char *buf, size_t len, unsigned flags); rather than void some_function(struct foo *foo, const char *buf, size_t len, unsigned flags); It's a little sad to be beholden to a tool that would mindlessly drop that grouping. But at the same time, that tool may be the lesser evil if it saves time adjusting (and reviewing) style. I'll continue to observe before forming more inclusions. Thanks for reporting on your "make style" experiments. -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v5 0/6] Support %(trailers) arguments in for-each-ref(1) 2017-10-01 16:17 ` [PATCH v4 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-10-01 16:18 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-10-02 0:31 ` Taylor Blau 2017-10-02 0:32 ` [PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-02 5:23 ` [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 1 sibling, 2 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 0:31 UTC (permalink / raw) To: git; +Cc: gitster, peff Hi, Attached is the fifth revision of my patch-set "Support %(trailers) arguments in for-each-ref(1)". It includes the following changes since v4: * Clarified "ref-filter.c: parse trailers arguments with %(contents) atom" to include reasoning for passing NULL as "" empty string in contents_atom_parser. * Changed instances "fencing" to "quoting" in "doc: use modern "`"-style code fencing". * Changed indentation in "pretty.c: delimit "%(trailers)" arguments with ","" to silence the output from `make style`. * Fix incorrect indentation in "ref-filter.c: use trailer_opts to format trailers". Thanks again. - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with "," 2017-10-02 0:31 ` [PATCH v5 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau @ 2017-10-02 0:32 ` Taylor Blau 2017-10-02 0:33 ` [PATCH v5 2/6] t6300: refactor %(trailers) tests Taylor Blau ` (4 more replies) 2017-10-02 5:23 ` [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 1 sibling, 5 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 0:32 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau In preparation for adding consistent "%(trailers)" atom options to `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in pretty.c to separate sub-arguments with a ",", instead of a ":". Multiple sub-arguments are given either as "%(trailers:unfold,only)" or "%(trailers:only,unfold)". This change disambiguates between "top-level" arguments, and arguments given to the trailers atom itself. It is consistent with the behavior of "%(upstream)" and "%(push)" atoms. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pretty.c | 32 +++++++++++++++++++++++++++----- t/t4205-log-pretty-formats.sh | 4 ++-- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/pretty.c b/pretty.c index 94eab5c89..7500fe694 100644 --- a/pretty.c +++ b/pretty.c @@ -1056,6 +1056,23 @@ 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) +{ + const char *p; + if (!(skip_prefix(to_parse, candidate, &p))) + return 0; + if (*p == ',') { + *end = p + 1; + return 1; + } + if (*p == ')') { + *end = p; + return 1; + } + return 0; +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1285,11 +1302,16 @@ 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; - while (*arg == ':') { - if (skip_prefix(arg, ":only", &arg)) - opts.only_trailers = 1; - else if (skip_prefix(arg, ":unfold", &arg)) - opts.unfold = 1; + 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 + break; + } } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index ec5f53010..977472f53 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' ' ' test_expect_success ':only and :unfold work together' ' - git log --no-walk --pretty="%(trailers:only:unfold)" >actual && - git log --no-walk --pretty="%(trailers:unfold:only)" >reverse && + git log --no-walk --pretty="%(trailers:only,unfold)" >actual && + git log --no-walk --pretty="%(trailers:unfold,only)" >reverse && test_cmp actual reverse && { grep -v patch.description <trailers | unfold && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v5 2/6] t6300: refactor %(trailers) tests 2017-10-02 0:32 ` [PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-10-02 0:33 ` Taylor Blau 2017-10-02 0:33 ` [PATCH v5 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau ` (3 subsequent siblings) 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 0:33 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau We currently have one test for %(trailers) in `git-for-each-ref(1)`, through "%(contents:trailers)". In preparation for more, let's add a few things: - Move the commit creation step to its own test so that it can be re-used. - Add a non-trailer to the commit's trailers to test that non-trailers aren't shown using "%(trailers:only)". - Add a multi-line trailer to ensure that trailers are unfolded correctly using "%(trailers:unfold)". Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t6300-for-each-ref.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 2274a4b73..39431908d 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -605,18 +605,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' ' cat >trailers <<EOF Reviewed-by: A U Thor <author@example.com> Signed-off-by: A U Thor <author@example.com> +[ v2 updated patch description ] +Acked-by: A U Thor + <author@example.com> EOF -test_expect_success 'basic atom: head contents:trailers' ' + +test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && git add two && - git commit -F - <<-EOF && + git commit -F - <<-EOF trailers: this commit message has trailers Some message contents $(cat trailers) EOF +' + +test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && # git for-each-ref ends with a blank line -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v5 3/6] doc: 'trailers' is the preferred way to format trailers 2017-10-02 0:32 ` [PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-02 0:33 ` [PATCH v5 2/6] t6300: refactor %(trailers) tests Taylor Blau @ 2017-10-02 0:33 ` Taylor Blau 2017-10-02 0:33 ` [PATCH v5 4/6] doc: use modern "`"-style code quoting Taylor Blau ` (2 subsequent siblings) 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 0:33 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The documentation makes reference to 'contents:trailers' as an example to dig the trailers out of a commit. 'trailers' is an unmentioned alternative, which is treated as an alias of 'contents:trailers'. Since 'trailers' is easier to type, prefer that as the designated way to dig out trailers information. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 66b4e0a40..323ce07de 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -217,7 +217,8 @@ line is 'contents:body', where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'contents:trailers'. +are obtained as 'trailers' (or by using the historical alias +'contents:trailers'). For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v5 4/6] doc: use modern "`"-style code quoting 2017-10-02 0:32 ` [PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-02 0:33 ` [PATCH v5 2/6] t6300: refactor %(trailers) tests Taylor Blau 2017-10-02 0:33 ` [PATCH v5 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau @ 2017-10-02 0:33 ` Taylor Blau 2017-10-02 0:33 ` [PATCH v5 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-10-02 0:33 ` [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 0:33 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau "'"- (single-quote) styled code quoting is no longer considered modern within the "Documentation/" subtree. In preparation for adding additional information to this section of git-for-each-ref(1)'s documentation, update old-style code quoting to use "`"-style quoting instead. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 323ce07de..1279b9733 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -213,12 +213,12 @@ and `date` to extract the named component. The complete message in a commit and tag object is `contents`. Its first line is `contents:subject`, where subject is the concatenation of all lines of the commit message up to the first blank line. The next -line is 'contents:body', where body is all of the lines after the first +line is `contents:body`, where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'trailers' (or by using the historical alias -'contents:trailers'). +are obtained as `trailers` (or by using the historical alias +`contents:trailers`). For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v5 5/6] ref-filter.c: use trailer_opts to format trailers 2017-10-02 0:32 ` [PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (2 preceding siblings ...) 2017-10-02 0:33 ` [PATCH v5 4/6] doc: use modern "`"-style code quoting Taylor Blau @ 2017-10-02 0:33 ` Taylor Blau 2017-10-02 0:33 ` [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 4 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 0:33 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau Fill trailer_opts with "unfold" and "only" to match the sub-arguments given to the "%(trailers)" atom. Then, let's use the filled trailer_opts instance with 'format_trailers_from_commit' in order to format trailers in the desired manner. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 5 ++++- ref-filter.c | 32 +++++++++++++++++++++--------- t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1279b9733..4a2c851e6 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -218,7 +218,10 @@ blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] are obtained as `trailers` (or by using the historical alias -`contents:trailers`). +`contents:trailers`). Non-trailer lines from the trailer block can be omitted +with `trailers:only`. Whitespace-continuations can be removed from trailers so +that each trailer appears on a line by itself with its full content with +`trailers:unfold`. Both can be used together as `trailers:unfold,only`. For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). diff --git a/ref-filter.c b/ref-filter.c index bc591f4f3..43ed10a5e 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -82,6 +82,7 @@ static struct used_atom { } remote_ref; struct { enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option; + struct process_trailer_options trailer_opts; unsigned int nlines; } contents; struct { @@ -182,9 +183,23 @@ static void subject_atom_parser(const struct ref_format *format, struct used_ato static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { - if (arg) - die(_("%%(trailers) does not take arguments")); + struct string_list params = STRING_LIST_INIT_DUP; + int i; + + if (arg) { + string_list_split(¶ms, arg, ',', -1); + for (i = 0; i < params.nr; i++) { + const char *s = params.items[i].string; + if (!strcmp(s, "unfold")) + atom->u.contents.trailer_opts.unfold = 1; + else if (!strcmp(s, "only")) + atom->u.contents.trailer_opts.only_trailers = 1; + else + die(_("unknown %%(trailers) argument: %s"), s); + } + } atom->u.contents.option = C_TRAILERS; + string_list_clear(¶ms, 0); } static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) @@ -1042,7 +1057,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj name++; if (strcmp(name, "subject") && strcmp(name, "body") && - strcmp(name, "trailers") && + !starts_with(name, "trailers") && !starts_with(name, "contents")) continue; if (!subpos) @@ -1067,13 +1082,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines); v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_TRAILERS) { - struct trailer_info info; + struct strbuf s = STRBUF_INIT; - /* Search for trailer info */ - trailer_info_get(&info, subpos); - v->s = xmemdupz(info.trailer_start, - info.trailer_end - info.trailer_start); - trailer_info_release(&info); + /* Format the trailer info according to the trailer_opts given */ + format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts); + + v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_BARE) v->s = xstrdup(subpos); } diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 39431908d..54e52d4e9 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -610,6 +610,9 @@ Acked-by: A U Thor <author@example.com> EOF +unfold () { + perl -0pe 's/\n\s+/ /g' +} test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && @@ -623,6 +626,43 @@ test_expect_success 'set up trailers for next test' ' EOF ' +test_expect_success '%(trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' + git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 0:32 ` [PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (3 preceding siblings ...) 2017-10-02 0:33 ` [PATCH v5 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau @ 2017-10-02 0:33 ` Taylor Blau 2017-10-02 4:26 ` Junio C Hamano ` (2 more replies) 4 siblings, 3 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 0:33 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The %(contents) atom takes a contents "field" as its argument. Since "trailers" is one of those fields, extend contents_atom_parser to parse "trailers"'s arguments when used through "%(contents)", like: %(contents:trailers:unfold,only) A caveat: trailers_atom_parser expects NULL when no arguments are given (see: `parse_ref_filter_atom`). This is because string_list_split (given a maxsplit of -1) returns a 1-ary string_list* containing the given string if the delimiter could not be found using `strchr`. To simulate this behavior without teaching trailers_atom_parser to accept strings with length zero, conditionally pass NULL to trailers_atom_parser if the arguments portion of the argument to %(contents) is empty. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- ref-filter.c | 7 ++++--- t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 43ed10a5e..5b64de6ea 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at atom->u.contents.option = C_SIG; else if (!strcmp(arg, "subject")) atom->u.contents.option = C_SUB; - else if (!strcmp(arg, "trailers")) - atom->u.contents.option = C_TRAILERS; - else if (skip_prefix(arg, "lines=", &arg)) { + else if (skip_prefix(arg, "trailers", &arg)) { + skip_prefix(arg, ":", &arg); + trailers_atom_parser(atom, *arg ? NULL : arg); + } else if (skip_prefix(arg, "lines=", &arg)) { atom->u.contents.option = C_LINES; if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) die(_("positive value expected contents:lines=%s"), arg); diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 54e52d4e9..872973b95 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -655,6 +655,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' ' + git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + test_expect_success '%(trailers) rejects unknown trailers arguments' ' cat >expect <<-EOF && fatal: unknown %(trailers) argument: unsupported @@ -663,6 +692,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 0:33 ` [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau @ 2017-10-02 4:26 ` Junio C Hamano 2017-10-02 4:51 ` Junio C Hamano 2017-10-02 5:03 ` Jeff King 2 siblings, 0 replies; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 4:26 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Taylor Blau <me@ttaylorr.com> writes: > A caveat: trailers_atom_parser expects NULL when no arguments are given > (see: `parse_ref_filter_atom`). This is because string_list_split (given > a maxsplit of -1) returns a 1-ary string_list* containing the given > string if the delimiter could not be found using `strchr`. OK. That's unfortunate, but the solution here is far cleaner and simpler than fixing string-list-split. Thanks for an updated explanation here. ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 0:33 ` [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 2017-10-02 4:26 ` Junio C Hamano @ 2017-10-02 4:51 ` Junio C Hamano 2017-10-02 5:13 ` Taylor Blau 2017-10-02 5:03 ` Jeff King 2 siblings, 1 reply; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 4:51 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Taylor Blau <me@ttaylorr.com> writes: > @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at > atom->u.contents.option = C_SIG; > else if (!strcmp(arg, "subject")) > atom->u.contents.option = C_SUB; > - else if (!strcmp(arg, "trailers")) > - atom->u.contents.option = C_TRAILERS; > - else if (skip_prefix(arg, "lines=", &arg)) { > + else if (skip_prefix(arg, "trailers", &arg)) { > + skip_prefix(arg, ":", &arg); > + trailers_atom_parser(atom, *arg ? NULL : arg); A parameter for the call is missing. I think you want 'format' there. ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 4:51 ` Junio C Hamano @ 2017-10-02 5:13 ` Taylor Blau 0 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:13 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, peff On Mon, Oct 02, 2017 at 01:51:11PM +0900, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at > > atom->u.contents.option = C_SIG; > > else if (!strcmp(arg, "subject")) > > atom->u.contents.option = C_SUB; > > - else if (!strcmp(arg, "trailers")) > > - atom->u.contents.option = C_TRAILERS; > > - else if (skip_prefix(arg, "lines=", &arg)) { > > + else if (skip_prefix(arg, "trailers", &arg)) { > > + skip_prefix(arg, ":", &arg); > > + trailers_atom_parser(atom, *arg ? NULL : arg); > > A parameter for the call is missing. I think you want 'format' > there. Thanks for pointing this out. I have fixed this in v6, which I am sending out shortly. I think that this revision should be ready for queueing. -- - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 0:33 ` [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 2017-10-02 4:26 ` Junio C Hamano 2017-10-02 4:51 ` Junio C Hamano @ 2017-10-02 5:03 ` Jeff King 2017-10-02 5:12 ` Taylor Blau 2 siblings, 1 reply; 78+ messages in thread From: Jeff King @ 2017-10-02 5:03 UTC (permalink / raw) To: Taylor Blau; +Cc: git, gitster On Sun, Oct 01, 2017 at 05:33:04PM -0700, Taylor Blau wrote: > The %(contents) atom takes a contents "field" as its argument. Since > "trailers" is one of those fields, extend contents_atom_parser to parse > "trailers"'s arguments when used through "%(contents)", like: > > %(contents:trailers:unfold,only) > > A caveat: trailers_atom_parser expects NULL when no arguments are given > (see: `parse_ref_filter_atom`). This is because string_list_split (given > a maxsplit of -1) returns a 1-ary string_list* containing the given > string if the delimiter could not be found using `strchr`. > > To simulate this behavior without teaching trailers_atom_parser to > accept strings with length zero, conditionally pass NULL to > trailers_atom_parser if the arguments portion of the argument to > %(contents) is empty. Doh, that string_list behavior is what I was missing in my earlier comments. I agree this is probably the best way of doing it. I'm tempted to say that parse_ref_filter_atom() should do a similar thing. Right now we've got: $ git for-each-ref --format='%(refname)' | wc 2206 2206 79929 $ git for-each-ref --format='%(refname:short)' | wc 2206 2206 53622 $ git for-each-ref --format='%(refname:)' | wc fatal: unrecognized %(refname:) argument: 0 0 0 which seems a bit unfriendly. As we're on v6 here, I don't want to suggest it as part of this series. But maybe a #leftoverbits candidate, if others agree it's a good direction. -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 5:03 ` Jeff King @ 2017-10-02 5:12 ` Taylor Blau 2017-10-02 5:14 ` Jeff King 0 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:12 UTC (permalink / raw) To: Jeff King; +Cc: git, gitster On Mon, Oct 02, 2017 at 01:03:51AM -0400, Jeff King wrote: > On Sun, Oct 01, 2017 at 05:33:04PM -0700, Taylor Blau wrote: > > > The %(contents) atom takes a contents "field" as its argument. Since > > "trailers" is one of those fields, extend contents_atom_parser to parse > > "trailers"'s arguments when used through "%(contents)", like: > > > > %(contents:trailers:unfold,only) > > > > A caveat: trailers_atom_parser expects NULL when no arguments are given > > (see: `parse_ref_filter_atom`). This is because string_list_split (given > > a maxsplit of -1) returns a 1-ary string_list* containing the given > > string if the delimiter could not be found using `strchr`. > > > > To simulate this behavior without teaching trailers_atom_parser to > > accept strings with length zero, conditionally pass NULL to > > trailers_atom_parser if the arguments portion of the argument to > > %(contents) is empty. > > Doh, that string_list behavior is what I was missing in my earlier > comments. I agree this is probably the best way of doing it. I'm tempted > to say that parse_ref_filter_atom() should do a similar thing. Right now > we've got: > > $ git for-each-ref --format='%(refname)' | wc > 2206 2206 79929 > $ git for-each-ref --format='%(refname:short)' | wc > 2206 2206 53622 > $ git for-each-ref --format='%(refname:)' | wc > fatal: unrecognized %(refname:) argument: > 0 0 0 > > which seems a bit unfriendly. As we're on v6 here, I don't want to > suggest it as part of this series. But maybe a #leftoverbits candidate, > if others agree it's a good direction. I think #leftoverbits is fine here, but I think addressing this before 2.15 is reasonable. I can take a look at this in a future patch series. -- - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 5:12 ` Taylor Blau @ 2017-10-02 5:14 ` Jeff King 0 siblings, 0 replies; 78+ messages in thread From: Jeff King @ 2017-10-02 5:14 UTC (permalink / raw) To: Taylor Blau; +Cc: git, gitster On Sun, Oct 01, 2017 at 10:12:16PM -0700, Taylor Blau wrote: > > Doh, that string_list behavior is what I was missing in my earlier > > comments. I agree this is probably the best way of doing it. I'm tempted > > to say that parse_ref_filter_atom() should do a similar thing. Right now > > we've got: > > > > $ git for-each-ref --format='%(refname)' | wc > > 2206 2206 79929 > > $ git for-each-ref --format='%(refname:short)' | wc > > 2206 2206 53622 > > $ git for-each-ref --format='%(refname:)' | wc > > fatal: unrecognized %(refname:) argument: > > 0 0 0 > > > > which seems a bit unfriendly. As we're on v6 here, I don't want to > > suggest it as part of this series. But maybe a #leftoverbits candidate, > > if others agree it's a good direction. > > I think #leftoverbits is fine here, but I think addressing this before > 2.15 is reasonable. I can take a look at this in a future patch series. I think it would be to just do it as a standalone patch immediately. I just didn't want to hold your series hostage to a potential disagreement. -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) 2017-10-02 0:31 ` [PATCH v5 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-10-02 0:32 ` [PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-10-02 5:23 ` Taylor Blau 2017-10-02 5:25 ` [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (2 more replies) 1 sibling, 3 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:23 UTC (permalink / raw) To: git; +Cc: gitster, peff Hi, Attached is the sixth revision of my patch-set "Support %(trailers) arguments in for-each-ref(1)". In includes the following changes since v5: * Added an additional patch to change t4205 to harden `unfold()` against multi-line trailer folding. * Added a missing parameter call in ref-filter.c to `trailers_atom_parser` through `contents_atom_parser`. I believe that this version of the series should be ready for queueing. I am going to address Peff's follow-up for teaching `parse_ref_atom_filter` to accept "empty" argument lists as `%(refname:)` in a follow-up series later this evening. Thanks again :-). -- - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
* [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," 2017-10-02 5:23 ` [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) Taylor Blau @ 2017-10-02 5:25 ` Taylor Blau 2017-10-02 5:25 ` [PATCH v6 2/7] t4205: unfold across multiple lines Taylor Blau ` (5 more replies) 2017-10-02 6:56 ` [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) Jeff King 2017-10-02 8:07 ` Junio C Hamano 2 siblings, 6 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:25 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau In preparation for adding consistent "%(trailers)" atom options to `git-for-each-ref(1)`'s "--format" argument, change "%(trailers)" in pretty.c to separate sub-arguments with a ",", instead of a ":". Multiple sub-arguments are given either as "%(trailers:unfold,only)" or "%(trailers:only,unfold)". This change disambiguates between "top-level" arguments, and arguments given to the trailers atom itself. It is consistent with the behavior of "%(upstream)" and "%(push)" atoms. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- pretty.c | 32 +++++++++++++++++++++++++++----- t/t4205-log-pretty-formats.sh | 4 ++-- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/pretty.c b/pretty.c index 94eab5c89..7500fe694 100644 --- a/pretty.c +++ b/pretty.c @@ -1056,6 +1056,23 @@ 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) +{ + const char *p; + if (!(skip_prefix(to_parse, candidate, &p))) + return 0; + if (*p == ',') { + *end = p + 1; + return 1; + } + if (*p == ')') { + *end = p; + return 1; + } + return 0; +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1285,11 +1302,16 @@ 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; - while (*arg == ':') { - if (skip_prefix(arg, ":only", &arg)) - opts.only_trailers = 1; - else if (skip_prefix(arg, ":unfold", &arg)) - opts.unfold = 1; + 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 + break; + } } if (*arg == ')') { format_trailers_from_commit(sb, msg + c->subject_off, &opts); diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index ec5f53010..977472f53 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -588,8 +588,8 @@ test_expect_success '%(trailers:unfold) unfolds trailers' ' ' test_expect_success ':only and :unfold work together' ' - git log --no-walk --pretty="%(trailers:only:unfold)" >actual && - git log --no-walk --pretty="%(trailers:unfold:only)" >reverse && + git log --no-walk --pretty="%(trailers:only,unfold)" >actual && + git log --no-walk --pretty="%(trailers:unfold,only)" >reverse && test_cmp actual reverse && { grep -v patch.description <trailers | unfold && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v6 2/7] t4205: unfold across multiple lines 2017-10-02 5:25 ` [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-10-02 5:25 ` Taylor Blau 2017-10-02 5:25 ` [PATCH v6 3/7] doc: 'trailers' is the preferred way to format trailers Taylor Blau ` (4 subsequent siblings) 5 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:25 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau Tests in t4205 test the following: git log --format='%(trailers:unfold)' ... By ensuring the multi-line trailers are unfolded back onto the same line. t4205 only includes tests for 2-line trailers, but `unfold()` will fail for folded trailers on 3 or more lines. In preparation for adding subsequent tests in t6300 that test similar behavior in `git-for-each-ref(1)`, let's harden t4205 (and make it consistent with the changes in t6300) by ensuring that 3 or more line folded trailers are unfolded correctly. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t4205-log-pretty-formats.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 977472f53..591f35daa 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -544,7 +544,7 @@ Signed-off-by: A U Thor EOF unfold () { - perl -0pe 's/\n\s+/ /' + perl -0pe 's/\n\s+/ /g' } test_expect_success 'set up trailer tests' ' -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v6 3/7] doc: 'trailers' is the preferred way to format trailers 2017-10-02 5:25 ` [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-02 5:25 ` [PATCH v6 2/7] t4205: unfold across multiple lines Taylor Blau @ 2017-10-02 5:25 ` Taylor Blau 2017-10-02 5:25 ` [PATCH v6 4/7] doc: use modern "`"-style code quoting Taylor Blau ` (3 subsequent siblings) 5 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:25 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The documentation makes reference to 'contents:trailers' as an example to dig the trailers out of a commit. 'trailers' is an unmentioned alternative, which is treated as an alias of 'contents:trailers'. Since 'trailers' is easier to type, prefer that as the designated way to dig out trailers information. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 66b4e0a40..323ce07de 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -217,7 +217,8 @@ line is 'contents:body', where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'contents:trailers'. +are obtained as 'trailers' (or by using the historical alias +'contents:trailers'). For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v6 4/7] doc: use modern "`"-style code quoting 2017-10-02 5:25 ` [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-02 5:25 ` [PATCH v6 2/7] t4205: unfold across multiple lines Taylor Blau 2017-10-02 5:25 ` [PATCH v6 3/7] doc: 'trailers' is the preferred way to format trailers Taylor Blau @ 2017-10-02 5:25 ` Taylor Blau 2017-10-02 5:25 ` [PATCH v6 5/7] t6300: refactor %(trailers) tests Taylor Blau ` (2 subsequent siblings) 5 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:25 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau "'"- (single-quote) styled code quoting is no longer considered modern within the "Documentation/" subtree. In preparation for adding additional information to this section of git-for-each-ref(1)'s documentation, update old-style code quoting to use "`"-style quoting instead. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 323ce07de..1279b9733 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -213,12 +213,12 @@ and `date` to extract the named component. The complete message in a commit and tag object is `contents`. Its first line is `contents:subject`, where subject is the concatenation of all lines of the commit message up to the first blank line. The next -line is 'contents:body', where body is all of the lines after the first +line is `contents:body`, where body is all of the lines after the first blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] -are obtained as 'trailers' (or by using the historical alias -'contents:trailers'). +are obtained as `trailers` (or by using the historical alias +`contents:trailers`). For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v6 5/7] t6300: refactor %(trailers) tests 2017-10-02 5:25 ` [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (2 preceding siblings ...) 2017-10-02 5:25 ` [PATCH v6 4/7] doc: use modern "`"-style code quoting Taylor Blau @ 2017-10-02 5:25 ` Taylor Blau 2017-10-02 5:25 ` [PATCH v6 6/7] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-10-02 5:25 ` [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 5 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:25 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau We currently have one test for %(trailers) in `git-for-each-ref(1)`, through "%(contents:trailers)". In preparation for more, let's add a few things: - Move the commit creation step to its own test so that it can be re-used. - Add a non-trailer to the commit's trailers to test that non-trailers aren't shown using "%(trailers:only)". - Add a multi-line trailer to ensure that trailers are unfolded correctly using "%(trailers:unfold)". Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t6300-for-each-ref.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 2274a4b73..39431908d 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -605,18 +605,25 @@ test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' ' cat >trailers <<EOF Reviewed-by: A U Thor <author@example.com> Signed-off-by: A U Thor <author@example.com> +[ v2 updated patch description ] +Acked-by: A U Thor + <author@example.com> EOF -test_expect_success 'basic atom: head contents:trailers' ' + +test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && git add two && - git commit -F - <<-EOF && + git commit -F - <<-EOF trailers: this commit message has trailers Some message contents $(cat trailers) EOF +' + +test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && # git for-each-ref ends with a blank line -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v6 6/7] ref-filter.c: use trailer_opts to format trailers 2017-10-02 5:25 ` [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (3 preceding siblings ...) 2017-10-02 5:25 ` [PATCH v6 5/7] t6300: refactor %(trailers) tests Taylor Blau @ 2017-10-02 5:25 ` Taylor Blau 2017-10-02 5:25 ` [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 5 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:25 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau Fill trailer_opts with "unfold" and "only" to match the sub-arguments given to the "%(trailers)" atom. Then, let's use the filled trailer_opts instance with 'format_trailers_from_commit' in order to format trailers in the desired manner. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/git-for-each-ref.txt | 5 ++++- ref-filter.c | 32 +++++++++++++++++++++--------- t/t6300-for-each-ref.sh | 40 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 10 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 1279b9733..4a2c851e6 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -218,7 +218,10 @@ blank line. The optional GPG signature is `contents:signature`. The first `N` lines of the message is obtained using `contents:lines=N`. Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1] are obtained as `trailers` (or by using the historical alias -`contents:trailers`). +`contents:trailers`). Non-trailer lines from the trailer block can be omitted +with `trailers:only`. Whitespace-continuations can be removed from trailers so +that each trailer appears on a line by itself with its full content with +`trailers:unfold`. Both can be used together as `trailers:unfold,only`. For sorting purposes, fields with numeric values sort in numeric order (`objectsize`, `authordate`, `committerdate`, `creatordate`, `taggerdate`). diff --git a/ref-filter.c b/ref-filter.c index bc591f4f3..43ed10a5e 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -82,6 +82,7 @@ static struct used_atom { } remote_ref; struct { enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option; + struct process_trailer_options trailer_opts; unsigned int nlines; } contents; struct { @@ -182,9 +183,23 @@ static void subject_atom_parser(const struct ref_format *format, struct used_ato static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { - if (arg) - die(_("%%(trailers) does not take arguments")); + struct string_list params = STRING_LIST_INIT_DUP; + int i; + + if (arg) { + string_list_split(¶ms, arg, ',', -1); + for (i = 0; i < params.nr; i++) { + const char *s = params.items[i].string; + if (!strcmp(s, "unfold")) + atom->u.contents.trailer_opts.unfold = 1; + else if (!strcmp(s, "only")) + atom->u.contents.trailer_opts.only_trailers = 1; + else + die(_("unknown %%(trailers) argument: %s"), s); + } + } atom->u.contents.option = C_TRAILERS; + string_list_clear(¶ms, 0); } static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) @@ -1042,7 +1057,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj name++; if (strcmp(name, "subject") && strcmp(name, "body") && - strcmp(name, "trailers") && + !starts_with(name, "trailers") && !starts_with(name, "contents")) continue; if (!subpos) @@ -1067,13 +1082,12 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct obj append_lines(&s, subpos, contents_end - subpos, atom->u.contents.nlines); v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_TRAILERS) { - struct trailer_info info; + struct strbuf s = STRBUF_INIT; - /* Search for trailer info */ - trailer_info_get(&info, subpos); - v->s = xmemdupz(info.trailer_start, - info.trailer_end - info.trailer_start); - trailer_info_release(&info); + /* Format the trailer info according to the trailer_opts given */ + format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts); + + v->s = strbuf_detach(&s, NULL); } else if (atom->u.contents.option == C_BARE) v->s = xstrdup(subpos); } diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 39431908d..54e52d4e9 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -610,6 +610,9 @@ Acked-by: A U Thor <author@example.com> EOF +unfold () { + perl -0pe 's/\n\s+/ /g' +} test_expect_success 'set up trailers for next test' ' echo "Some contents" > two && @@ -623,6 +626,43 @@ test_expect_success 'set up trailers for next test' ' EOF ' +test_expect_success '%(trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' + git for-each-ref --format="%(trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 5:25 ` [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau ` (4 preceding siblings ...) 2017-10-02 5:25 ` [PATCH v6 6/7] ref-filter.c: use trailer_opts to format trailers Taylor Blau @ 2017-10-02 5:25 ` Taylor Blau 2017-10-02 6:51 ` Jeff King 5 siblings, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-10-02 5:25 UTC (permalink / raw) To: git; +Cc: gitster, peff, Taylor Blau The %(contents) atom takes a contents "field" as its argument. Since "trailers" is one of those fields, extend contents_atom_parser to parse "trailers"'s arguments when used through "%(contents)", like: %(contents:trailers:unfold,only) A caveat: trailers_atom_parser expects NULL when no arguments are given (see: `parse_ref_filter_atom`). This is because string_list_split (given a maxsplit of -1) returns a 1-ary string_list* containing the given string if the delimiter could not be found using `strchr`. To simulate this behavior without teaching trailers_atom_parser to accept strings with length zero, conditionally pass NULL to trailers_atom_parser if the arguments portion of the argument to %(contents) is empty. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- ref-filter.c | 7 ++++--- t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 43ed10a5e..6c26b4733 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at atom->u.contents.option = C_SIG; else if (!strcmp(arg, "subject")) atom->u.contents.option = C_SUB; - else if (!strcmp(arg, "trailers")) - atom->u.contents.option = C_TRAILERS; - else if (skip_prefix(arg, "lines=", &arg)) { + else if (skip_prefix(arg, "trailers", &arg)) { + skip_prefix(arg, ":", &arg); + trailers_atom_parser(format, atom, *arg ? NULL : arg); + } else if (skip_prefix(arg, "lines=", &arg)) { atom->u.contents.option = C_LINES; if (strtoul_ui(arg, 10, &atom->u.contents.nlines)) die(_("positive value expected contents:lines=%s"), arg); diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 54e52d4e9..872973b95 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -655,6 +655,35 @@ test_expect_success '%(trailers:only) and %(trailers:unfold) work together' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers:unfold) unfolds trailers' ' + git for-each-ref --format="%(contents:trailers:unfold)" refs/heads/master >actual && + { + unfold <trailers + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) shows only "key: value" trailers' ' + git for-each-ref --format="%(contents:trailers:only)" refs/heads/master >actual && + { + grep -v patch.description <trailers && + echo + } >expect && + test_cmp expect actual +' + +test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) work together' ' + git for-each-ref --format="%(contents:trailers:only,unfold)" refs/heads/master >actual && + git for-each-ref --format="%(contents:trailers:unfold,only)" refs/heads/master >reverse && + test_cmp actual reverse && + { + grep -v patch.description <trailers | unfold && + echo + } >expect && + test_cmp expect actual +' + test_expect_success '%(trailers) rejects unknown trailers arguments' ' cat >expect <<-EOF && fatal: unknown %(trailers) argument: unsupported @@ -663,6 +692,14 @@ test_expect_success '%(trailers) rejects unknown trailers arguments' ' test_cmp expect actual ' +test_expect_success '%(contents:trailers) rejects unknown trailers arguments' ' + cat >expect <<-EOF && + fatal: unknown %(trailers) argument: unsupported + EOF + test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual && + test_cmp expect actual +' + test_expect_success 'basic atom: head contents:trailers' ' git for-each-ref --format="%(contents:trailers)" refs/heads/master >actual && sanitize_pgp <actual >actual.clean && -- 2.14.1.145.gb3622a4ee ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 5:25 ` [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau @ 2017-10-02 6:51 ` Jeff King 2017-10-02 9:52 ` Junio C Hamano 2017-10-02 15:49 ` Taylor Blau 0 siblings, 2 replies; 78+ messages in thread From: Jeff King @ 2017-10-02 6:51 UTC (permalink / raw) To: Taylor Blau; +Cc: git, gitster On Sun, Oct 01, 2017 at 10:25:24PM -0700, Taylor Blau wrote: > The %(contents) atom takes a contents "field" as its argument. Since > "trailers" is one of those fields, extend contents_atom_parser to parse > "trailers"'s arguments when used through "%(contents)", like: > > %(contents:trailers:unfold,only) > > A caveat: trailers_atom_parser expects NULL when no arguments are given > (see: `parse_ref_filter_atom`). This is because string_list_split (given > a maxsplit of -1) returns a 1-ary string_list* containing the given > string if the delimiter could not be found using `strchr`. > > To simulate this behavior without teaching trailers_atom_parser to > accept strings with length zero, conditionally pass NULL to > trailers_atom_parser if the arguments portion of the argument to > %(contents) is empty. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > ref-filter.c | 7 ++++--- > t/t6300-for-each-ref.sh | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+), 3 deletions(-) This patch seems to fail a bunch of tests in t6300. > diff --git a/ref-filter.c b/ref-filter.c > index 43ed10a5e..6c26b4733 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at > atom->u.contents.option = C_SIG; > else if (!strcmp(arg, "subject")) > atom->u.contents.option = C_SUB; > - else if (!strcmp(arg, "trailers")) > - atom->u.contents.option = C_TRAILERS; > - else if (skip_prefix(arg, "lines=", &arg)) { > + else if (skip_prefix(arg, "trailers", &arg)) { > + skip_prefix(arg, ":", &arg); > + trailers_atom_parser(format, atom, *arg ? NULL : arg); I think your logic is flipped. You want "*arg ? arg : NULL"; -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 6:51 ` Jeff King @ 2017-10-02 9:52 ` Junio C Hamano 2017-10-02 15:49 ` Taylor Blau 1 sibling, 0 replies; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 9:52 UTC (permalink / raw) To: Jeff King; +Cc: Taylor Blau, git Jeff King <peff@peff.net> writes: >> atom->u.contents.option = C_SUB; >> - else if (!strcmp(arg, "trailers")) >> - atom->u.contents.option = C_TRAILERS; >> - else if (skip_prefix(arg, "lines=", &arg)) { >> + else if (skip_prefix(arg, "trailers", &arg)) { >> + skip_prefix(arg, ":", &arg); >> + trailers_atom_parser(format, atom, *arg ? NULL : arg); > > I think your logic is flipped. You want "*arg ? arg : NULL"; Ahh, I was blind. Will fix locally and push the results out. Thanks. ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 6:51 ` Jeff King 2017-10-02 9:52 ` Junio C Hamano @ 2017-10-02 15:49 ` Taylor Blau 2017-10-02 23:44 ` Junio C Hamano 1 sibling, 1 reply; 78+ messages in thread From: Taylor Blau @ 2017-10-02 15:49 UTC (permalink / raw) To: Jeff King; +Cc: git, gitster On Mon, Oct 02, 2017 at 02:51:00AM -0400, Jeff King wrote: > > diff --git a/ref-filter.c b/ref-filter.c > > index 43ed10a5e..6c26b4733 100644 > > --- a/ref-filter.c > > +++ b/ref-filter.c > > @@ -212,9 +212,10 @@ static void contents_atom_parser(const struct ref_format *format, struct used_at > > atom->u.contents.option = C_SIG; > > else if (!strcmp(arg, "subject")) > > atom->u.contents.option = C_SUB; > > - else if (!strcmp(arg, "trailers")) > > - atom->u.contents.option = C_TRAILERS; > > - else if (skip_prefix(arg, "lines=", &arg)) { > > + else if (skip_prefix(arg, "trailers", &arg)) { > > + skip_prefix(arg, ":", &arg); > > + trailers_atom_parser(format, atom, *arg ? NULL : arg); > > I think your logic is flipped. You want "*arg ? arg : NULL"; Thank you for pointing this out. I should have run "make test" on this patch set (or, as you suggested, `git rebase -x "make test" HEAD~7`) before sending it out. I appreciate you catching my mistake, and I'll make sure to run "make test" more diligently in the future :-). It sounds like Junio picked this up while queueing. -- - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom 2017-10-02 15:49 ` Taylor Blau @ 2017-10-02 23:44 ` Junio C Hamano 0 siblings, 0 replies; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 23:44 UTC (permalink / raw) To: Taylor Blau; +Cc: Jeff King, git Taylor Blau <me@ttaylorr.com> writes: > Thank you for pointing this out. I should have run "make test" on this > patch set (or, as you suggested, `git rebase -x "make test" HEAD~7`) > before sending it out. I appreciate you catching my mistake, and I'll > make sure to run "make test" more diligently in the future :-). > > It sounds like Junio picked this up while queueing. "Yup" in the sense that "you do not have to worry about it unless there are other things you want to fix by rerolling" but I didn't spot it myself and noticed the breakage; I tweaked it only after Peff pointed out what is wrong. ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) 2017-10-02 5:23 ` [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-10-02 5:25 ` [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau @ 2017-10-02 6:56 ` Jeff King 2017-10-03 6:24 ` Junio C Hamano 2017-10-02 8:07 ` Junio C Hamano 2 siblings, 1 reply; 78+ messages in thread From: Jeff King @ 2017-10-02 6:56 UTC (permalink / raw) To: Taylor Blau; +Cc: git, gitster On Sun, Oct 01, 2017 at 10:23:26PM -0700, Taylor Blau wrote: > Attached is the sixth revision of my patch-set "Support %(trailers) > arguments in for-each-ref(1)". > > In includes the following changes since v5: > > * Added an additional patch to change t4205 to harden `unfold()` > against multi-line trailer folding. > > * Added a missing parameter call in ref-filter.c to > `trailers_atom_parser` through `contents_atom_parser`. > > I believe that this version of the series should be ready for queueing. This looks good to me, modulo the flipped logic in the final patch. Since that's the only thing I noticed, let's hold off on a reroll for the moment to see if there are any more comments (and I won't be surprised if Junio just picks it up with the tweak, but we'll see). Please do make sure that "make test" runs clean before posting (I usually run it on each commit to catch any "oops, we broke this and then refixed it" in the middle of the series, too). -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) 2017-10-02 6:56 ` [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) Jeff King @ 2017-10-03 6:24 ` Junio C Hamano 2017-10-03 6:36 ` Jeff King 0 siblings, 1 reply; 78+ messages in thread From: Junio C Hamano @ 2017-10-03 6:24 UTC (permalink / raw) To: Jeff King; +Cc: Taylor Blau, git Jeff King <peff@peff.net> writes: > Since that's the only thing I noticed, let's hold off on a reroll for > the moment to see if there are any more comments (and I won't be > surprised if Junio just picks it up with the tweak, but we'll see). > > Please do make sure that "make test" runs clean before posting (I > usually run it on each commit to catch any "oops, we broke this and then > refixed it" in the middle of the series, too). OK, I think fixes for both the "flipped" and the "gettext-poison" breakages are solved locally in my tree already, so I guess this is ready to be merged to 'next'. ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) 2017-10-03 6:24 ` Junio C Hamano @ 2017-10-03 6:36 ` Jeff King 2017-10-03 6:40 ` Junio C Hamano 0 siblings, 1 reply; 78+ messages in thread From: Jeff King @ 2017-10-03 6:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git On Tue, Oct 03, 2017 at 03:24:41PM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > Since that's the only thing I noticed, let's hold off on a reroll for > > the moment to see if there are any more comments (and I won't be > > surprised if Junio just picks it up with the tweak, but we'll see). > > > > Please do make sure that "make test" runs clean before posting (I > > usually run it on each commit to catch any "oops, we broke this and then > > refixed it" in the middle of the series, too). > > OK, I think fixes for both the "flipped" and the "gettext-poison" > breakages are solved locally in my tree already, so I guess this is > ready to be merged to 'next'. Yes, I think so. Out of curiosity, do you frequently test with GETTEXT_POISON, or did you just guess at a potential problem after reading the tests? Proper use of test_i18ncmp is definitely something we ought to be looking for during review, but I confess it's something I often miss. -Peff ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) 2017-10-03 6:36 ` Jeff King @ 2017-10-03 6:40 ` Junio C Hamano 0 siblings, 0 replies; 78+ messages in thread From: Junio C Hamano @ 2017-10-03 6:40 UTC (permalink / raw) To: Jeff King; +Cc: Taylor Blau, git Jeff King <peff@peff.net> writes: > Out of curiosity, do you frequently test with GETTEXT_POISON, or did you > just guess at a potential problem after reading the tests? Proper use > of test_i18ncmp is definitely something we ought to be looking for > during review, but I confess it's something I often miss. I learn of breakages after the fact, when Travis reports. ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) 2017-10-02 5:23 ` [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-10-02 5:25 ` [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-02 6:56 ` [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) Jeff King @ 2017-10-02 8:07 ` Junio C Hamano 2017-10-02 12:15 ` Junio C Hamano 2 siblings, 1 reply; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 8:07 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Taylor Blau <me@ttaylorr.com> writes: > Attached is the sixth revision of my patch-set "Support %(trailers) > arguments in for-each-ref(1)". > > In includes the following changes since v5: > > * Added an additional patch to change t4205 to harden `unfold()` > against multi-line trailer folding. > > * Added a missing parameter call in ref-filter.c to > `trailers_atom_parser` through `contents_atom_parser`. > > I believe that this version of the series should be ready for queueing. > > I am going to address Peff's follow-up for teaching > `parse_ref_atom_filter` to accept "empty" argument lists as > `%(refname:)` in a follow-up series later this evening. > > Thanks again :-). Thanks. t6300 seems to show that %(contents:trailers:unfold) does not quite work, among other things. https://travis-ci.org/git/git/jobs/282126607#L3658 I didn't have a chance to look into it myself. ^ permalink raw reply [flat|nested] 78+ messages in thread
* Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) 2017-10-02 8:07 ` Junio C Hamano @ 2017-10-02 12:15 ` Junio C Hamano 2017-10-02 16:07 ` Taylor Blau 0 siblings, 1 reply; 78+ messages in thread From: Junio C Hamano @ 2017-10-02 12:15 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff Junio C Hamano <gitster@pobox.com> writes: > Thanks. t6300 seems to show that %(contents:trailers:unfold) does > not quite work, among other things. > > https://travis-ci.org/git/git/jobs/282126607#L3658 > > I didn't have a chance to look into it myself. Peff's "oops, your logic is backwards" fixes the above failure. We also need this on top to pass the gettext-poison build. diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 872973b954..3bdfa02559 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -685,19 +685,21 @@ test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) w ' test_expect_success '%(trailers) rejects unknown trailers arguments' ' + # error message cannot be checked under i18n cat >expect <<-EOF && fatal: unknown %(trailers) argument: unsupported EOF test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && - test_cmp expect actual + test_i18ncmp expect actual ' test_expect_success '%(contents:trailers) rejects unknown trailers arguments' ' + # error message cannot be checked under i18n cat >expect <<-EOF && fatal: unknown %(trailers) argument: unsupported EOF test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual && - test_cmp expect actual + test_i18ncmp expect actual ' test_expect_success 'basic atom: head contents:trailers' ' ^ permalink raw reply related [flat|nested] 78+ messages in thread
* Re: [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) 2017-10-02 12:15 ` Junio C Hamano @ 2017-10-02 16:07 ` Taylor Blau 0 siblings, 0 replies; 78+ messages in thread From: Taylor Blau @ 2017-10-02 16:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, peff On Mon, Oct 02, 2017 at 09:15:00PM +0900, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Thanks. t6300 seems to show that %(contents:trailers:unfold) does > > not quite work, among other things. > > > > https://travis-ci.org/git/git/jobs/282126607#L3658 > > > > I didn't have a chance to look into it myself. > > Peff's "oops, your logic is backwards" fixes the above failure. > > We also need this on top to pass the gettext-poison build. > > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index 872973b954..3bdfa02559 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -685,19 +685,21 @@ test_expect_success '%(contents:trailers:only) and %(contents:trailers:unfold) w > ' > > test_expect_success '%(trailers) rejects unknown trailers arguments' ' > + # error message cannot be checked under i18n > cat >expect <<-EOF && > fatal: unknown %(trailers) argument: unsupported > EOF > test_must_fail git for-each-ref --format="%(trailers:unsupported)" 2>actual && > - test_cmp expect actual > + test_i18ncmp expect actual > ' > > test_expect_success '%(contents:trailers) rejects unknown trailers arguments' ' > + # error message cannot be checked under i18n > cat >expect <<-EOF && > fatal: unknown %(trailers) argument: unsupported > EOF > test_must_fail git for-each-ref --format="%(contents:trailers:unsupported)" 2>actual && > - test_cmp expect actual > + test_i18ncmp expect actual > ' > > test_expect_success 'basic atom: head contents:trailers' ' Thank you for pointing this out. I am not well-versed on gettext, and its usage within Git. I am happy to send out v7 of this series, or you can apply these changes in queueing. Whichever is easier :-). -- - Taylor ^ permalink raw reply [flat|nested] 78+ messages in thread
end of thread, other threads:[~2017-10-03 6:40 UTC | newest] Thread overview: 78+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-30 6:22 [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-09-30 6:22 ` [PATCH 1/5] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-09-30 6:49 ` Jeff King 2017-09-30 6:22 ` [PATCH 2/5] t6300: refactor %(trailers) tests Taylor Blau 2017-09-30 7:01 ` Jeff King 2017-09-30 6:22 ` [PATCH 3/5] ref-filter.c: add trailer options to used_atom Taylor Blau 2017-09-30 7:10 ` Jeff King 2017-09-30 6:22 ` [PATCH 4/5] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-09-30 7:21 ` Jeff King 2017-10-01 9:08 ` Junio C Hamano 2017-10-01 9:00 ` Junio C Hamano 2017-10-02 5:05 ` Jeff King 2017-10-02 5:11 ` Taylor Blau 2017-09-30 6:22 ` [PATCH 5/5] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 2017-09-30 7:36 ` Jeff King 2017-09-30 7:38 ` [PATCH 0/5] Support %(trailers) arguments in for-each-ref(1) Jeff King 2017-09-30 18:41 ` [PATCH v2 0/6] " Taylor Blau 2017-09-30 18:46 ` [PATCH v2 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-09-30 18:46 ` [PATCH v2 2/6] t6300: refactor %(trailers) tests Taylor Blau 2017-09-30 18:46 ` [PATCH v2 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau 2017-09-30 18:46 ` [PATCH v2 4/6] doc: use modern "`"-style code fencing Taylor Blau 2017-09-30 18:46 ` [PATCH v2 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-09-30 18:46 ` [PATCH v2 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 2017-10-01 0:06 ` [PATCH v2 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-10-01 0:10 ` [PATCH v3 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-01 0:10 ` [PATCH v3 2/6] t6300: refactor %(trailers) tests Taylor Blau 2017-10-01 0:10 ` [PATCH v3 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau 2017-10-01 0:10 ` [PATCH v3 4/6] doc: use modern "`"-style code fencing Taylor Blau 2017-10-01 0:10 ` [PATCH v3 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-10-01 0:10 ` [PATCH v3 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 2017-10-01 16:17 ` [PATCH v4 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-10-01 16:18 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-01 16:18 ` [PATCH v4 2/6] t6300: refactor %(trailers) tests Taylor Blau 2017-10-02 0:12 ` Junio C Hamano 2017-10-01 16:18 ` [PATCH v4 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau 2017-10-01 16:18 ` [PATCH v4 4/6] doc: use modern "`"-style code fencing Taylor Blau 2017-10-01 23:55 ` Junio C Hamano 2017-10-02 0:06 ` Taylor Blau 2017-10-02 1:35 ` Junio C Hamano 2017-10-02 4:53 ` Jeff King 2017-10-01 16:18 ` [PATCH v4 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-10-02 0:13 ` Junio C Hamano 2017-10-01 16:18 ` [PATCH v4 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 2017-10-02 0:19 ` Junio C Hamano 2017-10-02 0:11 ` [PATCH v4 1/6] pretty.c: delimit "%(trailers)" arguments with "," Junio C Hamano 2017-10-02 5:00 ` Jeff King 2017-10-02 0:31 ` [PATCH v5 0/6] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-10-02 0:32 ` [PATCH v5 1/6] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-02 0:33 ` [PATCH v5 2/6] t6300: refactor %(trailers) tests Taylor Blau 2017-10-02 0:33 ` [PATCH v5 3/6] doc: 'trailers' is the preferred way to format trailers Taylor Blau 2017-10-02 0:33 ` [PATCH v5 4/6] doc: use modern "`"-style code quoting Taylor Blau 2017-10-02 0:33 ` [PATCH v5 5/6] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-10-02 0:33 ` [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 2017-10-02 4:26 ` Junio C Hamano 2017-10-02 4:51 ` Junio C Hamano 2017-10-02 5:13 ` Taylor Blau 2017-10-02 5:03 ` Jeff King 2017-10-02 5:12 ` Taylor Blau 2017-10-02 5:14 ` Jeff King 2017-10-02 5:23 ` [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) Taylor Blau 2017-10-02 5:25 ` [PATCH v6 1/7] pretty.c: delimit "%(trailers)" arguments with "," Taylor Blau 2017-10-02 5:25 ` [PATCH v6 2/7] t4205: unfold across multiple lines Taylor Blau 2017-10-02 5:25 ` [PATCH v6 3/7] doc: 'trailers' is the preferred way to format trailers Taylor Blau 2017-10-02 5:25 ` [PATCH v6 4/7] doc: use modern "`"-style code quoting Taylor Blau 2017-10-02 5:25 ` [PATCH v6 5/7] t6300: refactor %(trailers) tests Taylor Blau 2017-10-02 5:25 ` [PATCH v6 6/7] ref-filter.c: use trailer_opts to format trailers Taylor Blau 2017-10-02 5:25 ` [PATCH v6 7/7] ref-filter.c: parse trailers arguments with %(contents) atom Taylor Blau 2017-10-02 6:51 ` Jeff King 2017-10-02 9:52 ` Junio C Hamano 2017-10-02 15:49 ` Taylor Blau 2017-10-02 23:44 ` Junio C Hamano 2017-10-02 6:56 ` [PATCH v6 0/7] Support %(trailers) arguments in for-each-ref(1) Jeff King 2017-10-03 6:24 ` Junio C Hamano 2017-10-03 6:36 ` Jeff King 2017-10-03 6:40 ` Junio C Hamano 2017-10-02 8:07 ` Junio C Hamano 2017-10-02 12:15 ` Junio C Hamano 2017-10-02 16:07 ` Taylor Blau
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git 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 read-only IMAP folder(s) and NNTP newsgroup(s).