Re: [PATCH] t1091: don't grep for `strerror()` string

From: Derrick Stolee <stolee@gmail.com>
To: "Martin Ågren" <martin.agren@gmail.com>,
	"Randall S . Becker" <rsbecker@nexbridge.com>
Cc: Derrick Stolee <dstolee@microsoft.com>, git@vger.kernel.org
Subject: Re: [PATCH] t1091: don't grep for `strerror()` string
Date: Mon, 9 Mar 2020 09:07:07 -0400	[thread overview]
Message-ID: <30680fbc-203d-ef31-9863-794ff8f55382@gmail.com> (raw)
In-Reply-To: <20200308084627.26677-1-martin.agren@gmail.com>

On 3/8/2020 4:46 AM, Martin Ågren wrote:
> We grep for "File exists" in stderr of the failing `git sparse-checkout`
> to make sure that it failed for the right reason. We expect the string
> to show up there since we call `strerror(errno)` in
> `unable_to_lock_message()` in lockfile.c.
> 
> On the NonStop platform, this fails because the error string is "File
> already exists", which doesn't match our grepping.
> 
> See 9042140097 ("test-dir-iterator: do not assume errno values",
> 2019-07-30) for a somewhat similar fix. There, we patched a test helper,
> which meant we had access to `errno` and could investigate it better in
> the test helper instead of just outputting the numerical value and
> evaluating it in the test script. The current situation is different,
> since (short of modifying the lockfile machinery, e.g., to be more
> verbose) we don't have more than the output from `strerror()` available.

This is the better design, and I should have used it in the first
place. The test is really trying to reveal that we are using a .lock
file, and the new error message check makes this even more clear.

> Except we do: We prefix `strerror(errno)` with `_("Unable to create
> '%s.lock': ")`. Let's grep for that part instead. It verifies that we
> were indeed unable to create the lock file. (If that fails for some
> other reason than the file existing, we really really should expect
> other tests to fail as well.)
> 
> An alternative fix would be to loosen the expression a bit and grep for
> "File.* exists" instead. There would be no guarantee that some other
> implementation couldn't come up with another error string, That is, that
> could be the first move in an endless game of whack-a-mole. Of course,
> it could also take us from "99" to "100" percent of the platforms and
> we'd never have this problem again. But since we have another way of
> addressing this, let's not even try the "loosen it up a bit" strategy.

Your alternate substring is better than loosening the pattern in
several ways.

> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  Hi Randall,
> 
>  Thanks for the report.

I'll second the thanks on the report, and extra thanks to Martin for
the fix!

>  > test to fail. The error message generated is "File already exists" not "File
>  > exists" as is required in the test. We should not be testing for specific
>  > text content originating from strerror - I thought we had this decision in a
>  > different thread.
>  > https://public-inbox.org/git/xmqq36intlpj.fsf@gitster-ct.c.googlers.com/
> 
>  > error: 'grep File exists err' didn't find a match in:
>  > fatal: Unable to create '/home/ituglib/randall/git/t/trash
>  > directory.t1091-sparse-checkout-builtin/repo/.git/info/sparse-checkout.lock'
>  > : File already exists.   <----- this is the test issue
> 
>  Does this patch solve it?
> 
>  t/t1091-sparse-checkout-builtin.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index b4c9c32a03..44a91205d6 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -305,7 +305,7 @@ test_expect_success 'fail when lock is taken' '
>  	test_when_finished rm -rf repo/.git/info/sparse-checkout.lock &&
>  	touch repo/.git/info/sparse-checkout.lock &&
>  	test_must_fail git -C repo sparse-checkout set deep 2>err &&
> -	test_i18ngrep "File exists" err
> +	test_i18ngrep "Unable to create .*\.lock" err
>  '
>  
>  test_expect_success '.gitignore should not warn about cone mode' '

This change looks good to me. The commit message is superb.

Thanks,
-Stolee