Re: [PATCH v2 2/5] pretty: allow showing specific trailers
On Mon, Nov 5, 2018 at 3:26 AM Anders Waldenborg wrote: > Eric Sunshine writes: > > Should the code tolerate a trailing colon? (Genuine question; it's > > easy to do and would be more user-friendly.) > > I would make sense to allow the trailing colon, it is easy enough to > just strip that away when reading the argument. > > However I'm not sure how that would fit together with the possibility to > later lifting it to a regexp, hard to strip a trailing colon from a > regexp in a generic way. Which is a good reason to think about these issues now, before being set in stone. > > What happens if 'key=...' is specified multiple times? > > My first thought was to simply disallow that. But that seemed hard to > fit into current model where errors just means don't expand. > > I would guess that most useful and intuitive to user would be to handle > multiple key arguments by showing any of those keys. Agreed. > > Thinking further on the last two points, should be a regular > > expression? > > It probably would make sense. I can see how the regexp '^.*-by$' would > be useful (but glob style matching would suffice in that case). > > Also handling multi-matching with an alternation group would be elegant > %(trailers:key="(A|B)"). Except for the fact that the parser would need to > understand some kind of quoting, which seems like an major undertaking. Maybe, maybe not. As long as we're careful not to paint ourselves into a corner, it might very well be okay to start with the current implementation of matching the full key as a literal string and (perhaps much) later introduce regex as an alternate way to specify the key. For instance, 'key=literal' and 'key=/regex/' can co-exist, and the extraction of the regex inside /.../ should not be especially difficult. > I guess having it as a regular exception would also mean that it needs > to get some way to cache the re so it is compiled once, and not for each > expansion. Yes, that's something I brought up a few years ago during a GSoC project; not regex specifically, but that this parsing of the format is happening repeatedly rather than just once. I had suggested to the GSoC student that the parsing could be done early, compiling the format expression into a "machine" which could be applied repeatedly. It's a larger job, of course, not necessarily something worth tackling for your current needs. > > If I understand correctly, this is making a copy of so that it > > will be NUL-terminated since the code added to trailer.c uses a simple > > strcasecmp() to match it. Would it make sense to avoid the copy by > > adding fields 'opts.filter_key' and 'opts.filter_key_len' and using > > strncasecmp() instead? (Genuine question; not necessarily a request > > for change.) > > I'm also not very happy about that copy. > Just using strncasecmp would cause it to be prefix match, no? Well, you could retain full key match by checking for NUL explicitly with something like this: !strncasecmp(tok.buf, opts->filter_key, opts->filter_key_len) && !tok.buf[opts->filter_key_len]
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
Eric Sunshine writes:
> Should the code tolerate a trailing colon? (Genuine question; it's
> easy to do and would be more user-friendly.)
I would make sense to allow the trailing colon, it is easy enough to
just strip that away when reading the argument.
However I'm not sure how that would fit together with the possibility to
later lifting it to a regexp, hard to strip a trailing colon from a
regexp in a generic way.
> What happens if 'key=...' is specified multiple times?
My first thought was to simply disallow that. But that seemed hard to
fit into current model where errors just means don't expand.
I would guess that most useful and intuitive to user would be to handle
multiple key arguments by showing any of those keys.
> Thinking further on the last two points, should be a regular expression?
It probably would make sense. I can see how the regexp '^.*-by$' would
be useful (but glob style matching would suffice in that case).
Also handling multi-matching with an alternation group would be elegant
%(trailers:key="(A|B)"). Except for the fact that the parser would need to
understand some kind of quoting, which seems like an major undertaking.
I guess having it as a regular exception would also mean that it needs
to get some way to cache the re so it is compiled once, and not for each
expansion.
>
>> + free(opts.filter_key);
>
> If I understand correctly, this is making a copy of so that it
> will be NUL-terminated since the code added to trailer.c uses a simple
> strcasecmp() to match it. Would it make sense to avoid the copy by
> adding fields 'opts.filter_key' and 'opts.filter_key_len' and using
> strncasecmp() instead? (Genuine question; not necessarily a request
> for change.)
I'm also not very happy about that copy.
Just using strncasecmp would cause it to be prefix match, no?
But if changing matching semantics to handle multiple key options to
something else I'm thinking opts.filter_key would be replaced with a
opts.filter callback function, and that part would need to be rewritten
anyway.
>
>> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
>> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
>> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
>> + git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
>> + {
>> + echo "Acked-by: A U Thor " &&
>> + echo
>> + } >expect &&
>> + test_cmp expect actual
>> +'
>
> I guess these new tests are modeled after one or two existing tests
> which use a series of 'echo' statements
I guess I could change it to "--pretty=format:%(trailers:key=Acked-by)"
to get separator semantics and avoid that extra blank line, making it
simpler.
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
Anders Waldenborg writes:
> + else if (skip_prefix(arg, "key=", &arg)) {
> + const char *end = arg + strcspn(arg,
> ",)");
> +
> + if (opts.filter_key)
> + free(opts.filter_key);
Call the free() unconditionally, cf. contrib/coccinelle/free.cocci.
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
On Sun, Nov 4, 2018 at 10:48 PM Junio C Hamano wrote: > Eric Sunshine writes: > > Does the user have to include the colon when specifying of > > 'key='? > > Does 'key=', do a full or partial match on trailers? > > What happens if 'key=...' is specified multiple times? > > Thinking further on the last two points, should be a regular > > expression? > > Another thing that needs to be clarified in the document would be > case sensitivity. People sometimes spell "Signed-Off-By:" by > mistake (or is it by malice?). The documentation does say parenthetically "(matching is done case-insensitively)", so I think that's already covered. Or did you have something else in mind?
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
Eric Sunshine writes: > Does the user have to include the colon when specifying of > 'key='? I can see from peeking at the implementation that the > colon must not be used, but this should be documented. Should the code > tolerate a trailing colon? (Genuine question; it's easy to do and > would be more user-friendly.) > > Does 'key=', do a full or partial match on trailers? And, if > partial, is the match anchored at the start or can it match anywhere > in the trailer key? I see from the implementation that it does a full > match, but this behavior should be documented. > > What happens if 'key=...' is specified multiple times? Are the > multiple keys conjunctive? Disjunctive? Last-wins? I can see from the > implementation that it is last-wins, but this behavior should be > documented. (I wonder how painful it will be for people who want to > match multiple keys. This doesn't have to be answered yet, as the > behavior can always be loosened later to allow multiple-key matching > since the current syntax does not disallow such expansion.) > > Thinking further on the last two points, should be a regular expression? Another thing that needs to be clarified in the document would be case sensitivity. People sometimes spell "Signed-Off-By:" by mistake (or is it by malice?). I do suspect that the parser should just make a list of sought-after keys, not doing "last-one-wins", as that won't be very difficult to do and makes what happens when given multiple keys trivially obvious.
Re: [PATCH v2 2/5] pretty: allow showing specific trailers
On Sun, Nov 4, 2018 at 10:24 AM Anders Waldenborg wrote:
> Adds a new "key=X" option to "%(trailers)" which will cause it to only
> print trailers lines which matches the specified key.
>
> Signed-off-by: Anders Waldenborg
> ---
> diff --git a/Documentation/pretty-formats.txt
> b/Documentation/pretty-formats.txt
> @@ -209,11 +209,14 @@ endif::git-rev-list[]
> - %(trailers[:options]): display the trailers of the body as interpreted
>by linkgit:git-interpret-trailers[1]. The `trailers` string may be
> + followed by a colon and zero or more comma-separated options. The
> + allowed options are `only` which omits non-trailer lines from the
> + trailer block, `unfold` to make it behave as if interpret-trailer's
> + `--unfold` option was given, and `key=T` to only show trailers with
> + specified key (matching is done
> + case-insensitively).
Does the user have to include the colon when specifying of
'key='? I can see from peeking at the implementation that the
colon must not be used, but this should be documented. Should the code
tolerate a trailing colon? (Genuine question; it's easy to do and
would be more user-friendly.)
Does 'key=', do a full or partial match on trailers? And, if
partial, is the match anchored at the start or can it match anywhere
in the trailer key? I see from the implementation that it does a full
match, but this behavior should be documented.
What happens if 'key=...' is specified multiple times? Are the
multiple keys conjunctive? Disjunctive? Last-wins? I can see from the
implementation that it is last-wins, but this behavior should be
documented. (I wonder how painful it will be for people who want to
match multiple keys. This doesn't have to be answered yet, as the
behavior can always be loosened later to allow multiple-key matching
since the current syntax does not disallow such expansion.)
Thinking further on the last two points, should be a regular expression?
> + shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`
> + unfolds and shows trailer lines with key `Reviewed-by`.
> diff --git a/pretty.c b/pretty.c
> @@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /*
> in UTF-8 */
> + opts.filter_key = xstrndup(arg, end -
> arg);
> @@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /*
> in UTF-8 */
> format_trailers_from_commit(sb, msg + c->subject_off,
> &opts);
> }
> + free(opts.filter_key);
If I understand correctly, this is making a copy of so that it
will be NUL-terminated since the code added to trailer.c uses a simple
strcasecmp() to match it. Would it make sense to avoid the copy by
adding fields 'opts.filter_key' and 'opts.filter_key_len' and using
strncasecmp() instead? (Genuine question; not necessarily a request
for change.)
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
> + git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
> + {
> + echo "Acked-by: A U Thor " &&
> + echo
> + } >expect &&
> + test_cmp expect actual
> +'
I guess these new tests are modeled after one or two existing tests
which use a series of 'echo' statements, but an alternative would be:
cat <<-\EOF >expect &&
Acked-by: A U Thor
EOF
or, even:
test_write_lines "Acked-by: A U Thor " "" &&
though, that's probably less readable.
[PATCH v2 2/5] pretty: allow showing specific trailers
Adds a new "key=X" option to "%(trailers)" which will cause it to only
print trailers lines which matches the specified key.
Signed-off-by: Anders Waldenborg
---
Documentation/pretty-formats.txt | 13 +
pretty.c | 15 ++-
t/t4205-log-pretty-formats.sh| 45
trailer.c| 8 +++---
trailer.h| 1 +
5 files changed, 73 insertions(+), 9 deletions(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd..8326fc45e 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -209,11 +209,14 @@ endif::git-rev-list[]
respectively, but padding both sides (i.e. the text is centered)
- %(trailers[:options]): display the trailers of the body as interpreted
by linkgit:git-interpret-trailers[1]. The `trailers` string may be
- followed by a colon and zero or more comma-separated options. If the
- `only` option is given, omit non-trailer lines from the trailer block.
- If the `unfold` option is given, behave as if interpret-trailer's
- `--unfold` option was given. E.g., `%(trailers:only,unfold)` to do
- both.
+ followed by a colon and zero or more comma-separated options. The
+ allowed options are `only` which omits non-trailer lines from the
+ trailer block, `unfold` to make it behave as if interpret-trailer's
+ `--unfold` option was given, and `key=T` to only show trailers with
+ specified key (matching is done
+ case-insensitively). E.g. `%(trailers:only,unfold)` unfolds and
+ shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`
+ unfolds and shows trailer lines with key `Reviewed-by`.
NOTE: Some placeholders may depend on other options given to the
revision traversal engine. For example, the `%g*` reflog options will
diff --git a/pretty.c b/pretty.c
index aa03d5b23..cdca9dce2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
opts.only_trailers = 1;
else if (match_placeholder_arg(arg, "unfold",
&arg))
opts.unfold = 1;
- else
+ else if (skip_prefix(arg, "key=", &arg)) {
+ const char *end = arg + strcspn(arg,
",)");
+
+ if (opts.filter_key)
+ free(opts.filter_key);
+
+ opts.filter_key = xstrndup(arg, end -
arg);
+ arg = end;
+ if (*arg == ',')
+ arg++;
+
+ opts.only_trailers = 1;
+ } else
break;
}
}
@@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in
UTF-8 */
format_trailers_from_commit(sb, msg + c->subject_off,
&opts);
ret = arg - placeholder + 1;
}
+ free(opts.filter_key);
return ret;
}
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66f..0f5207242 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
test_cmp expect actual
'
+test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
+ git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
+ {
+ echo "Acked-by: A U Thor " &&
+ echo
+ } >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'pretty format %(trailers:key=foo) is case insensitive' '
+ git log --no-walk --pretty="%(trailers:key=AcKed-bY)" >actual &&
+ {
+ echo "Acked-by: A U Thor " &&
+ echo
+ } >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=nonexistant) becomes empty' '
+ git log --no-walk --pretty="x%(trailers:key=Nacked-by)x" >actual &&
+ {
+ echo "xx"
+ } >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo) handles multiple lines even if
folded' '
+ git log --no-walk --pretty="%(trailers:key=Signed-Off-by)" >actual &&
+ {
+ grep -v patch.description expect &&
+ test_cmp expect actual
+'
+
+test_expect_success '%(trailers:key=foo,unfold) properly unfolds' '
+ git log --no-walk --pretty="%(trailers:key=Signed-Off-by,unfold)"
>actual &&
+ {
+ echo "Signed-off-by: A U Thor " &&
+ echo "Signed-off-by: A U Thor " &&
+
