Re: [PATCH v2 2/2] ref-filter: add support for %(contents:size)

[Christian Couder <christian.couder@gmail.com>]

On Mon, Jul 6, 2020 at 11:44 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > It's useful and efficient to be able to get the size of the
> > contents directly without having to pipe through `wc -c`.
> >
> > Also the result of the following:
> >
> > `git for-each-ref --format='%(contents)' | wc -c`
> >
> > is off by one as `git for-each-ref` appends a newline character
> > after the contents, which can be seen by comparing its ouput
> > with the output from `git cat-file`.
>
> So that's off by number of refs that are shown?

Yeah, true. I should have added a ref as I mean something like the
following is off by one:

`git for-each-ref --format='%(contents)' refs/heads/my-branch | wc -c`

I have changed it to the above in my current version.

> > +contents:size::
> > +     The size in bytes of the complete message.
> > +
>
> Complete as opposed to what?

In the existing documentation there is already "The complete message
of a commit or tag object is `contents`. "

So yeah I could add another preparatory patch to change the above to
something like:

"The complete message (subject, body, trailers and signature) of a
commit or tag object is `contents`. "

> What happens when the object referred to by the ref is not a commit
> or a tag?

Right now %(contents) shows nothing (a blank line actually) when the
ref points to something other than a commit or a tag, and
%(contents:size) does the same:

```
$ git update-ref refs/mytrees/first HEAD^{tree}
$ git for-each-ref --format='%(contents)' refs/mytrees/first

$ git for-each-ref --format='%(contents:size)' refs/mytrees/first

```

I am not sure if it's worth updating the existing documentation or
just the commit message of this patch. For now I have done the latter
in my current version.

> I am fine if it just is silently ignored (which is consistent with
> already existing behaviour of other requests that do not make sense
> for the given type) if the thing is a blob or a tree, but we'd need
> to cover the case with a test or two.  It seems you only expect this
> with a tag object and do not have any test that checks for other
> types of objects?

My patch already adds 1 test with a commit:

--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -125,6 +125,7 @@ test_atom head contents:body ''
 test_atom head contents:signature ''
 test_atom head contents 'Initial
 '
+test_atom head contents:size '8'
 test_atom head HEAD '*'

There is only one test with a commit, because that's already the case
for %(contents) too.

I am ok with adding another preparatory patch to the series that would
add a few more test cases with commits, trees and blobs though.