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

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

[Randall S. Becker]

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

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

[Duy Nguyen]

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

> 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

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

[Randall S. Becker]

On February 11, 2019 4:57, Duy Nguyen <pclouds@gmail.com> wrote:
> 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"

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

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

[Junio C Hamano]

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?

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.

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

[Randall S. Becker]

On February 11, 2019 16:07, Junio C Hamano wrote:
> 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?

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.

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

[Jeff King]

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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Mon, Feb 11, 2019 at 04:32:52PM -0800, Junio C Hamano wrote:

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

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

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

[Junio C Hamano]

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
 '
 

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

[Randall S. Becker]

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

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

[Junio C Hamano]

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

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

[Martin Ågren]

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

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

[Junio C Hamano]

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