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@xxxxxxxxxxxxx>
> Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx/
>
> > 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
