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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2020, 19:45 UTC
Message-ID: <xmqqk0zf3y8s.fsf@gitster.c.googlers.com>
In-Reply-To: <20200707174049.21714-5-chriscool@tuxfamily.org>

Christian Couder <christian.couder@gmail.com> writes:

Show 28 quoted lines

> 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)' refs/heads/my-branch | 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 output
> with the output from `git cat-file`.
>
> As with %(contents), %(contents:size) is silently ignored, if a
> ref points to something other than a commit or a tag:
>
> ```
> $ 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
>
> ```
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/git-for-each-ref.txt |  3 +++
>  ref-filter.c                       |  7 ++++++-
>  t/t6300-for-each-ref.sh            | 19 +++++++++++++++++++
>  3 files changed, 28 insertions(+), 1 deletion(-)

Nice. The only questionable thing here is if we later regret for assuming that all sizes are always measured in bytes. If we later find an application that wants an efficient access to "| wc -l" (instead of "| wc -c" that motivated this patch), we'd want to be able to say "%(contents:lines)" and at that point we may want to go back in time and call this "%(contents:bytes)" or something.

>  test_atom head contents 'Initial
>  '
> +test_atom head contents:size '8'

These two are tied together (any change to the test script that causes us to update the former also forces us to update the latter), but I do not think of a way to unify the test without writing too much boilerplate code, so let's say this is good enough at least for now (but I may change my opinion as I read along).

>  test_atom tag contents 'Tagging at 1151968727
>  '
> +test_atom tag contents:size '22'

Likewise.

Show 5 quoted lines

> @@ -580,6 +582,7 @@ test_atom refs/tags/subject-body contents 'the subject line
>  first body line
>  second body line
>  '
> +test_atom refs/tags/subject-body contents:size '51'

Likewise.

Of course, we _could_ update the test_atom to do something magic only when the 'contents' atom is being asked. We notice that $2 is 'contents', do the usual test_expect_success for 'contents', and then measure the byte length of $3 ourselves and test 'contents:size'. That way, all the above test updates would become unnecessary (and the last two hunks of this patch can also go).

That approach may even allow you to hide the details of sanitize-pgp in the updated test_atom so that the actual tests may not have to get updated even for signed tags.

Show 53 quoted lines

> +# We cannot use test_atom to check contents:size of signed tags due to sanitize_pgp
> +test_tag_contents_size_pgp () {
> +	ref="$1"
> +	test_expect_success $PREREQ "basic atom: $ref contents:size" "
> +		git cat-file tag $ref | tail -n +6 | wc -c >expected &&
> +		git for-each-ref --format='%(contents:size)' $ref >actual &&
> +		test_cmp expected actual
> +	"
> +}
> +
>  PREREQ=GPG
>  test_atom refs/tags/signed-empty subject ''
>  test_atom refs/tags/signed-empty contents:subject ''
> @@ -629,6 +643,7 @@ test_atom refs/tags/signed-empty body "$sig"
>  test_atom refs/tags/signed-empty contents:body ''
>  test_atom refs/tags/signed-empty contents:signature "$sig"
>  test_atom refs/tags/signed-empty contents "$sig"
> +test_tag_contents_size_pgp refs/tags/signed-empty
>  
>  test_atom refs/tags/signed-short subject 'subject line'
>  test_atom refs/tags/signed-short contents:subject 'subject line'
> @@ -637,6 +652,7 @@ test_atom refs/tags/signed-short contents:body ''
>  test_atom refs/tags/signed-short contents:signature "$sig"
>  test_atom refs/tags/signed-short contents "subject line
>  $sig"
> +test_tag_contents_size_pgp refs/tags/signed-short
>  
>  test_atom refs/tags/signed-long subject 'subject line'
>  test_atom refs/tags/signed-long contents:subject 'subject line'
> @@ -649,6 +665,7 @@ test_atom refs/tags/signed-long contents "subject line
>  
>  body contents
>  $sig"
> +test_tag_contents_size_pgp refs/tags/signed-long
>  
>  test_expect_success 'set up refs pointing to tree and blob' '
>  	git update-ref refs/mytrees/first refs/heads/master^{tree} &&
> @@ -664,6 +681,7 @@ test_atom refs/mytrees/first body ""
>  test_atom refs/mytrees/first contents:body ""
>  test_atom refs/mytrees/first contents:signature ""
>  test_atom refs/mytrees/first contents ""
> +test_atom refs/mytrees/first contents:size ""
>  
>  test_atom refs/myblobs/first subject ""
>  test_atom refs/myblobs/first contents:subject ""
> @@ -671,6 +689,7 @@ test_atom refs/myblobs/first body ""
>  test_atom refs/myblobs/first contents:body ""
>  test_atom refs/myblobs/first contents:signature ""
>  test_atom refs/myblobs/first contents ""
> +test_atom refs/myblobs/first contents:size ""
>  
>  test_expect_success 'set up multiple-sort tags' '
>  	for when in 100000 200000