[PATCH] t0410: modernize delete_object helper

[PATCH] t0410: modernize delete_object helper

[Siddharth Shrimali]

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 <r.siddharth.shrimali@gmail.com>
---
 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

Re: [PATCH] t0410: modernize delete_object helper

[Pushkar Singh]

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

Re: [PATCH] t0410: modernize delete_object helper

[Eric Sunshine]

On Thu, Mar 12, 2026 at 8:50 AM Siddharth Shrimali
<r.siddharth.shrimali@gmail.com> 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 <r.siddharth.shrimali@gmail.com>
> ---
> 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.

Re: [PATCH] t0410: modernize delete_object helper

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> 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.

Re: [PATCH] t0410: modernize delete_object helper

[Jeff King]

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

Re: [PATCH] t0410: modernize delete_object helper

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

[PATCH v2] t0410: modernize delete_object helper

[Siddharth Shrimali]

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 <pushkarkumarsingh1970@gmail.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
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

Re: [PATCH v2] t0410: modernize delete_object helper

[Jeff King]

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

[PATCH v3] t0410: modernize delete_object helper

[Siddharth Shrimali]

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 <pushkarkumarsingh1970@gmail.com>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
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

Re: [PATCH v2] t0410: modernize delete_object helper

[Junio C Hamano]

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> 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

Re: [PATCH v3] t0410: modernize delete_object helper

[Junio C Hamano]

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> 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 () {

Re: [PATCH v2] t0410: modernize delete_object helper

[Junio C Hamano]

Jeff King <peff@peff.net> 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
 <<GHCI,GitHub CI>> section for details.

Re: [PATCH v2] t0410: modernize delete_object helper

[Jeff King]

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