threads / discuss / 66650

[Discussion] t1006: testing %(rest) with object names containing whitespace

Subject: [Discussion] t1006: testing %(rest) with object names containing whitespace

## tl;dr

A newcomer's attempt to make a cat-file test pass led to agreement that the test_expect_failure case misdescribes intended behavior. Read the story.

replies: 4people: 3as markdown or json

jyotish kumar· Oct 24, 2026, 06:47 UTC · lore

Hi,

While exploring t/t1006-cat-file.sh, I came across the test for the %(rest) format atom in git cat-file --batch-check.

The test currently contains a FIXME explaining that %(rest) is incompatible with object names containing whitespace, such as HEAD:path/to/a/file with spaces. It uses test_expect_failure for that case and mentions using the resolved object ID as input instead of the original object name.

I investigated this test and noticed that the run_tests() helper already has access to the resolved object ID through $oid. I tried using $oid as the input to the %(rest) test rather than $object_name.

With this change, the focused t1006-cat-file.sh test passed all 423 tests locally.

Before preparing a patch, I wanted to ask whether this is the intended approach for testing %(rest) here, and whether removing the conditional expected-failure logic and using the resolved object ID would be a worthwhile improvement.

I would appreciate your guidance on whether this is worth pursuing or if there is a reason to retain the current test behavior.

Thanks,

Jeff King· Oct 26, 2026, 01:32 UTC · re: jyotish kumar · lore

Re: [Discussion] t1006: testing %(rest) with object names containing whitespace

On Sat, Oct 24, 2026 at 12:17:53PM +0530, jyotish kumar wrote:

Show 20 quoted lines

> While exploring t/t1006-cat-file.sh, I came across the test for the
> %(rest) format atom in git cat-file --batch-check.
> 
> The test currently contains a FIXME explaining that %(rest) is
> incompatible with object names containing whitespace, such as
> HEAD:path/to/a/file with spaces. It uses test_expect_failure for that
> case and mentions using the resolved object ID as input instead of the
> original object name.
> 
> I investigated this test and noticed that the run_tests() helper
> already has access to the resolved object ID through $oid. I tried
> using $oid as the input to the %(rest) test rather than $object_name.
> 
> With this change, the focused t1006-cat-file.sh test passed all 423
> tests locally.
> 
> Before preparing a patch, I wanted to ask whether this is the intended
> approach for testing %(rest) here, and whether removing the
> conditional expected-failure logic and using the resolved object ID
> would be a worthwhile improvement.

There's a similar discussion here:

  https://lore.kernel.org/git/20260219152407.12160-1-deveshigurgaon@gmail.com/

I didn't participate there, but I was the person who implemented this crappy %(rest) behavior in the first place. ;)

I think what Junio noted at the end of the thread is important. The limitation of %(rest) is documented and how it's "supposed" to work. And the reason that is the case is because %(rest) was added much later than --batch-check, and was stuck with the existing semantics.

So IMHO everything is working as designed, and there is nothing to fix in the code. But using test_expect_failure seems weird to me. It avoids the test suite announcing a failure, but it is not really a breakage to be fixed. A test with %(rest) and an object name with space should probably just be skipped. We _could_ run just those cases with the resolved id, but the point of that test is checking how %(rest) works with the name, so I don't know that it would be testing anything useful.

-Peff

Junio C Hamano· Oct 26, 2026, 03:41 UTC · re: Jeff King · lore

Re: [Discussion] t1006: testing %(rest) with object names containing whitespace

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

> So IMHO everything is working as designed, and there is nothing to fix
> in the code. But using test_expect_failure seems weird to me. It avoids
> the test suite announcing a failure, but it is not really a breakage to
> be fixed. A test with %(rest) and an object name with space should
> probably just be skipped. We _could_ run just those cases with the
> resolved id, but the point of that test is checking how %(rest) works
> with the name, so I don't know that it would be testing anything useful.

Yup, thanks for finding old (but not so old) discussion. We really should have fixed the test including the fixme comment as we knew already back then that there was nothing to fix.

Perhaps somebody involved in this discussion thread can volunteer to do so this time?

jyotish kumar· Oct 26, 2026, 04:56 UTC · re: Junio C Hamano · lore

Re: [Discussion] t1006: testing %(rest) with object names containing whitespace

Hi Jeff and Junio,

Thanks for the clarification. I understand that the limitation is intentional and that using the resolved object ID would not test how %(rest) behaves with the original object name.

I'd be happy to volunteer to clean up the test in t/t1006-cat-file.sh, so that it handles this known limitation appropriately and removes the obsolete FIXME.

I'll work on the test change and send a patch for review.

Thanks, Jyotish

On Mon, Oct 26, 2026 at 9:11 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

>
> Jeff King <peff@peff.net> writes:
>
> > So IMHO everything is working as designed, and there is nothing to fix
> > in the code. But using test_expect_failure seems weird to me. It avoids
> > the test suite announcing a failure, but it is not really a breakage to
> > be fixed. A test with %(rest) and an object name with space should
> > probably just be skipped. We _could_ run just those cases with the
> > resolved id, but the point of that test is checking how %(rest) works
> > with the name, so I don't know that it would be testing anything useful.
>
> Yup, thanks for finding old (but not so old) discussion.  We really
> should have fixed the test including the fixme comment as we knew
> already back then that there was nothing to fix.
>
> Perhaps somebody involved in this discussion thread can volunteer to
> do so this time?

jyotishkumar725015@gmail.com· Oct 26, 2026, 06:13 UTC · re: jyotish kumar · lore

[PATCH] t1006: skip %(rest) test for whitespace in object names

From: jyotish kumar <jyotishkumar725015@gmail.com>

---
 t/t1006-cat-file.sh | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 72fc8acc61..47d9434bcc 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -183,21 +183,18 @@ $content"
 	test_cmp expect actual
     '
 
-    # FIXME: %(rest) is incompatible with object names that include whitespace,
-    # e.g. HEAD:path/to/a/file with spaces. Use the resolved OID as input to
-    # test this instead of the raw object name.
-    if echo "$object_name" | grep -q " "; then
-	test_rest=test_expect_failure
-    else
-	test_rest=test_expect_success
-    fi
-
-    $test_rest '--batch-check with %(rest)' '
-	echo "$type this is some extra content" >expect &&
-	echo "$object_name    this is some extra content" |
-		git cat-file --batch-check="%(objecttype) %(rest)" >actual &&
-	test_cmp expect actual
-    '
+	case "$object_name" in
+	*[[:space:]]*)
+		;;
+	*)
+		test_expect_success '--batch-check with %(rest)' '
+			echo "$type this is some extra content" >expect &&
+			echo "$object_name    this is some extra content" |
+				git cat-file --batch-check="%(objecttype) %(rest)" >actual &&
+			test_cmp expect actual
+		'
+		;;
+	esac
 
     test_expect_success '--batch-check with %(objectmode)' '
 	echo "$mode $oid" >expect &&

-- 
2.43.0