git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Derrick Stolee <stolee@gmail.com>
Date
Mar 9, 2020, 13:07 UTC
Message-ID
<30680fbc-203d-ef31-9863-794ff8f55382@gmail.com>
In-Reply-To
<20200308084627.26677-1-martin.agren@gmail.com>
On 3/8/2020 4:46 AM, Martin Ågren wrote:
Show 15 quoted lines
> 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.

Show 13 quoted lines
> 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.

Show 6 quoted lines
> 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!

Show 29 quoted lines
>  > 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

Previous: Martin ÅgrenNext: Junio C Hamano
Message 3 of 4 in “[Test] t1901 - sparse checkout file when lock is taken fails (subtest 19)”
  1. Randall S. BeckerMar 5, 2020
  2. t1091: don't grep for `strerror()` stringMartin Ågren, Mar 8, 2020
  3. Derrick StoleeMar 9, 2020
  4. Junio C HamanoMar 9, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.