[PATCH v3 0/4] Add support for %(contents:size) in ref-filter

[PATCH v3 0/4] Add support for %(contents:size) in ref-filter

[Christian Couder]

This is version 3 of a small patch series to teach ref-filter about
%(contents:size).

This patch series is based on current master.

Previous versions and related discussions are here:

V1: https://lore.kernel.org/git/20200701132308.16691-1-chriscool@tuxfamily.org/
V2: https://lore.kernel.org/git/20200702140845.24945-1-chriscool@tuxfamily.org/

Thanks to Junio and Peff for their reviews of this series!

The changes compared to V2 are the following:

  - Added patch 2/4 that clarifies the meaning of "complete message"
    in the doc.

  - Added patch 3/4 that adds tests for refs pointing to a tree or a
    blob.

  - Improved commit message in patch 4/4 as suggested by Junio.

  - Added %(contents:size) tests in patch 4/4 for refs pointing to a
    tree or a blob.

The range diff is:

1:  c6e80b8bc1 = 1:  b04b390f32 Documentation: clarify %(contents:XXXX) doc
-:  ---------- > 2:  b62cab2630 Documentation: clarify 'complete message'
-:  ---------- > 3:  b9584472a1 t6300: test refs pointing to tree and blob
2:  9853b37091 ! 4:  23f941132e ref-filter: add support for %(contents:size)
    @@ Commit message
     
         Also the result of the following:
     
    -    `git for-each-ref --format='%(contents)' | wc -c`
    +    `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 ouput
    +    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 ##
    -@@ Documentation/git-for-each-ref.txt: and `date` to extract the named component.
    - The complete message of a commit or tag object is `contents`. This
    - field can also be used in the following ways:
    +@@ Documentation/git-for-each-ref.txt: The complete message (subject, body, trailers and signature) of a
    + commit or tag object is `contents`. This field can also be used in the
    + following ways:
      
     +contents:size::
     +  The size in bytes of the complete message.
    @@ t/t6300-for-each-ref.sh: test_atom refs/tags/signed-long contents "subject line
      $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} &&
    +@@ t/t6300-for-each-ref.sh: 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 ""
    +@@ t/t6300-for-each-ref.sh: 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

Christian Couder (4):
  Documentation: clarify %(contents:XXXX) doc
  Documentation: clarify 'complete message'
  t6300: test refs pointing to tree and blob
  ref-filter: add support for %(contents:size)

 Documentation/git-for-each-ref.txt | 28 +++++++++++++++-----
 ref-filter.c                       |  7 ++++-
 t/t6300-for-each-ref.sh            | 41 ++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 7 deletions(-)

-- 
2.27.0.460.g66f3a24dd5

[PATCH v3 1/4] Documentation: clarify %(contents:XXXX) doc

[Christian Couder]

Let's avoid a big dense paragraph by using an unordered
list for the %(contents:XXXX) format specifiers.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-for-each-ref.txt | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 6dcd39f6f6..2db9779d54 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -232,12 +232,24 @@ Fields that have name-email-date tuple as its value (`author`,
 `committer`, and `tagger`) can be suffixed with `name`, `email`,
 and `date` to extract the named component.
 
-The complete message in a commit and tag object is `contents`.
-Its first line is `contents:subject`, where subject is the concatenation
-of all lines of the commit message up to the first blank line.  The next
-line is `contents:body`, where body is all of the lines after the first
-blank line.  The optional GPG signature is `contents:signature`.  The
-first `N` lines of the message is obtained using `contents:lines=N`.
+The complete message of a commit or tag object is `contents`. This
+field can also be used in the following ways:
+
+contents:subject::
+	The "subject" of the commit or tag message.  It's actually the
+	concatenation of all lines of the commit message up to the
+	first blank line.
+
+contents:body::
+	The "body" of the commit or tag message.  It's made of the
+	lines after the first blank line.
+
+contents:signature::
+	The optional GPG signature.
+
+contents:lines=N::
+	The first `N` lines of the message.
+
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
 are obtained as `trailers` (or by using the historical alias
 `contents:trailers`).  Non-trailer lines from the trailer block can be omitted
-- 
2.27.0.460.g66f3a24dd5

[PATCH v3 2/4] Documentation: clarify 'complete message'

[Christian Couder]

In Documentation/git-for-each-ref.txt let's clarify what
we mean by "complete message".

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-for-each-ref.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 2db9779d54..788258c3ad 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -232,8 +232,9 @@ Fields that have name-email-date tuple as its value (`author`,
 `committer`, and `tagger`) can be suffixed with `name`, `email`,
 and `date` to extract the named component.
 
-The complete message of a commit or tag object is `contents`. This
-field can also be used in the following ways:
+The complete message (subject, body, trailers and signature) of a
+commit or tag object is `contents`. This field can also be used in the
+following ways:
 
 contents:subject::
 	The "subject" of the commit or tag message.  It's actually the
-- 
2.27.0.460.g66f3a24dd5

[PATCH v3 3/4] t6300: test refs pointing to tree and blob

[Christian Couder]

Adding tests for refs pointing to tree and blob shows that
we care about testing both positive ("see, my shiny new toy
does work") and negative ("and it won't do nonsensical
things when given an input it is not designed to work with")
cases.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6300-for-each-ref.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index da59fadc5d..371e45e5ad 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -650,6 +650,28 @@ test_atom refs/tags/signed-long contents "subject line
 body contents
 $sig"
 
+test_expect_success 'set up refs pointing to tree and blob' '
+	git update-ref refs/mytrees/first refs/heads/master^{tree} &&
+	git ls-tree refs/mytrees/first one >one_info &&
+	test $(cut -d" " -f2 one_info) = "blob" &&
+	blob_hash=$(cut "-d	" -f1 one_info | cut -d" " -f3) &&
+	git update-ref refs/myblobs/first "$blob_hash"
+'
+
+test_atom refs/mytrees/first subject ""
+test_atom refs/mytrees/first contents:subject ""
+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/myblobs/first subject ""
+test_atom refs/myblobs/first contents:subject ""
+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_expect_success 'set up multiple-sort tags' '
 	for when in 100000 200000
 	do
-- 
2.27.0.460.g66f3a24dd5

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

[Christian Couder]

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(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 788258c3ad..049bc93e6a 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -236,6 +236,9 @@ The complete message (subject, body, trailers and signature) of a
 commit or tag object is `contents`. This field can also be used in the
 following ways:
 
+contents:size::
+	The size in bytes of the complete message.
+
 contents:subject::
 	The "subject" of the commit or tag message.  It's actually the
 	concatenation of all lines of the commit message up to the
diff --git a/ref-filter.c b/ref-filter.c
index 8447cb09be..8ec28f0535 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -127,7 +127,8 @@ static struct used_atom {
 			unsigned int nobracket : 1, push : 1, push_remote : 1;
 		} remote_ref;
 		struct {
-			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
+			enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH,
+			       C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
 			struct process_trailer_options trailer_opts;
 			unsigned int nlines;
 		} contents;
@@ -338,6 +339,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
 		atom->u.contents.option = C_BARE;
 	else if (!strcmp(arg, "body"))
 		atom->u.contents.option = C_BODY;
+	else if (!strcmp(arg, "size"))
+		atom->u.contents.option = C_LENGTH;
 	else if (!strcmp(arg, "signature"))
 		atom->u.contents.option = C_SIG;
 	else if (!strcmp(arg, "subject"))
@@ -1253,6 +1256,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			v->s = copy_subject(subpos, sublen);
 		else if (atom->u.contents.option == C_BODY_DEP)
 			v->s = xmemdupz(bodypos, bodylen);
+		else if (atom->u.contents.option == C_LENGTH)
+			v->s = xstrfmt("%ld", strlen(subpos));
 		else if (atom->u.contents.option == C_BODY)
 			v->s = xmemdupz(bodypos, nonsiglen);
 		else if (atom->u.contents.option == C_SIG)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 371e45e5ad..b580e27a32 100755
--- 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 '*'
 
 test_atom tag refname refs/tags/testtag
@@ -170,6 +171,7 @@ test_atom tag contents:body ''
 test_atom tag contents:signature ''
 test_atom tag contents 'Tagging at 1151968727
 '
+test_atom tag contents:size '22'
 test_atom tag HEAD ' '
 
 test_expect_success 'Check invalid atoms names are errors' '
@@ -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'
 
 test_expect_success 'create tag with multiline subject' '
 	cat >msg <<-\EOF &&
@@ -606,6 +609,7 @@ second subject line
 first body line
 second body line
 '
+test_atom refs/tags/multiline contents:size '73'
 
 test_expect_success GPG 'create signed tags' '
 	git tag -s -m "" signed-empty &&
@@ -622,6 +626,16 @@ sig='-----BEGIN PGP SIGNATURE-----
 -----END PGP SIGNATURE-----
 '
 
+# 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
-- 
2.27.0.460.g66f3a24dd5

Re: [PATCH v3 2/4] Documentation: clarify 'complete message'

[Junio C Hamano]

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

> In Documentation/git-for-each-ref.txt let's clarify what
> we mean by "complete message".
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/git-for-each-ref.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 2db9779d54..788258c3ad 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -232,8 +232,9 @@ Fields that have name-email-date tuple as its value (`author`,
>  `committer`, and `tagger`) can be suffixed with `name`, `email`,
>  and `date` to extract the named component.
>  
> -The complete message of a commit or tag object is `contents`. This
> -field can also be used in the following ways:
> +The complete message (subject, body, trailers and signature) of a
> +commit or tag object is `contents`. This field can also be used in the
> +following ways:

Hmph, I regret asking what is "complete", i.e. as opposed to what.

The above makes it even unclear if things like "signature on commit"
is part of the complete message.  I _think_ you meant the part after
stripping the object header, so "signature in a signed tag is part
of 'complete message', while signature in a signed commit is not",
which feels somewhat strange.

But then, it may be easier to understand if we said

    The message in a commit or a tag object is `contents`, from
    which `contents:<part>` can be used to extract various parts out
    of.

without introducing "complete".

In any case I think patches 1 & 2 are definite improvement.

Thanks.

Re: [PATCH v3 1/4] Documentation: clarify %(contents:XXXX) doc

[Junio C Hamano]

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

> +The complete message of a commit or tag object is `contents`. This
> +field can also be used in the following ways:
> +
> +contents:subject::
> +	The "subject" of the commit or tag message.  It's actually the
> +	concatenation of all lines of the commit message up to the
> +	first blank line.

Let's avoid confusing readers by saying "A is X. It's actually Y".

    The first paragraph of the message, which typically is a single
    line, is taken as the "subject" of the commit or the tag
    message.

> +contents:body::
> +	The "body" of the commit or tag message.  It's made of the
> +	lines after the first blank line.

    The remainder of the commit or the tag message that follows the
    "subject".

> +contents:signature::
> +	The optional GPG signature.

I _think_ this only applies to signed tag objects and not signed
commit objects, but this text does not help to decide if I am
right.

> +contents:lines=N::
> +	The first `N` lines of the message.

Good.

Re: [PATCH v3 3/4] t6300: test refs pointing to tree and blob

[Junio C Hamano]

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

> Adding tests for refs pointing to tree and blob shows that
> we care about testing both positive ("see, my shiny new toy
> does work") and negative ("and it won't do nonsensical
> things when given an input it is not designed to work with")
> cases.

Nice.

>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t6300-for-each-ref.sh | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index da59fadc5d..371e45e5ad 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -650,6 +650,28 @@ test_atom refs/tags/signed-long contents "subject line
>  body contents
>  $sig"
>  
> +test_expect_success 'set up refs pointing to tree and blob' '
> +	git update-ref refs/mytrees/first refs/heads/master^{tree} &&
> +	git ls-tree refs/mytrees/first one >one_info &&
> +	test $(cut -d" " -f2 one_info) = "blob" &&
> +	blob_hash=$(cut "-d	" -f1 one_info | cut -d" " -f3) &&
> +	git update-ref refs/myblobs/first "$blob_hash"

Wouldn't it be sufficient to say

	git update-ref refs/myblobs/first refs/heads/master:one

instead of the last 4 lines in this set-up?

> +'
> +
> +test_atom refs/mytrees/first subject ""
> +test_atom refs/mytrees/first contents:subject ""
> +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/myblobs/first subject ""
> +test_atom refs/myblobs/first contents:subject ""
> +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 ""

All makes sense.  We require "git for-each-ref" that asks for these
atoms in the format to silently exit successfully when the object at
the tip of a ref is of these types, so all these test_atom should
succeed.

Nicely written.

>  test_expect_success 'set up multiple-sort tags' '
>  	for when in 100000 200000
>  	do

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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

> +		else if (atom->u.contents.option == C_LENGTH)
> +			v->s = xstrfmt("%ld", strlen(subpos));

Please squash something like this in.  32-bit builds are failing.

 ref-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 8ec28f0535..73d8bfa86d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1257,7 +1257,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 		else if (atom->u.contents.option == C_BODY_DEP)
 			v->s = xmemdupz(bodypos, bodylen);
 		else if (atom->u.contents.option == C_LENGTH)
-			v->s = xstrfmt("%ld", strlen(subpos));
+			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
 		else if (atom->u.contents.option == C_BODY)
 			v->s = xmemdupz(bodypos, nonsiglen);
 		else if (atom->u.contents.option == C_SIG)

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

[Junio C Hamano]

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

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

Of course, this will BREAK the tests on macOS and possibly others
with "wc" that emits leading whitespaces before the number.

The tip of 'seen' has been failing ever since this topic was merged
there e.g. https://travis-ci.org/github/git/git/jobs/705986794

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

After seeing the "wc -c" portability issues, I am now even more
inclined to say that the above is the right direction.  The
portability worries can and should be encapsulated in a single
test_atom helper function, just as it can be used to hide the
differences between signed tags, annotated tags and commits.

Thanks.

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

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

[Christian Couder]

On Thu, Jul 9, 2020 at 2:14 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > 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.
>
> After seeing the "wc -c" portability issues, I am now even more
> inclined to say that the above is the right direction.  The
> portability worries can and should be encapsulated in a single
> test_atom helper function, just as it can be used to hide the
> differences between signed tags, annotated tags and commits.

Yeah, I have been working on that and will send a new patch series soon.
The current test_atom() change looks like this:

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 371e45e5ad..e514d98574 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -52,6 +52,26 @@ test_atom() {
                sanitize_pgp <actual >actual.clean &&
                test_cmp expected actual.clean
        "
+       # Automatically test "contents:size" atom after testing "contents"
+       if test "$2" = "contents"
+       then
+               case "$1" in
+               refs/tags/signed-*)
+                       # We cannot use $3 as it expects sanitize_pgp to run
+                       git cat-file tag $ref | tail -n +6 | \
+                               wc -c | sed -e 's/^ *//' >expected ;;
+               refs/mytrees/*)
+                       echo >expected ;;
+               refs/myblobs/*)
+                       echo >expected ;;
+               *)
+                       printf '%s' "$3" | wc -c | sed -e 's/^ *//' >expected ;;
+               esac
+               test_expect_${4:-success} $PREREQ "basic atom: $1 $2:size" "
+                       git for-each-ref --format='%($2:size)' $ref >actual &&
+                       test_cmp expected actual
+               "
+       fi
 }

I am wondering if it's worth adding a preparatory patch to introduce
an helper function like the following in test-lib-functions.sh:

+# test_byte_count outputs the number of bytes in files or stdin
+#
+# It is like wc -c but without portability issues, as on macOS and
+# possibly other platforms leading whitespaces are emitted before the
+# number.
+
+test_byte_count () {
+       wc -c "$@" | sed -e 's/^ *//'
+}

Not sure about the name of this helper function as it works
differently than test_line_count().

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

[Junio C Hamano]

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

> Yeah, I have been working on that and will send a new patch series soon.
> The current test_atom() change looks like this:
>
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 371e45e5ad..e514d98574 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -52,6 +52,26 @@ test_atom() {
>                 sanitize_pgp <actual >actual.clean &&
>                 test_cmp expected actual.clean
>         "
> +       # Automatically test "contents:size" atom after testing "contents"
> +       if test "$2" = "contents"
> +       then
> +               case "$1" in
> +               refs/tags/signed-*)
> +                       # We cannot use $3 as it expects sanitize_pgp to run
> +                       git cat-file tag $ref | tail -n +6 | \
> +                               wc -c | sed -e 's/^ *//' >expected ;;
> +               refs/mytrees/*)
> +                       echo >expected ;;
> +               refs/myblobs/*)
> +                       echo >expected ;;
> +               *)
> +                       printf '%s' "$3" | wc -c | sed -e 's/^ *//' >expected ;;
> +               esac
> +               test_expect_${4:-success} $PREREQ "basic atom: $1 $2:size" "
> +                       git for-each-ref --format='%($2:size)' $ref >actual &&
> +                       test_cmp expected actual
> +               "
> +       fi
>  }
>
> I am wondering if it's worth adding a preparatory patch to introduce
> an helper function like the following in test-lib-functions.sh:
>
> +# test_byte_count outputs the number of bytes in files or stdin
> +#
> +# It is like wc -c but without portability issues, as on macOS and
> +# possibly other platforms leading whitespaces are emitted before the
> +# number.
> +
> +test_byte_count () {
> +       wc -c "$@" | sed -e 's/^ *//'
> +}
>
> Not sure about the name of this helper function as it works
> differently than test_line_count().

Yeah, if I were writing it, I'd call it "sane_wc_c" or something.

But more importantly, I think the invocation of "|sed" is overkill.
If I were writing it, I would go more like...

	if test $2 = contents
	then
		case "$1" in 
		...)
			expect=$(git cat-file ... | wc -c)
			;;
		refs/mytrees/* | refs/myblobs/*)
			expect=0
			;;
		*)
			expect=$(printf ... | wc -c)
			;;
		esac

		# leave $expect unquoted to lose possible leading whitespaces
	        echo $expect >expect
		test_expect_success "..." '
			...
			test_cmp expect actual
		'
	fi

Re: [PATCH v3 1/4] Documentation: clarify %(contents:XXXX) doc

[Christian Couder]

On Tue, Jul 7, 2020 at 9:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > +contents:subject::
> > +     The "subject" of the commit or tag message.  It's actually the
> > +     concatenation of all lines of the commit message up to the
> > +     first blank line.
>
> Let's avoid confusing readers by saying "A is X. It's actually Y".
>
>     The first paragraph of the message, which typically is a single
>     line, is taken as the "subject" of the commit or the tag
>     message.

Ok.

> > +contents:body::
> > +     The "body" of the commit or tag message.  It's made of the
> > +     lines after the first blank line.
>
>     The remainder of the commit or the tag message that follows the
>     "subject".

Ok.

> > +contents:signature::
> > +     The optional GPG signature.
>
> I _think_ this only applies to signed tag objects and not signed
> commit objects, but this text does not help to decide if I am
> right.

You are right. It doesn't work for commits:

---------------------------------------------------
$ git cat-file commit refs/heads/signed_commit
tree 9773e6a54521a5d99928685e5f62e937fc6a7593
parent 1d1083b4c06fbb6055a2bd3d665a6d81468db5f5
author Christian Couder <chriscool@tuxfamily.org> 1594397089 +0200
committer Christian Couder <chriscool@tuxfamily.org> 1594397089 +0200
gpgsig -----BEGIN PGP SIGNATURE-----

 iQGzBAABCgAdFiEElaRidyyI6IQbXM36ch8PKcZMVRkFAl8IkaEACgkQch8PKcZM
[...]
 dkYKcRC3
 =fRMN
 -----END PGP SIGNATURE-----

Signed commit
$ git for-each-ref --format='%(contents:signature)' refs/heads/signed_commit

---------------------------------------------------

So I changed the description to:

contents:signature::
    The optional GPG signature of the tag.

[PATCH v4 0/3] Add support for %(contents:size) in ref-filter

[Christian Couder]

This is version 4 of a small patch series to teach ref-filter about
%(contents:size).

This patch series is based on master at 4a0fcf9f76 (The seventh batch,
2020-07-06).

Previous versions and related discussions are there:

V1: https://lore.kernel.org/git/20200701132308.16691-1-chriscool@tuxfamily.org/
V2: https://lore.kernel.org/git/20200702140845.24945-1-chriscool@tuxfamily.org/
V3: https://lore.kernel.org/git/20200707174049.21714-1-chriscool@tuxfamily.org/

Thanks to Junio and Peff for their reviews of this series!

The changes compared to V3 are the following:

  - Squashed patches 1/4 and 2/4 into 1/3 as they were both about
    %(contents) related doc improvements.

  - Improved patch 1/3 as suggested by Junio.

  - Simplified setup test in patch 2/3 about creating a ref pointing
    to a blob as suggested by Junio.

  - Modified test_atom() in patch 3/3 to automatically test
    %(contents:size) after testing %(contents) as suggested by Junio.

The range diff is:

1:  b04b390f32 ! 1:  f750832fc7 Documentation: clarify %(contents:XXXX) doc
    @@ Commit message
         Let's avoid a big dense paragraph by using an unordered
         list for the %(contents:XXXX) format specifiers.
     
    +    While at it let's also make the following improvements:
    +
    +      - Let's not describe %(contents) using "complete message"
    +        as it's not clear what an incomplete message is.
    +
    +      - Let's improve how the "subject" and "body" are
    +        described.
    +
    +      - Let's state that "signature" is only available for
    +        tag objects.
    +
         Suggested-by: Jeff King <peff@peff.net>
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
     
    @@ Documentation/git-for-each-ref.txt: Fields that have name-email-date tuple as it
     -line is `contents:body`, where body is all of the lines after the first
     -blank line.  The optional GPG signature is `contents:signature`.  The
     -first `N` lines of the message is obtained using `contents:lines=N`.
    -+The complete message of a commit or tag object is `contents`. This
    -+field can also be used in the following ways:
    ++The message in a commit or a tag object is `contents`, from which
    ++`contents:<part>` can be used to extract various parts out of:
     +
     +contents:subject::
    -+  The "subject" of the commit or tag message.  It's actually the
    -+  concatenation of all lines of the commit message up to the
    -+  first blank line.
    ++  The first paragraph of the message, which typically is a
    ++  single line, is taken as the "subject" of the commit or the
    ++  tag message.
     +
     +contents:body::
    -+  The "body" of the commit or tag message.  It's made of the
    -+  lines after the first blank line.
    ++  The remainder of the commit or the tag message that follows
    ++  the "subject".
     +
     +contents:signature::
    -+  The optional GPG signature.
    ++  The optional GPG signature of the tag.
     +
     +contents:lines=N::
     +  The first `N` lines of the message.
2:  b62cab2630 < -:  ---------- Documentation: clarify 'complete message'
3:  b9584472a1 ! 2:  51c72e09d2 t6300: test refs pointing to tree and blob
    @@ t/t6300-for-each-ref.sh: test_atom refs/tags/signed-long contents "subject line
      
     +test_expect_success 'set up refs pointing to tree and blob' '
     +  git update-ref refs/mytrees/first refs/heads/master^{tree} &&
    -+  git ls-tree refs/mytrees/first one >one_info &&
    -+  test $(cut -d" " -f2 one_info) = "blob" &&
    -+  blob_hash=$(cut "-d     " -f1 one_info | cut -d" " -f3) &&
    -+  git update-ref refs/myblobs/first "$blob_hash"
    ++  git update-ref refs/myblobs/first refs/heads/master:one
     +'
     +
     +test_atom refs/mytrees/first subject ""
4:  23f941132e < -:  ---------- ref-filter: add support for %(contents:size)
-:  ---------- > 3:  c2ed3e228b ref-filter: add support for %(contents:size)

Christian Couder (3):
  Documentation: clarify %(contents:XXXX) doc
  t6300: test refs pointing to tree and blob
  ref-filter: add support for %(contents:size)

 Documentation/git-for-each-ref.txt | 27 ++++++++++++++++-----
 ref-filter.c                       |  7 +++++-
 t/t6300-for-each-ref.sh            | 38 ++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 7 deletions(-)

-- 
2.27.0.347.gb620d8b0da

[PATCH v4 1/3] Documentation: clarify %(contents:XXXX) doc

[Christian Couder]

Let's avoid a big dense paragraph by using an unordered
list for the %(contents:XXXX) format specifiers.

While at it let's also make the following improvements:

  - Let's not describe %(contents) using "complete message"
    as it's not clear what an incomplete message is.

  - Let's improve how the "subject" and "body" are
    described.

  - Let's state that "signature" is only available for
    tag objects.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-for-each-ref.txt | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 6dcd39f6f6..b739412c30 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -232,12 +232,24 @@ Fields that have name-email-date tuple as its value (`author`,
 `committer`, and `tagger`) can be suffixed with `name`, `email`,
 and `date` to extract the named component.
 
-The complete message in a commit and tag object is `contents`.
-Its first line is `contents:subject`, where subject is the concatenation
-of all lines of the commit message up to the first blank line.  The next
-line is `contents:body`, where body is all of the lines after the first
-blank line.  The optional GPG signature is `contents:signature`.  The
-first `N` lines of the message is obtained using `contents:lines=N`.
+The message in a commit or a tag object is `contents`, from which
+`contents:<part>` can be used to extract various parts out of:
+
+contents:subject::
+	The first paragraph of the message, which typically is a
+	single line, is taken as the "subject" of the commit or the
+	tag message.
+
+contents:body::
+	The remainder of the commit or the tag message that follows
+	the "subject".
+
+contents:signature::
+	The optional GPG signature of the tag.
+
+contents:lines=N::
+	The first `N` lines of the message.
+
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
 are obtained as `trailers` (or by using the historical alias
 `contents:trailers`).  Non-trailer lines from the trailer block can be omitted
-- 
2.27.0.347.gb620d8b0da

[PATCH v4 2/3] t6300: test refs pointing to tree and blob

[Christian Couder]

Adding tests for refs pointing to tree and blob shows that
we care about testing both positive ("see, my shiny new toy
does work") and negative ("and it won't do nonsensical
things when given an input it is not designed to work with")
cases.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6300-for-each-ref.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index da59fadc5d..e9f468d360 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -650,6 +650,25 @@ test_atom refs/tags/signed-long contents "subject line
 body contents
 $sig"
 
+test_expect_success 'set up refs pointing to tree and blob' '
+	git update-ref refs/mytrees/first refs/heads/master^{tree} &&
+	git update-ref refs/myblobs/first refs/heads/master:one
+'
+
+test_atom refs/mytrees/first subject ""
+test_atom refs/mytrees/first contents:subject ""
+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/myblobs/first subject ""
+test_atom refs/myblobs/first contents:subject ""
+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_expect_success 'set up multiple-sort tags' '
 	for when in 100000 200000
 	do
-- 
2.27.0.347.gb620d8b0da

[PATCH v4 3/3] ref-filter: add support for %(contents:size)

[Christian Couder]

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(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index b739412c30..2ea71c5f6c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -235,6 +235,9 @@ and `date` to extract the named component.
 The message in a commit or a tag object is `contents`, from which
 `contents:<part>` can be used to extract various parts out of:
 
+contents:size::
+	The size in bytes of the commit or tag message.
+
 contents:subject::
 	The first paragraph of the message, which typically is a
 	single line, is taken as the "subject" of the commit or the
diff --git a/ref-filter.c b/ref-filter.c
index 8447cb09be..73d8bfa86d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -127,7 +127,8 @@ static struct used_atom {
 			unsigned int nobracket : 1, push : 1, push_remote : 1;
 		} remote_ref;
 		struct {
-			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
+			enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH,
+			       C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
 			struct process_trailer_options trailer_opts;
 			unsigned int nlines;
 		} contents;
@@ -338,6 +339,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
 		atom->u.contents.option = C_BARE;
 	else if (!strcmp(arg, "body"))
 		atom->u.contents.option = C_BODY;
+	else if (!strcmp(arg, "size"))
+		atom->u.contents.option = C_LENGTH;
 	else if (!strcmp(arg, "signature"))
 		atom->u.contents.option = C_SIG;
 	else if (!strcmp(arg, "subject"))
@@ -1253,6 +1256,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			v->s = copy_subject(subpos, sublen);
 		else if (atom->u.contents.option == C_BODY_DEP)
 			v->s = xmemdupz(bodypos, bodylen);
+		else if (atom->u.contents.option == C_LENGTH)
+			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
 		else if (atom->u.contents.option == C_BODY)
 			v->s = xmemdupz(bodypos, nonsiglen);
 		else if (atom->u.contents.option == C_SIG)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index e9f468d360..467871ac10 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -52,6 +52,25 @@ test_atom() {
 		sanitize_pgp <actual >actual.clean &&
 		test_cmp expected actual.clean
 	"
+	# Automatically test "contents:size" atom after testing "contents"
+	if test "$2" = "contents"
+	then
+		case "$1" in
+		refs/tags/signed-*)
+			# We cannot use $3 as it expects sanitize_pgp to run
+			expect=$(git cat-file tag $ref | tail -n +6 | wc -c) ;;
+		refs/mytrees/* | refs/myblobs/*)
+			expect='' ;;
+		*)
+			expect=$(printf '%s' "$3" | wc -c) ;;
+		esac
+		# Leave $expect unquoted to lose possible leading whitespaces
+		echo $expect >expected
+		test_expect_${4:-success} $PREREQ "basic atom: $1 $2:size" "
+			git for-each-ref --format='%($2:size)' $ref >actual &&
+			test_cmp expected actual
+		"
+	fi
 }
 
 hexlen=$(test_oid hexsz)
-- 
2.27.0.347.gb620d8b0da

Re: [PATCH v4 1/3] Documentation: clarify %(contents:XXXX) doc

[Junio C Hamano]

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

> Let's avoid a big dense paragraph by using an unordered
> list for the %(contents:XXXX) format specifiers.
>
> While at it let's also make the following improvements:
>
>   - Let's not describe %(contents) using "complete message"
>     as it's not clear what an incomplete message is.
>
>   - Let's improve how the "subject" and "body" are
>     described.
>
>   - Let's state that "signature" is only available for
>     tag objects.
>
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  Documentation/git-for-each-ref.txt | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)

Looking good.  Thanks.


> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 6dcd39f6f6..b739412c30 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -232,12 +232,24 @@ Fields that have name-email-date tuple as its value (`author`,
>  `committer`, and `tagger`) can be suffixed with `name`, `email`,
>  and `date` to extract the named component.
>  
> -The complete message in a commit and tag object is `contents`.
> -Its first line is `contents:subject`, where subject is the concatenation
> -of all lines of the commit message up to the first blank line.  The next
> -line is `contents:body`, where body is all of the lines after the first
> -blank line.  The optional GPG signature is `contents:signature`.  The
> -first `N` lines of the message is obtained using `contents:lines=N`.
> +The message in a commit or a tag object is `contents`, from which
> +`contents:<part>` can be used to extract various parts out of:
> +
> +contents:subject::
> +	The first paragraph of the message, which typically is a
> +	single line, is taken as the "subject" of the commit or the
> +	tag message.
> +
> +contents:body::
> +	The remainder of the commit or the tag message that follows
> +	the "subject".
> +
> +contents:signature::
> +	The optional GPG signature of the tag.
> +
> +contents:lines=N::
> +	The first `N` lines of the message.
> +
>  Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
>  are obtained as `trailers` (or by using the historical alias
>  `contents:trailers`).  Non-trailer lines from the trailer block can be omitted

Re: [PATCH v4 2/3] t6300: test refs pointing to tree and blob

[Junio C Hamano]

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

> Adding tests for refs pointing to tree and blob shows that
> we care about testing both positive ("see, my shiny new toy
> does work") and negative ("and it won't do nonsensical
> things when given an input it is not designed to work with")
> cases.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t6300-for-each-ref.sh | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)

Nice addition.  Thanks.

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index da59fadc5d..e9f468d360 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -650,6 +650,25 @@ test_atom refs/tags/signed-long contents "subject line
>  body contents
>  $sig"
>  
> +test_expect_success 'set up refs pointing to tree and blob' '
> +	git update-ref refs/mytrees/first refs/heads/master^{tree} &&
> +	git update-ref refs/myblobs/first refs/heads/master:one
> +'
> +
> +test_atom refs/mytrees/first subject ""
> +test_atom refs/mytrees/first contents:subject ""
> +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/myblobs/first subject ""
> +test_atom refs/myblobs/first contents:subject ""
> +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_expect_success 'set up multiple-sort tags' '
>  	for when in 100000 200000
>  	do

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

[Junio C Hamano]

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(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index b739412c30..2ea71c5f6c 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -235,6 +235,9 @@ and `date` to extract the named component.
>  The message in a commit or a tag object is `contents`, from which
>  `contents:<part>` can be used to extract various parts out of:
>  
> +contents:size::
> +	The size in bytes of the commit or tag message.
> +
>  contents:subject::
>  	The first paragraph of the message, which typically is a
>  	single line, is taken as the "subject" of the commit or the

OK.

> diff --git a/ref-filter.c b/ref-filter.c
> index 8447cb09be..73d8bfa86d 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -127,7 +127,8 @@ static struct used_atom {
>  			unsigned int nobracket : 1, push : 1, push_remote : 1;
>  		} remote_ref;
>  		struct {
> -			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
> +			enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH,
> +			       C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
>  			struct process_trailer_options trailer_opts;
>  			unsigned int nlines;
>  		} contents;
> @@ -338,6 +339,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
>  		atom->u.contents.option = C_BARE;
>  	else if (!strcmp(arg, "body"))
>  		atom->u.contents.option = C_BODY;
> +	else if (!strcmp(arg, "size"))
> +		atom->u.contents.option = C_LENGTH;
>  	else if (!strcmp(arg, "signature"))
>  		atom->u.contents.option = C_SIG;
>  	else if (!strcmp(arg, "subject"))
> @@ -1253,6 +1256,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
>  			v->s = copy_subject(subpos, sublen);
>  		else if (atom->u.contents.option == C_BODY_DEP)
>  			v->s = xmemdupz(bodypos, bodylen);
> +		else if (atom->u.contents.option == C_LENGTH)
> +			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
>  		else if (atom->u.contents.option == C_BODY)
>  			v->s = xmemdupz(bodypos, nonsiglen);
>  		else if (atom->u.contents.option == C_SIG)

OK.

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index e9f468d360..467871ac10 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -52,6 +52,25 @@ test_atom() {

You need to stare at the precontext to see if the added lines are
correct.  We have these before the precontext of the patch:

	case "$1" in
		head) ref=refs/heads/master ;;
		 tag) ref=refs/tags/testtag ;;
		 sym) ref=refs/heads/sym ;;
		   *) ref=$1 ;;
	esac
	printf '%s\n' "$3" >expected
	test_expect_${4:-success} $PREREQ "basic atom: $1 $2" "
		git for-each-ref --format='%($2)' $ref >actual &&

Here it uses "$1" for mere reporting on the test title, while using
"$ref" as the reliable way to uniquely identify it as a full ref.

>  		sanitize_pgp <actual >actual.clean &&
>  		test_cmp expected actual.clean
>  	"
> +	# Automatically test "contents:size" atom after testing "contents"
> +	if test "$2" = "contents"
> +	then
> +		case "$1" in
> +		refs/tags/signed-*)

Shouldn't this be $ref to be compared with full refnames like we see
below?

I know the callers won't pass 'head', 'tag' and 'sym' with
'contents' to this helper so the distinction may not currently
matter in practice, but still this use of "$1" does not sound quite
right, no?  

I actually was expecting you to switch on

	case $(git cat-file -t "$ref") in
	tag)
		...;;
	tree | blob)
		...;;
	commit)
		...;;
	easc

instead of the namespace, as %(contents:size) silently becomes empty
due to the underlying object type, not where the object that does
not support the "method" sits in the refs/ namespace.

> +			# We cannot use $3 as it expects sanitize_pgp to run
> +			expect=$(git cat-file tag $ref | tail -n +6 | wc -c) ;;
> +		refs/mytrees/* | refs/myblobs/*)
> +			expect='' ;;

Thanks for catching my thinko; I think I wrote 0 here in my
illustration.

> +		*)
> +			expect=$(printf '%s' "$3" | wc -c) ;;
> +		esac
> +		# Leave $expect unquoted to lose possible leading whitespaces
> +		echo $expect >expected

OK.

> +		test_expect_${4:-success} $PREREQ "basic atom: $1 $2:size" "
> +			git for-each-ref --format='%($2:size)' $ref >actual &&
> +			test_cmp expected actual
> +		"

This is harder to read than necessary; let's not say "$2" when we
know it is 'contents' and nothing else.  Also avoid double-quoted
test body when you can.  The body is evaled and $ref we assigned is
visible inside the test just fine, so make it a habit to quote the
body with single quote pair, i.e.

	test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
		git for-each-ref --format="%(contents:size)" "$ref" >actual &&
		test_cmp expect actual
	'

Thanks.

> +	fi
>  }
>  
>  hexlen=$(test_oid hexsz)

[PATCH v5 0/3] Add support for %(contents:size) in ref-filter

[Christian Couder]

This is version 5 of a small patch series to teach ref-filter about
%(contents:size).

This patch series is based on master at 4a0fcf9f76 (The seventh batch,
2020-07-06).

Previous versions and related discussions are there:

V1: https://lore.kernel.org/git/20200701132308.16691-1-chriscool@tuxfamily.org/
V2: https://lore.kernel.org/git/20200702140845.24945-1-chriscool@tuxfamily.org/
V3: https://lore.kernel.org/git/20200707174049.21714-1-chriscool@tuxfamily.org/
V4: https://lore.kernel.org/git/20200710164739.6616-1-chriscool@tuxfamily.org/

Thanks to Junio and Peff for their reviews of this series!

The changes compared to V4 are the following:

  - Modified test_atom() in patch 3/3 to as suggested by Junio.

The range diff is:

1:  f750832fc7 = 1:  f750832fc7 Documentation: clarify %(contents:XXXX) doc
2:  51c72e09d2 = 2:  51c72e09d2 t6300: test refs pointing to tree and blob
3:  c2ed3e228b ! 3:  cf6a60036e ref-filter: add support for %(contents:size)
    @@ t/t6300-for-each-ref.sh: test_atom() {
     +  # Automatically test "contents:size" atom after testing "contents"
     +  if test "$2" = "contents"
     +  then
    -+          case "$1" in
    -+          refs/tags/signed-*)
    ++          case $(git cat-file -t "$ref") in
    ++          tag)
     +                  # We cannot use $3 as it expects sanitize_pgp to run
     +                  expect=$(git cat-file tag $ref | tail -n +6 | wc -c) ;;
    -+          refs/mytrees/* | refs/myblobs/*)
    ++          tree | blob)
     +                  expect='' ;;
    -+          *)
    ++          commit)
     +                  expect=$(printf '%s' "$3" | wc -c) ;;
     +          esac
     +          # Leave $expect unquoted to lose possible leading whitespaces
     +          echo $expect >expected
    -+          test_expect_${4:-success} $PREREQ "basic atom: $1 $2:size" "
    -+                  git for-each-ref --format='%($2:size)' $ref >actual &&
    -+                  test_cmp expected actual
    -+          "
    ++          test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
    ++                  git for-each-ref --format="%(contents:size)" "$ref" >actual &&
    ++                  test_cmp expect actual
    ++          '
     +  fi
      }
      
Christian Couder (3):
  Documentation: clarify %(contents:XXXX) doc
  t6300: test refs pointing to tree and blob
  ref-filter: add support for %(contents:size)

 Documentation/git-for-each-ref.txt | 27 ++++++++++++++++-----
 ref-filter.c                       |  7 +++++-
 t/t6300-for-each-ref.sh            | 38 ++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 7 deletions(-)

-- 
2.27.0.227.g757ac19d14.dirty

[PATCH v5 1/3] Documentation: clarify %(contents:XXXX) doc

[Christian Couder]

Let's avoid a big dense paragraph by using an unordered
list for the %(contents:XXXX) format specifiers.

While at it let's also make the following improvements:

  - Let's not describe %(contents) using "complete message"
    as it's not clear what an incomplete message is.

  - Let's improve how the "subject" and "body" are
    described.

  - Let's state that "signature" is only available for
    tag objects.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-for-each-ref.txt | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 6dcd39f6f6..b739412c30 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -232,12 +232,24 @@ Fields that have name-email-date tuple as its value (`author`,
 `committer`, and `tagger`) can be suffixed with `name`, `email`,
 and `date` to extract the named component.
 
-The complete message in a commit and tag object is `contents`.
-Its first line is `contents:subject`, where subject is the concatenation
-of all lines of the commit message up to the first blank line.  The next
-line is `contents:body`, where body is all of the lines after the first
-blank line.  The optional GPG signature is `contents:signature`.  The
-first `N` lines of the message is obtained using `contents:lines=N`.
+The message in a commit or a tag object is `contents`, from which
+`contents:<part>` can be used to extract various parts out of:
+
+contents:subject::
+	The first paragraph of the message, which typically is a
+	single line, is taken as the "subject" of the commit or the
+	tag message.
+
+contents:body::
+	The remainder of the commit or the tag message that follows
+	the "subject".
+
+contents:signature::
+	The optional GPG signature of the tag.
+
+contents:lines=N::
+	The first `N` lines of the message.
+
 Additionally, the trailers as interpreted by linkgit:git-interpret-trailers[1]
 are obtained as `trailers` (or by using the historical alias
 `contents:trailers`).  Non-trailer lines from the trailer block can be omitted
-- 
2.27.0.227.g757ac19d14.dirty

[PATCH v5 2/3] t6300: test refs pointing to tree and blob

[Christian Couder]

Adding tests for refs pointing to tree and blob shows that
we care about testing both positive ("see, my shiny new toy
does work") and negative ("and it won't do nonsensical
things when given an input it is not designed to work with")
cases.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6300-for-each-ref.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index da59fadc5d..e9f468d360 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -650,6 +650,25 @@ test_atom refs/tags/signed-long contents "subject line
 body contents
 $sig"
 
+test_expect_success 'set up refs pointing to tree and blob' '
+	git update-ref refs/mytrees/first refs/heads/master^{tree} &&
+	git update-ref refs/myblobs/first refs/heads/master:one
+'
+
+test_atom refs/mytrees/first subject ""
+test_atom refs/mytrees/first contents:subject ""
+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/myblobs/first subject ""
+test_atom refs/myblobs/first contents:subject ""
+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_expect_success 'set up multiple-sort tags' '
 	for when in 100000 200000
 	do
-- 
2.27.0.227.g757ac19d14.dirty

[PATCH v5 3/3] ref-filter: add support for %(contents:size)

[Christian Couder]

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(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index b739412c30..2ea71c5f6c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -235,6 +235,9 @@ and `date` to extract the named component.
 The message in a commit or a tag object is `contents`, from which
 `contents:<part>` can be used to extract various parts out of:
 
+contents:size::
+	The size in bytes of the commit or tag message.
+
 contents:subject::
 	The first paragraph of the message, which typically is a
 	single line, is taken as the "subject" of the commit or the
diff --git a/ref-filter.c b/ref-filter.c
index 8447cb09be..73d8bfa86d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -127,7 +127,8 @@ static struct used_atom {
 			unsigned int nobracket : 1, push : 1, push_remote : 1;
 		} remote_ref;
 		struct {
-			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
+			enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH,
+			       C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
 			struct process_trailer_options trailer_opts;
 			unsigned int nlines;
 		} contents;
@@ -338,6 +339,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
 		atom->u.contents.option = C_BARE;
 	else if (!strcmp(arg, "body"))
 		atom->u.contents.option = C_BODY;
+	else if (!strcmp(arg, "size"))
+		atom->u.contents.option = C_LENGTH;
 	else if (!strcmp(arg, "signature"))
 		atom->u.contents.option = C_SIG;
 	else if (!strcmp(arg, "subject"))
@@ -1253,6 +1256,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
 			v->s = copy_subject(subpos, sublen);
 		else if (atom->u.contents.option == C_BODY_DEP)
 			v->s = xmemdupz(bodypos, bodylen);
+		else if (atom->u.contents.option == C_LENGTH)
+			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
 		else if (atom->u.contents.option == C_BODY)
 			v->s = xmemdupz(bodypos, nonsiglen);
 		else if (atom->u.contents.option == C_SIG)
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index e9f468d360..ea9bb6dade 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -52,6 +52,25 @@ test_atom() {
 		sanitize_pgp <actual >actual.clean &&
 		test_cmp expected actual.clean
 	"
+	# Automatically test "contents:size" atom after testing "contents"
+	if test "$2" = "contents"
+	then
+		case $(git cat-file -t "$ref") in
+		tag)
+			# We cannot use $3 as it expects sanitize_pgp to run
+			expect=$(git cat-file tag $ref | tail -n +6 | wc -c) ;;
+		tree | blob)
+			expect='' ;;
+		commit)
+			expect=$(printf '%s' "$3" | wc -c) ;;
+		esac
+		# Leave $expect unquoted to lose possible leading whitespaces
+		echo $expect >expected
+		test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
+			git for-each-ref --format="%(contents:size)" "$ref" >actual &&
+			test_cmp expect actual
+		'
+	fi
 }
 
 hexlen=$(test_oid hexsz)
-- 
2.27.0.227.g757ac19d14.dirty

Re: [PATCH v5 0/3] Add support for %(contents:size) in ref-filter

[Junio C Hamano]

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

> The range diff is:
>
> 1:  f750832fc7 = 1:  f750832fc7 Documentation: clarify %(contents:XXXX) doc
> 2:  51c72e09d2 = 2:  51c72e09d2 t6300: test refs pointing to tree and blob
> 3:  c2ed3e228b ! 3:  cf6a60036e ref-filter: add support for %(contents:size)
>     @@ t/t6300-for-each-ref.sh: test_atom() {
>      +  # Automatically test "contents:size" atom after testing "contents"
>      +  if test "$2" = "contents"
>      +  then
>     -+          case "$1" in
>     -+          refs/tags/signed-*)
>     ++          case $(git cat-file -t "$ref") in
>     ++          tag)
>      +                  # We cannot use $3 as it expects sanitize_pgp to run
>      +                  expect=$(git cat-file tag $ref | tail -n +6 | wc -c) ;;
>     -+          refs/mytrees/* | refs/myblobs/*)
>     ++          tree | blob)
>      +                  expect='' ;;
>     -+          *)
>     ++          commit)
>      +                  expect=$(printf '%s' "$3" | wc -c) ;;
>      +          esac
>      +          # Leave $expect unquoted to lose possible leading whitespaces
>      +          echo $expect >expected
>     -+          test_expect_${4:-success} $PREREQ "basic atom: $1 $2:size" "
>     -+                  git for-each-ref --format='%($2:size)' $ref >actual &&
>     -+                  test_cmp expected actual
>     -+          "
>     ++          test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
>     ++                  git for-each-ref --format="%(contents:size)" "$ref" >actual &&
>     ++                  test_cmp expect actual
>     ++          '
>      +  fi
>       }

Ah, I almost forgot about this topic X-<, but the above reminds me
and it does read more clearly, at least to me.

Thanks, will replace.

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

[Alban Gruin]

Hi Christian,

Le 16/07/2020 à 14:19, Christian Couder a écrit :
> 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(-)
> 
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index b739412c30..2ea71c5f6c 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -235,6 +235,9 @@ and `date` to extract the named component.
>  The message in a commit or a tag object is `contents`, from which
>  `contents:<part>` can be used to extract various parts out of:
>  
> +contents:size::
> +	The size in bytes of the commit or tag message.
> +
>  contents:subject::
>  	The first paragraph of the message, which typically is a
>  	single line, is taken as the "subject" of the commit or the
> diff --git a/ref-filter.c b/ref-filter.c
> index 8447cb09be..73d8bfa86d 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -127,7 +127,8 @@ static struct used_atom {
>  			unsigned int nobracket : 1, push : 1, push_remote : 1;
>  		} remote_ref;
>  		struct {
> -			enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
> +			enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH,
> +			       C_LINES, C_SIG, C_SUB, C_TRAILERS } option;
>  			struct process_trailer_options trailer_opts;
>  			unsigned int nlines;
>  		} contents;
> @@ -338,6 +339,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato
>  		atom->u.contents.option = C_BARE;
>  	else if (!strcmp(arg, "body"))
>  		atom->u.contents.option = C_BODY;
> +	else if (!strcmp(arg, "size"))
> +		atom->u.contents.option = C_LENGTH;
>  	else if (!strcmp(arg, "signature"))
>  		atom->u.contents.option = C_SIG;
>  	else if (!strcmp(arg, "subject"))
> @@ -1253,6 +1256,8 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)
>  			v->s = copy_subject(subpos, sublen);
>  		else if (atom->u.contents.option == C_BODY_DEP)
>  			v->s = xmemdupz(bodypos, bodylen);
> +		else if (atom->u.contents.option == C_LENGTH)
> +			v->s = xstrfmt("%"PRIuMAX, (uintmax_t)strlen(subpos));
>  		else if (atom->u.contents.option == C_BODY)
>  			v->s = xmemdupz(bodypos, nonsiglen);
>  		else if (atom->u.contents.option == C_SIG)
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index e9f468d360..ea9bb6dade 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -52,6 +52,25 @@ test_atom() {
>  		sanitize_pgp <actual >actual.clean &&
>  		test_cmp expected actual.clean
>  	"
> +	# Automatically test "contents:size" atom after testing "contents"
> +	if test "$2" = "contents"
> +	then
> +		case $(git cat-file -t "$ref") in
> +		tag)
> +			# We cannot use $3 as it expects sanitize_pgp to run
> +			expect=$(git cat-file tag $ref | tail -n +6 | wc -c) ;;
> +		tree | blob)
> +			expect='' ;;
> +		commit)
> +			expect=$(printf '%s' "$3" | wc -c) ;;
> +		esac
> +		# Leave $expect unquoted to lose possible leading whitespaces
> +		echo $expect >expected
> +		test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '

There is a typo here, and $expect is written to `expected', but
`test_cmp' wants `expect'.  Fixing those mistakes does not reveal any
broken tests.

> +			git for-each-ref --format="%(contents:size)" "$ref" >actual &&
> +			test_cmp expect actual
> +		'
> +	fi
>  }
>  
>  hexlen=$(test_oid hexsz)
> 

Alban

[PATCH v1] t6300: fix issues related to %(contents:size)

[Alban Gruin]

b6839fda68 (ref-filter: add support for %(contents:size), 2020-07-16)
added a new format for ref-filter, and added a function to generate
tests for this new feature in t6300.  Unfortunately, it tries to run
`test_expect_sucess' instead of `test_expect_success', and writes
$expect to `expected', but tries to read `expect'.  Those two issues
were probably unnoticed because the script only printed errors, but did
not crash.  This fixes these issues.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 t/t6300-for-each-ref.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index ea9bb6dade..bbec555977 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -65,8 +65,8 @@ test_atom() {
 			expect=$(printf '%s' "$3" | wc -c) ;;
 		esac
 		# Leave $expect unquoted to lose possible leading whitespaces
-		echo $expect >expected
-		test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
+		echo $expect >expect
+		test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" '
 			git for-each-ref --format="%(contents:size)" "$ref" >actual &&
 			test_cmp expect actual
 		'
-- 
2.20.1

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

[Jeff King]

On Fri, Jul 31, 2020 at 07:37:22PM +0200, Alban Gruin wrote:

> > +		# Leave $expect unquoted to lose possible leading whitespaces
> > +		echo $expect >expected
> > +		test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
> 
> There is a typo here, and $expect is written to `expected', but
> `test_cmp' wants `expect'.  Fixing those mistakes does not reveal any
> broken tests.

I thought at first you meant that the typo was s/expected/expect, and
wondered how this could possibly have passed. But the typo is
s/sucess/success/, so we were in fact not running the test at all (and
were generating "test_expect_sucess: not found" messages to stderr, but
outside of any test block. Yikes.

Thanks for spotting.

-Peff

Re: [PATCH v1] t6300: fix issues related to %(contents:size)

[Jeff King]

On Fri, Jul 31, 2020 at 07:45:09PM +0200, Alban Gruin wrote:

> b6839fda68 (ref-filter: add support for %(contents:size), 2020-07-16)
> added a new format for ref-filter, and added a function to generate
> tests for this new feature in t6300.  Unfortunately, it tries to run
> `test_expect_sucess' instead of `test_expect_success', and writes
> $expect to `expected', but tries to read `expect'.  Those two issues
> were probably unnoticed because the script only printed errors, but did
> not crash.  This fixes these issues.

Oh, this just crossed with my mail. :)

Definitely fixes the issue, though I wonder:

> -		echo $expect >expected
> -		test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
> +		echo $expect >expect
> +		test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" '
>  			git for-each-ref --format="%(contents:size)" "$ref" >actual &&
>  			test_cmp expect actual
>  		'

Should we instead switch the test_cmp to look at "expected" to be
consistent with the rest of the tests in this file?

-Peff

Re: [PATCH v1] t6300: fix issues related to %(contents:size)

[Alban Gruin]

Le 31/07/2020 à 19:47, Jeff King a écrit :
> On Fri, Jul 31, 2020 at 07:45:09PM +0200, Alban Gruin wrote:
> 
>> b6839fda68 (ref-filter: add support for %(contents:size), 2020-07-16)
>> added a new format for ref-filter, and added a function to generate
>> tests for this new feature in t6300.  Unfortunately, it tries to run
>> `test_expect_sucess' instead of `test_expect_success', and writes
>> $expect to `expected', but tries to read `expect'.  Those two issues
>> were probably unnoticed because the script only printed errors, but did
>> not crash.  This fixes these issues.
> 
> Oh, this just crossed with my mail. :)
> 
> Definitely fixes the issue, though I wonder:
> 
>> -		echo $expect >expected
>> -		test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
>> +		echo $expect >expect
>> +		test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" '
>>  			git for-each-ref --format="%(contents:size)" "$ref" >actual &&
>>  			test_cmp expect actual
>>  		'
> 
> Should we instead switch the test_cmp to look at "expected" to be
> consistent with the rest of the tests in this file?
> 
> -Peff
> 

OK, I'm fixing that.

Alban

[PATCH v2] t6300: fix issues related to %(contents:size)

[Alban Gruin]

b6839fda68 (ref-filter: add support for %(contents:size), 2020-07-16)
added a new format for ref-filter, and added a function to generate
tests for this new feature in t6300.  Unfortunately, it tries to run
`test_expect_sucess' instead of `test_expect_success', and writes
$expect to `expected', but tries to read `expect'.  Those two issues
were probably unnoticed because the script only printed errors, but did
not crash.  This fixes these issues.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 t/t6300-for-each-ref.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index ea9bb6dade..a83579fbdf 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -66,9 +66,9 @@ test_atom() {
 		esac
 		# Leave $expect unquoted to lose possible leading whitespaces
 		echo $expect >expected
-		test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
+		test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" '
 			git for-each-ref --format="%(contents:size)" "$ref" >actual &&
-			test_cmp expect actual
+			test_cmp expected actual
 		'
 	fi
 }
-- 
2.20.1

Re: [PATCH v2] t6300: fix issues related to %(contents:size)

[Jeff King]

On Fri, Jul 31, 2020 at 08:26:07PM +0200, Alban Gruin wrote:

> b6839fda68 (ref-filter: add support for %(contents:size), 2020-07-16)
> added a new format for ref-filter, and added a function to generate
> tests for this new feature in t6300.  Unfortunately, it tries to run
> `test_expect_sucess' instead of `test_expect_success', and writes
> $expect to `expected', but tries to read `expect'.  Those two issues
> were probably unnoticed because the script only printed errors, but did
> not crash.  This fixes these issues.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  t/t6300-for-each-ref.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Looks good. Good eyes for spotting this, and thanks for the quick fix.

-Peff

Re: [PATCH v1] t6300: fix issues related to %(contents:size)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 31, 2020 at 07:45:09PM +0200, Alban Gruin wrote:
>
>> b6839fda68 (ref-filter: add support for %(contents:size), 2020-07-16)
>> added a new format for ref-filter, and added a function to generate
>> tests for this new feature in t6300.  Unfortunately, it tries to run
>> `test_expect_sucess' instead of `test_expect_success', and writes
>> $expect to `expected', but tries to read `expect'.  Those two issues
>> were probably unnoticed because the script only printed errors, but did
>> not crash.  This fixes these issues.
>
> Oh, this just crossed with my mail. :)
>
> Definitely fixes the issue, though I wonder:
>
>> -		echo $expect >expected
>> -		test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
>> +		echo $expect >expect
>> +		test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" '
>>  			git for-each-ref --format="%(contents:size)" "$ref" >actual &&
>>  			test_cmp expect actual
>>  		'
>
> Should we instead switch the test_cmp to look at "expected" to be
> consistent with the rest of the tests in this file?

If I recall correctly, "expect vs actual" were more common when I
counted across all the tests last time.  Matching local convention
is fine, though.

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

[Christian Couder]

Hi Alban and Peff,

On Fri, Jul 31, 2020 at 7:45 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Jul 31, 2020 at 07:37:22PM +0200, Alban Gruin wrote:
>
> > > +           # Leave $expect unquoted to lose possible leading whitespaces
> > > +           echo $expect >expected
> > > +           test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
> >
> > There is a typo here, and $expect is written to `expected', but
> > `test_cmp' wants `expect'.  Fixing those mistakes does not reveal any
> > broken tests.
>
> I thought at first you meant that the typo was s/expected/expect, and
> wondered how this could possibly have passed. But the typo is
> s/sucess/success/, so we were in fact not running the test at all (and
> were generating "test_expect_sucess: not found" messages to stderr, but
> outside of any test block. Yikes.
>
> Thanks for spotting.

Yeah, I copied a suggestion from Junio in the last iteration without
properly checking it. Sorry about that and thanks for spotting and
fixing it.

Re: [PATCH v1] t6300: fix issues related to %(contents:size)

[Jeff King]

On Fri, Jul 31, 2020 at 01:04:10PM -0700, Junio C Hamano wrote:

> > Definitely fixes the issue, though I wonder:
> >
> >> -		echo $expect >expected
> >> -		test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
> >> +		echo $expect >expect
> >> +		test_expect_${4:-success} $PREREQ "basic atom: $1 contents:size" '
> >>  			git for-each-ref --format="%(contents:size)" "$ref" >actual &&
> >>  			test_cmp expect actual
> >>  		'
> >
> > Should we instead switch the test_cmp to look at "expected" to be
> > consistent with the rest of the tests in this file?
> 
> If I recall correctly, "expect vs actual" were more common when I
> counted across all the tests last time.  Matching local convention
> is fine, though.

Yes, I agree that "expect" is where we should be heading overall. I
think matching local convention is best here to avoid introducing new
mistakes like this one, but I wouldn't be opposed to somebody switching
out s/expected/expect/ in the whole file.

-Peff

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

[Junio C Hamano]

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

> Hi Alban and Peff,
>
> On Fri, Jul 31, 2020 at 7:45 PM Jeff King <peff@peff.net> wrote:
>>
>> On Fri, Jul 31, 2020 at 07:37:22PM +0200, Alban Gruin wrote:
>>
>> > > +           # Leave $expect unquoted to lose possible leading whitespaces
>> > > +           echo $expect >expected
>> > > +           test_expect_${4:-sucess} $PREREQ "basic atom: $1 contents:size" '
>> >
>> > There is a typo here, and $expect is written to `expected', but
>> > `test_cmp' wants `expect'.  Fixing those mistakes does not reveal any
>> > broken tests.
>>
>> I thought at first you meant that the typo was s/expected/expect, and
>> wondered how this could possibly have passed. But the typo is
>> s/sucess/success/, so we were in fact not running the test at all (and
>> were generating "test_expect_sucess: not found" messages to stderr, but
>> outside of any test block. Yikes.
>>
>> Thanks for spotting.
>
> Yeah, I copied a suggestion from Junio in the last iteration without
> properly checking it. Sorry about that and thanks for spotting and
> fixing it.

I probably should stop giving "perhaps along the lines of this"
suggestion too lightly and/or when I do not have enough time to
apply and test myself.  Sorry for the gotcha.

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

[Jeff King]

On Fri, Jul 31, 2020 at 01:30:19PM -0700, Junio C Hamano wrote:

> > Yeah, I copied a suggestion from Junio in the last iteration without
> > properly checking it. Sorry about that and thanks for spotting and
> > fixing it.
> 
> I probably should stop giving "perhaps along the lines of this"
> suggestion too lightly and/or when I do not have enough time to
> apply and test myself.  Sorry for the gotcha.

I dunno. I appreciate getting them, especially in patch form. It's often
a more precise description than hand-wavy English, and being a patch
makes it easy to apply into my tree as a starting point. The real trick
is that the receiver needs to know enough to distrust the suggestion and
take ownership of it. Maybe you just need a bigger disclaimer. ;)

(Only half-joking; I do try to say "not tested" or "not even compiled"
when that is the case in stuff I sent out, but I'm sure I'm not
consistent).

-Peff