threads / discuss / 52964

RE: [Test] t1901 - sparse checkout file when lock is taken fails (subtest 19)

Subject: RE: [Test] t1901 - sparse checkout file when lock is taken fails (subtest 19)

## tl;dr

3 messages between Mar 10, 2020 and Mar 12, 2020.

replies: 2people: 2as markdown or json

Randall S. Becker· Mar 10, 2020, 14:40 UTC · lore
> From: Randall S. Becker <rsbecker@nexbridge.com>
On March 5, 2020 10:45 AM, I wrote:
> This one has me confused. It fails 100% of the time on NonStop. The test
> looks reasonable, as do the messages. I am not certain that test_i18ngrep
> works properly - it falls down to the return 1 statement which causes the
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
Show 14 quoted lines
> 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/
> 
> Thoughts?
> 
> expecting success of 1091.19 '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
> 
> error: 'grep File exists err' didn't find a match in:
> fatal: Unable to create '/home/ituglib/randall/git/t/trash
directory.t1091-
Show 6 quoted lines
> sparse-checkout-builtin/repo/.git/info/sparse-checkout.lock': File already
> exists.   <----- this is the test issue
> 
> Another git process seems to be running in this repository, e.g.
> an editor opened by 'git commit'. Please make sure all processes are
> terminated then try again. If it still fails, a git process may have
crashed in
Show 5 quoted lines
> this repository earlier:
> remove the file manually to continue.
> not ok 19 - 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
> #

This situation still occurs at 2.26.0-rc0. As above, this results from a text compare to a platform-specific message that should not be used. To hack around it, a possible fix (which I don't like) could be as follows:

index b4c9c32a03..d1fd225dad 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 "File \(already \)*exists" err
 '

This does not remove the problem of platform error compares, but does allow
the test to temporarily pass.

Randall
Junio C Hamano· Mar 11, 2020, 22:06 UTC · re: Randall S. Becker · lore

Re: [Test] t1901 - sparse checkout file when lock is taken fails (subtest 19)

"Randall S. Becker" <rsbecker@nexbridge.com> writes:
> This situation still occurs at 2.26.0-rc0. As above, this results from a
> text compare to a platform-specific message that should not be used. To hack
> around it, a possible fix (which I don't like) could be as follows:

Martin's fix 4605a730 (t1091: don't grep for `strerror()` string, 2020-03-08) was merged to 'next' on the 9th and then down to 'master' today, and will be in the final (unless there is some issues with it, which I do not think will be the case).

Thanks.
-- >8 --
From: Martin Ågren <martin.agren@gmail.com>
Subject: [PATCH] t1091: don't grep for `strerror()` string

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.

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.

Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Acked-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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' '

-- 
2.26.0-rc1-6-ga56d361f66
Randall S. Becker· Mar 12, 2020, 15:08 UTC · re: Junio C Hamano · lore
On March 11, 2020 6:06 PM, Junio C Hamano wrote:
Show 36 quoted lines
> "Randall S. Becker" <rsbecker@nexbridge.com> writes:
> 
> > This situation still occurs at 2.26.0-rc0. As above, this results from
> > a text compare to a platform-specific message that should not be used.
> > To hack around it, a possible fix (which I don't like) could be as follows:
> 
> Martin's fix 4605a730 (t1091: don't grep for `strerror()` string,
> 2020-03-08) was merged to 'next' on the 9th and then down to 'master'
> today, and will be in the final (unless there is some issues with it, which I do
> not think will be the case).
> 
> Thanks.
> 
> -- >8 --
> From: Martin Ågren <martin.agren@gmail.com>
> Subject: [PATCH] t1091: don't grep for `strerror()` string
> 
> 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.
> 
> 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.)
I'm hoping that that error message matches. Will test it as soon as it is final is out there.
Show 32 quoted lines
> 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.
> 
> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> Acked-by: Derrick Stolee <dstolee@microsoft.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  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' '
> --
> 2.26.0-rc1-6-ga56d361f66
This still really helps stabilize things for the NonStop platform.

Thanks! Randall

← back to recent threads