threads / patch / 65023

patcht1006: fix %(rest) test for object names with whitespace

Subject: [PATCH] t1006: fix %(rest) test for object names with whitespace

## tl;dr

6 messages between Feb 19, 2026 and Feb 20, 2026. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Deveshi Dwivedi· Feb 19, 2026, 15:24 UTC · lore

The '--batch-check with %(rest)' test in run_tests() used $object_name directly as input to git cat-file. When the object name contained whitespace (e.g., "HEAD:path with spaces"), this led to ambiguity between the object name and the %(rest) placeholder.

As a result, git cat-file could not reliably determine where the object name ended and %(rest) began.

Fix this by using the resolved object ID (OID) instead of the raw object name as input. OIDs are hexadecimal strings and never contain whitespace, making the split unambiguous. This also removes the need for the existing FIXME comment and the test_expect_failure workaround.

Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>
---
 t/t1006-cat-file.sh | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)
Show changes to t/t1006-cat-file.sh +4 −11
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index 0eee3bb878..cac88acf65 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -194,18 +194,11 @@ $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)' '
+    # Use the resolved OID so %(rest) parsing is independent of whitespace
+    # in object names (e.g. HEAD:path with spaces).
+    test_expect_success '--batch-check with %(rest)' '
 	echo "$type this is some extra content" >expect &&
-	echo "$object_name    this is some extra content" |
+	echo "$oid    this is some extra content" |
 		git cat-file --batch-check="%(objecttype) %(rest)" >actual &&
 	test_cmp expect actual
     '

-- 
2.52.0.230.gd8af7cadaa
Junio C Hamano· Feb 19, 2026, 19:50 UTC · re: Deveshi Dwivedi · lore

Re: [PATCH] t1006: fix %(rest) test for object names with whitespace

Deveshi Dwivedi <deveshigurgaon@gmail.com> writes:
Show 19 quoted lines
> The '--batch-check with %(rest)' test in run_tests() used
> $object_name directly as input to git cat-file. When the
> object name contained whitespace (e.g., "HEAD:path with spaces"),
> this led to ambiguity between the object name and the %(rest)
> placeholder.
>
> As a result, git cat-file could not reliably determine where
> the object name ended and %(rest) began.
>
> Fix this by using the resolved object ID (OID) instead of the
> raw object name as input. OIDs are hexadecimal strings and
> never contain whitespace, making the split unambiguous. This
> also removes the need for the existing FIXME comment and the
> test_expect_failure workaround.
>
> Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>
> ---
>  t/t1006-cat-file.sh | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
I am not sure if this particular FIXME has much value, but ...
Show 24 quoted lines
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index 0eee3bb878..cac88acf65 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -194,18 +194,11 @@ $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)' '
> +    # Use the resolved OID so %(rest) parsing is independent of whitespace
> +    # in object names (e.g. HEAD:path with spaces).
> +    test_expect_success '--batch-check with %(rest)' '
>  	echo "$type this is some extra content" >expect &&
> -	echo "$object_name    this is some extra content" |
> +	echo "$oid    this is some extra content" |

... I somehow doubt that this is what 9fd38038 (t1006: update 'run_tests' to test generic object specifiers, 2025-06-02) meant by that comment.

I would have understood if the fix were more like
	test_expect_success '--batch-check with %(rest)' '
                case "$object_name" in
                ?*" "?* | ?*"       "?*) # has space or tab
                        token_to_test=$oid
                        ;;
                *)
                        token_to_test=$object_name
                        ;;
                esac &&
                echo "$type this is some extra content" >expect &&
		echo "$token_to_test this is some extra content" |
		git cat-file --batch-check="%(objecttype) %(rest) >actual &&
		test_cmp expect actual
	'

i.e., when testing the object with non-problemtic name, use that name, but otherwise, use the resolved name to avoid breakage. That is because the whole point of 9fd38038, as I understand it from the earlier part of the change (see "git show 9fd38038") was to test the use of name as much as possible.

>  		git cat-file --batch-check="%(objecttype) %(rest)" >actual &&
>  	test_cmp expect actual
>      '
Victoria Dye· Feb 19, 2026, 20:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1006: fix %(rest) test for object names with whitespace

On 2/19/26 11:50 AM, Junio C Hamano wrote:
Show 30 quoted lines
> I am not sure if this particular FIXME has much value, but ...
> 
>> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
>> index 0eee3bb878..cac88acf65 100755
>> --- a/t/t1006-cat-file.sh
>> +++ b/t/t1006-cat-file.sh
>> @@ -194,18 +194,11 @@ $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)' '
>> +    # Use the resolved OID so %(rest) parsing is independent of whitespace
>> +    # in object names (e.g. HEAD:path with spaces).
>> +    test_expect_success '--batch-check with %(rest)' '
>>   	echo "$type this is some extra content" >expect &&
>> -	echo "$object_name    this is some extra content" |
>> +	echo "$oid    this is some extra content" |
> 
> ... I somehow doubt that this is what 9fd38038 (t1006: update
> 'run_tests' to test generic object specifiers, 2025-06-02) meant by
> that comment.

That FIXME was intended to call out the behavior of %(rest) in cat-file itself as something that we may eventually want to fix. The comment is only here because this test happens to demonstrate that behavior. For that reason, I'm also not sure I see the value of this patch; it's removing some visibility to a quirk of cat-file without fixing the underlying issue.

Junio C Hamano· Feb 19, 2026, 20:48 UTC · re: Victoria Dye · lore

Re: [PATCH] t1006: fix %(rest) test for object names with whitespace

Victoria Dye <vdye@github.com> writes:
Show 27 quoted lines
>>> -    # 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)' '
>>> +    # Use the resolved OID so %(rest) parsing is independent of whitespace
>>> +    # in object names (e.g. HEAD:path with spaces).
>>> +    test_expect_success '--batch-check with %(rest)' '
>>>   	echo "$type this is some extra content" >expect &&
>>> -	echo "$object_name    this is some extra content" |
>>> +	echo "$oid    this is some extra content" |
>> 
>> ... I somehow doubt that this is what 9fd38038 (t1006: update
>> 'run_tests' to test generic object specifiers, 2025-06-02) meant by
>> that comment.
>
> That FIXME was intended to call out the behavior of %(rest) in cat-file
> itself as something that we may eventually want to fix. The comment is
> only here because this test happens to demonstrate that behavior. For
> that reason, I'm also not sure I see the value of this patch; it's
> removing some visibility to a quirk of cat-file without fixing the
> underlying issue.

I agree that fixing underlying issue would be a much more valuable outcome of resolving that FIXME comment, but isn't the approach to give $object_name fundamentally incompatible with %(rest), making the issue something %(rest) implementation cannot "fix", is it?

That is a part of the reason why I said I am dubious about the FIXME comment in my comment.

Thanks.
Junio C Hamano· Feb 20, 2026, 03:30 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1006: fix %(rest) test for object names with whitespace

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> I agree that fixing underlying issue would be a much more valuable
> outcome of resolving that FIXME comment, but isn't the approach to
> give $object_name fundamentally incompatible with %(rest), making
> the issue something %(rest) implementation cannot "fix", is it?
>
> That is a part of the reason why I said I am dubious about the FIXME
> comment in my comment.
Actually, it is worse than that.

We already _promise_ to chop the input line at the first whitespace boundary in our documentation when we use %(rest), so there is nothing we can do to "fix" on the implementation side. What your original tested, i.e., if the early part of the input up to the first whitespace does *not* name an object, then the test cannot succeed (not just that, the test should fail, unless it happens to name another valid object), is the advertised behaviour of this feature.

Thanks.
Deveshi Dwivedi· Feb 20, 2026, 16:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] t1006: fix %(rest) test for object names with whitespace

Show 8 quoted lines
> We already _promise_ to chop the input line at the first whitespace
> boundary in our documentation when we use %(rest), so there is
> nothing we can do to "fix" on the implementation side.  What your
> original tested, i.e., if the early part of the input up to the
> first whitespace does *not* name an object, then the test cannot
> succeed (not just that, the test should fail, unless it happens to
> name another valid object), is the advertised behaviour of this
> feature.

Thank you for the clarification. I understand that the whitespace split is the documented behavior of %(rest), so there is nothing to fix on the implementation side.

← back to recent threads