Skip to contentSkip to conversation

t0081: modernize the code testing whether a file exists

PATCH Superseded Shashank S via GitGitGadget 5 participants 11 messages Oct 14 – Nov 6

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 t0081: modernize the code testing whether a file exists

From: Shashank <[email protected]>

Replace the old-style shell test -f command with Git's modern test_path_is_file.

This instance was found using:
`git grep "test -[efd]" t/'

---
[outreachy] t0081: Replace the old style 'test -f' to new 'test_path_is_file'

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2254%2Fshashu8660%2Fupdate-t0081-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2254/shashu8660/update-t0081-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2254

 t/t0081-find-pack.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
t/t0081-find-pack.sh +1 -1
index ff9d56fbf1..7d5082c487 100755
@@ -32,7 +32,7 @@ test_expect_success 'repack everything into a single packfile' '
 		".git/objects/pack/pack-"*".pack") true ;;
 		*) false ;;
 	esac &&
-	test -f "$head_commit_pack" &&
+	test_path_is_file "$head_commit_pack" &&
 
 	# Everything is in the same pack
 	test "$head_commit_pack" = "$head_tree_pack" &&

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 7 lines

From: Shashank <[email protected]>

Replace the old-style shell test -f command with Git's modern test_path_is_file.

This instance was found using:
`git grep "test -[efd]" t/'

This is not necessary, remove it.

Quoted 18 lines

---
[outreachy] t0081: Replace the old style 'test -f' to new 'test_path_is_file'

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2254%2Fshashu8660%2Fupdate-t0081-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2254/shashu8660/update-t0081-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2254

 t/t0081-find-pack.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
t/t0081-find-pack.sh +0 -0
index ff9d56fbf1..7d5082c487 100755

t/t0081-find-pack.sh test_expect_success 'repack everything into a single packfile' ' · line 35

Quoted 5 lines
t/t0081-find-pack.sh +1 -1
@@ -32,7 +32,7 @@ test_expect_success 'repack everything into a single packfile' '
                ".git/objects/pack/pack-"*".pack") true ;;
                *) false ;;
        esac &&
-       test -f "$head_commit_pack" &&
+       test_path_is_file "$head_commit_pack" &&

looks good to me.

Quoted 7 lines
t/t0081-find-pack.sh +0 -0
        # Everything is in the same pack
        test "$head_commit_pack" = "$head_tree_pack" &&
 

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd --
gitgitgadget

Thanks.

#2 lore raw
Quoted 8 lines · Usman Akinyemi
Quoted 7 lines

From: Shashank <[email protected]>

Replace the old-style shell test -f command with Git's modern test_path_is_file.

This instance was found using:
`git grep "test -[efd]" t/'

This is not necessary, remove it.

Since many candidates are working on similar microprojects (which is fine) and making the same mistakes (which is expected), perhaps mentors should tell them to read the review exchanges on other candidates' patches and learn from them, instead of or in addition to giving the same advice repeatedly.

#3 lore raw
Quoted 10 lines · Junio C Hamano
Quoted 1 line · Usman Akinyemi
Quoted 1 line

From: Shashank <[email protected]>

Since many candidates are working on similar microprojects (which is fine) and making the same mistakes (which is expected), perhaps mentors should tell them to read the review exchanges on other candidates' patches and learn from them, instead of or in addition to giving the same advice repeatedly.

thank you for the explanation and the review , from now on i will go through the other candidates patches and look in to them and mentor reviews in them and learn from that

#4 lore raw
In reply to Junio C Hamano
Quoted 9 lines · Junio C Hamano
Quoted 7 lines · Usman Akinyemi
Quoted 7 lines

From: Shashank <[email protected]>

Replace the old-style shell test -f command with Git's modern test_path_is_file.

This instance was found using:
`git grep "test -[efd]" t/'

Quoted 7 lines
Quoted 1 line

This is not necessary, remove it.

Since many candidates are working on similar microprojects (which is fine) and making the same mistakes (which is expected), perhaps mentors should tell them to read the review exchanges on other candidates' patches and learn from them, instead of or in addition to giving the same advice repeatedly.

I agree that it is a good advice to ask candidate to read other candidates' patches. We do recommend this in our guide related to working on Microprojects:

https://git.github.io/General-Microproject-Information/#research-existing-related-work

That said, I just wanted to highlight one thing. In the Microproject ideas page[1], we have the following for this specific idea:

Notes

- Start small: Pick a test file with just a few instances to convert
- Run the test suite after your changes to ensure nothing breaks
- Follow Git’s commit message style
- Include which command you used to find the instances in your commit message

So, we can't really blame the candidates for including the command in the commit message. It is our mistake asking them to include it in the first place when we don't prefer it to be present there.

I raised a PR to fix that:

https://github.com/git/git.github.io/pull/875

[1]: https://git.github.io/SoC-2026-Microprojects/

Signature · 1 line
Sivaraam
#5 lore raw
Quoted 11 lines · Kaartic Sivaraam

That said, I just wanted to highlight one thing. In the Microproject ideas page[1], we have the following for this specific idea:

Notes

- Start small: Pick a test file with just a few instances to convert
- Run the test suite after your changes to ensure nothing breaks
- Follow Git’s commit message style
- Include which command you used to find the instances in your commit message

Quoted 7 lines

So, we can't really blame the candidates for including the command in the commit message. It is our mistake asking them to include it in the first place when we don't prefer it to be present there.

I raised a PR to fix that:

   https://github.com/git/git.github.io/pull/875

I do not think your update is taking us in the right direction, though. Recommend to have it under three-dash lines instead, perhaps?

#6 lore raw
Quoted 9 lines · Junio C Hamano
Quoted 3 lines · Kaartic Sivaraam

I raised a PR to fix that:

    https://github.com/git/git.github.io/pull/875

I do not think your update is taking us in the right direction, though. Recommend to have it under three-dash lines instead, perhaps?

Got it. Does the following change read better?

links/mentoring/soc/SoC-2026-Microprojects.md +3 -2
index 41354b50..17519e2d 100644
@@ -49,8 +49,9 @@ with Git's dedicated test helpers like `test_path_is_file`.
 - Start small: Pick a test file with just a few instances to convert
 - Run the test suite after your changes to ensure nothing breaks
 - Follow Git's commit message style
-- You _do not need to_ include the command you used to find the instances
-  in your commit message
+- You can include the command you used to find the instances in your
+  cover letter or between the three-dash line and the diffstat, whichever
+  is appropriate. Do not mention it in your commit message
 
 #### Need Help?
 - Reference [this 

discussion](https://public-inbox.org/git/CAPig+cRfO8t1tdCL6MB4b9XopF3HkZ==hU83AFZ38b-2zsXDjQ@mail.gmail.com/)

Signature · 1 line
Sivaraam
#7 lore raw
Quoted 13 lines · Kaartic Sivaraam

Got it. Does the following change read better?

links/mentoring/soc/SoC-2026-Microprojects.md +0 -0
index 41354b50..17519e2d 100644
@@ -49,8 +49,9 @@ with Git's dedicated test helpers like 

test_path_is_file.
- Start small: Pick a test file with just a few instances to convert
- Run the test suite after your changes to ensure nothing breaks
- Follow Git's commit message style
-- You _do not need to_ include the command you used to find the instances

links/mentoring/soc/SoC-2026-Microprojects.md with Git's dedicated test helpers like `test_path_is_file`. · line 54

Quoted 4 lines
links/mentoring/soc/SoC-2026-Microprojects.md +3 -1
-  in your commit message
+- You can include the command you used to find the instances in your
+  cover letter or between the three-dash line and the diffstat, whichever
+  is appropriate. Do not mention it in your commit message

Yes, this reads much better. It clearly guides contributors to put the command below the three-dash line or in the cover letter without cluttering the commit message.

#8 lore raw
Quoted 15 lines · Junio C Hamano
Quoted 13 lines · Kaartic Sivaraam

Got it. Does the following change read better?

links/mentoring/soc/SoC-2026-Microprojects.md +0 -0
index 41354b50..17519e2d 100644
@@ -49,8 +49,9 @@ with Git's dedicated test helpers like

test_path_is_file.
- Start small: Pick a test file with just a few instances to convert
- Run the test suite after your changes to ensure nothing breaks
- Follow Git's commit message style
-- You _do not need to_ include the command you used to find the instances

Quoted 8 lines

links/mentoring/soc/SoC-2026-Microprojects.md with Git's dedicated test helpers like `test_path_is_file`. · line 54

Quoted 4 lines
links/mentoring/soc/SoC-2026-Microprojects.md +3 -1
-  in your commit message
+- You can include the command you used to find the instances in your
+  cover letter or between the three-dash line and the diffstat, whichever
+  is appropriate. Do not mention it in your commit message

Yes, this reads much better. It clearly guides contributors to put the command below the three-dash line or in the cover letter without cluttering the commit message.

Thank you! I've updated the description of the microproject. It should be live at https://git.github.io/SoC-2026-Microprojects/

Signature · 1 line
Sivaraam
#9 lore raw
In reply to Usman Akinyemi
Quoted 9 lines · Usman Akinyemi
Quoted 8 lines

From: Shashank <[email protected]>

Replace the old-style shell test -f command with Git's modern test_path_is_file.

This instance was found using:
`git grep "test -[efd]" t/'

This is not necessary, remove it.

thank you for the review , i will remove it and i will submit patch v2

#10 lore raw

PATCH v2 1/1 t0081: modernize the code testing whether a file exists

In reply to Shashank S via GitGitGadget

From: Shashank <[email protected]>

Replace the old-style shell test -f command with Git's modern test_path_is_file.

---
[outreachy] t0081: Replace the old style 'test -f' to new 'test_path_is_file'

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2254%2Fshashu8660%2Fupdate-t0081-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2254/shashu8660/update-t0081-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2254

Range-diff vs v1:

1: 906c628765 ! 1: d4807f2751 t0081: modernize the code testing whether a file exists
@@ Metadata
Author: Shashank <[email protected]>

## Commit message ##
- t0081: modernize the code testing whether a file exists
+ [Outreachy] t0081: modernize the code testing whether a file exists

Replace the old-style shell test -f command with Git's modern test_path_is_file.

- This instance was found using:
- `git grep "test -[efd]" t/'
-

## t/t0081-find-pack.sh ##

 t/t0081-find-pack.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
t/t0081-find-pack.sh +1 -1
index ff9d56fbf1..7d5082c487 100755
@@ -32,7 +32,7 @@ test_expect_success 'repack everything into a single packfile' '
 		".git/objects/pack/pack-"*".pack") true ;;
 		*) false ;;
 	esac &&
-	test -f "$head_commit_pack" &&
+	test_path_is_file "$head_commit_pack" &&
 
 	# Everything is in the same pack
 	test "$head_commit_pack" = "$head_tree_pack" &&

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd

Signature · 1 line
gitgitgadget