{"thread":{"id":"52940","subject":"[Test] t1901 - sparse checkout file when lock is taken fails (subtest 19)","startedAt":"2020-03-05T15:45:14Z","lastAt":"2020-03-09T15:04:29Z","messageCount":4,"participants":["Randall S. Becker","Martin Ågren","Derrick Stolee","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"392882","messageId":"018001d5f305$0c709af0$2551d0d0$@nexbridge.com","threadId":"52940","inReplyTo":null,"subject":"[Test] t1901 - sparse checkout file when lock is taken fails (subtest 19)","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2020-03-05T15:45:03Z","receivedAt":"2020-03-05T15:45:14Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"Hi All,\n\nThis one has me confused. It fails 100% of the time on NonStop. The test\nlooks reasonable, as do the messages. I am not certain that test_i18ngrep\nworks properly - it falls down to the return 1 statement which causes the\ntest to fail. The error message generated is \"File already exists\" not \"File\nexists\" as is required in the test. We should not be testing for specific\ntext content originating from strerror - I thought we had this decision in a\ndifferent thread.\nhttps://public-inbox.org/git/xmqq36intlpj.fsf@gitster-ct.c.googlers.com/\n\nThoughts?\n\nexpecting success of 1091.19 'fail when lock is taken':\n        test_when_finished rm -rf repo/.git/info/sparse-checkout.lock &&\n        touch repo/.git/info/sparse-checkout.lock &&\n        test_must_fail git -C repo sparse-checkout set deep 2>err &&\n        test_i18ngrep \"File exists\" err\n\nerror: 'grep File exists err' didn't find a match in:\nfatal: Unable to create '/home/ituglib/randall/git/t/trash\ndirectory.t1091-sparse-checkout-builtin/repo/.git/info/sparse-checkout.lock'\n: File already exists.   <----- this is the test issue\n\nAnother git process seems to be running in this repository, e.g.\nan editor opened by 'git commit'. Please make sure all processes\nare terminated then try again. If it still fails, a git process\nmay have crashed in this repository earlier:\nremove the file manually to continue.\nnot ok 19 - fail when lock is taken\n#\n#               test_when_finished rm -rf\nrepo/.git/info/sparse-checkout.lock &&\n#               touch repo/.git/info/sparse-checkout.lock &&\n#               test_must_fail git -C repo sparse-checkout set deep 2>err &&\n#               test_i18ngrep \"File exists\" err\n#\n\nRegards,\nRandall\n\n-- Brief whoami:\n NonStop developer since approximately 211288444200000000\n UNIX developer since approximately 421664400\n-- In my real life, I talk too much.\n\n\n"},{"id":"392942","messageId":"20200308084627.26677-1-martin.agren@gmail.com","threadId":"52940","inReplyTo":"018001d5f305$0c709af0$2551d0d0$@nexbridge.com","subject":"[PATCH] t1091: don't grep for `strerror()` string","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-03-08T08:46:27Z","receivedAt":"2020-03-08T08:46:40Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We grep for \"File exists\" in stderr of the failing `git sparse-checkout`\nto make sure that it failed for the right reason. We expect the string\nto show up there since we call `strerror(errno)` in\n`unable_to_lock_message()` in lockfile.c.\n\nOn the NonStop platform, this fails because the error string is \"File\nalready exists\", which doesn't match our grepping.\n\nSee 9042140097 (\"test-dir-iterator: do not assume errno values\",\n2019-07-30) for a somewhat similar fix. There, we patched a test helper,\nwhich meant we had access to `errno` and could investigate it better in\nthe test helper instead of just outputting the numerical value and\nevaluating it in the test script. The current situation is different,\nsince (short of modifying the lockfile machinery, e.g., to be more\nverbose) we don't have more than the output from `strerror()` available.\n\nExcept we do: We prefix `strerror(errno)` with `_(\"Unable to create\n'%s.lock': \")`. Let's grep for that part instead. It verifies that we\nwere indeed unable to create the lock file. (If that fails for some\nother reason than the file existing, we really really should expect\nother tests to fail as well.)\n\nAn alternative fix would be to loosen the expression a bit and grep for\n\"File.* exists\" instead. There would be no guarantee that some other\nimplementation couldn't come up with another error string, That is, that\ncould be the first move in an endless game of whack-a-mole. Of course,\nit could also take us from \"99\" to \"100\" percent of the platforms and\nwe'd never have this problem again. But since we have another way of\naddressing this, let's not even try the \"loosen it up a bit\" strategy.\n\nReported-by: Randall S. Becker <rsbecker@nexbridge.com>\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Hi Randall,\n\n Thanks for the report.\n\n > test to fail. The error message generated is \"File already exists\" not \"File\n > exists\" as is required in the test. We should not be testing for specific\n > text content originating from strerror - I thought we had this decision in a\n > different thread.\n > https://public-inbox.org/git/xmqq36intlpj.fsf@gitster-ct.c.googlers.com/\n\n > error: 'grep File exists err' didn't find a match in:\n > fatal: Unable to create '/home/ituglib/randall/git/t/trash\n > directory.t1091-sparse-checkout-builtin/repo/.git/info/sparse-checkout.lock'\n > : File already exists.   <----- this is the test issue\n\n Does this patch solve it?\n\n t/t1091-sparse-checkout-builtin.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\nindex b4c9c32a03..44a91205d6 100755\n--- a/t/t1091-sparse-checkout-builtin.sh\n+++ b/t/t1091-sparse-checkout-builtin.sh\n@@ -305,7 +305,7 @@ test_expect_success 'fail when lock is taken' '\n \ttest_when_finished rm -rf repo/.git/info/sparse-checkout.lock &&\n \ttouch repo/.git/info/sparse-checkout.lock &&\n \ttest_must_fail git -C repo sparse-checkout set deep 2>err &&\n-\ttest_i18ngrep \"File exists\" err\n+\ttest_i18ngrep \"Unable to create .*\\.lock\" err\n '\n \n test_expect_success '.gitignore should not warn about cone mode' '\n-- \n2.25.1\n\n"},{"id":"392961","messageId":"30680fbc-203d-ef31-9863-794ff8f55382@gmail.com","threadId":"52940","inReplyTo":"20200308084627.26677-1-martin.agren@gmail.com","subject":"Re: [PATCH] t1091: don't grep for `strerror()` string","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-09T13:07:07Z","receivedAt":"2020-03-09T13:07:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/8/2020 4:46 AM, Martin Ågren wrote:\n> We grep for \"File exists\" in stderr of the failing `git sparse-checkout`\n> to make sure that it failed for the right reason. We expect the string\n> to show up there since we call `strerror(errno)` in\n> `unable_to_lock_message()` in lockfile.c.\n> \n> On the NonStop platform, this fails because the error string is \"File\n> already exists\", which doesn't match our grepping.\n> \n> See 9042140097 (\"test-dir-iterator: do not assume errno values\",\n> 2019-07-30) for a somewhat similar fix. There, we patched a test helper,\n> which meant we had access to `errno` and could investigate it better in\n> the test helper instead of just outputting the numerical value and\n> evaluating it in the test script. The current situation is different,\n> since (short of modifying the lockfile machinery, e.g., to be more\n> verbose) we don't have more than the output from `strerror()` available.\n\nThis is the better design, and I should have used it in the first\nplace. The test is really trying to reveal that we are using a .lock\nfile, and the new error message check makes this even more clear.\n\n> Except we do: We prefix `strerror(errno)` with `_(\"Unable to create\n> '%s.lock': \")`. Let's grep for that part instead. It verifies that we\n> were indeed unable to create the lock file. (If that fails for some\n> other reason than the file existing, we really really should expect\n> other tests to fail as well.)\n> \n> An alternative fix would be to loosen the expression a bit and grep for\n> \"File.* exists\" instead. There would be no guarantee that some other\n> implementation couldn't come up with another error string, That is, that\n> could be the first move in an endless game of whack-a-mole. Of course,\n> it could also take us from \"99\" to \"100\" percent of the platforms and\n> we'd never have this problem again. But since we have another way of\n> addressing this, let's not even try the \"loosen it up a bit\" strategy.\n\nYour alternate substring is better than loosening the pattern in\nseveral ways.\n\n> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  Hi Randall,\n> \n>  Thanks for the report.\n\nI'll second the thanks on the report, and extra thanks to Martin for\nthe fix!\n\n>  > test to fail. The error message generated is \"File already exists\" not \"File\n>  > exists\" as is required in the test. We should not be testing for specific\n>  > text content originating from strerror - I thought we had this decision in a\n>  > different thread.\n>  > https://public-inbox.org/git/xmqq36intlpj.fsf@gitster-ct.c.googlers.com/\n> \n>  > error: 'grep File exists err' didn't find a match in:\n>  > fatal: Unable to create '/home/ituglib/randall/git/t/trash\n>  > directory.t1091-sparse-checkout-builtin/repo/.git/info/sparse-checkout.lock'\n>  > : File already exists.   <----- this is the test issue\n> \n>  Does this patch solve it?\n> \n>  t/t1091-sparse-checkout-builtin.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh\n> index b4c9c32a03..44a91205d6 100755\n> --- a/t/t1091-sparse-checkout-builtin.sh\n> +++ b/t/t1091-sparse-checkout-builtin.sh\n> @@ -305,7 +305,7 @@ test_expect_success 'fail when lock is taken' '\n>  \ttest_when_finished rm -rf repo/.git/info/sparse-checkout.lock &&\n>  \ttouch repo/.git/info/sparse-checkout.lock &&\n>  \ttest_must_fail git -C repo sparse-checkout set deep 2>err &&\n> -\ttest_i18ngrep \"File exists\" err\n> +\ttest_i18ngrep \"Unable to create .*\\.lock\" err\n>  '\n>  \n>  test_expect_success '.gitignore should not warn about cone mode' '\n\nThis change looks good to me. The commit message is superb.\n\nThanks,\n-Stolee\n\n\n"},{"id":"392964","messageId":"xmqqmu8p1seh.fsf@gitster-ct.c.googlers.com","threadId":"52940","inReplyTo":"20200308084627.26677-1-martin.agren@gmail.com","subject":"Re: [PATCH] t1091: don't grep for `strerror()` string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-09T15:04:22Z","receivedAt":"2020-03-09T15:04:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> We grep for \"File exists\" in stderr of the failing `git sparse-checkout`\n> to make sure that it failed for the right reason. We expect the string\n> to show up there since we call `strerror(errno)` in\n> `unable_to_lock_message()` in lockfile.c.\n>\n> On the NonStop platform, this fails because the error string is \"File\n> already exists\", which doesn't match our grepping.\n\nThanks for a quick fix.  I agree that looking for our own string\nmakes a lot more sense here.\n"}]}