{"thread":{"id":"65219","subject":"[PATCH] t0410: modernize delete_object helper","startedAt":"2026-03-12T12:50:38Z","lastAt":"2026-03-18T02:37:59Z","messageCount":13,"participants":["Siddharth Shrimali","Pushkar Singh","Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"538742","messageId":"20260312125030.7799-1-r.siddharth.shrimali@gmail.com","threadId":"65219","inReplyTo":null,"subject":"[PATCH] t0410: modernize delete_object helper","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-03-12T12:50:30Z","receivedAt":"2026-03-12T12:50:38Z","isPatch":true,"sender":{"key":"r.siddharth.shrimali@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183274193?v=4"},"body":"The delete_object helper currently relies on a manual sed command to\ncalculate object paths. This works, but it's a bit brittle and forces\nus to maintain shell logic that Git's own test suite can already\nhandle more elegantly.\n\nSwitch to 'test_oid_to_path' to let Git handle the path logic. This\nmakes the helper hash independent, which is much cleaner than manual\nstring manipulation. While we're at it, add a call to\n'test_path_is_file' so that the test fails early and clearly if we\ntry to delete an object that isn't there, rather than failing\nsilently.\n\nSigned-off-by: Siddharth Shrimali \n---\n t/t0410-partial-clone.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 2a5bdbeeb8..eca113f0f8 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -11,7 +11,11 @@ test_description='partial clone'\n GIT_TEST_COMMIT_GRAPH=0\n \n delete_object () {\n-\trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n+\trepo=$1\n+\tobj=$2\n+\tpath=\"$repo/.git/objects/$(test_oid_to_path $obj)\" &&\n+\ttest_path_is_file \"$path\" &&\n+\trm \"$path\"\n }\n \n pack_as_from_promisor () {\n-- \n2.51.2\n\n"},{"id":"538750","messageId":"CALE2CrTzYbMam_fi5HszSUFVZADE1haLtpBqhUmd1ki9biM2hA@mail.gmail.com","threadId":"65219","inReplyTo":"20260312125030.7799-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH] t0410: modernize delete_object helper","fromName":"Pushkar Singh","fromEmail":"pushkarkumarsingh1970@gmail.com","sentAt":"2026-03-12T14:17:05Z","receivedAt":"2026-03-12T14:17:18Z","isPatch":true,"sender":{"key":"pushkarkumarsingh1970@gmail.com","avatar":"https://avatars.githubusercontent.com/u/173247767?v=4"},"body":"Hi Siddharth,\n\nThanks for the cleanup! Using `test_oid_to_path` here makes the helper\ncleaner and avoids the `sed` trick.\n\nI tested this patch locally and the tests passed.\n\nOne small thought: maybe we could quote `$obj` in the\n`test_oid_to_path` call, like this:\n\n>\n> + path=\"$repo/.git/objects/$(test_oid_to_path $obj)\" &&\n path=\"$repo/.git/objects/$(test_oid_to_path \"$obj\")\" &&\n\nAlso, would it make sense to use `local` for `repo` and `obj` to avoid\nleaking variables outside the helper? Not a strong opinion, but it\nmight make the helper a bit safer.\n\nThanks,\nPushkar\n"},{"id":"538790","messageId":"CAPig+cS3v=OT6BJ0WWh=qvWBm1TVck+O7eKd7gJ2fe_d5Rny_A@mail.gmail.com","threadId":"65219","inReplyTo":"20260312125030.7799-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH] t0410: modernize delete_object helper","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2026-03-12T19:05:24Z","receivedAt":"2026-03-12T19:05:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 12, 2026 at 8:50 AM Siddharth Shrimali\n wrote:\n> The delete_object helper currently relies on a manual sed command to\n> calculate object paths. This works, but it's a bit brittle and forces\n> us to maintain shell logic that Git's own test suite can already\n> handle more elegantly.\n>\n> Switch to 'test_oid_to_path' to let Git handle the path logic. This\n> makes the helper hash independent, which is much cleaner than manual\n> string manipulation. While we're at it, add a call to\n> 'test_path_is_file' so that the test fails early and clearly if we\n> try to delete an object that isn't there, rather than failing\n> silently.\n>\n> Signed-off-by: Siddharth Shrimali \n> ---\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> @@ -11,7 +11,11 @@ test_description='partial clone'\n> delete_object () {\n> - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n> + repo=$1\n> + obj=$2\n> + path=\"$repo/.git/objects/$(test_oid_to_path $obj)\" &&\n> + test_path_is_file \"$path\" &&\n> + rm \"$path\"\n> }\n\nDespite what the commit message says, adding a call to\n`test_path_is_file` here does not add value since `rm` will already\nfail noisily and exit with an error code if the path does not exist.\nMoreover, because it's unnecessary, the `test_path_is_file` invocation\nmay confuse readers into thinking that something subtle is going on\nthat requires extra scrutiny and care even though that's not the case.\nSo let's not add this needless extra code.\n"},{"id":"538792","messageId":"xmqqo6kseuge.fsf@gitster.g","threadId":"65219","inReplyTo":"CAPig+cS3v=OT6BJ0WWh=qvWBm1TVck+O7eKd7gJ2fe_d5Rny_A@mail.gmail.com","subject":"Re: [PATCH] t0410: modernize delete_object helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-12T19:09:05Z","receivedAt":"2026-03-12T19:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n>> @@ -11,7 +11,11 @@ test_description='partial clone'\n>> delete_object () {\n>> - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n>> + repo=$1\n>> + obj=$2\n>> + path=\"$repo/.git/objects/$(test_oid_to_path $obj)\" &&\n>> + test_path_is_file \"$path\" &&\n>> + rm \"$path\"\n>> }\n>\n> Despite what the commit message says, adding a call to\n> `test_path_is_file` here does not add value since `rm` will already\n> fail noisily and exit with an error code if the path does not exist.\n> Moreover, because it's unnecessary, the `test_path_is_file` invocation\n> may confuse readers into thinking that something subtle is going on\n> that requires extra scrutiny and care even though that's not the case.\n> So let's not add this needless extra code.\n\nGood to point this out. Use of test_oid_to_path would still be\ngood, though.\n\nThanks.\n"},{"id":"538813","messageId":"20260312203337.GA2552877@coredump.intra.peff.net","threadId":"65219","inReplyTo":"20260312125030.7799-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH] t0410: modernize delete_object helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-12T20:33:37Z","receivedAt":"2026-03-12T20:33:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 12, 2026 at 06:20:30PM +0530, Siddharth Shrimali wrote:\n\n> delete_object () {\n> -\trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n> +\trepo=$1\n> +\tobj=$2\n> +\tpath=\"$repo/.git/objects/$(test_oid_to_path $obj)\" &&\n> +\ttest_path_is_file \"$path\" &&\n> +\trm \"$path\"\n> }\n\nIt might worth marking these new variables with \"local\". They are not\nused elsewhere by the script currently, but they are common enough names\nthat a collision in the future doesn't seem that unlikely.\n\n-Peff\n"},{"id":"538820","messageId":"xmqqwlzgaeo2.fsf@gitster.g","threadId":"65219","inReplyTo":"20260312203337.GA2552877@coredump.intra.peff.net","subject":"Re: [PATCH] t0410: modernize delete_object helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-12T22:03:41Z","receivedAt":"2026-03-12T22:03:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Mar 12, 2026 at 06:20:30PM +0530, Siddharth Shrimali wrote:\n>\n>> delete_object () {\n>> -\trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n>> +\trepo=$1\n>> +\tobj=$2\n>> +\tpath=\"$repo/.git/objects/$(test_oid_to_path $obj)\" &&\n>> +\ttest_path_is_file \"$path\" &&\n>> +\trm \"$path\"\n>> }\n>\n> It might worth marking these new variables with \"local\". They are not\n> used elsewhere by the script currently, but they are common enough names\n> that a collision in the future doesn't seem that unlikely.\n\nYes, and another thing that was pointed out is it is dubious to use\ntest_path_is_file here, as \"rm\" would be loud enough when it fails.\n"},{"id":"538835","messageId":"20260313025852.30010-1-r.siddharth.shrimali@gmail.com","threadId":"65219","inReplyTo":"20260312125030.7799-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH v2] t0410: modernize delete_object helper","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-03-13T02:58:52Z","receivedAt":"2026-03-13T02:59:04Z","isPatch":true,"sender":{"key":"r.siddharth.shrimali@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183274193?v=4"},"body":"The delete_object helper currently relies on a manual sed command to\ncalculate object paths. This works, but it's a bit brittle and forces\nus to maintain shell logic that Git's own test suite can already\nhandle more elegantly.\n\nSwitch to 'test_oid_to_path' to let Git handle the path logic. This\nmakes the helper hash independent, which is much cleaner than manual\nstring manipulation. While at it, use 'local' to declare helper-specific\nvariables and quote them to follow Git's coding style. This prevents\nthem from leaking into global shell scope and avoids potential naming\nconflicts with other parts of the test suite.\n\nHelped-by: Pushkar Singh \nSuggested-by: Jeff King \nSigned-off-by: Siddharth Shrimali \n---\nThanks to Pushkar for initially testing the patch locally and suggesting\nimprovements. Thanks also to Jeff, Eric, and Junio for the technical \nfeedback regarding 'local' variables and the redundancy of \n'test_path_is_file'.\n\nChanges in v2:\n- Added 'local' to variables and ensured they are properly quoted.\n- Removed 'test_path_is_file' as 'rm' provides sufficient error reporting.\n\n t/t0410-partial-clone.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 2a5bdbeeb8..d36d1c3a5f 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -11,7 +11,10 @@ test_description='partial clone'\n GIT_TEST_COMMIT_GRAPH=0\n \n delete_object () {\n-\trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n+\tlocal repo=$1\n+\tlocal obj=$2\n+\tlocal path=\"$repo/.git/objects/$(test_oid_to_path \"$obj\")\" &&\n+\trm \"$path\"\n }\n \n pack_as_from_promisor () {\n-- \n2.51.2\n\n"},{"id":"538843","messageId":"20260313045825.GC3753825@coredump.intra.peff.net","threadId":"65219","inReplyTo":"20260313025852.30010-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH v2] t0410: modernize delete_object helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-13T04:58:25Z","receivedAt":"2026-03-13T04:58:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 13, 2026 at 08:28:52AM +0530, Siddharth Shrimali wrote:\n\n> delete_object () {\n> -\trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n> +\tlocal repo=$1\n> +\tlocal obj=$2\n> +\tlocal path=\"$repo/.git/objects/$(test_oid_to_path \"$obj\")\" &&\n> +\trm \"$path\"\n\nYou'll need to write these first two with extra quotes, like:\n\n local repo=\"$1\"\n local obj=\"$2\"\n\nIt is fine without the quotes on most shells, but there are some\nhistorical versions that need it (broken dash, according to\nCodingGuidelines?).\n\nThis will be caught by \"make test\", which runs our\ncheck-non-portable-shell script. But that's not run if you're just doing\na one-shot ./0410 invocation.\n\n-Peff\n"},{"id":"538844","messageId":"20260313053159.36492-1-r.siddharth.shrimali@gmail.com","threadId":"65219","inReplyTo":"20260313025852.30010-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH v3] t0410: modernize delete_object helper","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-03-13T05:31:59Z","receivedAt":"2026-03-13T05:32:08Z","isPatch":true,"sender":{"key":"r.siddharth.shrimali@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183274193?v=4"},"body":"The delete_object helper currently relies on a manual sed command to\ncalculate object paths. This works, but it's a bit brittle and forces\nus to maintain shell logic that Git's own test suite can already\nhandle more elegantly.\n\nSwitch to 'test_oid_to_path' to let Git handle the path logic. This\nmakes the helper hash independent, which is much cleaner than manual\nstring manipulation. While at it, use 'local' to declare helper-specific\nvariables and quote them to follow Git's coding style. This prevents\nthem from leaking into global shell scope and avoids potential naming\nconflicts with other parts of the test suite.\n\nHelped-by: Pushkar Singh \nSuggested-by: Jeff King \nSigned-off-by: Siddharth Shrimali \n---\nThanks to Pushkar for the initial review and testing. Thanks to Jeff\nfor the catch regarding 'local' assignment portability. Paths with \nspaces could cause issues on some shells without the extra quotes. \nThanks also to Eric and Junio for the feedback on 'test_path_is_file'.\n\nChanges in v3:\n- Added quotes to 'local' variable assignments to improve shell \n portability.\n\nChanges in v2:\n- Added 'local' to variables and ensured they are properly quoted.\n- Removed 'test_path_is_file' as 'rm' provides sufficient \n error reporting.\n\n t/t0410-partial-clone.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 2a5bdbeeb8..52e19728a3 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -11,7 +11,10 @@ test_description='partial clone'\n GIT_TEST_COMMIT_GRAPH=0\n \n delete_object () {\n-\trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n+\tlocal repo=\"$1\"\n+\tlocal obj=\"$2\"\n+\tlocal path=\"$repo/.git/objects/$(test_oid_to_path \"$obj\")\" &&\n+\trm \"$path\"\n }\n \n pack_as_from_promisor () {\n-- \n2.51.2\n\n"},{"id":"538902","messageId":"xmqqecln92q9.fsf@gitster.g","threadId":"65219","inReplyTo":"20260313025852.30010-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH v2] t0410: modernize delete_object helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-13T15:19:10Z","receivedAt":"2026-03-13T15:19:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Shrimali writes:\n\n> t/t0410-partial-clone.sh | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index 2a5bdbeeb8..d36d1c3a5f 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -11,7 +11,10 @@ test_description='partial clone'\n> GIT_TEST_COMMIT_GRAPH=0\n> \n> delete_object () {\n> -\trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n> +\tlocal repo=$1\n> +\tlocal obj=$2\n> +\tlocal path=\"$repo/.git/objects/$(test_oid_to_path \"$obj\")\" &&\n> +\trm \"$path\"\n> }\n> \n> pack_as_from_promisor () {\n\nPlease do not forget to run test-lint.\n\n $ make -C t test-lint\n\nIf you did so, you would have been told\n\nt0410-partial-clone.sh:14: error: quote \"$val\" in 'local var=$val': local repo=$1\nt0410-partial-clone.sh:15: error: quote \"$val\" in 'local var=$val': local obj=$2\n\nSquash this in, perhaps?\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex d36d1c3a5f..52e19728a3 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -11,8 +11,8 @@ test_description='partial clone'\n GIT_TEST_COMMIT_GRAPH=0\n \n delete_object () {\n-\tlocal repo=$1\n-\tlocal obj=$2\n+\tlocal repo=\"$1\"\n+\tlocal obj=\"$2\"\n \tlocal path=\"$repo/.git/objects/$(test_oid_to_path \"$obj\")\" &&\n \trm \"$path\"\n }\n-- \n2.53.0-713-g4f09e58cf8\n\n\n"},{"id":"538903","messageId":"xmqqa4wb92n8.fsf@gitster.g","threadId":"65219","inReplyTo":"20260313053159.36492-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH v3] t0410: modernize delete_object helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-13T15:20:59Z","receivedAt":"2026-03-13T15:21:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Siddharth Shrimali writes:\n\n> Changes in v3:\n> - Added quotes to 'local' variable assignments to improve shell \n> portability.\n\nOh, our mails crossed with some timezone differences. The update\nlooks good.\n\nThanks. Will replace.\n\n>\n> Changes in v2:\n> - Added 'local' to variables and ensured they are properly quoted.\n> - Removed 'test_path_is_file' as 'rm' provides sufficient \n> error reporting.\n>\n> t/t0410-partial-clone.sh | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index 2a5bdbeeb8..52e19728a3 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -11,7 +11,10 @@ test_description='partial clone'\n> GIT_TEST_COMMIT_GRAPH=0\n> \n> delete_object () {\n> -\trm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|')\n> +\tlocal repo=\"$1\"\n> +\tlocal obj=\"$2\"\n> +\tlocal path=\"$repo/.git/objects/$(test_oid_to_path \"$obj\")\" &&\n> +\trm \"$path\"\n> }\n> \n> pack_as_from_promisor () {\n"},{"id":"538904","messageId":"xmqq3423928q.fsf@gitster.g","threadId":"65219","inReplyTo":"20260313045825.GC3753825@coredump.intra.peff.net","subject":"Re: [PATCH v2] t0410: modernize delete_object helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-13T15:29:41Z","receivedAt":"2026-03-13T15:29:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This will be caught by \"make test\", which runs our\n> check-non-portable-shell script. But that's not run if you're just doing\n> a one-shot ./0410 invocation.\n\nI am tempted to add this to SubmittingPatches::[test]; the first\nline of the new paragraph appears several lines before the pre-\ncontext but without \"`make test` from the top-level\".\n\n Documentation/SubmittingPatches | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\nindex e270ccbe85..eef07d6670 100644\n--- c/Documentation/SubmittingPatches\n+++ w/Documentation/SubmittingPatches\n@@ -204,6 +204,10 @@ fixed by accident to avoid regression. Also, try merging your work to\n that are still in flight may have unexpected interactions with what\n you are trying to do in your topic.\n \n+After any code change, make sure that the entire test suite passes,\n+with `make test` from the top-level. We say this twice here because\n+it is important.\n+\n Pushing to a fork of https://github.com/git/git will use their CI\n integration to test your changes on Linux, Mac and Windows. See the\n <> section for details.\n"},{"id":"539275","messageId":"20260318023757.GA1925517@coredump.intra.peff.net","threadId":"65219","inReplyTo":"xmqq3423928q.fsf@gitster.g","subject":"Re: [PATCH v2] t0410: modernize delete_object helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-18T02:37:57Z","receivedAt":"2026-03-18T02:37:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 13, 2026 at 08:29:41AM -0700, Junio C Hamano wrote:\n\n> I am tempted to add this to SubmittingPatches::[test]; the first\n> line of the new paragraph appears several lines before the pre-\n> context but without \"`make test` from the top-level\".\n> [...]\n> +After any code change, make sure that the entire test suite passes,\n> +with `make test` from the top-level. We say this twice here because\n> +it is important.\n\nSeems reasonable. The other mention of \"run the whole suite\" is buried\ninside a big paragraph.\n\nI have no idea if people read the document at all, though. I certainly\ndon't, but I am also not a new person trying to join the project. I've\nusually read, if not participated in, most discussions which lead to\nchanges in them anyway. ;)\n\n-Peff\n"}]}