[Breakage] 2.20.0-rc0 t1404: test_i18ngrep reports 1 instead of 0 on NonStop in one case

>
> 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
>

> On Mon, Feb 11, 2019 at 2:09 AM Randall S. Becker
> <rsbecker@nexbridge.com> 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"

> On Mon, Feb 11, 2019 at 2:09 AM Randall S. Becker
> <rsbecker@nexbridge.com> 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".

>> 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.

> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > On Mon, Feb 11, 2019 at 2:09 AM Randall S. Becker
> > <rsbecker@nexbridge.com> 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?

> 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.

> >> 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.

> 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.

> Jeff King <peff@peff.net> 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.

> 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.

> 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

> 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

> 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
>  '

> 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

>
> 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.

> 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?