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

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

[Deveshi Dwivedi]

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

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

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

[Junio C Hamano]

Deveshi Dwivedi <deveshigurgaon@gmail.com> writes:

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

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

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

[Victoria Dye]

On 2/19/26 11:50 AM, Junio C Hamano wrote:
> 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.

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

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

[Deveshi Dwivedi]

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