Re: [PATCH v3 3/3] ref-filter: use pretty.c logic for trailers

From: Hariom verma <hariom18599@gmail.com>
Date: Feb 7, 2021, 12:06 UTC
Message-ID: <CA+CkUQ9-OCiEkMDRTpyF3rp-g1mSSzn4s9MgqJZ2BJY=XJCoEw@mail.gmail.com>
In-Reply-To: <xmqqpn1c8m7u.fsf@gitster.c.googlers.com>

Hi,

On Sun, Feb 7, 2021 at 11:15 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 43 quoted lines

>
> "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +test_trailer_option() {
> > +     title="$1"
> > +     option="$2"
> > +     expect="$3"
> > +     test_expect_success "$title" '
> > +             printf "$expect\n" >expect &&
> > +             git for-each-ref --format="%($option)" refs/heads/main >actual &&
> > +             test_cmp expect actual &&
> > +             git for-each-ref --format="%(contents:$option)" refs/heads/main >actual &&
> > +             test_cmp expect actual
> > +     '
> > +}
> > +
> > +test_trailer_option '%(trailers:key=foo) shows that trailer' \
> > +     'trailers:key=Signed-off-by' 'Signed-off-by: A U Thor <author@example.com>\n'
>
> This is *not* an issue about the test script and its helper
> function, but I just noticed that --format="%(trailers:key=<key>)"
> is expected to write the matching trailers *AND* an empty line, and
> I wonder if that is a sensible thing to expect.
>
> The "--pretty" side does not give such an extra blank line after the
> output, though.
>
>  $ git show -s --pretty=format:"%(trailers:key=Signed-off-by:)" \
>    js/range-diff-wo-dotdot
>  Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>  Signed-off-by: Junio C Hamano <gitster@pobox.com>
>  $ git show -s --pretty=format:"%(trailers:key=None:)" \
>    js/range-diff-wo-dotdot
>  $ exit
>
> Unlike the above, when there is no matching trailer lines, the
> "for-each-ref" in this series shows zero lines, and when there is
> one matching trailer line, it gives that single line plus an empty
> line, two lines in total.  The inconsistency is a bit disturbing.
>
> Is the extra blank line given on purpose?  I don't see why we would
> want it.  Or is it a bug we did not catch during the previous two
> rounds of reviews?

I don't think that "extra blank line" is due to this patch series. Wait. Let me see.

Since "for-each-ref"'s original code does not support
"trailers:key=<KEY>", Let's check original code for "trailers:only":
```
  $ git for-each-ref refs/heads/master --format="%(trailers:only)"
  Signed-off-by: Junio C Hamano <gitster@pobox.com>

  $ exit
```
I see. The original code also gives an extra blank line.

Now, let's check for this patch series:
```
  $ ./bin-wrappers/git for-each-ref refs/heads/master
--format="%(trailers:key=Signed-off-by)"
  Signed-off-by: Junio C Hamano <gitster@pobox.com>

  $ ./bin-wrappers/git for-each-ref refs/heads/master
--format="%(trailers:key=None)"

  $ exit
```
when there is no matching trailer lines, the "for-each-ref" in this
series shows one empty line, and when there is one matching trailer
line, it gives that single line plus an empty line, two lines in
total. Seems consistent to me.

So this isn't about the patch series. Question still remains the same. Why extra blank line? Let's dig a bit. Ah. I guess I found the reason. It's due to `putchar('\n');` in `show_ref_array_item() [1]`. It puts a new line after each ref item.

Do you want me to include a patch to get rid of this "extra blank line" for trailers in "for-each-ref"?

Thanks, Hariom.

[1]: https://github.com/git/git/blob/fb7fa4a1fd273f22efcafdd13c7f897814fd1eb9/ref-filter.c#L2435