| Message ID | xmqqef8a86sr.fsf_-_@gitster-ct.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Re* [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case | expand |
On February 14, 2019 15:16, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Mon, Feb 11, 2019 at 01:07:15PM -0800, Junio C Hamano wrote: > > > >> >> test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File > >> >> exists" err > >> > > >> > The message does not match, does it? Here we grep for "File exists" > >> > but the message you showed says "File already exists". > >> > >> Hmph, this is from strerror(), right? > >> > >> The question is if we should be using grep to match on strerror() > >> result in the C locale. > > > > Yeah, I agree that's questionable. And I'm mildly surprised it hasn't > > been a problem before now. > > > >> Do we really care that the reason of the failure is due to EEXIST for > >> this particular test? > > > > Hmm. We care to _some_ degree, since that's the condition we set up > > for making sure that update-ref cannot take the lock. But it would > > probably be fine to just confirm that we failed to take the lock. And > > there, checking for just "Unable to create $Q.*packed-refs.lock" would > > be sufficient. > > Yup. > > As this came from 6a2a7736 ("t1404: demonstrate two problems with > reference transactions", 2017-09-08), that is as old as Git 2.15, I'd throw it > into "not so urgent" pile. > > -- >8 -- > Subject: [PATCH] t1404: do not rely on the exact phrasing of strerror() > > Not even in C locale, it is wrong to expect that the exact phrasing "File > exists" is used to show EEXIST. > > Reported-by: Randall S. Becker <rsbecker@nexbridge.com> > Helped-by: Duy Nguyen <pclouds@gmail.com> > Helped-by: Jeff King <peff@peff.net> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > > I've grepped in t/ directory for the exact phrases of all errno on a > recent Debian box, and this was the only hit it found. There > are two other hits but both in the comments. > > t/t1404-update-ref-errors.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh index > 51a4f4c0ac..f95a64c911 100755 > --- a/t/t1404-update-ref-errors.sh > +++ b/t/t1404-update-ref-errors.sh > @@ -614,7 +614,7 @@ test_expect_success 'delete fails cleanly if packed- > refs file is locked' ' > test_when_finished "rm -f .git/packed-refs.lock" && > test_must_fail git update-ref -d $prefix/foo >out 2>err && > git for-each-ref $prefix >actual && > - test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File exists" > err && > + test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q:" err && > test_cmp unchanged actual > ' This passes on NonStop. Thanks. Randall
"Randall S. Becker" <rsbecker@nexbridge.com> writes: > On February 14, 2019 15:16, Junio C Hamano wrote: >> Jeff King <peff@peff.net> writes: >> >> > On Mon, Feb 11, 2019 at 01:07:15PM -0800, Junio C Hamano wrote: >> > >> >> >> test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File >> >> >> exists" err >> >> > >> >> > The message does not match, does it? Here we grep for "File exists" >> >> > but the message you showed says "File already exists". >> >> >> >> Hmph, this is from strerror(), right? >> >> >> >> The question is if we should be using grep to match on strerror() >> >> result in the C locale. >> > >> > Yeah, I agree that's questionable. And I'm mildly surprised it hasn't >> > been a problem before now. >> > >> >> Do we really care that the reason of the failure is due to EEXIST for >> >> this particular test? >> > >> > Hmm. We care to _some_ degree, since that's the condition we set up >> > for making sure that update-ref cannot take the lock. But it would >> > probably be fine to just confirm that we failed to take the lock. And >> > there, checking for just "Unable to create $Q.*packed-refs.lock" would >> > be sufficient. >> >> Yup. >> >> As this came from 6a2a7736 ("t1404: demonstrate two problems with >> reference transactions", 2017-09-08), that is as old as Git 2.15, I'd > throw it >> into "not so urgent" pile. >> >> -- >8 -- >> Subject: [PATCH] t1404: do not rely on the exact phrasing of strerror() >> >> Not even in C locale, it is wrong to expect that the exact phrasing "File >> exists" is used to show EEXIST. >> >> Reported-by: Randall S. Becker <rsbecker@nexbridge.com> >> Helped-by: Duy Nguyen <pclouds@gmail.com> >> Helped-by: Jeff King <peff@peff.net> >> Signed-off-by: Junio C Hamano <gitster@pobox.com> >> --- >> >> I've grepped in t/ directory for the exact phrases of all errno on a >> recent Debian box, and this was the only hit it found. There >> are two other hits but both in the comments. >> >> t/t1404-update-ref-errors.sh | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh > index >> 51a4f4c0ac..f95a64c911 100755 >> --- a/t/t1404-update-ref-errors.sh >> +++ b/t/t1404-update-ref-errors.sh >> @@ -614,7 +614,7 @@ test_expect_success 'delete fails cleanly if packed- >> refs file is locked' ' >> test_when_finished "rm -f .git/packed-refs.lock" && >> test_must_fail git update-ref -d $prefix/foo >out 2>err && >> git for-each-ref $prefix >actual && >> - test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File exists" >> err && >> + test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q:" err && >> test_cmp unchanged actual >> ' > > This passes on NonStop. Thanks. > > Randall Thanks.
On Fri, 15 Feb 2019 at 03:13, Junio C Hamano <gitster@pobox.com> wrote: > > Subject: [PATCH] t1404: do not rely on the exact phrasing of strerror() > > Not even in C locale, it is wrong to expect that the exact phrasing > "File exists" is used to show EEXIST. s/Not even/Even/? Or s/wrong to expect that/portable to rely on/, or something? Martin
Martin Ågren <martin.agren@gmail.com> writes: > On Fri, 15 Feb 2019 at 03:13, Junio C Hamano <gitster@pobox.com> wrote: >> >> Subject: [PATCH] t1404: do not rely on the exact phrasing of strerror() >> >> Not even in C locale, it is wrong to expect that the exact phrasing >> "File exists" is used to show EEXIST. > > s/Not even/Even/? Or s/wrong to expect that/portable to rely on/, or > something? Thanks for catching. Negation is not my forté.
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh index 51a4f4c0ac..f95a64c911 100755 --- a/t/t1404-update-ref-errors.sh +++ b/t/t1404-update-ref-errors.sh @@ -614,7 +614,7 @@ test_expect_success 'delete fails cleanly if packed-refs file is locked' ' test_when_finished "rm -f .git/packed-refs.lock" && test_must_fail git update-ref -d $prefix/foo >out 2>err && git for-each-ref $prefix >actual && - test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File exists" err && + test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q:" err && test_cmp unchanged actual '