Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Christian Couder <christian.couder@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH v3 4/4] ref-filter: add support for %(contents:size)
Date: Tue, 07 Jul 2020 12:45:39 -0700
Message-ID: <xmqqk0zf3y8s.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200707174049.21714-5-chriscool@tuxfamily.org> (Christian Couder's message of "Tue, 7 Jul 2020 19:40:49 +0200")

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)' 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.

> @@ -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.

> +# 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

  reply index

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-07 17:40 [PATCH v3 0/4] Add support for %(contents:size) in ref-filter Christian Couder
2020-07-07 17:40 ` [PATCH v3 1/4] Documentation: clarify %(contents:XXXX) doc Christian Couder
2020-07-07 19:26   ` Junio C Hamano
2020-07-10 16:47     ` Christian Couder
2020-07-07 17:40 ` [PATCH v3 2/4] Documentation: clarify 'complete message' Christian Couder
2020-07-07 19:19   ` Junio C Hamano
2020-07-07 17:40 ` [PATCH v3 3/4] t6300: test refs pointing to tree and blob Christian Couder
2020-07-07 19:32   ` Junio C Hamano
2020-07-07 17:40 ` [PATCH v3 4/4] ref-filter: add support for %(contents:size) Christian Couder
2020-07-07 19:45   ` Junio C Hamano [this message]
2020-07-09  0:14     ` Junio C Hamano
2020-07-09  8:10       ` Christian Couder
2020-07-09 13:47         ` Junio C Hamano
2020-07-07 22:21   ` Junio C Hamano
2020-07-08 23:05   ` Junio C Hamano
2020-07-10 16:47 ` [PATCH v4 0/3] Add support for %(contents:size) in ref-filter Christian Couder
2020-07-10 16:47   ` [PATCH v4 1/3] Documentation: clarify %(contents:XXXX) doc Christian Couder
2020-07-10 20:24     ` Junio C Hamano
2020-07-10 16:47   ` [PATCH v4 2/3] t6300: test refs pointing to tree and blob Christian Couder
2020-07-10 20:24     ` Junio C Hamano
2020-07-10 16:47   ` [PATCH v4 3/3] ref-filter: add support for %(contents:size) Christian Couder
2020-07-10 20:38     ` Junio C Hamano
2020-07-16 12:19   ` [PATCH v5 0/3] Add support for %(contents:size) in ref-filter Christian Couder
2020-07-16 12:19     ` [PATCH v5 1/3] Documentation: clarify %(contents:XXXX) doc Christian Couder
2020-07-16 12:19     ` [PATCH v5 2/3] t6300: test refs pointing to tree and blob Christian Couder
2020-07-16 12:19     ` [PATCH v5 3/3] ref-filter: add support for %(contents:size) Christian Couder
2020-07-31 17:37       ` Alban Gruin
2020-07-31 17:45         ` [PATCH v1] t6300: fix issues related to %(contents:size) Alban Gruin
2020-07-31 17:47           ` Jeff King
2020-07-31 18:24             ` Alban Gruin
2020-07-31 20:04             ` Junio C Hamano
2020-07-31 20:30               ` Jeff King
2020-07-31 18:26           ` [PATCH v2] " Alban Gruin
2020-07-31 19:15             ` Jeff King
2020-07-31 17:45         ` [PATCH v5 3/3] ref-filter: add support for %(contents:size) Jeff King
2020-07-31 20:12           ` Christian Couder
2020-07-31 20:30             ` Junio C Hamano
2020-07-31 20:40               ` Jeff King
2020-07-16 17:48     ` [PATCH v5 0/3] Add support for %(contents:size) in ref-filter Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqk0zf3y8s.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git