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

[Junio C Hamano <gitster@pobox.com>]

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