{"thread":{"id":"65023","subject":"[PATCH] t1006: fix %(rest) test for object names with whitespace","startedAt":"2026-02-19T15:24:18Z","lastAt":"2026-02-20T16:59:50Z","messageCount":6,"participants":["Deveshi Dwivedi","Junio C Hamano","Victoria Dye"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"536413","messageId":"20260219152407.12160-1-deveshigurgaon@gmail.com","threadId":"65023","inReplyTo":null,"subject":"[PATCH] t1006: fix %(rest) test for object names with whitespace","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-02-19T15:24:07Z","receivedAt":"2026-02-19T15:24:18Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"The '--batch-check with %(rest)' test in run_tests() used\n$object_name directly as input to git cat-file. When the\nobject name contained whitespace (e.g., \"HEAD:path with spaces\"),\nthis led to ambiguity between the object name and the %(rest)\nplaceholder.\n\nAs a result, git cat-file could not reliably determine where\nthe object name ended and %(rest) began.\n\nFix this by using the resolved object ID (OID) instead of the\nraw object name as input. OIDs are hexadecimal strings and\nnever contain whitespace, making the split unambiguous. This\nalso removes the need for the existing FIXME comment and the\ntest_expect_failure workaround.\n\nSigned-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n---\n t/t1006-cat-file.sh | 15 ++++-----------\n 1 file changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 0eee3bb878..cac88acf65 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -194,18 +194,11 @@ $content\"\n \ttest_cmp expect actual\n     '\n \n-    # FIXME: %(rest) is incompatible with object names that include whitespace,\n-    # e.g. HEAD:path/to/a/file with spaces. Use the resolved OID as input to\n-    # test this instead of the raw object name.\n-    if echo \"$object_name\" | grep -q \" \"; then\n-\ttest_rest=test_expect_failure\n-    else\n-\ttest_rest=test_expect_success\n-    fi\n-\n-    $test_rest '--batch-check with %(rest)' '\n+    # Use the resolved OID so %(rest) parsing is independent of whitespace\n+    # in object names (e.g. HEAD:path with spaces).\n+    test_expect_success '--batch-check with %(rest)' '\n \techo \"$type this is some extra content\" >expect &&\n-\techo \"$object_name    this is some extra content\" |\n+\techo \"$oid    this is some extra content\" |\n \t\tgit cat-file --batch-check=\"%(objecttype) %(rest)\" >actual &&\n \ttest_cmp expect actual\n     '\n-- \n2.52.0.230.gd8af7cadaa\n\n"},{"id":"536436","messageId":"xmqqikbs4iod.fsf@gitster.g","threadId":"65023","inReplyTo":"20260219152407.12160-1-deveshigurgaon@gmail.com","subject":"Re: [PATCH] t1006: fix %(rest) test for object names with whitespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-19T19:50:10Z","receivedAt":"2026-02-19T19:50:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Deveshi Dwivedi <deveshigurgaon@gmail.com> writes:\n\n> The '--batch-check with %(rest)' test in run_tests() used\n> $object_name directly as input to git cat-file. When the\n> object name contained whitespace (e.g., \"HEAD:path with spaces\"),\n> this led to ambiguity between the object name and the %(rest)\n> placeholder.\n>\n> As a result, git cat-file could not reliably determine where\n> the object name ended and %(rest) began.\n>\n> Fix this by using the resolved object ID (OID) instead of the\n> raw object name as input. OIDs are hexadecimal strings and\n> never contain whitespace, making the split unambiguous. This\n> also removes the need for the existing FIXME comment and the\n> test_expect_failure workaround.\n>\n> Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com>\n> ---\n>  t/t1006-cat-file.sh | 15 ++++-----------\n>  1 file changed, 4 insertions(+), 11 deletions(-)\n\nI am not sure if this particular FIXME has much value, but ...\n\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 0eee3bb878..cac88acf65 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -194,18 +194,11 @@ $content\"\n>  \ttest_cmp expect actual\n>      '\n>  \n> -    # FIXME: %(rest) is incompatible with object names that include whitespace,\n> -    # e.g. HEAD:path/to/a/file with spaces. Use the resolved OID as input to\n> -    # test this instead of the raw object name.\n> -    if echo \"$object_name\" | grep -q \" \"; then\n> -\ttest_rest=test_expect_failure\n> -    else\n> -\ttest_rest=test_expect_success\n> -    fi\n> -\n> -    $test_rest '--batch-check with %(rest)' '\n> +    # Use the resolved OID so %(rest) parsing is independent of whitespace\n> +    # in object names (e.g. HEAD:path with spaces).\n> +    test_expect_success '--batch-check with %(rest)' '\n>  \techo \"$type this is some extra content\" >expect &&\n> -\techo \"$object_name    this is some extra content\" |\n> +\techo \"$oid    this is some extra content\" |\n\n... I somehow doubt that this is what 9fd38038 (t1006: update\n'run_tests' to test generic object specifiers, 2025-06-02) meant by\nthat comment.\n\nI would have understood if the fix were more like\n\n\ttest_expect_success '--batch-check with %(rest)' '\n                case \"$object_name\" in\n                ?*\" \"?* | ?*\"       \"?*) # has space or tab\n                        token_to_test=$oid\n                        ;;\n                *)\n                        token_to_test=$object_name\n                        ;;\n                esac &&\n                echo \"$type this is some extra content\" >expect &&\n\t\techo \"$token_to_test this is some extra content\" |\n\t\tgit cat-file --batch-check=\"%(objecttype) %(rest) >actual &&\n\t\ttest_cmp expect actual\n\t'\n\ni.e., when testing the object with non-problemtic name, use that\nname, but otherwise, use the resolved name to avoid breakage.  That\nis because the whole point of 9fd38038, as I understand it from the\nearlier part of the change (see \"git show 9fd38038\") was to test the\nuse of name as much as possible.\n\n>  \t\tgit cat-file --batch-check=\"%(objecttype) %(rest)\" >actual &&\n>  \ttest_cmp expect actual\n>      '\n"},{"id":"536437","messageId":"d2bf79b3-4407-4fa0-ae2b-fcb3178f36f7@github.com","threadId":"65023","inReplyTo":"xmqqikbs4iod.fsf@gitster.g","subject":"Re: [PATCH] t1006: fix %(rest) test for object names with whitespace","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2026-02-19T20:23:57Z","receivedAt":"2026-02-19T20:23:59Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"On 2/19/26 11:50 AM, Junio C Hamano wrote:\n> I am not sure if this particular FIXME has much value, but ...\n> \n>> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n>> index 0eee3bb878..cac88acf65 100755\n>> --- a/t/t1006-cat-file.sh\n>> +++ b/t/t1006-cat-file.sh\n>> @@ -194,18 +194,11 @@ $content\"\n>>   \ttest_cmp expect actual\n>>       '\n>>   \n>> -    # FIXME: %(rest) is incompatible with object names that include whitespace,\n>> -    # e.g. HEAD:path/to/a/file with spaces. Use the resolved OID as input to\n>> -    # test this instead of the raw object name.\n>> -    if echo \"$object_name\" | grep -q \" \"; then\n>> -\ttest_rest=test_expect_failure\n>> -    else\n>> -\ttest_rest=test_expect_success\n>> -    fi\n>> -\n>> -    $test_rest '--batch-check with %(rest)' '\n>> +    # Use the resolved OID so %(rest) parsing is independent of whitespace\n>> +    # in object names (e.g. HEAD:path with spaces).\n>> +    test_expect_success '--batch-check with %(rest)' '\n>>   \techo \"$type this is some extra content\" >expect &&\n>> -\techo \"$object_name    this is some extra content\" |\n>> +\techo \"$oid    this is some extra content\" |\n> \n> ... I somehow doubt that this is what 9fd38038 (t1006: update\n> 'run_tests' to test generic object specifiers, 2025-06-02) meant by\n> that comment.\n\nThat FIXME was intended to call out the behavior of %(rest) in cat-file\nitself as something that we may eventually want to fix. The comment is\nonly here because this test happens to demonstrate that behavior. For\nthat reason, I'm also not sure I see the value of this patch; it's\nremoving some visibility to a quirk of cat-file without fixing the\nunderlying issue.\n\n"},{"id":"536440","messageId":"xmqqzf5431ek.fsf@gitster.g","threadId":"65023","inReplyTo":"d2bf79b3-4407-4fa0-ae2b-fcb3178f36f7@github.com","subject":"Re: [PATCH] t1006: fix %(rest) test for object names with whitespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-19T20:48:35Z","receivedAt":"2026-02-19T20:48:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye <vdye@github.com> writes:\n\n>>> -    # FIXME: %(rest) is incompatible with object names that include whitespace,\n>>> -    # e.g. HEAD:path/to/a/file with spaces. Use the resolved OID as input to\n>>> -    # test this instead of the raw object name.\n>>> -    if echo \"$object_name\" | grep -q \" \"; then\n>>> -\ttest_rest=test_expect_failure\n>>> -    else\n>>> -\ttest_rest=test_expect_success\n>>> -    fi\n>>> -\n>>> -    $test_rest '--batch-check with %(rest)' '\n>>> +    # Use the resolved OID so %(rest) parsing is independent of whitespace\n>>> +    # in object names (e.g. HEAD:path with spaces).\n>>> +    test_expect_success '--batch-check with %(rest)' '\n>>>   \techo \"$type this is some extra content\" >expect &&\n>>> -\techo \"$object_name    this is some extra content\" |\n>>> +\techo \"$oid    this is some extra content\" |\n>> \n>> ... I somehow doubt that this is what 9fd38038 (t1006: update\n>> 'run_tests' to test generic object specifiers, 2025-06-02) meant by\n>> that comment.\n>\n> That FIXME was intended to call out the behavior of %(rest) in cat-file\n> itself as something that we may eventually want to fix. The comment is\n> only here because this test happens to demonstrate that behavior. For\n> that reason, I'm also not sure I see the value of this patch; it's\n> removing some visibility to a quirk of cat-file without fixing the\n> underlying issue.\n\nI agree that fixing underlying issue would be a much more valuable\noutcome of resolving that FIXME comment, but isn't the approach to\ngive $object_name fundamentally incompatible with %(rest), making\nthe issue something %(rest) implementation cannot \"fix\", is it?\n\nThat is a part of the reason why I said I am dubious about the FIXME\ncomment in my comment.\n\nThanks.\n"},{"id":"536457","messageId":"xmqqldgo148k.fsf@gitster.g","threadId":"65023","inReplyTo":"xmqqzf5431ek.fsf@gitster.g","subject":"Re: [PATCH] t1006: fix %(rest) test for object names with whitespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-02-20T03:30:19Z","receivedAt":"2026-02-20T03:30:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I agree that fixing underlying issue would be a much more valuable\n> outcome of resolving that FIXME comment, but isn't the approach to\n> give $object_name fundamentally incompatible with %(rest), making\n> the issue something %(rest) implementation cannot \"fix\", is it?\n>\n> That is a part of the reason why I said I am dubious about the FIXME\n> comment in my comment.\n\nActually, it is worse than that.\n\nWe already _promise_ to chop the input line at the first whitespace\nboundary in our documentation when we use %(rest), so there is\nnothing we can do to \"fix\" on the implementation side.  What your\noriginal tested, i.e., if the early part of the input up to the\nfirst whitespace does *not* name an object, then the test cannot\nsucceed (not just that, the test should fail, unless it happens to\nname another valid object), is the advertised behaviour of this\nfeature.\n\nThanks.\n"},{"id":"536539","messageId":"CAG7UgETZFiB_J3wO+OD+R76GtOR5eNNNv7XAT_HPBksM9FHGsw@mail.gmail.com","threadId":"65023","inReplyTo":"xmqqldgo148k.fsf@gitster.g","subject":"Re: [PATCH] t1006: fix %(rest) test for object names with whitespace","fromName":"Deveshi Dwivedi","fromEmail":"deveshigurgaon@gmail.com","sentAt":"2026-02-20T16:59:35Z","receivedAt":"2026-02-20T16:59:50Z","isPatch":true,"sender":{"key":"deveshigurgaon@gmail.com","avatar":"https://avatars.githubusercontent.com/u/120312681?v=4"},"body":"> We already _promise_ to chop the input line at the first whitespace\n> boundary in our documentation when we use %(rest), so there is\n> nothing we can do to \"fix\" on the implementation side.  What your\n> original tested, i.e., if the early part of the input up to the\n> first whitespace does *not* name an object, then the test cannot\n> succeed (not just that, the test should fail, unless it happens to\n> name another valid object), is the advertised behaviour of this\n> feature.\n\nThank you for the clarification. I understand that the whitespace\nsplit is the documented behavior of %(rest), so there is nothing to\nfix on the implementation side.\n"}]}