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

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

[jyotish kumar]

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,

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

[Jeff King]

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

> 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

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

[Junio C Hamano]

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?

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

[jyotish kumar]

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

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

[jyotishkumar725015]

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