On Thu, Feb 29, 2024 at 10:05 AM shejialuo <shejialuo@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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 &&
