# [Discussion] t1006: testing %(rest) with object names containing whitespace 5 messages from 2026-10-24 to 2026-10-26. Participants: jyotish kumar, Jeff King, Junio C Hamano, jyotishkumar725015@gmail.com. Thread: https://gitlist.dev/t/66650 ## jyotish kumar, 2026-10-24 06:47 Subject: [Discussion] t1006: testing %(rest) with object names containing whitespace Message-ID: URL: https://gitlist.dev/e/CAGjZMyRqv5RsLF4Vo439zy4urfr%2BR%3DCTwuDzmYjiemauuZan0Q%40mail.gmail.com ``` Hi, While exploring t/t1006-cat-file.sh, I came across the test for the %(rest) format atom in git cat-file --batch-check. The test currently contains a FIXME explaining that %(rest) is incompatible with object names containing whitespace, such as HEAD:path/to/a/file with spaces. It uses test_expect_failure for that case and mentions using the resolved object ID as input instead of the original object name. I investigated this test and noticed that the run_tests() helper already has access to the resolved object ID through $oid. I tried using $oid as the input to the %(rest) test rather than $object_name. With this change, the focused t1006-cat-file.sh test passed all 423 tests locally. Before preparing a patch, I wanted to ask whether this is the intended approach for testing %(rest) here, and whether removing the conditional expected-failure logic and using the resolved object ID would be a worthwhile improvement. I would appreciate your guidance on whether this is worth pursuing or if there is a reason to retain the current test behavior. Thanks, ``` ## Jeff King, 2026-10-26 01:32 Subject: Re: [Discussion] t1006: testing %(rest) with object names containing whitespace Message-ID: <20261026013254.GB3575069@coredump.intra.peff.net> URL: https://gitlist.dev/e/20261026013254.GB3575069%40coredump.intra.peff.net In-Reply-To: ``` On Sat, Oct 24, 2026 at 12:17:53PM +0530, jyotish kumar wrote: > While exploring t/t1006-cat-file.sh, I came across the test for the > %(rest) format atom in git cat-file --batch-check. > > The test currently contains a FIXME explaining that %(rest) is > incompatible with object names containing whitespace, such as > HEAD:path/to/a/file with spaces. It uses test_expect_failure for that > case and mentions using the resolved object ID as input instead of the > original object name. > > I investigated this test and noticed that the run_tests() helper > already has access to the resolved object ID through $oid. I tried > using $oid as the input to the %(rest) test rather than $object_name. > > With this change, the focused t1006-cat-file.sh test passed all 423 > tests locally. > > Before preparing a patch, I wanted to ask whether this is the intended > approach for testing %(rest) here, and whether removing the > conditional expected-failure logic and using the resolved object ID > would be a worthwhile improvement. There's a similar discussion here: https://lore.kernel.org/git/20260219152407.12160-1-deveshigurgaon@gmail.com/ I didn't participate there, but I was the person who implemented this crappy %(rest) behavior in the first place. ;) I think what Junio noted at the end of the thread is important. The limitation of %(rest) is documented and how it's "supposed" to work. And the reason that is the case is because %(rest) was added much later than --batch-check, and was stuck with the existing semantics. So IMHO everything is working as designed, and there is nothing to fix in the code. But using test_expect_failure seems weird to me. It avoids the test suite announcing a failure, but it is not really a breakage to be fixed. A test with %(rest) and an object name with space should probably just be skipped. We _could_ run just those cases with the resolved id, but the point of that test is checking how %(rest) works with the name, so I don't know that it would be testing anything useful. -Peff ``` ## Junio C Hamano, 2026-10-26 03:41 Subject: Re: [Discussion] t1006: testing %(rest) with object names containing whitespace Message-ID: URL: https://gitlist.dev/e/xmqqjyn5w2l2.fsf%40gitster.g In-Reply-To: <20261026013254.GB3575069@coredump.intra.peff.net> ``` Jeff King writes: > So IMHO everything is working as designed, and there is nothing to fix > in the code. But using test_expect_failure seems weird to me. It avoids > the test suite announcing a failure, but it is not really a breakage to > be fixed. A test with %(rest) and an object name with space should > probably just be skipped. We _could_ run just those cases with the > resolved id, but the point of that test is checking how %(rest) works > with the name, so I don't know that it would be testing anything useful. Yup, thanks for finding old (but not so old) discussion. We really should have fixed the test including the fixme comment as we knew already back then that there was nothing to fix. Perhaps somebody involved in this discussion thread can volunteer to do so this time? ``` ## jyotish kumar, 2026-10-26 04:56 Subject: Re: [Discussion] t1006: testing %(rest) with object names containing whitespace Message-ID: URL: https://gitlist.dev/e/CAGjZMyRF-hV8PMK2VBJT-9kzvgtQYP2EfYSMPjxSKmc2ZgscDw%40mail.gmail.com In-Reply-To: ``` Hi Jeff and Junio, Thanks for the clarification. I understand that the limitation is intentional and that using the resolved object ID would not test how %(rest) behaves with the original object name. I'd be happy to volunteer to clean up the test in t/t1006-cat-file.sh, so that it handles this known limitation appropriately and removes the obsolete FIXME. I'll work on the test change and send a patch for review. Thanks, Jyotish On Mon, Oct 26, 2026 at 9:11 AM Junio C Hamano wrote: > > Jeff King writes: > > > So IMHO everything is working as designed, and there is nothing to fix > > in the code. But using test_expect_failure seems weird to me. It avoids > > the test suite announcing a failure, but it is not really a breakage to > > be fixed. A test with %(rest) and an object name with space should > > probably just be skipped. We _could_ run just those cases with the > > resolved id, but the point of that test is checking how %(rest) works > > with the name, so I don't know that it would be testing anything useful. > > Yup, thanks for finding old (but not so old) discussion. We really > should have fixed the test including the fixme comment as we knew > already back then that there was nothing to fix. > > Perhaps somebody involved in this discussion thread can volunteer to > do so this time? ``` ## jyotishkumar725015@gmail.com, 2026-10-26 06:13 Subject: [PATCH] t1006: skip %(rest) test for whitespace in object names Message-ID: <20261026061310.62950-1-jyotishkumar725015@gmail.com> URL: https://gitlist.dev/e/20261026061310.62950-1-jyotishkumar725015%40gmail.com In-Reply-To: ``` From: jyotish kumar --- t/t1006-cat-file.sh | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 72fc8acc61..47d9434bcc 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -183,21 +183,18 @@ $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)' ' - echo "$type this is some extra content" >expect && - echo "$object_name this is some extra content" | - git cat-file --batch-check="%(objecttype) %(rest)" >actual && - test_cmp expect actual - ' + case "$object_name" in + *[[:space:]]*) + ;; + *) + test_expect_success '--batch-check with %(rest)' ' + echo "$type this is some extra content" >expect && + echo "$object_name this is some extra content" | + git cat-file --batch-check="%(objecttype) %(rest)" >actual && + test_cmp expect actual + ' + ;; + esac test_expect_success '--batch-check with %(objectmode)' ' echo "$mode $oid" >expect && -- 2.43.0 ```