[GSoC][PATCH 0/1] microproject: Use test_path_is_* functions in test scripts

[GSoC][PATCH 0/1] microproject: Use test_path_is_* functions in test scripts

[shejialuo]

Hello everyone,

My name is Jialuo She, mastering in the software engineering. This is my
last semester. And I will graduate this summer and works as a full time
employee. So I wanna make good use of my time by contributing to open
source software, and take this opportunity to continue contributing to
Git after I start working in the future.

My reason for choosing to participate in Git is actually quite simple.
It's because I once wrote a toy version of Git myself.Throught this
project, https://github.com/shejialuo/ugit-cpp. I came to understand
the magic of Git. I also want to do my part and contribute to something
meaningful.

For myself, the most attractive GSoC idea for me is "Implement
consistency checks for refs". I will dive into this idea soon.

At last, Wish everyone good health and happiness every day.

shejialuo (1):
  [GSoC][PATCH] t3070: refactor test -e command

 t/t3070-wildmatch.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
-- 
2.44.0

[PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command

[shejialuo]

The "test_path_exists" function was proposed at 7e9055b. It provides
parameter number check and more robust error messages.

This patch converts all "test -e" into "test_path_exists" to improve
test debug when failure.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/t3070-wildmatch.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index 4dd42df38c..d18ddc1a52 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -107,7 +107,7 @@ match_with_ls_files() {
 
 	if test "$match_expect" = 'E'
 	then
-		if test -e .git/created_test_file
+		if test_path_exists .git/created_test_file
 		then
 			test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): match dies on '$pattern' '$text'" "
 				printf '%s' '$text' >expect &&
@@ -118,7 +118,7 @@ match_with_ls_files() {
 		fi
 	elif test "$match_expect" = 1
 	then
-		if test -e .git/created_test_file
+		if test_path_exists .git/created_test_file
 		then
 			test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): match '$pattern' '$text'" "
 				printf '%s' '$text' >expect &&
@@ -130,7 +130,7 @@ match_with_ls_files() {
 		fi
 	elif test "$match_expect" = 0
 	then
-		if test -e .git/created_test_file
+		if test_path_exists .git/created_test_file
 		then
 			test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): no match '$pattern' '$text'" "
 				>expect &&
@@ -175,7 +175,7 @@ match() {
 	fi
 
 	test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '
-		if test -e .git/created_test_file
+		if test_path_exists .git/created_test_file
 		then
 			git reset &&
 			git clean -df
@@ -198,7 +198,7 @@ match() {
 			fi &&
 			git add -A &&
 			printf "%s" "$file" >.git/created_test_file
-		elif test -e .git/created_test_file
+		elif test_path_exists .git/created_test_file
 		then
 			rm .git/created_test_file
 		fi
-- 
2.44.0

Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command

[Eric Sunshine]

On Thu, Feb 29, 2024 at 10:05 AM shejialuo <shejialuo@gmail.com> wrote:
> t3070: refactor test -e command
>
> The "test_path_exists" function was proposed at 7e9055b. It provides
> parameter number check and more robust error messages.
>
> This patch converts all "test -e" into "test_path_exists" to improve
> test debug when failure.

Thanks for providing this GSoC submission. The aim of this patch makes
sense, but it turns out that t3070 is not a good choice for this
exercise. Before getting into that, though, a few minor comments about
the commit message.

This patch isn't actually refactoring the code, so using "refactor" in
the title is misleading.

Rather than mentioning only the object-ID, we normally reference other
commits like this (using `git log --pretty=reference -1 <object-id>`):

    7e9055bb00 (t7406: prefer test_* helper functions to test -[feds],
2018-08-08)

In this case, it's not clear why you chose to reference that
particular commit over any of the others which make similar changes.
It probably would be simpler to drop mention of that commit and just
copy its reasoning into your commit message.

Taking all the above into account, a possible rewrite of the commit
message might be:

    t3070: prefer test_path_exists helper function

    test -e does not provide a nice error message when we hit test
    failures, so use test_path_exists instead.

> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
> @@ -107,7 +107,7 @@ match_with_ls_files() {
>         if test "$match_expect" = 'E'
>         then
> -               if test -e .git/created_test_file
> +               if test_path_exists .git/created_test_file
>                 then
>                         test_expect_success EXPENSIVE_ON_WINDOWS "$match_function (via ls-files): match dies on '$pattern' '$text'" "

The point of functions such as test_path_exists() is to _assert_ that
some condition is true, thus allowing the test to succeed; if the
condition is not true, then the function prints an error message and
the test aborts and fails. Here is how test_path_exists() is defined:

    test_path_exists () {
        test "$#" -ne 1 && BUG "1 param"
        if ! test -e "$1"
        then
            echo "Path $1 doesn't exist"
            false
        fi
    }

It is meant to replace noisy code such as:

    if ! test -e bloop
    then
        echo >&2 "error message" &&
        exit 1
    fi &&
    other-code

with much simpler:

    test_path_exists bloop &&
    other-code

It is also meant to be used within `test_expect_success` (or
`test_expect_failure`) blocks. So, the changes made by this patch are
undesirable for a couple reasons...

First, this code is outside a `test_expect_success` (or
`test_expect_failure`) block.

Second, as noted above, test_path_exists() is an _assertion_ which
requires the file to exist, and aborts the test if the file does not
exist. But the `test -e` being changed here is part of the proper
control-flow of this logic; it is not asserting anything, but merely
branching to one or another part of the code depending upon the result
of the `test -e` test. Thus, replacing this control-flow check with
the assertion function test_path_exists() changes the logic in an
undesirable way.

The above comments are applicable to most of the changes made by this
patch. The only exceptions are the last two changes...

> @@ -175,7 +175,7 @@ match() {
>         test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '
> -               if test -e .git/created_test_file
> +               if test_path_exists .git/created_test_file
>                 then
>                         git reset &&

... which _do_ use test_path_exists() within a `test_expect_success`
block. However, the changes are still undesirable because, as above,
this `test -e` is merely part of the normal control-flow; it's not
acting as an assertion, thus test_path_exists() -- which is an
assertion -- is not correct.

Unfortunately, none of the uses of`test -e` in t3070 are being used as
assertions worthy of replacement with test_path_exists(), thus this
isn't a good script in which to make such changes. If you reroll, you
may be able to find a good candidate script by searching for code
which looks something like this:

    foo &&
    test -e path &&
    bar &&

and replacing it with:

    foo &&
    test_path_exists path &&
    bar &&

Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> @@ -175,7 +175,7 @@ match() {
>>         test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '
>> -               if test -e .git/created_test_file
>> +               if test_path_exists .git/created_test_file
>>                 then
>>                         git reset &&
>
> ... which _do_ use test_path_exists() within a `test_expect_success`
> block. However, the changes are still undesirable because, as above,
> this `test -e` is merely part of the normal control-flow; it's not
> acting as an assertion, thus test_path_exists() -- which is an
> assertion -- is not correct.
>
> Unfortunately, none of the uses of`test -e` in t3070 are being used as
> assertions worthy of replacement with test_path_exists(), thus this
> isn't a good script in which to make such changes.

It seems that there is a recurring confusion among mentorship
program applicants that use test_path_* helpers as their practice
material.  Perhaps the source of the information that suggests it as
a microproject is poorly phrased and needs to be rewritten to avoid
misleading them.

I found one at https://git.github.io/Outreachy-23-Microprojects/,
which can be one source of such confusion:

    Find one test script that verifies the presence/absence of
    files/directories with ‘test -(e|f|d|…)’ and replace them
    with the appropriate test_path_is_file, test_path_is_dir,
    etc. helper functions.

but there may be others.

This task specification does not differenciate "test -[efdx]" used
as a conditional of a control flow statement (which should never be
replaced by test_path_* helpers) and those used to directly fail the
&&-chain in test_expect_success with their exit status (which is the
target that test_path_* helpers are meant to improve).

[PATCH] SoC 2024: clarify `test_path_is_*` conversion microproject

[Patrick Steinhardt]

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

One of our proposed microprojects is to convert instances of `test -e`
and related functions to instead use `test_path_exists` or similar. This
conversion is only feasible when `test -e` is not used as part of a
control statement, as the replacement is used to _assert_ a condition
instead of merely testing for it.

Clarify the microproject's description accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 SoC-2024-Microprojects.md | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/SoC-2024-Microprojects.md b/SoC-2024-Microprojects.md
index 644c0a6..782441f 100644
--- a/SoC-2024-Microprojects.md
+++ b/SoC-2024-Microprojects.md
@@ -41,7 +41,10 @@ to search, so that we can remove this microproject idea.
 Find one test script that verifies the presence/absence of
 files/directories with 'test -(e|f|d|...)' and replace them with the
 appropriate `test_path_is_file`, `test_path_is_dir`, etc. helper
-functions.
+functions. Note that this conversion does not directly apply to control
+flow constructs like `if test -e ./path; then ...; fi` because the
+replacements are intended to assert the condition instead of merely
+testing for it.
 
 If you can't find one please tell us, along with the command you used
 to search, so that we can remove this microproject idea.
-- 
2.44.0


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

Re: [PATCH] SoC 2024: clarify `test_path_is_*` conversion microproject

[Christian Couder]

On Mon, Mar 4, 2024 at 10:17 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> One of our proposed microprojects is to convert instances of `test -e`
> and related functions to instead use `test_path_exists` or similar. This
> conversion is only feasible when `test -e` is not used as part of a
> control statement, as the replacement is used to _assert_ a condition
> instead of merely testing for it.
>
> Clarify the microproject's description accordingly.

Applied and pushed, thanks!

I wonder if it would be better to create a PR in
https://github.com/git/git.github.io/ and perhaps just send a link to
it, rather than sending patches to the mailing list, as patches on the
mailing list could be mistaken by tools and perhaps people as applying
to the Git code base.

Re: [PATCH] SoC 2024: clarify `test_path_is_*` conversion microproject

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> One of our proposed microprojects is to convert instances of `test -e`
> and related functions to instead use `test_path_exists` or similar. This
> conversion is only feasible when `test -e` is not used as part of a
> control statement, as the replacement is used to _assert_ a condition
> instead of merely testing for it.
>
> Clarify the microproject's description accordingly.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  SoC-2024-Microprojects.md | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/SoC-2024-Microprojects.md b/SoC-2024-Microprojects.md
> index 644c0a6..782441f 100644
> --- a/SoC-2024-Microprojects.md
> +++ b/SoC-2024-Microprojects.md
> @@ -41,7 +41,10 @@ to search, so that we can remove this microproject idea.
>  Find one test script that verifies the presence/absence of
>  files/directories with 'test -(e|f|d|...)' and replace them with the
>  appropriate `test_path_is_file`, `test_path_is_dir`, etc. helper
> -functions.
> +functions. Note that this conversion does not directly apply to control
> +flow constructs like `if test -e ./path; then ...; fi` because the
> +replacements are intended to assert the condition instead of merely
> +testing for it.

Thanks for picking it up.  Of course there is one case in which we
should use test_path_* helpers to replace such an if...then...fi
construct; e.g., c431a235 (t9146: replace test -d/-e/-f with
appropriate test_path_is_* function, 2024-02-14) did exactly that.

I am not sure how best to express that in the already crowded
description above, though.  Rewriting the existing test this way

	Find one test script that uses 'test [!] -(e|f|d|...)' to
	assert the presence/absense of files/directories to make the
	test fail directly with the exit status of such "test"
	commands, and replace them with the appropriate helper
	functions like `test_path_is_file`, that give more
	informative error messages when they fail.

would exclude use of "test -e" as a conditional in control statements,
so we could mention what c431a235 did as an exception to the rule,
perhaps like

	Note that the above excludes "test -f" and friends used as a
	condition in control statements such as "if test -e path
	...", but as an exception, if such a "if" statement just
	open-codes what these helpers do, replacing it is warranted.

But that does not read very well, even to myself.  Sigh....

Thanks.

Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command

[Patrick Steinhardt]

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

On Thu, Feb 29, 2024 at 11:06:41AM -0800, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >> @@ -175,7 +175,7 @@ match() {
> >>         test_expect_success EXPENSIVE_ON_WINDOWS 'cleanup after previous file test' '
> >> -               if test -e .git/created_test_file
> >> +               if test_path_exists .git/created_test_file
> >>                 then
> >>                         git reset &&
> >
> > ... which _do_ use test_path_exists() within a `test_expect_success`
> > block. However, the changes are still undesirable because, as above,
> > this `test -e` is merely part of the normal control-flow; it's not
> > acting as an assertion, thus test_path_exists() -- which is an
> > assertion -- is not correct.
> >
> > Unfortunately, none of the uses of`test -e` in t3070 are being used as
> > assertions worthy of replacement with test_path_exists(), thus this
> > isn't a good script in which to make such changes.
> 
> It seems that there is a recurring confusion among mentorship
> program applicants that use test_path_* helpers as their practice
> material.  Perhaps the source of the information that suggests it as
> a microproject is poorly phrased and needs to be rewritten to avoid
> misleading them.
> 
> I found one at https://git.github.io/Outreachy-23-Microprojects/,
> which can be one source of such confusion:
> 
>     Find one test script that verifies the presence/absence of
>     files/directories with ‘test -(e|f|d|…)’ and replace them
>     with the appropriate test_path_is_file, test_path_is_dir,
>     etc. helper functions.
> 
> but there may be others.
> 
> This task specification does not differenciate "test -[efdx]" used
> as a conditional of a control flow statement (which should never be
> replaced by test_path_* helpers) and those used to directly fail the
> &&-chain in test_expect_success with their exit status (which is the
> target that test_path_* helpers are meant to improve).

Good point. I've sent a patch in reply to your message that hopefully
clarifies this a bit. Thanks!

Patrick

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

Re: [PATCH 1/1] [GSoC][PATCH] t3070: refactor test -e command

[shejialuo]

Thanks for your comment, I will find a candidate script later and submit
a new version patch.

[PATCH V2 0/1] [GSoC][PATCH] t9117: prefer test_path_* helper functions

[shejialuo]

As discussed, the original patch is unsutiable, t9117 is a good
candidate script.

shejialuo (1):
  t9117: prefer test_path_* helper functions

 t/t9117-git-svn-init-clone.sh | 40 +++++++++++++++++------------------
 1 file changed, 20 insertions(+), 20 deletions(-)


base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
-- 
2.44.0

[PATCH 1/1] t9117: prefer test_path_* helper functions

[shejialuo]

test -(e|f|d) does not provide a nice error message when we hit test
failures, so use test_path_exists, test_path_is_dir and
test_path_is_file instead.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/t9117-git-svn-init-clone.sh | 40 +++++++++++++++++------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 62de819a44..2f964f66aa 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -15,39 +15,39 @@ test_expect_success 'setup svnrepo' '
 	'
 
 test_expect_success 'basic clone' '
-	test ! -d trunk &&
+	! test_path_is_dir trunk &&
 	git svn clone "$svnrepo"/project/trunk &&
-	test -d trunk/.git/svn &&
-	test -e trunk/foo &&
+	test_path_is_dir trunk/.git/svn &&
+	test_path_exists trunk/foo &&
 	rm -rf trunk
 	'
 
 test_expect_success 'clone to target directory' '
-	test ! -d target &&
+	! test_path_is_dir target &&
 	git svn clone "$svnrepo"/project/trunk target &&
-	test -d target/.git/svn &&
-	test -e target/foo &&
+	test_path_is_dir target/.git/svn &&
+	test_path_exists target/foo &&
 	rm -rf target
 	'
 
 test_expect_success 'clone with --stdlayout' '
-	test ! -d project &&
+	! test_path_is_dir project &&
 	git svn clone -s "$svnrepo"/project &&
-	test -d project/.git/svn &&
-	test -e project/foo &&
+	test_path_is_dir project/.git/svn &&
+	test_path_exists project/foo &&
 	rm -rf project
 	'
 
 test_expect_success 'clone to target directory with --stdlayout' '
-	test ! -d target &&
+	! test_path_is_dir target &&
 	git svn clone -s "$svnrepo"/project target &&
-	test -d target/.git/svn &&
-	test -e target/foo &&
+	test_path_is_dir target/.git/svn &&
+	test_path_exists target/foo &&
 	rm -rf target
 	'
 
 test_expect_success 'init without -s/-T/-b/-t does not warn' '
-	test ! -d trunk &&
+	! test_path_is_dir trunk &&
 	git svn init "$svnrepo"/project/trunk trunk 2>warning &&
 	! grep -q prefix warning &&
 	rm -rf trunk &&
@@ -55,7 +55,7 @@ test_expect_success 'init without -s/-T/-b/-t does not warn' '
 	'
 
 test_expect_success 'clone without -s/-T/-b/-t does not warn' '
-	test ! -d trunk &&
+	! test_path_is_dir trunk &&
 	git svn clone "$svnrepo"/project/trunk 2>warning &&
 	! grep -q prefix warning &&
 	rm -rf trunk &&
@@ -69,7 +69,7 @@ project/trunk:refs/remotes/${prefix}trunk
 project/branches/*:refs/remotes/${prefix}*
 project/tags/*:refs/remotes/${prefix}tags/*
 EOF
-	test ! -f actual &&
+	! test_path_is_file actual &&
 	git --git-dir=project/.git config svn-remote.svn.fetch >>actual &&
 	git --git-dir=project/.git config svn-remote.svn.branches >>actual &&
 	git --git-dir=project/.git config svn-remote.svn.tags >>actual &&
@@ -78,7 +78,7 @@ EOF
 }
 
 test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
-	test ! -d project &&
+	! test_path_is_dir project &&
 	git svn init -s "$svnrepo"/project project 2>warning &&
 	! grep -q prefix warning &&
 	test_svn_configured_prefix "origin/" &&
@@ -87,7 +87,7 @@ test_expect_success 'init with -s/-T/-b/-t assumes --prefix=origin/' '
 	'
 
 test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
-	test ! -d project &&
+	! test_path_is_dir project &&
 	git svn clone -s "$svnrepo"/project 2>warning &&
 	! grep -q prefix warning &&
 	test_svn_configured_prefix "origin/" &&
@@ -96,7 +96,7 @@ test_expect_success 'clone with -s/-T/-b/-t assumes --prefix=origin/' '
 	'
 
 test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
-	test ! -d project &&
+	! test_path_is_dir project &&
 	git svn init -s "$svnrepo"/project project --prefix "" 2>warning &&
 	! grep -q prefix warning &&
 	test_svn_configured_prefix "" &&
@@ -105,7 +105,7 @@ test_expect_success 'init with -s/-T/-b/-t and --prefix "" still works' '
 	'
 
 test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' '
-	test ! -d project &&
+	! test_path_is_dir project &&
 	git svn clone -s "$svnrepo"/project --prefix "" 2>warning &&
 	! grep -q prefix warning &&
 	test_svn_configured_prefix "" &&
@@ -114,7 +114,7 @@ test_expect_success 'clone with -s/-T/-b/-t and --prefix "" still works' '
 	'
 
 test_expect_success 'init with -T as a full url works' '
-	test ! -d project &&
+	! test_path_is_dir project &&
 	git svn init -T "$svnrepo"/project/trunk project &&
 	rm -rf project
 	'
-- 
2.44.0

Re: [PATCH 1/1] t9117: prefer test_path_* helper functions

[Eric Sunshine]

On Thu, Feb 29, 2024 at 10:46 PM shejialuo <shejialuo@gmail.com> wrote:
> test -(e|f|d) does not provide a nice error message when we hit test
> failures, so use test_path_exists, test_path_is_dir and
> test_path_is_file instead.

Thanks for rerolling. t9117 is indeed a better choice[1] than t3070
for the exercise of replacing `test -blah` with `test_path_foo`.

[1]: https://lore.kernel.org/git/CAPig+cR2-6qONkosu7=qEQSJa_fvYuVQ0to47D5qx904zW08Eg@mail.gmail.com/

> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
> diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
> @@ -15,39 +15,39 @@ test_expect_success 'setup svnrepo' '
>  test_expect_success 'basic clone' '
> -       test ! -d trunk &&
> +       ! test_path_is_dir trunk &&

Generally speaking, you don't want to use `!` to negate the result of
a `path_is_foo` assertion function. To understand why, take a look at
the definition of `test_path_is_dir`:

    test_path_is_dir () {
        if ! test -d "$1"
        then
            echo "Directory $1 doesn't exist"
            false
        fi
    }

The test in question (t9117: "basic clone") is using `test ! -d` to
assert that the directory `trunk` does not yet exist when the test
begins; indeed, under normal circumstances, this directory should not
yet be present. However, the call to test_path_is_dir() asserts that
the directory _does_ exist, which is the opposite of `test ! -d`, and
complains ("Directory trunk doesn't exist") when it doesn't exist. So,
in the normal and typical case for all the tests in this script,
`test_path_is_dir` is going to be complaining even though the
non-existence of that directory is an expected condition.

Although you make the test pass by using `!` to invert the result of
`test_path_is_dir`, the complaint will nevertheless get lodged, and
may very well be confusing for anyone scrutinizing the output of the
tests when running the script with `-v` or `-x`.

So, `test_path_is_dir` is not a good fit for this case which wants to
assert that the path `trunk` does not yet exist. A better choice for
this particular case would be `test_path_is_missing`.

>         git svn clone "$svnrepo"/project/trunk &&
> -       test -d trunk/.git/svn &&
> -       test -e trunk/foo &&
> +       test_path_is_dir trunk/.git/svn &&
> +       test_path_exists trunk/foo &&

These two changes make sense and the intent directly corresponds to
the original code.

>  test_expect_success 'clone to target directory' '
> -       test ! -d target &&
> +       ! test_path_is_dir target &&
>         git svn clone "$svnrepo"/project/trunk target &&
> -       test -d target/.git/svn &&
> -       test -e target/foo &&
> +       test_path_is_dir target/.git/svn &&
> +       test_path_exists target/foo &&
>         rm -rf target
>         '

What follows is probably beyond the scope of your GSoC microproject,
but there is a bit more of interest to note about these tests.

Rather than asserting some initial condition at the start of the test,
it is more common and more robust simply to _ensure_ that the desired
initial condition holds. So, for instance, instead of asserting `test
! -d target`, modern practice is to ensure that `target` doesn't
exist. Thus:

    test_expect_success 'clone to target directory' '
        rm -rf target &&
        git svn clone "$svnrepo"/project/trunk target &&
        ...

is a more robust implementation. This also addresses the problem that
the `rm -rf target` at the very end of each test won't be executed if
any command earlier in the test fails (due to the short-circuiting
behavior of the &&-operator).

As noted, this type of cleanup is probably overkill for your GSoC
microproject so you need not tackle it. I mention it only for
completeness. Also, if someone does tackle such a cleanup, it should
be done as multiple patches, each making one distinct change (i.e. one
patch dropping `test !-d` and moving `rm -rf` to the start of the
test, and one which employs `test_path_foo` for the remaining `test
-blah` invocations).

Re: [PATCH 1/1] t9117: prefer test_path_* helper functions

[shejialuo]

Thanks for your comment.

> Although you make the test pass by using `!` to invert the result of
> `test_path_is_dir`, the complaint will nevertheless get lodged, and
> may very well be confusing for anyone scrutinizing the output of the
> tests when running the script with `-v` or `-x`.

I have run the script with `-v`, I have got the following result:

  Directory trunk doesn't exist

I come to realisize the fault with your dedicated comments. An assertion
is an assertion.

And I am impressed by the following idea:

> Rather than asserting some initial condition at the start of the test,
> it is more common and more robust simply to _ensure_ that the desired
> initial condition holds. So, for instance, instead of asserting `test
> ! -d target`, modern practice is to ensure that `target` doesn't
> exist. Thus:
>
>    test_expect_success 'clone to target directory' '
>        rm -rf target &&
>        git svn clone "$svnrepo"/project/trunk target &&
>        ...
>
> is a more robust implementation. This also addresses the problem that
> the `rm -rf target` at the very end of each test won't be executed if
> any command earlier in the test fails (due to the short-circuiting
> behavior of the &&-operator).

The command `rm -rf target` ensures an exit status of 0 regardless of
whether the `target` exists. Thus the code will elegant make sure the
initial condition holds. I think I could add a patch to clean the code.

Re: [PATCH 1/1] t9117: prefer test_path_* helper functions

[Junio C Hamano]

shejialuo <shejialuo@gmail.com> writes:

>  test_expect_success 'basic clone' '
> -	test ! -d trunk &&
> +	! test_path_is_dir trunk &&

This is not quite right.  Step back and think why we are trying to
use the test_path_* helpers instead of "test [!] -d".  What are the
differences between them?

The answer is that, unlike "test [!] -d dir" that is silent whether
"dir" exists or missing, "test_path_is_dir dir" is *not* always
silent.  It gives useful messages as necessary.  When does it do so?

Here is the definition, from t/test-lib-functions.sh around line
930:

        test_path_is_dir () {
                test "$#" -ne 1 && BUG "1 param"
                if ! test -d "$1"
                then
                        echo "Directory $1 doesn't exist"
                        false
                fi
        }

It succeeds silently when "test -d dir" is true, but it complains
loudly when "test -d dir" does not hold.  You will be told that the
test is unhappy because "dir" does not exist.  That would be easier
to debug than one step among many in &&-chain silently fails.

Now, let's look at the original you rewrote again:

> -	test ! -d trunk &&

It says "it is a failure if 'trunk' exists as a directory".  If
'trunk' does not exist, it is a very happy state for us.  So instead
of silently failing when 'trunk' exists as a directory, you would
want to improve it so that you will get a complaint in such a case,
saying "trunk should *not* exist but it does".

Did you succeed to do so with this rewrite?

> +	! test_path_is_dir trunk &&

The helper "test_path_is_dir" is called with "trunk".  As we saw, we
will see complaint when "trunk" does *NOT* exist.  When "trunk" does
exist, it will be silent and "test_path_is_dir" will return a success,
which will be inverted with "!" to make it a failure, causing &&-chain
to fail.

So the exit status is not wrong, but it issues a complaint under the
wrong condition.  That is not an improvement.

Let's step back one more time.  Is the original test happy when
"trunk" existed as a regular file?  "test ! -d trunk" says so, but
should it really be?  Think.

I suspect that the test is not happy as long as 'trunk' exists,
whether it is a directory or a regular file or a symbolic link.
IOW, it says "I am unhappy if 'trunk' is a directory", but what it
really meant to say was "I am unhappy if there is anything at the
path 'trunk'".  IOW, "test ! -e trunk" would be what it really
meant, no?

So the correct rewrite for it would rather be something like

	test_path_is_missing trunk &&

instead.  This will fail if anything is at path 'trunk', with an
error message saying there shouldn't be anything but there is.

In a peculiar case, which I do not think this one is, a test may
legitimately accept "path" to either (1) exist as long as it is not
a directory, or (2) be missing, as success.  In such a case, the
original construct '! test -d path" (or "test ! -d path") would be
appropriate.

But I do not think we have a suitable wrapper to express such a
case, i.e. we do not have a helper like this.

	test_path_is_not_dir () {
		if test -d "$1"
		then
			echo "$1 is a directory but it should not be"
			false
		fi
	}

If such a use case were common, we might even do this:

	# "test_path_is_dir <dir>" expects <dir> to be a directory.
	# "test_path_is_dir ! <dir>"  expects <dir> not to be a
	# directory.
	# In either case, complain only when the expectation is not met.
	test_path_is_dir () {
		if test "$1" = "!"
		then
			shift
                        if test -d "$1"
			then
				echo "$1 is a directory but it should not be"
				return 1
			fi
		else
			if test ! -d "$1"
			then
				echo "$1 is not a directory"
				return 1
			fi
		fi
		true
	}

but "we are happy even if path exists as long as it is not a
directory" is a very uncommon thing we want to say in our tests, so
that is why we do not have such a helper function.

HTH.

Re: [PATCH 1/1] t9117: prefer test_path_* helper functions

[shejialuo]

Thanks for your wonderful comments. I have known that semantics is
important not only the functionality. I will send a new patch at now.

[PATCH v3 0/1] t9117: prefer test_path_* helper functions

[shejialuo]

As discussed in v2, it is improper to use ! test_path_is_dir to replace
the test ! -f. This patch reverts the code.

shejialuo (1):
  t9117: prefer test_path_* helper functions

 t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)


base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
-- 
2.44.0

[PATCH v3 1/1] [PATCH] t9117: prefer test_path_* helper functions

[shejialuo]

test -(e|f) does not provide a nice error message when we hit test
failures, so use test_path_exists, test_path_is_dir instead.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 62de819a44..3b038c338f 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '
 test_expect_success 'basic clone' '
 	test ! -d trunk &&
 	git svn clone "$svnrepo"/project/trunk &&
-	test -d trunk/.git/svn &&
-	test -e trunk/foo &&
+	test_path_is_dir trunk/.git/svn &&
+	test_path_exists trunk/foo &&
 	rm -rf trunk
 	'
 
 test_expect_success 'clone to target directory' '
 	test ! -d target &&
 	git svn clone "$svnrepo"/project/trunk target &&
-	test -d target/.git/svn &&
-	test -e target/foo &&
+	test_path_is_dir target/.git/svn &&
+	test_path_exists target/foo &&
 	rm -rf target
 	'
 
 test_expect_success 'clone with --stdlayout' '
 	test ! -d project &&
 	git svn clone -s "$svnrepo"/project &&
-	test -d project/.git/svn &&
-	test -e project/foo &&
+	test_path_is_dir project/.git/svn &&
+	test_path_exists project/foo &&
 	rm -rf project
 	'
 
 test_expect_success 'clone to target directory with --stdlayout' '
 	test ! -d target &&
 	git svn clone -s "$svnrepo"/project target &&
-	test -d target/.git/svn &&
-	test -e target/foo &&
+	test_path_is_dir target/.git/svn &&
+	test_path_exists target/foo &&
 	rm -rf target
 	'
 
-- 
2.44.0

Re: [PATCH v3 1/1] [PATCH] t9117: prefer test_path_* helper functions

[Patrick Steinhardt]

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

On Fri, Mar 01, 2024 at 09:03:34PM +0800, shejialuo wrote:
> test -(e|f) does not provide a nice error message when we hit test
> failures, so use test_path_exists, test_path_is_dir instead.

Nit: you mention `test -e` and `test -f`, but then talk about
`test_path_exists` (correct) and `test_path_is_dir` (wrong). You
probably meant to write `test -(e|d)`.

Other than that all the conversions look correct to me. Thanks!

Patrick

> 
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
> index 62de819a44..3b038c338f 100755
> --- a/t/t9117-git-svn-init-clone.sh
> +++ b/t/t9117-git-svn-init-clone.sh
> @@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '
>  test_expect_success 'basic clone' '
>  	test ! -d trunk &&
>  	git svn clone "$svnrepo"/project/trunk &&
> -	test -d trunk/.git/svn &&
> -	test -e trunk/foo &&
> +	test_path_is_dir trunk/.git/svn &&
> +	test_path_exists trunk/foo &&
>  	rm -rf trunk
>  	'
>  
>  test_expect_success 'clone to target directory' '
>  	test ! -d target &&
>  	git svn clone "$svnrepo"/project/trunk target &&
> -	test -d target/.git/svn &&
> -	test -e target/foo &&
> +	test_path_is_dir target/.git/svn &&
> +	test_path_exists target/foo &&
>  	rm -rf target
>  	'
>  
>  test_expect_success 'clone with --stdlayout' '
>  	test ! -d project &&
>  	git svn clone -s "$svnrepo"/project &&
> -	test -d project/.git/svn &&
> -	test -e project/foo &&
> +	test_path_is_dir project/.git/svn &&
> +	test_path_exists project/foo &&
>  	rm -rf project
>  	'
>  
>  test_expect_success 'clone to target directory with --stdlayout' '
>  	test ! -d target &&
>  	git svn clone -s "$svnrepo"/project target &&
> -	test -d target/.git/svn &&
> -	test -e target/foo &&
> +	test_path_is_dir target/.git/svn &&
> +	test_path_exists target/foo &&
>  	rm -rf target
>  	'
>  
> -- 
> 2.44.0
> 
> 

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

[PATCH v4 0/1] Change commit message

[shejialuo]

This version changes the last version's error message.

shejialuo (1):
  t9117: prefer test_path_* helper functions

 t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)


base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
-- 
2.44.0

[PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions

[shejialuo]

test -(e|d) does not provide a nice error message when we hit test
failures, so use test_path_exists, test_path_is_dir instead.

Signed-off-by: shejialuo <shejialuo@gmail.com>
---
 t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 62de819a44..3b038c338f 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '
 test_expect_success 'basic clone' '
 	test ! -d trunk &&
 	git svn clone "$svnrepo"/project/trunk &&
-	test -d trunk/.git/svn &&
-	test -e trunk/foo &&
+	test_path_is_dir trunk/.git/svn &&
+	test_path_exists trunk/foo &&
 	rm -rf trunk
 	'
 
 test_expect_success 'clone to target directory' '
 	test ! -d target &&
 	git svn clone "$svnrepo"/project/trunk target &&
-	test -d target/.git/svn &&
-	test -e target/foo &&
+	test_path_is_dir target/.git/svn &&
+	test_path_exists target/foo &&
 	rm -rf target
 	'
 
 test_expect_success 'clone with --stdlayout' '
 	test ! -d project &&
 	git svn clone -s "$svnrepo"/project &&
-	test -d project/.git/svn &&
-	test -e project/foo &&
+	test_path_is_dir project/.git/svn &&
+	test_path_exists project/foo &&
 	rm -rf project
 	'
 
 test_expect_success 'clone to target directory with --stdlayout' '
 	test ! -d target &&
 	git svn clone -s "$svnrepo"/project target &&
-	test -d target/.git/svn &&
-	test -e target/foo &&
+	test_path_is_dir target/.git/svn &&
+	test_path_exists target/foo &&
 	rm -rf target
 	'
 
-- 
2.44.0

Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions

[Patrick Steinhardt]

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

On Mon, Mar 04, 2024 at 05:54:36PM +0800, shejialuo wrote:
> test -(e|d) does not provide a nice error message when we hit test
> failures, so use test_path_exists, test_path_is_dir instead.
> 
> Signed-off-by: shejialuo <shejialuo@gmail.com>

This version looks good to me, thanks!

One suggestion for potential future contributions by you: it's always
helpful to create a "range-diff" of what has changed between the
previous version of your patch series and the next one. Like this,
reviewers can immediately see what the difference is between the two
versions, which helps them to get the review done faster.

Assuming you use git-format-patch(1) you can generate such a range diff
with the `--range-diff=` parameter.

Patrick

> ---
>  t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
> index 62de819a44..3b038c338f 100755
> --- a/t/t9117-git-svn-init-clone.sh
> +++ b/t/t9117-git-svn-init-clone.sh
> @@ -17,32 +17,32 @@ test_expect_success 'setup svnrepo' '
>  test_expect_success 'basic clone' '
>  	test ! -d trunk &&
>  	git svn clone "$svnrepo"/project/trunk &&
> -	test -d trunk/.git/svn &&
> -	test -e trunk/foo &&
> +	test_path_is_dir trunk/.git/svn &&
> +	test_path_exists trunk/foo &&
>  	rm -rf trunk
>  	'
>  
>  test_expect_success 'clone to target directory' '
>  	test ! -d target &&
>  	git svn clone "$svnrepo"/project/trunk target &&
> -	test -d target/.git/svn &&
> -	test -e target/foo &&
> +	test_path_is_dir target/.git/svn &&
> +	test_path_exists target/foo &&
>  	rm -rf target
>  	'
>  
>  test_expect_success 'clone with --stdlayout' '
>  	test ! -d project &&
>  	git svn clone -s "$svnrepo"/project &&
> -	test -d project/.git/svn &&
> -	test -e project/foo &&
> +	test_path_is_dir project/.git/svn &&
> +	test_path_exists project/foo &&
>  	rm -rf project
>  	'
>  
>  test_expect_success 'clone to target directory with --stdlayout' '
>  	test ! -d target &&
>  	git svn clone -s "$svnrepo"/project target &&
> -	test -d target/.git/svn &&
> -	test -e target/foo &&
> +	test_path_is_dir target/.git/svn &&
> +	test_path_exists target/foo &&
>  	rm -rf target
>  	'
>  
> -- 
> 2.44.0
> 

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

Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions

[shejialuo]

Thanks for your advice. I will remember this suggestion.

Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> This version looks good to me, thanks!
>
> One suggestion for potential future contributions by you: it's always
> helpful to create a "range-diff" of what has changed between the
> previous version of your patch series and the next one. Like this,
> reviewers can immediately see what the difference is between the two
> versions, which helps them to get the review done faster.
>
> Assuming you use git-format-patch(1) you can generate such a range diff
> with the `--range-diff=` parameter.

Thanks for a review.

Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions

[Junio C Hamano]

shejialuo <shejialuo@gmail.com> writes:

> test -(e|d) does not provide a nice error message when we hit test
> failures, so use test_path_exists, test_path_is_dir instead.

OK.

>
> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---

Just for the next single-patch topic you'd work on, here below the
three-dash line is where you may mention what's different between
the previous iteration and this one, if you wanted to, instead of
having a separate cover-letter message.

>  t/t9117-git-svn-init-clone.sh | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

The patch looks good to me.  Thanks (and thanks for all the
reviewers of the previous rounds).

Re: [PATCH v4 1/1] [PATCH] t9117: prefer test_path_* helper functions

[shejialuo]

> Just for the next single-patch topic you'd work on, here below the
> three-dash line is where you may mention what's different between
> the previous iteration and this one, if you wanted to, instead of
> having a separate cover-letter message.

Thanks for your suggestions.

At last, Thank every reviewer for your dedicated comments which make me
learn a lot.

Re: [PATCH v4 0/1] Change commit message

[Junio C Hamano]

shejialuo <shejialuo@gmail.com> writes:

> This version changes the last version's error message.

A cover letter is way overkill to tell the above to those who have
read the previous iteration (which is minority of the reviewer
population).  A comment after the three-dash line in the main patch
would be more appropriate.

If you need to have a cover letter, its title shouldn't be about the
differences between the previous round and this round.  It should be
about the topic of the "series".

Thanks.