[PATCH] t5404: relax overzealous test

[PATCH] t5404: relax overzealous test

[Johannes Schindelin]

In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we
added a test to verify that deleting an already-deleted ref does not
show an error.

Our test simply looks for the substring 'error' in the output of the
`git push`, which might look innocuous on the face of it.

Suppose, however, that you are a big fan of whales. Or even better: your
IT administrator has a whale of a time picking cute user names, e.g.
referring to you (due to your like of India Pale Ales) as "one of the
cuter rorquals" (see https://en.wikipedia.org/wiki/Rorqual to learn a
thing or two about rorquals) and hence your home directory becomes
/home/cuterrorqual. If you now run t5404, it fails! Why? Because the
test calls `git push origin :b3` which outputs:

    To /home/cuterrorqual/git/t/trash directory.t5404-tracking-branches/.
     - [deleted]         b3

Note how there is no error displayed in that output? But of course
"error" is a substring of "cuterrorqual". And so that `grep error
output` finds something.

This bug was not, actually, caught having "error" as a substring of the
user name but while working in a worktree called "colorize-push-errors",
whose name was part of that output, too, suggesting that not even
testing for the *word* `error` via `git grep -w error output` would fix
the underlying issue.

This patch chooses instead to look for the prefix "error:" at the
beginning of the line, so that there can be no ambiguity that any catch
was indeed a message generated by Git's `error_builtin()` function.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/fix-t5404-error-v1
Fetch-It-Via: git fetch https://github.com/dscho/git fix-t5404-error-v1
 t/t5404-tracking-branches.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5404-tracking-branches.sh b/t/t5404-tracking-branches.sh
index 2b8c0bac7db..2762f420bc2 100755
--- a/t/t5404-tracking-branches.sh
+++ b/t/t5404-tracking-branches.sh
@@ -56,7 +56,7 @@ test_expect_success 'deleted branches have their tracking branches removed' '
 test_expect_success 'already deleted tracking branches ignored' '
 	git branch -d -r origin/b3 &&
 	git push origin :b3 >output 2>&1 &&
-	! grep error output
+	! grep "^error: " output
 '
 
 test_done

base-commit: 468165c1d8a442994a825f3684528361727cd8c0
-- 
2.17.0.windows.1.4.g7e4058d72e3

Re: [PATCH] t5404: relax overzealous test

[Jeff King]

On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:

> In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we
> added a test to verify that deleting an already-deleted ref does not
> show an error.

Amazing that it took this long to come up.

> Suppose, however, that you are a big fan of whales. Or even better: your
> IT administrator has a whale of a time picking cute user names, e.g.
> referring to you (due to your like of India Pale Ales) as "one of the
> cuter rorquals" (see https://en.wikipedia.org/wiki/Rorqual to learn a
> thing or two about rorquals) and hence your home directory becomes
> /home/cuterrorqual. If you now run t5404, it fails! Why? Because the
> test calls `git push origin :b3` which outputs:
> 
>     To /home/cuterrorqual/git/t/trash directory.t5404-tracking-branches/.
>      - [deleted]         b3

This is from the same Dscho who complains about the length of some of my
commit messages, right? ;P

> This patch chooses instead to look for the prefix "error:" at the
> beginning of the line, so that there can be no ambiguity that any catch
> was indeed a message generated by Git's `error_builtin()` function.

Yep, this seems obviously correct.

Right now we do not localize the "error" string, but I wonder if this
ought to use test_i18ngrep. With or without that change, the patch looks
good to me.

-Peff

Re: [PATCH] t5404: relax overzealous test

[Johannes Schindelin]

Hi Peff,

On Fri, 6 Apr 2018, Jeff King wrote:

> On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:
> 
> > Suppose, however, that you are a big fan of whales. Or even better: your
> > IT administrator has a whale of a time picking cute user names, e.g.
> > referring to you (due to your like of India Pale Ales) as "one of the
> > cuter rorquals" (see https://en.wikipedia.org/wiki/Rorqual to learn a
> > thing or two about rorquals) and hence your home directory becomes
> > /home/cuterrorqual. If you now run t5404, it fails! Why? Because the
> > test calls `git push origin :b3` which outputs:
> > 
> >     To /home/cuterrorqual/git/t/trash directory.t5404-tracking-branches/.
> >      - [deleted]         b3
> 
> This is from the same Dscho who complains about the length of some of my
> commit messages, right? ;P

Yes. I've quieted down, though, didn't I? Mainly because I see the merit
in your long commit messages now. Never too late to learn.

Ciao,
Dscho

Re: [PATCH] t5404: relax overzealous test

[Jeff King]

On Fri, Apr 06, 2018 at 10:13:46PM +0200, Johannes Schindelin wrote:

> On Fri, 6 Apr 2018, Jeff King wrote:
> 
> > On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:
> > 
> > > Suppose, however, that you are a big fan of whales. Or even better: your
> > > IT administrator has a whale of a time picking cute user names, e.g.
> > > referring to you (due to your like of India Pale Ales) as "one of the
> > > cuter rorquals" (see https://en.wikipedia.org/wiki/Rorqual to learn a
> > > thing or two about rorquals) and hence your home directory becomes
> > > /home/cuterrorqual. If you now run t5404, it fails! Why? Because the
> > > test calls `git push origin :b3` which outputs:
> > > 
> > >     To /home/cuterrorqual/git/t/trash directory.t5404-tracking-branches/.
> > >      - [deleted]         b3
> > 
> > This is from the same Dscho who complains about the length of some of my
> > commit messages, right? ;P
> 
> Yes. I've quieted down, though, didn't I? Mainly because I see the merit
> in your long commit messages now. Never too late to learn.

Heh, well I'm glad I've won you over, then. And thank you for cleaning
up my (ancient) mess here.

-Peff

Re: [PATCH] t5404: relax overzealous test

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 06 2018, Jeff King wrote:

> On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:
>> This patch chooses instead to look for the prefix "error:" at the
>> beginning of the line, so that there can be no ambiguity that any catch
>> was indeed a message generated by Git's `error_builtin()` function.
>
> Yep, this seems obviously correct.
>
> Right now we do not localize the "error" string, but I wonder if this
> ought to use test_i18ngrep. With or without that change, the patch looks
> good to me.

I think it's much better not to do that.

There's value in the i18n versions checking for only those things that
are translated, and no more, because we should really be on the lookout
for any i18n changes that potentially change plumbing output, or other
things programs expect to parse.

Re: [PATCH] t5404: relax overzealous test

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:
>
>> In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we
>> added a test to verify that deleting an already-deleted ref does not
>> show an error.
>
> Amazing that it took this long to come up.
> ...
>> This patch chooses instead to look for the prefix "error:" at the
>> beginning of the line, so that there can be no ambiguity that any catch
>> was indeed a message generated by Git's `error_builtin()` function.
>
> Yep, this seems obviously correct.

Hits in

    $ git grep 'grep ["'\'']*error' t

shows that many checks for errors that are not anchored, but I do
not think any of them are looking for the string in a pathname other
than this instance, so it would be OK.

Thanks, both.  Will apply.

Re: [PATCH] t5404: relax overzealous test

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:
>
>> In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we
>> added a test to verify that deleting an already-deleted ref does not
>> show an error.
>
> Amazing that it took this long to come up.
> ...
>> This patch chooses instead to look for the prefix "error:" at the
>> beginning of the line, so that there can be no ambiguity that any catch
>> was indeed a message generated by Git's `error_builtin()` function.
>
> Yep, this seems obviously correct.

Hits in

    $ git grep 'grep ["'\'']*error' t

shows that many checks for errors that are not anchored, but I do
not think any of them are looking for the string in a pathname other
than this instance, so it would be OK.

Thanks, both.  Will apply.

Re: [PATCH] t5404: relax overzealous test

[Johannes Schindelin]

Hi Junio,

On Mon, 9 Apr 2018, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Apr 06, 2018 at 09:31:22PM +0200, Johannes Schindelin wrote:
> >
> >> In 0b294c0abf0 (make deleting a missing ref more quiet, 2008-07-08), we
> >> added a test to verify that deleting an already-deleted ref does not
> >> show an error.
> >
> > Amazing that it took this long to come up.
> > ...
> >> This patch chooses instead to look for the prefix "error:" at the
> >> beginning of the line, so that there can be no ambiguity that any catch
> >> was indeed a message generated by Git's `error_builtin()` function.
> >
> > Yep, this seems obviously correct.
> 
> Hits in
> 
>     $ git grep 'grep ["'\'']*error' t
> 
> shows that many checks for errors that are not anchored, but I do
> not think any of them are looking for the string in a pathname other
> than this instance, so it would be OK.

Yes, they are okay, I did run the entire test suite in a worktree with the
same name as my branch name: fix-t5404-error.

Ciao,
Dscho