[Bug] for-each-ref: %(object) and %(type) unimplemented

[Bug] for-each-ref: %(object) and %(type) unimplemented

[Lea Wiemann]

Try the following on a repository with tag objects in it:

git for-each-ref --format='%(object)'
git for-each-ref --format='%(type)'

Each command prints only newlines.  The %(type) and %(object) options 
are not rejected with an error message (like "%(doesnotexist)"), but 
they don't seem to be implemented.  "%(tag)" works though.

Anyone care to implement the missing options?  Or should they rather be 
removed from the documentation?

-- Lea

Re: [Bug] for-each-ref: %(object) and %(type) unimplemented

[Jeff King]

On Wed, Jun 25, 2008 at 05:01:01PM +0200, Lea Wiemann wrote:

> Try the following on a repository with tag objects in it:
>
> git for-each-ref --format='%(object)'
> git for-each-ref --format='%(type)'
>
> Each command prints only newlines.  The %(type) and %(object) options are 
> not rejected with an error message (like "%(doesnotexist)"), but they 
> don't seem to be implemented.  "%(tag)" works though.
>
> Anyone care to implement the missing options?  Or should they rather be  
> removed from the documentation?

Looks like they were part of the original set of atoms, but they just
never ended up implemented. Clearly nobody has actually cared in the
intervening time, but it is easy enough to add them. See below.

Since you seem to be testing for-each-ref, maybe it would make sense to
put together a test script that exercises each of the atoms?

-- >8 --
for-each-ref: implement missing tag values

The "type" and "object" fields for tags were accepted as
valid atoms, but never implemented. Consequently, they
simply returned the empty string, even for valid tags.

Noticed by Lea Wiemann.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-for-each-ref.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 07d9c57..fef93d7 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -234,6 +234,13 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 			name++;
 		if (!strcmp(name, "tag"))
 			v->s = tag->tag;
+		else if (!strcmp(name, "type") && tag->tagged)
+			v->s = typename(tag->tagged->type);
+		else if (!strcmp(name, "object") && tag->tagged) {
+			char *s = xmalloc(41);
+			strcpy(s, sha1_to_hex(tag->tagged->sha1));
+			v->s = s;
+		}
 	}
 }
 
-- 
1.5.6.129.g8dea5.dirty

Re: [Bug] for-each-ref: %(object) and %(type) unimplemented

[Jeff King]

On Wed, Jun 25, 2008 at 12:08:15PM -0400, Jeff King wrote:

> Since you seem to be testing for-each-ref, maybe it would make sense to
> put together a test script that exercises each of the atoms?

Hmm. Actually, there is a test in t6300 that runs with each atom name.
Unfortunately, it doesn't bother actually checking the output for
sanity, so the fact that these atoms returned the empty string was
missed.

-Peff

Re: [Bug] for-each-ref: %(object) and %(type) unimplemented

[Mikael Magnusson]

2008/6/25 Jeff King <peff@peff.net>:
> On Wed, Jun 25, 2008 at 12:08:15PM -0400, Jeff King wrote:
>
>> Since you seem to be testing for-each-ref, maybe it would make sense to
>> put together a test script that exercises each of the atoms?
>
> Hmm. Actually, there is a test in t6300 that runs with each atom name.
> Unfortunately, it doesn't bother actually checking the output for
> sanity, so the fact that these atoms returned the empty string was
> missed.

The documentation also says "tree" and "parent" should work (which they
also don't).

-- 
Mikael Magnusson

Re: [Bug] for-each-ref: %(object) and %(type) unimplemented

[Junio C Hamano]

"Mikael Magnusson" <mikachu@gmail.com> writes:

> 2008/6/25 Jeff King <peff@peff.net>:
>> On Wed, Jun 25, 2008 at 12:08:15PM -0400, Jeff King wrote:
>>
>>> Since you seem to be testing for-each-ref, maybe it would make sense to
>>> put together a test script that exercises each of the atoms?
>>
>> Hmm. Actually, there is a test in t6300 that runs with each atom name.
>> Unfortunately, it doesn't bother actually checking the output for
>> sanity, so the fact that these atoms returned the empty string was
>> missed.
>
> The documentation also says "tree" and "parent" should work (which they
> also don't).

The doc does not say any such thing.  A tag object does not have 'parent'
nor 'tree' header fields.

Neither a commit have 'object' nor 'type' header fields and you cannot ask
for them when showing a commit object.

Re: [Bug] for-each-ref: %(object) and %(type) unimplemented

[Mikael Magnusson]

2008/6/25 Junio C Hamano <gitster@pobox.com>:
> "Mikael Magnusson" <mikachu@gmail.com> writes:
>
>> 2008/6/25 Jeff King <peff@peff.net>:
>>> On Wed, Jun 25, 2008 at 12:08:15PM -0400, Jeff King wrote:
>>>
>>>> Since you seem to be testing for-each-ref, maybe it would make sense to
>>>> put together a test script that exercises each of the atoms?
>>>
>>> Hmm. Actually, there is a test in t6300 that runs with each atom name.
>>> Unfortunately, it doesn't bother actually checking the output for
>>> sanity, so the fact that these atoms returned the empty string was
>>> missed.
>>
>> The documentation also says "tree" and "parent" should work (which they
>> also don't).
>
> The doc does not say any such thing.  A tag object does not have 'parent'
> nor 'tree' header fields.
>
> Neither a commit have 'object' nor 'type' header fields and you cannot ask
> for them when showing a commit object.

Ah, sorry, I read this paragraph:
              In addition to the above, for commit and tag objects,
the header field
              names (tree, parent, object, type, and tag) can be used
to specify the
              value in the header field.

But not this one (which perhaps should be right after the
                  above instead of 3 paragraphs down):
              In any case, a field name that refers to a field
inapplicable to the
              object referred by the ref does not cause an error. It
returns an empty
              string instead.


-- 
Mikael Magnusson

Re: [Bug] for-each-ref: %(object) and %(type) unimplemented

[Jeff King]

On Wed, Jun 25, 2008 at 12:14:33PM -0400, Jeff King wrote:

> Hmm. Actually, there is a test in t6300 that runs with each atom name.
> Unfortunately, it doesn't bother actually checking the output for
> sanity, so the fact that these atoms returned the empty string was
> missed.

And here is a two patch series cleaning up the test script and fixing
the bug.

-Peff

[PATCH 1/2] improve for-each-ref test script

[Jeff King]

Previously, we did a sanity check by doing for-each-ref
using each possible format atom. However, we never checked
the actual output produced by that atom. This patch checks
the output, and in doing so reveals breakage on a few atoms.

While we're at it, also clean up a few '!' into
test_must_fail.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t6300-for-each-ref.sh |   87 +++++++++++++++++++++++++++++++++++++---------
 1 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 91ea85d..415e1d1 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -26,25 +26,78 @@ test_expect_success 'Create sample commit with known timestamp' '
 	git tag -a -m "Tagging at $datestamp" testtag
 '
 
-test_expect_success 'Check atom names are valid' '
-	bad=
-	for token in \
-		refname objecttype objectsize objectname tree parent \
-		numparent object type author authorname authoremail \
-		authordate committer committername committeremail \
-		committerdate tag tagger taggername taggeremail \
-		taggerdate creator creatordate subject body contents
-	do
-		git for-each-ref --format="$token=%($token)" refs/heads || {
-			bad=$token
-			break
-		}
-	done
-	test -z "$bad"
+test_atom() {
+	case "$1" in
+		head) ref=refs/heads/master ;;
+		 tag) ref=refs/tags/testtag ;;
+	esac
+	printf '%s\n' "$3" >expected
+	test_expect_${4:-success} "basic atom: $1 $2" "
+		git for-each-ref --format='%($2)' $ref >actual &&
+		test_cmp expected actual
+	"
+}
+
+test_atom head refname refs/heads/master
+test_atom head objecttype commit
+test_atom head objectsize 171
+test_atom head objectname 67a36f10722846e891fbada1ba48ed035de75581
+test_atom head tree 0e51c00fcb93dffc755546f27593d511e1bdb46f
+test_atom head parent ''
+test_atom head numparent 0
+test_atom head object ''
+test_atom head type ''
+test_atom head author 'A U Thor <author@example.com> 1151939924 +0200'
+test_atom head authorname 'A U Thor'
+test_atom head authoremail '<author@example.com>'
+test_atom head authordate 'Mon Jul 3 17:18:44 2006 +0200'
+test_atom head committer 'C O Mitter <committer@example.com> 1151939923 +0200'
+test_atom head committername 'C O Mitter'
+test_atom head committeremail '<committer@example.com>'
+test_atom head committerdate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head tag ''
+test_atom head tagger ''
+test_atom head taggername ''
+test_atom head taggeremail ''
+test_atom head taggerdate ''
+test_atom head creator 'C O Mitter <committer@example.com> 1151939923 +0200'
+test_atom head creatordate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head subject 'Initial'
+test_atom head body ''
+test_atom head contents 'Initial
+'
+
+test_atom tag refname refs/tags/testtag
+test_atom tag objecttype tag
+test_atom tag objectsize 154
+test_atom tag objectname 98b46b1d36e5b07909de1b3886224e3e81e87322
+test_atom tag tree ''
+test_atom tag parent ''
+test_atom tag numparent ''
+test_atom tag object '67a36f10722846e891fbada1ba48ed035de75581' failure
+test_atom tag type 'commit' failure
+test_atom tag author ''
+test_atom tag authorname ''
+test_atom tag authoremail ''
+test_atom tag authordate ''
+test_atom tag committer ''
+test_atom tag committername ''
+test_atom tag committeremail ''
+test_atom tag committerdate ''
+test_atom tag tag 'testtag'
+test_atom tag tagger 'C O Mitter <committer@example.com> 1151939925 +0200'
+test_atom tag taggername 'C O Mitter'
+test_atom tag taggeremail '<committer@example.com>'
+test_atom tag taggerdate 'Mon Jul 3 17:18:45 2006 +0200'
+test_atom tag creator 'C O Mitter <committer@example.com> 1151939925 +0200'
+test_atom tag creatordate 'Mon Jul 3 17:18:45 2006 +0200'
+test_atom tag subject 'Tagging at 1151939927'
+test_atom tag body ''
+test_atom tag contents 'Tagging at 1151939927
 '
 
 test_expect_success 'Check invalid atoms names are errors' '
-	! git-for-each-ref --format="%(INVALID)" refs/heads
+	test_must_fail git-for-each-ref --format="%(INVALID)" refs/heads
 '
 
 test_expect_success 'Check format specifiers are ignored in naming date atoms' '
@@ -64,7 +117,7 @@ test_expect_success 'Check valid format specifiers for date fields' '
 '
 
 test_expect_success 'Check invalid format specifiers are errors' '
-	! git-for-each-ref --format="%(authordate:INVALID)" refs/heads
+	test_must_fail git-for-each-ref --format="%(authordate:INVALID)" refs/heads
 '
 
 cat >expected <<\EOF
-- 
1.5.6

[PATCH 2/2] for-each-ref: implement missing tag values

[Jeff King]

The "type" and "object" fields for tags were accepted as
valid atoms, but never implemented. Consequently, they
simply returned the empty string, even for valid tags.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-for-each-ref.c  |    7 +++++++
 t/t6300-for-each-ref.sh |    4 ++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
index 07d9c57..fef93d7 100644
--- a/builtin-for-each-ref.c
+++ b/builtin-for-each-ref.c
@@ -234,6 +234,13 @@ static void grab_tag_values(struct atom_value *val, int deref, struct object *ob
 			name++;
 		if (!strcmp(name, "tag"))
 			v->s = tag->tag;
+		else if (!strcmp(name, "type") && tag->tagged)
+			v->s = typename(tag->tagged->type);
+		else if (!strcmp(name, "object") && tag->tagged) {
+			char *s = xmalloc(41);
+			strcpy(s, sha1_to_hex(tag->tagged->sha1));
+			v->s = s;
+		}
 	}
 }
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 415e1d1..a3c8941 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -74,8 +74,8 @@ test_atom tag objectname 98b46b1d36e5b07909de1b3886224e3e81e87322
 test_atom tag tree ''
 test_atom tag parent ''
 test_atom tag numparent ''
-test_atom tag object '67a36f10722846e891fbada1ba48ed035de75581' failure
-test_atom tag type 'commit' failure
+test_atom tag object '67a36f10722846e891fbada1ba48ed035de75581'
+test_atom tag type 'commit'
 test_atom tag author ''
 test_atom tag authorname ''
 test_atom tag authoremail ''
-- 
1.5.6

Re: [Bug] for-each-ref: %(object) and %(type) unimplemented

[Jeff King]

On Thu, Jun 26, 2008 at 01:35:51AM -0400, Jeff King wrote:

> > Hmm. Actually, there is a test in t6300 that runs with each atom name.
> > Unfortunately, it doesn't bother actually checking the output for
> > sanity, so the fact that these atoms returned the empty string was
> > missed.
> 
> And here is a two patch series cleaning up the test script and fixing
> the bug.

Hmm, looks like I just missed you applying the original fix to maint.
Ignore 2/2, then, as you already have it. I think it is still worth
doing the test script cleanup, though. Here it is, rebased on the
current master (the only difference is we don't have to mark the
breakage as expect_failure).

-- >8 --
improve for-each-ref test script

Previously, we did a sanity check by doing for-each-ref
using each possible format atom. However, we never checked
the actual output produced by that atom, which recently let
an obvious bug go undetected for some time.

While we're at it, also clean up a few '!' into
test_must_fail.
---
 t/t6300-for-each-ref.sh |   87 +++++++++++++++++++++++++++++++++++++---------
 1 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 91ea85d..a3c8941 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -26,25 +26,78 @@ test_expect_success 'Create sample commit with known timestamp' '
 	git tag -a -m "Tagging at $datestamp" testtag
 '
 
-test_expect_success 'Check atom names are valid' '
-	bad=
-	for token in \
-		refname objecttype objectsize objectname tree parent \
-		numparent object type author authorname authoremail \
-		authordate committer committername committeremail \
-		committerdate tag tagger taggername taggeremail \
-		taggerdate creator creatordate subject body contents
-	do
-		git for-each-ref --format="$token=%($token)" refs/heads || {
-			bad=$token
-			break
-		}
-	done
-	test -z "$bad"
+test_atom() {
+	case "$1" in
+		head) ref=refs/heads/master ;;
+		 tag) ref=refs/tags/testtag ;;
+	esac
+	printf '%s\n' "$3" >expected
+	test_expect_${4:-success} "basic atom: $1 $2" "
+		git for-each-ref --format='%($2)' $ref >actual &&
+		test_cmp expected actual
+	"
+}
+
+test_atom head refname refs/heads/master
+test_atom head objecttype commit
+test_atom head objectsize 171
+test_atom head objectname 67a36f10722846e891fbada1ba48ed035de75581
+test_atom head tree 0e51c00fcb93dffc755546f27593d511e1bdb46f
+test_atom head parent ''
+test_atom head numparent 0
+test_atom head object ''
+test_atom head type ''
+test_atom head author 'A U Thor <author@example.com> 1151939924 +0200'
+test_atom head authorname 'A U Thor'
+test_atom head authoremail '<author@example.com>'
+test_atom head authordate 'Mon Jul 3 17:18:44 2006 +0200'
+test_atom head committer 'C O Mitter <committer@example.com> 1151939923 +0200'
+test_atom head committername 'C O Mitter'
+test_atom head committeremail '<committer@example.com>'
+test_atom head committerdate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head tag ''
+test_atom head tagger ''
+test_atom head taggername ''
+test_atom head taggeremail ''
+test_atom head taggerdate ''
+test_atom head creator 'C O Mitter <committer@example.com> 1151939923 +0200'
+test_atom head creatordate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head subject 'Initial'
+test_atom head body ''
+test_atom head contents 'Initial
+'
+
+test_atom tag refname refs/tags/testtag
+test_atom tag objecttype tag
+test_atom tag objectsize 154
+test_atom tag objectname 98b46b1d36e5b07909de1b3886224e3e81e87322
+test_atom tag tree ''
+test_atom tag parent ''
+test_atom tag numparent ''
+test_atom tag object '67a36f10722846e891fbada1ba48ed035de75581'
+test_atom tag type 'commit'
+test_atom tag author ''
+test_atom tag authorname ''
+test_atom tag authoremail ''
+test_atom tag authordate ''
+test_atom tag committer ''
+test_atom tag committername ''
+test_atom tag committeremail ''
+test_atom tag committerdate ''
+test_atom tag tag 'testtag'
+test_atom tag tagger 'C O Mitter <committer@example.com> 1151939925 +0200'
+test_atom tag taggername 'C O Mitter'
+test_atom tag taggeremail '<committer@example.com>'
+test_atom tag taggerdate 'Mon Jul 3 17:18:45 2006 +0200'
+test_atom tag creator 'C O Mitter <committer@example.com> 1151939925 +0200'
+test_atom tag creatordate 'Mon Jul 3 17:18:45 2006 +0200'
+test_atom tag subject 'Tagging at 1151939927'
+test_atom tag body ''
+test_atom tag contents 'Tagging at 1151939927
 '
 
 test_expect_success 'Check invalid atoms names are errors' '
-	! git-for-each-ref --format="%(INVALID)" refs/heads
+	test_must_fail git-for-each-ref --format="%(INVALID)" refs/heads
 '
 
 test_expect_success 'Check format specifiers are ignored in naming date atoms' '
@@ -64,7 +117,7 @@ test_expect_success 'Check valid format specifiers for date fields' '
 '
 
 test_expect_success 'Check invalid format specifiers are errors' '
-	! git-for-each-ref --format="%(authordate:INVALID)" refs/heads
+	test_must_fail git-for-each-ref --format="%(authordate:INVALID)" refs/heads
 '
 
 cat >expected <<\EOF
-- 
1.5.6.129.g8dea5.dirty

Re: [Bug] for-each-ref: %(object) and %(type) unimplemented

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmm, looks like I just missed you applying the original fix to maint.
> Ignore 2/2, then, as you already have it. I think it is still worth
> doing the test script cleanup, though.

Absolutely.  And thanks.