patchmicroproject: Use test_path_is_* functions in test scripts

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

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

On Thu, Feb 29, 2024 at 10:05 AM shejialuo <shejialuo@gmail.com> wrote:

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.

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...

... 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 &&

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

On Mon, Mar 4, 2024 at 10:17 AM Patrick Steinhardt <ps@pks.im> wrote:

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.

Patrick Steinhardt <ps@pks.im> writes:

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.

On Thu, Feb 29, 2024 at 11:06:41AM -0800, Junio C Hamano wrote:

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

Patrick

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

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

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

On Thu, Feb 29, 2024 at 10:46 PM shejialuo <shejialuo@gmail.com> wrote:

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/

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`.

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

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

Thanks for your comment.

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:

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.

shejialuo <shejialuo@gmail.com> writes:

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:

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?

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.

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

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

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

On Fri, Mar 01, 2024 at 09:03:34PM +0800, shejialuo wrote:

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

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

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

On Mon, Mar 04, 2024 at 05:54:36PM +0800, shejialuo wrote:

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

Thanks for your advice. I will remember this suggestion.

Patrick Steinhardt <ps@pks.im> writes:

Thanks for a review.

shejialuo <shejialuo@gmail.com> writes:

OK.

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.

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

Thanks for your suggestions.

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

shejialuo <shejialuo@gmail.com> writes:

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.