v3, 4 partsAdd support for %(contents:size) in ref-filter

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

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

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

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

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

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

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

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

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

> 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

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

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

> 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

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

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

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

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

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

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

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

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

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

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

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

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