# [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case 13 messages from 2019-02-10 to 2019-02-15. Participants: Randall S. Becker, Duy Nguyen, Junio C Hamano, Jeff King, Martin Ågren. Thread: https://gitlist.dev/t/50456 ## Randall S. Becker, 2019-02-10 19:08 Subject: [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: <000801d4c174$05b76860$11263920$@nexbridge.com> URL: https://gitlist.dev/e/000801d4c174%2405b76860%2411263920%24%40nexbridge.com ``` Hi All, I tracked down a breakage in t1404 subtest 52. The line test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File exists" err is correctly working, but is reporting a completion 1. The verbose output, with diagnostics, is: error: 'grep Unable to create '.*packed-refs.lock': File exists err' didn't find a match in: error: Unable to create '/home/git/git/t/trash directory.t1404-update-ref-errors/.git/packed-refs.lock': File already exists. 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 this repository earlier: remove the file manually to continue. Reported 1 from test_i18ngrep err contains (without the double quotes): "error: Unable to create '/home/git/git/t/trash directory.t1404-update-ref-errors/.git/packed-refs.lock': File already exists. 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 this repository earlier: remove the file manually to continue." Which means that ! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@" is completing with a 1. Is that the intent? Any clues? Thanks, Randall ``` ## Duy Nguyen, 2019-02-11 09:56 Subject: Re: [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: URL: https://gitlist.dev/e/CACsJy8Bn%2B2zY6y_QqCjbB3qWM-F%3D3d0H5vgWj4az%3Dmd2FZ8RhA%40mail.gmail.com In-Reply-To: <000801d4c174$05b76860$11263920$@nexbridge.com> ``` On Mon, Feb 11, 2019 at 2:09 AM Randall S. Becker wrote: > > Hi All, > > I tracked down a breakage in t1404 subtest 52. The line > > 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". > is correctly working, but is reporting a completion 1. > > The verbose output, with diagnostics, is: > > error: 'grep Unable to create '.*packed-refs.lock': File exists err' didn't > find a match in: > error: Unable to create '/home/git/git/t/trash > directory.t1404-update-ref-errors/.git/packed-refs.lock': File already > exists. > > 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 this repository earlier: > remove the file manually to continue. > > Reported 1 from test_i18ngrep > > err contains (without the double quotes): > "error: Unable to create '/home/git/git/t/trash > directory.t1404-update-ref-errors/.git/packed-refs.lock': File already > exists. > > 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 this repository earlier: > remove the file manually to continue." > > Which means that > > ! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@" > > is completing with a 1. Is that the intent? Any clues? > > Thanks, > Randall > -- Duy ``` ## Randall S. Becker, 2019-02-11 14:09 Subject: RE: [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: <000601d4c213$6c7ee650$457cb2f0$@nexbridge.com> URL: https://gitlist.dev/e/000601d4c213%246c7ee650%24457cb2f0%24%40nexbridge.com In-Reply-To: ``` On February 11, 2019 4:57, Duy Nguyen wrote: > On Mon, Feb 11, 2019 at 2:09 AM Randall S. Becker > wrote: > > > > Hi All, > > > > I tracked down a breakage in t1404 subtest 52. The line > > > > 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" So if I understand this correctly, it means that NonStop is reporting a different textual error that other platforms, but is still sane. Would a fix as follows be appropriate? @@ -614,7 +614,12 @@ 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 && + # Handle a difference in error reporting text on NonStop + if [ `uname` != "NONSTOP_KERNEL" ]; then \ + test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File exists" err ; \ + else \ + test_i18ngrep "Unable to create $Q.*packed-refs.lock$Q: File already exists" err ; \ + fi && test_cmp unchanged actual I'm not at all confident that the committers will like a hack like this but it does work. Regards, Randall ``` ## Junio C Hamano, 2019-02-11 21:07 Subject: Re: [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: URL: https://gitlist.dev/e/xmqqftsughks.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Duy Nguyen writes: > On Mon, Feb 11, 2019 at 2:09 AM Randall S. Becker > wrote: >> >> Hi All, >> >> I tracked down a breakage in t1404 subtest 52. The line >> >> 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. Do we really care that the reason of the failure is due to EEXIST for this particular test? >> The verbose output, with diagnostics, is: >> >> error: 'grep Unable to create '.*packed-refs.lock': File exists err' didn't >> find a match in: >> error: Unable to create '/home/git/git/t/trash >> directory.t1404-update-ref-errors/.git/packed-refs.lock': File already >> exists. Otherwise, perhaps we should loosen the grep pattern, not as a part of "NonStop fix" topic, but as "tests should not depend on having a canonical spelling of strerror() result even in C locale" topic. ``` ## Randall S. Becker, 2019-02-11 21:48 Subject: RE: [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: <003601d4c253$8208dcc0$861a9640$@nexbridge.com> URL: https://gitlist.dev/e/003601d4c253%248208dcc0%24861a9640%24%40nexbridge.com In-Reply-To: ``` On February 11, 2019 16:07, Junio C Hamano wrote: > Duy Nguyen writes: > > > On Mon, Feb 11, 2019 at 2:09 AM Randall S. Becker > > wrote: > >> > >> Hi All, > >> > >> I tracked down a breakage in t1404 subtest 52. The line > >> > >> 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? You can reasonably expect that NonStop error messages deviate occasionally. Scaping from strerror() is not a good plan. A worse plan is to use errno values, which I can guarantee do not match, but that's just an FYI. > The question is if we should be using grep to match on strerror() result in the > C locale. Do we really care that the reason of the failure is due to EEXIST for > this particular test? > > >> The verbose output, with diagnostics, is: > >> > >> error: 'grep Unable to create '.*packed-refs.lock': File exists err' > >> didn't find a match in: > >> error: Unable to create '/home/git/git/t/trash > >> directory.t1404-update-ref-errors/.git/packed-refs.lock': File > >> already exists. > > Otherwise, perhaps we should loosen the grep pattern, not as a part of > "NonStop fix" topic, but as "tests should not depend on having a canonical > spelling of strerror() result even in C locale" topic. I'm happy not to have the fix I supplied used if there's a better way. ``` ## Jeff King, 2019-02-12 00:27 Subject: Re: [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: <20190212002705.GD13301@sigill.intra.peff.net> URL: https://gitlist.dev/e/20190212002705.GD13301%40sigill.intra.peff.net In-Reply-To: ``` 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. -Peff ``` ## Junio C Hamano, 2019-02-12 00:32 Subject: Re: [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: URL: https://gitlist.dev/e/xmqqpnrxg823.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190212002705.GD13301@sigill.intra.peff.net> ``` Jeff King writes: > 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. Yeah, that was what I was getting at. On top, we could also see "test -e" after noticing the failure but that probably does not help us prove anything. ``` ## Jeff King, 2019-02-12 00:35 Subject: Re: [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: <20190212003540.GG13301@sigill.intra.peff.net> URL: https://gitlist.dev/e/20190212003540.GG13301%40sigill.intra.peff.net In-Reply-To: ``` On Mon, Feb 11, 2019 at 04:32:52PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > 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. > > Yeah, that was what I was getting at. On top, we could also see > "test -e" after noticing the failure but that probably does not help > us prove anything. Yeah, I don't think there is any point in "test -e". We were the ones who created the file in the first place, so it would be very surprising if it weren't there. ;) -Peff ``` ## Junio C Hamano, 2019-02-14 20:16 Subject: Re* [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: URL: https://gitlist.dev/e/xmqqef8a86sr.fsf_-_%40gitster-ct.c.googlers.com In-Reply-To: <20190212002705.GD13301@sigill.intra.peff.net> ``` Jeff King 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 Helped-by: Duy Nguyen Helped-by: Jeff King Signed-off-by: Junio C Hamano --- 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 ' ``` ## Randall S. Becker, 2019-02-14 20:32 Subject: RE: Re* [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: <005001d4c4a4$6af097a0$40d1c6e0$@nexbridge.com> URL: https://gitlist.dev/e/005001d4c4a4%246af097a0%2440d1c6e0%24%40nexbridge.com In-Reply-To: ``` On February 14, 2019 15:16, Junio C Hamano wrote: > Jeff King 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 > Helped-by: Duy Nguyen > Helped-by: Jeff King > Signed-off-by: Junio C Hamano > --- > > 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 ``` ## Junio C Hamano, 2019-02-14 21:15 Subject: Re: Re* [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: URL: https://gitlist.dev/e/xmqqva1m6ph4.fsf%40gitster-ct.c.googlers.com In-Reply-To: <005001d4c4a4$6af097a0$40d1c6e0$@nexbridge.com> ``` "Randall S. Becker" writes: > On February 14, 2019 15:16, Junio C Hamano wrote: >> Jeff King 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 >> Helped-by: Duy Nguyen >> Helped-by: Jeff King >> Signed-off-by: Junio C Hamano >> --- >> >> 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. ``` ## Martin Ågren, 2019-02-15 06:08 Subject: Re: Re* [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: URL: https://gitlist.dev/e/CAN0heSooOiJkkfr%3DsKn%2BdkMmU9knabeTnY90EJyEu2fNT8XMzw%40mail.gmail.com In-Reply-To: ``` On Fri, 15 Feb 2019 at 03:13, Junio C Hamano 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 ``` ## Junio C Hamano, 2019-02-15 17:51 Subject: Re: Re* [Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case Message-ID: URL: https://gitlist.dev/e/xmqqwom1549w.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Martin Ågren writes: > On Fri, 15 Feb 2019 at 03:13, Junio C Hamano 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é. ```