Re: [PATCH v2 2/5] pretty: allow showing specific trailers

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Nov 4, 2018 at 10:24 AM Anders Waldenborg <anders@0x63.nu> wrote:
> Adds a new "key=X" option to "%(trailers)" which will cause it to only
> print trailers lines which matches the specified key.
>
> Signed-off-by: Anders Waldenborg <anders@0x63.nu>
> ---
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> @@ -209,11 +209,14 @@ endif::git-rev-list[]
>  - %(trailers[:options]): display the trailers of the body as interpreted
>    by linkgit:git-interpret-trailers[1]. The `trailers` string may be
> +  followed by a colon and zero or more comma-separated options. The
> +  allowed options are `only` which omits non-trailer lines from the
> +  trailer block, `unfold` to make it behave as if interpret-trailer's
> +  `--unfold` option was given, and `key=T` to only show trailers with
> +  specified key (matching is done
> +  case-insensitively).

Does the user have to include the colon when specifying <val> of
'key=<val>'? I can see from peeking at the implementation that the
colon must not be used, but this should be documented. Should the code
tolerate a trailing colon? (Genuine question; it's easy to do and
would be more user-friendly.)

Does 'key=<val>', do a full or partial match on trailers? And, if
partial, is the match anchored at the start or can it match anywhere
in the trailer key? I see from the implementation that it does a full
match, but this behavior should be documented.

What happens if 'key=...' is specified multiple times? Are the
multiple keys conjunctive? Disjunctive? Last-wins? I can see from the
implementation that it is last-wins, but this behavior should be
documented. (I wonder how painful it will be for people who want to
match multiple keys. This doesn't have to be answered yet, as the
behavior can always be loosened later to allow multiple-key matching
since the current syntax does not disallow such expansion.)

Thinking further on the last two points, should <val> be a regular expression?

> +  shows all trailer lines, `%(trailers:key=Reviewed-by,unfold)`
> +  unfolds and shows trailer lines with key `Reviewed-by`.
> diff --git a/pretty.c b/pretty.c
> @@ -1323,7 +1323,19 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
> +                                       opts.filter_key = xstrndup(arg, end - arg);
> @@ -1331,6 +1343,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>                         format_trailers_from_commit(sb, msg + c->subject_off, &opts);
>                 }
> +               free(opts.filter_key);

If I understand correctly, this is making a copy of <val> so that it
will be NUL-terminated since the code added to trailer.c uses a simple
strcasecmp() to match it. Would it make sense to avoid the copy by
adding fields 'opts.filter_key' and 'opts.filter_key_len' and using
strncasecmp() instead? (Genuine question; not necessarily a request
for change.)

> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> @@ -598,6 +598,51 @@ test_expect_success ':only and :unfold work together' '
> +test_expect_success 'pretty format %(trailers:key=foo) shows that trailer' '
> +       git log --no-walk --pretty="%(trailers:key=Acked-by)" >actual &&
> +       {
> +               echo "Acked-by: A U Thor <author@example.com>" &&
> +               echo
> +       } >expect &&
> +       test_cmp expect actual
> +'

I guess these new tests are modeled after one or two existing tests
which use a series of 'echo' statements, but an alternative would be:

    cat <<-\EOF >expect &&
    Acked-by: A U Thor <author@example.com>

    EOF

or, even:

    test_write_lines "Acked-by: A U Thor <author@example.com>" "" &&

though, that's probably less readable.