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
next prev parent reply other threads:[~2020-03-09 13:07 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-05 15:45 [Test] t1901 - sparse checkout file when lock is taken fails (subtest 19) Randall S. Becker
2020-03-08 8:46 ` [PATCH] t1091: don't grep for `strerror()` string Martin Ågren
2020-03-09 13:07 ` Derrick Stolee [this message]
2020-03-09 15:04 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=30680fbc-203d-ef31-9863-794ff8f55382@gmail.com \
--to=stolee@gmail.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=martin.agren@gmail.com \
--cc=rsbecker@nexbridge.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).