Modernize Test Path Checking: test -(e|f|d)

From: rimo <[email protected]>

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

Signed-off-by Sampriyo Guin <[email protected]>

---
, Jialuo She [email protected] , Christian Couder [email protected], Ghanshyam Thakkar [email protected]

 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(-)

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

Quoted 1 line · Sampriyo Guin via GitGitGadget

From: rimo <[email protected]>

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.

Quoted 2 lines

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/

Quoted 4 lines

Signed-off-by Sampriyo Guin <[email protected]>

---
, Jialuo She [email protected] , Christian Couder [email protected], Ghanshyam Thakkar [email protected]

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

Quoted 5 lines

 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.

Quoted 9 lines · Eric Sunshine

Quoted 2 lines · Sampriyo Guin via GitGitGadget

 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?

Quoted 20 lines · Junio C Hamano

Quoted 9 lines · Eric Sunshine

Quoted 2 lines · Sampriyo Guin via GitGitGadget

 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,

Quoted 7 lines

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)