On Mon, Jul 6, 2020 at 11:44 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> 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.
