[PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)

[PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)

[Sampriyo Guin via GitGitGadget]

From: rimo <sampriyoguin@gmail.com>

test -e changed to test_path_exists
test -f changed to test_path_is_file

Signed-off-by: Sampriyo Guin <sampriyoguin@gmail.com>
---
    [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite
    
    This is my first git contribution. A simple fix as specified in Git
    Microprojects. I have tested using Github Actions on my private
    repository. Your comments and feedbacks are much appreciated. Thanks!
    
    , Jialuo She shejialuo@gmail.com , Christian Couder
    christian.couder@gmail.com, Ghanshyam Thakkar shyamthakkar001@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1918%2FRimoGuin%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1918/RimoGuin/master-v1
Pull-Request: https://github.com/git/git/pull/1918

 t/chainlint/chained-subshell.expect | 2 +-
 t/chainlint/chained-subshell.test   | 2 +-
 t/chainlint/function.expect         | 2 +-
 t/chainlint/function.test           | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/chainlint/chained-subshell.expect b/t/chainlint/chained-subshell.expect
index 93fb1a6578b..76393efcd20 100644
--- a/t/chainlint/chained-subshell.expect
+++ b/t/chainlint/chained-subshell.expect
@@ -5,6 +5,6 @@
 6 ) &&
 7 
 8 cut "-d " -f actual | (read s1 s2 s3 &&
-9 test -f $s1 ?!LINT: missing '&&'?!
+9 test_path_is_file $s1 ?!LINT: missing '&&'?!
 10 test $(cat $s2) = tree2path1 &&
 11 test $(cat $s3) = tree3path1)
diff --git a/t/chainlint/chained-subshell.test b/t/chainlint/chained-subshell.test
index 1f11f653982..997f30eadd7 100644
--- a/t/chainlint/chained-subshell.test
+++ b/t/chainlint/chained-subshell.test
@@ -8,7 +8,7 @@ mkdir sub && (
 
 # LINT: preceding command pipes to subshell on same line
 cut "-d " -f actual | (read s1 s2 s3 &&
-test -f $s1
+test_path_is_file $s1
 test $(cat $s2) = tree2path1 &&
 # LINT: closing subshell ")" correctly detected on same line as "$(...)"
 test $(cat $s3) = tree3path1)
diff --git a/t/chainlint/function.expect b/t/chainlint/function.expect
index 9e46a3554a1..2edbeb5e4e2 100644
--- a/t/chainlint/function.expect
+++ b/t/chainlint/function.expect
@@ -4,7 +4,7 @@
 5 
 6 remove_object() {
 7 	file=$(sha1_file "$*") &&
-8 	test -e "$file" ?!LINT: missing '&&'?!
+8 	test_path_exists "$file" ?!LINT: missing '&&'?!
 9 	rm -f "$file"
 10 } ?!LINT: missing '&&'?!
 11 
diff --git a/t/chainlint/function.test b/t/chainlint/function.test
index 763fcf3f878..2f2a5c251f4 100644
--- a/t/chainlint/function.test
+++ b/t/chainlint/function.test
@@ -7,7 +7,7 @@ sha1_file() {
 # LINT: broken &&-chain in function and after function
 remove_object() {
 	file=$(sha1_file "$*") &&
-	test -e "$file"
+	test_path_exists "$file"
 	rm -f "$file"
 }
 

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
-- 
gitgitgadget

Re: [PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)

[Eric Sunshine]

Thanks for submitting this GSoC microproject. See comments below...

On Tue, Mar 18, 2025 at 7:58 AM Sampriyo Guin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> From: rimo <sampriyoguin@gmail.com>

This name should match the Signed-off-by: name. Since the "From:"
header is generated from the author information in the commit, you
probably need to adjust your "user.name" configuration to fix this.

> test -e changed to test_path_exists
> test -f changed to test_path_is_file

People reading the patch would like to know why a change is being
made, so this is where you should explain the reason (for instance,
"the test_path_* functions provide better diagnostics upon failure" or
such). As Karthik mentioned[*], read the "Describe your changes well"
section in Documentation/SubmittingPatches to learn how to craft a
good commit message.

[*]: https://lore.kernel.org/git/CAOLa=ZSkMp+H9PZeBZXK47=fx1sH=S54AuPT=oUosm7F7V8MGg@mail.gmail.com/

> Signed-off-by: Sampriyo Guin <sampriyoguin@gmail.com>
> ---
>     , Jialuo She shejialuo@gmail.com , Christian Couder
>     christian.couder@gmail.com, Ghanshyam Thakkar shyamthakkar001@gmail.com

It appears that GitGitGadget didn't like how this list was formatted.
Instead, place each recipient on its own Cc: line.

>  t/chainlint/chained-subshell.expect | 2 +-
>  t/chainlint/chained-subshell.test   | 2 +-
>  t/chainlint/function.expect         | 2 +-
>  t/chainlint/function.test           | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)

Let's not touch any of the "chainlint" files; they are checking
validity of a completely separate tool ("chainlint"), and have nothing
to do with checking Git itself. Instead, pick one of the t/t???-*.sh
files.

Re: [PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Thanks for submitting this GSoC microproject. See comments below...
>
> On Tue, Mar 18, 2025 at 7:58 AM Sampriyo Guin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> From: rimo <sampriyoguin@gmail.com>
>
> This name should match the Signed-off-by: name. Since the "From:"
> header is generated from the author information in the commit, you
> probably need to adjust your "user.name" configuration to fix this.
>
>> test -e changed to test_path_exists
>> test -f changed to test_path_is_file
>
> People reading the patch would like to know why a change is being
> made, so this is where you should explain the reason (for instance,
> "the test_path_* functions provide better diagnostics upon failure" or
> such). As Karthik mentioned[*], read the "Describe your changes well"
> section in Documentation/SubmittingPatches to learn how to craft a
> good commit message.
>
> [*]: https://lore.kernel.org/git/CAOLa=ZSkMp+H9PZeBZXK47=fx1sH=S54AuPT=oUosm7F7V8MGg@mail.gmail.com/
>
>> Signed-off-by: Sampriyo Guin <sampriyoguin@gmail.com>
>> ---
>>     , Jialuo She shejialuo@gmail.com , Christian Couder
>>     christian.couder@gmail.com, Ghanshyam Thakkar shyamthakkar001@gmail.com
>
> It appears that GitGitGadget didn't like how this list was formatted.
> Instead, place each recipient on its own Cc: line.
>
>>  t/chainlint/chained-subshell.expect | 2 +-
>>  t/chainlint/chained-subshell.test   | 2 +-
>>  t/chainlint/function.expect         | 2 +-
>>  t/chainlint/function.test           | 2 +-
>>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> Let's not touch any of the "chainlint" files; they are checking
> validity of a completely separate tool ("chainlint"), and have nothing
> to do with checking Git itself. Instead, pick one of the t/t???-*.sh
> files.

Yeah, these changes to make them use test_path_* are not "fixes" but
something else.  The first step for a contributor is to understand
why "test_path_*" are preferred over "test -[def]" and in what
context, but touching these files shows that such understanding is
missing, unfortunately.

I find the "as specified in Git Microprojects" in the patch
description the most disturbing,

    A simple fix as specified in Git Microprojects.

as it may be an indication that some introductory write-up is
misleading potential students in a wrong direction.  Our mentors may
need a bit more handholding at this early stage of dipping your toes
in the water step, perhaps?  Or is it up to the aspiring students to
do their homework?

Re: [PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)

[Eric Sunshine]

On Tue, Mar 18, 2025 at 3:34 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Tue, Mar 18, 2025 at 7:58 AM Sampriyo Guin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>  t/chainlint/chained-subshell.expect | 2 +-
> >>  t/chainlint/chained-subshell.test   | 2 +-
> >
> > Let's not touch any of the "chainlint" files; they are checking
> > validity of a completely separate tool ("chainlint"), and have nothing
> > to do with checking Git itself. Instead, pick one of the t/t???-*.sh
> > files.
>
> Yeah, these changes to make them use test_path_* are not "fixes" but
> something else.  The first step for a contributor is to understand
> why "test_path_*" are preferred over "test -[def]" and in what
> context, but touching these files shows that such understanding is
> missing, unfortunately.
>
> I find the "as specified in Git Microprojects" in the patch
> description the most disturbing,
>
>     A simple fix as specified in Git Microprojects.
>
> as it may be an indication that some introductory write-up is
> misleading potential students in a wrong direction.  Our mentors may
> need a bit more handholding at this early stage of dipping your toes
> in the water step, perhaps?  Or is it up to the aspiring students to
> do their homework?

I'm not sure where the GSoC microproject ideas are maintained these
days, but it may indeed be the case that (at least this microproject)
could be spelled out in more detail to help lead newcomers in the
correct direction. If not already mentioned, at least these
clarifications probably ought to be made:

* only work on t/t????-*.sh scripts

* pick just one script (so as to avoid exhausting the pool for other candidates)

* only convert `test -[def]` instances which semantically are
assertions (i.e. used as part of a &&-chain)

Re: [PATCH] [GSoC Patch] Modernize Test Path Checking: test -(e|f|d)

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Mar 18, 2025 at 3:34 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> > On Tue, Mar 18, 2025 at 7:58 AM Sampriyo Guin via GitGitGadget
>> > <gitgitgadget@gmail.com> wrote:
>> >>  t/chainlint/chained-subshell.expect | 2 +-
>> >>  t/chainlint/chained-subshell.test   | 2 +-
>> >
>> > Let's not touch any of the "chainlint" files; they are checking
>> > validity of a completely separate tool ("chainlint"), and have nothing
>> > to do with checking Git itself. Instead, pick one of the t/t???-*.sh
>> > files.
>>
>> Yeah, these changes to make them use test_path_* are not "fixes" but
>> something else.  The first step for a contributor is to understand
>> why "test_path_*" are preferred over "test -[def]" and in what
>> context, but touching these files shows that such understanding is
>> missing, unfortunately.
>>
>> I find the "as specified in Git Microprojects" in the patch
>> description the most disturbing,
>>
>>     A simple fix as specified in Git Microprojects.
>>
>> as it may be an indication that some introductory write-up is
>> misleading potential students in a wrong direction.  Our mentors may
>> need a bit more handholding at this early stage of dipping your toes
>> in the water step, perhaps?  Or is it up to the aspiring students to
>> do their homework?
>
> I'm not sure where the GSoC microproject ideas are maintained these
> days, but it may indeed be the case that (at least this microproject)
> could be spelled out in more detail to help lead newcomers in the
> correct direction. If not already mentioned, at least these
> clarifications probably ought to be made:
>
> * only work on t/t????-*.sh scripts
>
> * pick just one script (so as to avoid exhausting the pool for other candidates)
>
> * only convert `test -[def]` instances which semantically are
> assertions (i.e. used as part of a &&-chain)

I think these are really good points, we ought to add to the
microproject page, I've raised a pull request on the repository to do so
[1].

[1]: https://github.com/git/git.github.io/pull/760

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]