threads / patch / 48235

patcht5404: relax overzealous test

Subject: [PATCH] t5404: relax overzealous test

## tl;dr

7 messages between Apr 6, 2018 and Apr 9, 2018. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Johannes Schindelin· Apr 6, 2018, 19:31 UTC · lore

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

Show changes to t/t5404-tracking-branches.sh +1 −1

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

Jeff King· Apr 6, 2018, 19:53 UTC · re: Johannes Schindelin · lore

Re: [PATCH] t5404: relax overzealous test

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.

Show 10 quoted lines

> 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

Johannes Schindelin· Apr 6, 2018, 20:13 UTC · re: Jeff King · lore

Re: [PATCH] t5404: relax overzealous test

Hi Peff,

On Fri, 6 Apr 2018, Jeff King wrote:

Show 15 quoted lines

> 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

Jeff King· Apr 6, 2018, 20:16 UTC · re: Johannes Schindelin · lore

Re: [PATCH] t5404: relax overzealous test

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

Show 20 quoted lines

> 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

Ævar Arnfjörð Bjarmason· Apr 6, 2018, 20:35 UTC · re: Jeff King · lore

Re: [PATCH] t5404: relax overzealous test

On Fri, Apr 06 2018, Jeff King wrote:

Show 10 quoted lines

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

Junio C Hamano· Apr 9, 2018, 02:21 UTC · re: Jeff King · lore

Re: [PATCH] t5404: relax overzealous test

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

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

Johannes Schindelin· Apr 9, 2018, 13:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] t5404: relax overzealous test

Hi Junio,

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

Show 23 quoted lines

> 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