{"thread":{"id":"66650","subject":"[Discussion] t1006: testing %(rest) with object names containing whitespace","startedAt":"2026-10-24T06:47:53Z","lastAt":"2026-10-26T06:13:10Z","messageCount":5,"participants":["jyotish kumar","Jeff King","Junio C Hamano","jyotishkumar725015@gmail.com"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"556143","messageId":"CAGjZMyRqv5RsLF4Vo439zy4urfr+R=CTwuDzmYjiemauuZan0Q@mail.gmail.com","threadId":"66650","inReplyTo":null,"subject":"[Discussion] t1006: testing %(rest) with object names containing whitespace","fromName":"jyotish kumar","fromEmail":"jyotishkumar725015@gmail.com","sentAt":"2026-10-24T06:47:53Z","receivedAt":"2026-10-24T06:47:53Z","isPatch":false,"sender":{"key":"jyotishkumar725015@gmail.com","avatar":null},"body":"Hi,\n\nWhile exploring t/t1006-cat-file.sh, I came across the test for the\n%(rest) format atom in git cat-file --batch-check.\n\nThe test currently contains a FIXME explaining that %(rest) is\nincompatible with object names containing whitespace, such as\nHEAD:path/to/a/file with spaces. It uses test_expect_failure for that\ncase and mentions using the resolved object ID as input instead of the\noriginal object name.\n\nI investigated this test and noticed that the run_tests() helper\nalready has access to the resolved object ID through $oid. I tried\nusing $oid as the input to the %(rest) test rather than $object_name.\n\nWith this change, the focused t1006-cat-file.sh test passed all 423\ntests locally.\n\nBefore preparing a patch, I wanted to ask whether this is the intended\napproach for testing %(rest) here, and whether removing the\nconditional expected-failure logic and using the resolved object ID\nwould be a worthwhile improvement.\n\nI would appreciate your guidance on whether this is worth pursuing or\nif there is a reason to retain the current test behavior.\n\nThanks,\n\n"},{"id":"556243","messageId":"20261026013254.GB3575069@coredump.intra.peff.net","threadId":"66650","inReplyTo":"CAGjZMyRqv5RsLF4Vo439zy4urfr+R=CTwuDzmYjiemauuZan0Q@mail.gmail.com","subject":"Re: [Discussion] t1006: testing %(rest) with object names containing whitespace","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-10-26T01:32:54Z","receivedAt":"2026-10-26T01:32:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 24, 2026 at 12:17:53PM +0530, jyotish kumar wrote:\n\n> While exploring t/t1006-cat-file.sh, I came across the test for the\n> %(rest) format atom in git cat-file --batch-check.\n> \n> The test currently contains a FIXME explaining that %(rest) is\n> incompatible with object names containing whitespace, such as\n> HEAD:path/to/a/file with spaces. It uses test_expect_failure for that\n> case and mentions using the resolved object ID as input instead of the\n> original object name.\n> \n> I investigated this test and noticed that the run_tests() helper\n> already has access to the resolved object ID through $oid. I tried\n> using $oid as the input to the %(rest) test rather than $object_name.\n> \n> With this change, the focused t1006-cat-file.sh test passed all 423\n> tests locally.\n> \n> Before preparing a patch, I wanted to ask whether this is the intended\n> approach for testing %(rest) here, and whether removing the\n> conditional expected-failure logic and using the resolved object ID\n> would be a worthwhile improvement.\n\nThere's a similar discussion here:\n\n https://lore.kernel.org/git/20260219152407.12160-1-deveshigurgaon@gmail.com/\n\nI didn't participate there, but I was the person who implemented this\ncrappy %(rest) behavior in the first place. ;)\n\nI think what Junio noted at the end of the thread is important. The\nlimitation of %(rest) is documented and how it's \"supposed\" to work. And\nthe reason that is the case is because %(rest) was added much later than\n--batch-check, and was stuck with the existing semantics.\n\nSo IMHO everything is working as designed, and there is nothing to fix\nin the code. But using test_expect_failure seems weird to me. It avoids\nthe test suite announcing a failure, but it is not really a breakage to\nbe fixed. A test with %(rest) and an object name with space should\nprobably just be skipped. We _could_ run just those cases with the\nresolved id, but the point of that test is checking how %(rest) works\nwith the name, so I don't know that it would be testing anything useful.\n\n-Peff\n\n"},{"id":"556245","messageId":"xmqqjyn5w2l2.fsf@gitster.g","threadId":"66650","inReplyTo":"20261026013254.GB3575069@coredump.intra.peff.net","subject":"Re: [Discussion] t1006: testing %(rest) with object names containing whitespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-26T03:41:29Z","receivedAt":"2026-10-26T03:41:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So IMHO everything is working as designed, and there is nothing to fix\n> in the code. But using test_expect_failure seems weird to me. It avoids\n> the test suite announcing a failure, but it is not really a breakage to\n> be fixed. A test with %(rest) and an object name with space should\n> probably just be skipped. We _could_ run just those cases with the\n> resolved id, but the point of that test is checking how %(rest) works\n> with the name, so I don't know that it would be testing anything useful.\n\nYup, thanks for finding old (but not so old) discussion. We really\nshould have fixed the test including the fixme comment as we knew\nalready back then that there was nothing to fix.\n\nPerhaps somebody involved in this discussion thread can volunteer to\ndo so this time?\n\n"},{"id":"556247","messageId":"CAGjZMyRF-hV8PMK2VBJT-9kzvgtQYP2EfYSMPjxSKmc2ZgscDw@mail.gmail.com","threadId":"66650","inReplyTo":"xmqqjyn5w2l2.fsf@gitster.g","subject":"Re: [Discussion] t1006: testing %(rest) with object names containing whitespace","fromName":"jyotish kumar","fromEmail":"jyotishkumar725015@gmail.com","sentAt":"2026-10-26T04:56:56Z","receivedAt":"2026-10-26T04:56:56Z","isPatch":false,"sender":{"key":"jyotishkumar725015@gmail.com","avatar":null},"body":"Hi Jeff and Junio,\n\nThanks for the clarification. I understand that the limitation is\nintentional and that using the resolved object ID would not test how\n%(rest) behaves with the original object name.\n\nI'd be happy to volunteer to clean up the test in t/t1006-cat-file.sh,\nso that it handles this known limitation appropriately and removes the\nobsolete FIXME.\n\nI'll work on the test change and send a patch for review.\n\nThanks,\nJyotish\n\nOn Mon, Oct 26, 2026 at 9:11 AM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> > So IMHO everything is working as designed, and there is nothing to fix\n> > in the code. But using test_expect_failure seems weird to me. It avoids\n> > the test suite announcing a failure, but it is not really a breakage to\n> > be fixed. A test with %(rest) and an object name with space should\n> > probably just be skipped. We _could_ run just those cases with the\n> > resolved id, but the point of that test is checking how %(rest) works\n> > with the name, so I don't know that it would be testing anything useful.\n>\n> Yup, thanks for finding old (but not so old) discussion. We really\n> should have fixed the test including the fixme comment as we knew\n> already back then that there was nothing to fix.\n>\n> Perhaps somebody involved in this discussion thread can volunteer to\n> do so this time?\n\n"},{"id":"556250","messageId":"20261026061310.62950-1-jyotishkumar725015@gmail.com","threadId":"66650","inReplyTo":"CAGjZMyRF-hV8PMK2VBJT-9kzvgtQYP2EfYSMPjxSKmc2ZgscDw@mail.gmail.com","subject":"[PATCH] t1006: skip %(rest) test for whitespace in object names","fromName":"","fromEmail":"jyotishkumar725015@gmail.com","sentAt":"2026-10-26T06:13:10Z","receivedAt":"2026-10-26T06:13:10Z","isPatch":true,"sender":{"key":"jyotishkumar725015@gmail.com","avatar":null},"body":"From: jyotish kumar \n\n---\n t/t1006-cat-file.sh | 27 ++++++++++++---------------\n 1 file changed, 12 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 72fc8acc61..47d9434bcc 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -183,21 +183,18 @@ $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-\techo \"$type this is some extra content\" >expect &&\n-\techo \"$object_name this is some extra content\" |\n-\t\tgit cat-file --batch-check=\"%(objecttype) %(rest)\" >actual &&\n-\ttest_cmp expect actual\n- '\n+\tcase \"$object_name\" in\n+\t*[[:space:]]*)\n+\t\t;;\n+\t*)\n+\t\ttest_expect_success '--batch-check with %(rest)' '\n+\t\t\techo \"$type this is some extra content\" >expect &&\n+\t\t\techo \"$object_name this is some extra content\" |\n+\t\t\t\tgit cat-file --batch-check=\"%(objecttype) %(rest)\" >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t'\n+\t\t;;\n+\tesac\n \n test_expect_success '--batch-check with %(objectmode)' '\n \techo \"$mode $oid\" >expect &&\n-- \n2.43.0\n\n\n"}]}