Volume XXII, number 280Wednesday, October 7, 2026Latest message 2 hours ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patcht0410: modernize delete_object helper

13 messages between Mar 12, 2026 and Mar 18, 2026, from Siddharth Shrimali, Pushkar Singh, Eric Sunshine, Junio C Hamano, Jeff King.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Siddharth ShrimaliMar 12, 2026, 12:50 UTC on lore

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(-)
Show changes to t/t0410-partial-clone.sh +5 −1
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 SinghMar 12, 2026, 14:17 UTC in reply to Siddharth Shrimali on lore

Re: [PATCH] t0410: modernize delete_object helper

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 SunshineMar 12, 2026, 19:05 UTC in reply to Siddharth Shrimali on lore

Re: [PATCH] t0410: modernize delete_object helper

On Thu, Mar 12, 2026 at 8:50 AM Siddharth Shrimali <r.siddharth.shrimali@gmail.com> wrote:

Show 24 quoted lines
> 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.

Junio C HamanoMar 12, 2026, 19:09 UTC in reply to Eric Sunshine on lore

Re: [PATCH] t0410: modernize delete_object helper

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 18 quoted lines
>> 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 KingMar 12, 2026, 20:33 UTC in reply to Siddharth Shrimali on lore

Re: [PATCH] t0410: modernize delete_object helper

On Thu, Mar 12, 2026 at 06:20:30PM +0530, Siddharth Shrimali wrote:
Show 8 quoted lines
>  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 HamanoMar 12, 2026, 22:03 UTC in reply to Jeff King on lore

Re: [PATCH] t0410: modernize delete_object helper

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> 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 ShrimaliMar 13, 2026, 02:58 UTC in reply to Siddharth Shrimali on lore

[PATCH v2] t0410: modernize delete_object helper

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(-)
Show changes to t/t0410-partial-clone.sh +4 −1
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 KingMar 13, 2026, 04:58 UTC in reply to Siddharth Shrimali on lore

Re: [PATCH v2] t0410: modernize delete_object helper

On Fri, Mar 13, 2026 at 08:28:52AM +0530, Siddharth Shrimali wrote:
Show 6 quoted lines
>  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 ShrimaliMar 13, 2026, 05:31 UTC in reply to Siddharth Shrimali on lore

[PATCH v3] t0410: modernize delete_object helper

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(-)
Show changes to t/t0410-partial-clone.sh +4 −1
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 HamanoMar 13, 2026, 15:19 UTC in reply to Siddharth Shrimali on lore

Re: [PATCH v2] t0410: modernize delete_object helper

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:
Show 19 quoted lines
>  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?
Show changes to t/t0410-partial-clone.sh +2 −2
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 HamanoMar 13, 2026, 15:20 UTC in reply to Siddharth Shrimali on lore

Re: [PATCH v3] t0410: modernize delete_object helper

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.
Show 25 quoted lines
>
> 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 HamanoMar 13, 2026, 15:29 UTC in reply to Jeff King on lore

Re: [PATCH v2] t0410: modernize delete_object helper

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(+)
Show changes to diff +4 −0
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.
Jeff KingMar 18, 2026, 02:37 UTC in reply to Junio C Hamano on lore

Re: [PATCH v2] t0410: modernize delete_object helper

On Fri, Mar 13, 2026 at 08:29:41AM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> 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

Back to recent threads