# [PATCH] t0410: modernize delete_object helper 13 messages from 2026-03-12 to 2026-03-18. Participants: Siddharth Shrimali, Pushkar Singh, Eric Sunshine, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/65219 ## Siddharth Shrimali, 2026-03-12 12:50 Subject: [PATCH] t0410: modernize delete_object helper Message-ID: <20260312125030.7799-1-r.siddharth.shrimali@gmail.com> URL: https://gitlist.dev/e/20260312125030.7799-1-r.siddharth.shrimali%40gmail.com ``` The delete_object helper currently relies on a manual sed command to calculate object paths. This works, but it's a bit brittle and forces us to maintain shell logic that Git's own test suite can already handle more elegantly. Switch to 'test_oid_to_path' to let Git handle the path logic. This makes the helper hash independent, which is much cleaner than manual string manipulation. While we're at it, add a call to 'test_path_is_file' so that the test fails early and clearly if we try to delete an object that isn't there, rather than failing silently. Signed-off-by: Siddharth Shrimali --- t/t0410-partial-clone.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 2a5bdbeeb8..eca113f0f8 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -11,7 +11,11 @@ test_description='partial clone' GIT_TEST_COMMIT_GRAPH=0 delete_object () { - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') + repo=$1 + obj=$2 + path="$repo/.git/objects/$(test_oid_to_path $obj)" && + test_path_is_file "$path" && + rm "$path" } pack_as_from_promisor () { -- 2.51.2 ``` ## Pushkar Singh, 2026-03-12 14:17 Subject: Re: [PATCH] t0410: modernize delete_object helper Message-ID: URL: https://gitlist.dev/e/CALE2CrTzYbMam_fi5HszSUFVZADE1haLtpBqhUmd1ki9biM2hA%40mail.gmail.com In-Reply-To: <20260312125030.7799-1-r.siddharth.shrimali@gmail.com> ``` Hi Siddharth, Thanks for the cleanup! Using `test_oid_to_path` here makes the helper cleaner and avoids the `sed` trick. I tested this patch locally and the tests passed. One small thought: maybe we could quote `$obj` in the `test_oid_to_path` call, like this: > > + path="$repo/.git/objects/$(test_oid_to_path $obj)" && path="$repo/.git/objects/$(test_oid_to_path "$obj")" && Also, would it make sense to use `local` for `repo` and `obj` to avoid leaking variables outside the helper? Not a strong opinion, but it might make the helper a bit safer. Thanks, Pushkar ``` ## Eric Sunshine, 2026-03-12 19:05 Subject: Re: [PATCH] t0410: modernize delete_object helper Message-ID: URL: https://gitlist.dev/e/CAPig%2BcS3v%3DOT6BJ0WWh%3DqvWBm1TVck%2BO7eKd7gJ2fe_d5Rny_A%40mail.gmail.com In-Reply-To: <20260312125030.7799-1-r.siddharth.shrimali@gmail.com> ``` On Thu, Mar 12, 2026 at 8:50 AM Siddharth Shrimali wrote: > The delete_object helper currently relies on a manual sed command to > calculate object paths. This works, but it's a bit brittle and forces > us to maintain shell logic that Git's own test suite can already > handle more elegantly. > > Switch to 'test_oid_to_path' to let Git handle the path logic. This > makes the helper hash independent, which is much cleaner than manual > string manipulation. While we're at it, add a call to > 'test_path_is_file' so that the test fails early and clearly if we > try to delete an object that isn't there, rather than failing > silently. > > Signed-off-by: Siddharth Shrimali > --- > diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh > @@ -11,7 +11,11 @@ test_description='partial clone' > delete_object () { > - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') > + repo=$1 > + obj=$2 > + path="$repo/.git/objects/$(test_oid_to_path $obj)" && > + test_path_is_file "$path" && > + rm "$path" > } Despite what the commit message says, adding a call to `test_path_is_file` here does not add value since `rm` will already fail noisily and exit with an error code if the path does not exist. Moreover, because it's unnecessary, the `test_path_is_file` invocation may confuse readers into thinking that something subtle is going on that requires extra scrutiny and care even though that's not the case. So let's not add this needless extra code. ``` ## Junio C Hamano, 2026-03-12 19:09 Subject: Re: [PATCH] t0410: modernize delete_object helper Message-ID: URL: https://gitlist.dev/e/xmqqo6kseuge.fsf%40gitster.g In-Reply-To: ``` Eric Sunshine writes: >> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh >> @@ -11,7 +11,11 @@ test_description='partial clone' >> delete_object () { >> - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') >> + repo=$1 >> + obj=$2 >> + path="$repo/.git/objects/$(test_oid_to_path $obj)" && >> + test_path_is_file "$path" && >> + rm "$path" >> } > > Despite what the commit message says, adding a call to > `test_path_is_file` here does not add value since `rm` will already > fail noisily and exit with an error code if the path does not exist. > Moreover, because it's unnecessary, the `test_path_is_file` invocation > may confuse readers into thinking that something subtle is going on > that requires extra scrutiny and care even though that's not the case. > So let's not add this needless extra code. Good to point this out. Use of test_oid_to_path would still be good, though. Thanks. ``` ## Jeff King, 2026-03-12 20:33 Subject: Re: [PATCH] t0410: modernize delete_object helper Message-ID: <20260312203337.GA2552877@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260312203337.GA2552877%40coredump.intra.peff.net In-Reply-To: <20260312125030.7799-1-r.siddharth.shrimali@gmail.com> ``` On Thu, Mar 12, 2026 at 06:20:30PM +0530, Siddharth Shrimali wrote: > delete_object () { > - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') > + repo=$1 > + obj=$2 > + path="$repo/.git/objects/$(test_oid_to_path $obj)" && > + test_path_is_file "$path" && > + rm "$path" > } It might worth marking these new variables with "local". They are not used elsewhere by the script currently, but they are common enough names that a collision in the future doesn't seem that unlikely. -Peff ``` ## Junio C Hamano, 2026-03-12 22:03 Subject: Re: [PATCH] t0410: modernize delete_object helper Message-ID: URL: https://gitlist.dev/e/xmqqwlzgaeo2.fsf%40gitster.g In-Reply-To: <20260312203337.GA2552877@coredump.intra.peff.net> ``` Jeff King writes: > On Thu, Mar 12, 2026 at 06:20:30PM +0530, Siddharth Shrimali wrote: > >> delete_object () { >> - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') >> + repo=$1 >> + obj=$2 >> + path="$repo/.git/objects/$(test_oid_to_path $obj)" && >> + test_path_is_file "$path" && >> + rm "$path" >> } > > It might worth marking these new variables with "local". They are not > used elsewhere by the script currently, but they are common enough names > that a collision in the future doesn't seem that unlikely. Yes, and another thing that was pointed out is it is dubious to use test_path_is_file here, as "rm" would be loud enough when it fails. ``` ## Siddharth Shrimali, 2026-03-13 02:58 Subject: [PATCH v2] t0410: modernize delete_object helper Message-ID: <20260313025852.30010-1-r.siddharth.shrimali@gmail.com> URL: https://gitlist.dev/e/20260313025852.30010-1-r.siddharth.shrimali%40gmail.com In-Reply-To: <20260312125030.7799-1-r.siddharth.shrimali@gmail.com> ``` The delete_object helper currently relies on a manual sed command to calculate object paths. This works, but it's a bit brittle and forces us to maintain shell logic that Git's own test suite can already handle more elegantly. Switch to 'test_oid_to_path' to let Git handle the path logic. This makes the helper hash independent, which is much cleaner than manual string manipulation. While at it, use 'local' to declare helper-specific variables and quote them to follow Git's coding style. This prevents them from leaking into global shell scope and avoids potential naming conflicts with other parts of the test suite. Helped-by: Pushkar Singh Suggested-by: Jeff King Signed-off-by: Siddharth Shrimali --- Thanks to Pushkar for initially testing the patch locally and suggesting improvements. Thanks also to Jeff, Eric, and Junio for the technical feedback regarding 'local' variables and the redundancy of 'test_path_is_file'. Changes in v2: - Added 'local' to variables and ensured they are properly quoted. - Removed 'test_path_is_file' as 'rm' provides sufficient error reporting. t/t0410-partial-clone.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 2a5bdbeeb8..d36d1c3a5f 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -11,7 +11,10 @@ test_description='partial clone' GIT_TEST_COMMIT_GRAPH=0 delete_object () { - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') + local repo=$1 + local obj=$2 + local path="$repo/.git/objects/$(test_oid_to_path "$obj")" && + rm "$path" } pack_as_from_promisor () { -- 2.51.2 ``` ## Jeff King, 2026-03-13 04:58 Subject: Re: [PATCH v2] t0410: modernize delete_object helper Message-ID: <20260313045825.GC3753825@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260313045825.GC3753825%40coredump.intra.peff.net In-Reply-To: <20260313025852.30010-1-r.siddharth.shrimali@gmail.com> ``` On Fri, Mar 13, 2026 at 08:28:52AM +0530, Siddharth Shrimali wrote: > delete_object () { > - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') > + local repo=$1 > + local obj=$2 > + local path="$repo/.git/objects/$(test_oid_to_path "$obj")" && > + rm "$path" You'll need to write these first two with extra quotes, like: local repo="$1" local obj="$2" It is fine without the quotes on most shells, but there are some historical versions that need it (broken dash, according to CodingGuidelines?). This will be caught by "make test", which runs our check-non-portable-shell script. But that's not run if you're just doing a one-shot ./0410 invocation. -Peff ``` ## Siddharth Shrimali, 2026-03-13 05:31 Subject: [PATCH v3] t0410: modernize delete_object helper Message-ID: <20260313053159.36492-1-r.siddharth.shrimali@gmail.com> URL: https://gitlist.dev/e/20260313053159.36492-1-r.siddharth.shrimali%40gmail.com In-Reply-To: <20260313025852.30010-1-r.siddharth.shrimali@gmail.com> ``` The delete_object helper currently relies on a manual sed command to calculate object paths. This works, but it's a bit brittle and forces us to maintain shell logic that Git's own test suite can already handle more elegantly. Switch to 'test_oid_to_path' to let Git handle the path logic. This makes the helper hash independent, which is much cleaner than manual string manipulation. While at it, use 'local' to declare helper-specific variables and quote them to follow Git's coding style. This prevents them from leaking into global shell scope and avoids potential naming conflicts with other parts of the test suite. Helped-by: Pushkar Singh Suggested-by: Jeff King Signed-off-by: Siddharth Shrimali --- Thanks to Pushkar for the initial review and testing. Thanks to Jeff for the catch regarding 'local' assignment portability. Paths with spaces could cause issues on some shells without the extra quotes. Thanks also to Eric and Junio for the feedback on 'test_path_is_file'. Changes in v3: - Added quotes to 'local' variable assignments to improve shell portability. Changes in v2: - Added 'local' to variables and ensured they are properly quoted. - Removed 'test_path_is_file' as 'rm' provides sufficient error reporting. t/t0410-partial-clone.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 2a5bdbeeb8..52e19728a3 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -11,7 +11,10 @@ test_description='partial clone' GIT_TEST_COMMIT_GRAPH=0 delete_object () { - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') + local repo="$1" + local obj="$2" + local path="$repo/.git/objects/$(test_oid_to_path "$obj")" && + rm "$path" } pack_as_from_promisor () { -- 2.51.2 ``` ## Junio C Hamano, 2026-03-13 15:19 Subject: Re: [PATCH v2] t0410: modernize delete_object helper Message-ID: URL: https://gitlist.dev/e/xmqqecln92q9.fsf%40gitster.g In-Reply-To: <20260313025852.30010-1-r.siddharth.shrimali@gmail.com> ``` Siddharth Shrimali writes: > t/t0410-partial-clone.sh | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh > index 2a5bdbeeb8..d36d1c3a5f 100755 > --- a/t/t0410-partial-clone.sh > +++ b/t/t0410-partial-clone.sh > @@ -11,7 +11,10 @@ test_description='partial clone' > GIT_TEST_COMMIT_GRAPH=0 > > delete_object () { > - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') > + local repo=$1 > + local obj=$2 > + local path="$repo/.git/objects/$(test_oid_to_path "$obj")" && > + rm "$path" > } > > pack_as_from_promisor () { Please do not forget to run test-lint. $ make -C t test-lint If you did so, you would have been told t0410-partial-clone.sh:14: error: quote "$val" in 'local var=$val': local repo=$1 t0410-partial-clone.sh:15: error: quote "$val" in 'local var=$val': local obj=$2 Squash this in, perhaps? diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index d36d1c3a5f..52e19728a3 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -11,8 +11,8 @@ test_description='partial clone' GIT_TEST_COMMIT_GRAPH=0 delete_object () { - local repo=$1 - local obj=$2 + local repo="$1" + local obj="$2" local path="$repo/.git/objects/$(test_oid_to_path "$obj")" && rm "$path" } -- 2.53.0-713-g4f09e58cf8 ``` ## Junio C Hamano, 2026-03-13 15:20 Subject: Re: [PATCH v3] t0410: modernize delete_object helper Message-ID: URL: https://gitlist.dev/e/xmqqa4wb92n8.fsf%40gitster.g In-Reply-To: <20260313053159.36492-1-r.siddharth.shrimali@gmail.com> ``` Siddharth Shrimali writes: > Changes in v3: > - Added quotes to 'local' variable assignments to improve shell > portability. Oh, our mails crossed with some timezone differences. The update looks good. Thanks. Will replace. > > Changes in v2: > - Added 'local' to variables and ensured they are properly quoted. > - Removed 'test_path_is_file' as 'rm' provides sufficient > error reporting. > > t/t0410-partial-clone.sh | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh > index 2a5bdbeeb8..52e19728a3 100755 > --- a/t/t0410-partial-clone.sh > +++ b/t/t0410-partial-clone.sh > @@ -11,7 +11,10 @@ test_description='partial clone' > GIT_TEST_COMMIT_GRAPH=0 > > delete_object () { > - rm $1/.git/objects/$(echo $2 | sed -e 's|^..|&/|') > + local repo="$1" > + local obj="$2" > + local path="$repo/.git/objects/$(test_oid_to_path "$obj")" && > + rm "$path" > } > > pack_as_from_promisor () { ``` ## Junio C Hamano, 2026-03-13 15:29 Subject: Re: [PATCH v2] t0410: modernize delete_object helper Message-ID: URL: https://gitlist.dev/e/xmqq3423928q.fsf%40gitster.g In-Reply-To: <20260313045825.GC3753825@coredump.intra.peff.net> ``` Jeff King writes: > This will be caught by "make test", which runs our > check-non-portable-shell script. But that's not run if you're just doing > a one-shot ./0410 invocation. I am tempted to add this to SubmittingPatches::[test]; the first line of the new paragraph appears several lines before the pre- context but without "`make test` from the top-level". Documentation/SubmittingPatches | 4 ++++ 1 file changed, 4 insertions(+) diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches index e270ccbe85..eef07d6670 100644 --- c/Documentation/SubmittingPatches +++ w/Documentation/SubmittingPatches @@ -204,6 +204,10 @@ fixed by accident to avoid regression. Also, try merging your work to that are still in flight may have unexpected interactions with what you are trying to do in your topic. +After any code change, make sure that the entire test suite passes, +with `make test` from the top-level. We say this twice here because +it is important. + Pushing to a fork of https://github.com/git/git will use their CI integration to test your changes on Linux, Mac and Windows. See the <> section for details. ``` ## Jeff King, 2026-03-18 02:37 Subject: Re: [PATCH v2] t0410: modernize delete_object helper Message-ID: <20260318023757.GA1925517@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260318023757.GA1925517%40coredump.intra.peff.net In-Reply-To: ``` On Fri, Mar 13, 2026 at 08:29:41AM -0700, Junio C Hamano wrote: > I am tempted to add this to SubmittingPatches::[test]; the first > line of the new paragraph appears several lines before the pre- > context but without "`make test` from the top-level". > [...] > +After any code change, make sure that the entire test suite passes, > +with `make test` from the top-level. We say this twice here because > +it is important. Seems reasonable. The other mention of "run the whole suite" is buried inside a big paragraph. I have no idea if people read the document at all, though. I certainly don't, but I am also not a new person trying to join the project. I've usually read, if not participated in, most discussions which lead to changes in them anyway. ;) -Peff ```