Re: [GSoC PATCH v2] Use `test_path_*` helper functions instead of `test -[efd]`.

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 18, 2025, 22:06 UTC
Message-ID: <CAPig+cSnhjmni5FQhrWxfbtxkZq_sbHZp5CNAGHEQCaET5=iAg@mail.gmail.com>
In-Reply-To: <20250318131033.48691-1-contact@aynp.dev>

Thanks for your GSoC microproject submission. See comments below...

On Tue, Mar 18, 2025 at 9:11 AM Aryan Pathania <contact@aynp.dev> wrote:

> Use `test_path_*` helper functions instead of `test -[efd]`

According to Documentation/SubmittingPatches, you'd probably instead want to compose the commit message summary line something like this:

    t9400: use test_path_*` functions to improve diagnostic output

> Change testcase `gitcvs.enabled = false` to check for missing path
> instead of a missing file. The change is justified as new assertion is
> stronger.

The description seems somehow outdated since this particular change is being made to more than just that one test, isn't it?

Rather than describing the new assertion as "stronger", it might make more sense to state that it is more semantically in line with what is actually being tested (i.e. that the directory/path should not exist when, as expected, the command fails).

> All other testcases remain equivalent.

Okay, but...

Show 6 quoted lines

> @@ -296,8 +296,8 @@ test_expect_success 'gitcvs.ext.dbname' '
>         test_cmp cvswork cvswork2 &&
> -       test -f "$SERVERDIR/gitcvs1.ext.main.sqlite" &&
> -       test ! -f "$SERVERDIR/gitcvs2.ext.main.sqlite" &&
> +       test_path_is_file "$SERVERDIR/gitcvs1.ext.main.sqlite" &&
> +       test_path_is_missing "$SERVERDIR/gitcvs2.ext.main.sqlite" &&

... although `test_path_is_file` is a faithful replacement for `test -f`, it could be argued that `test_path_exists` would be a semantically better choice, especially given the use of `test_path_is_missing` for the sibling case.

The same comment applies to one or two other changes in this patch.