patch, 10 parts'test_i18ngrep'-related fixes and improvements

When 'test_i18ngrep' can't find the expected pattern, it exits completely silently; when its negated form does find the pattern that shouldn't be there, it prints the matching line(s) but otherwise exits without any error message. This leaves the developer puzzled about what could have gone wrong. Well, at least it left me puzzled...

Initially all I wanted to do was to make 'test_i18ngrep' more informative on failure, but then skeletons started to fall out of the closet^Wour test suite, and BAM! before I knew it I had 10 patches:

  t5541: add 'test_i18ngrep's missing filename parameter
  t5812: add 'test_i18ngrep's missing filename parameter
  t6022: don't run 'git merge' upstream of a pipe
  t4001: don't run 'git status' upstream of a pipe

Bugfixes for a few tests.  The first two are fun.

  t5510: consolidate 'grep' and 'test_i18ngrep' patterns
  t5536: let 'test_i18ngrep' read the file without redirection

Cleanups.

  t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'

Pure code movement.

  t: forbid piping into 'test_i18ngrep'
  t: make sure that 'test_i18ngrep' got enough parameters

These try to prevent similar bugs in our tests in the future. Both are imperfect, see the commit messages about their limitations and why I think they are good enough.

  t: make 'test_i18ngrep' more informative on failure

Again, see the commit message about its limitations and why it's good enough.

 t/t4001-diff-rename.sh        | 11 ++++++---
 t/t5510-fetch.sh              |  9 +++-----
 t/t5536-fetch-conflicts.sh    |  2 +-
 t/t5541-http-push-smart.sh    |  2 +-
 t/t5812-proto-disable-http.sh |  3 +--
 t/t6022-merge-rename.sh       |  6 +++--
 t/test-lib-functions.sh       | 53 +++++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh                 | 26 ---------------------
 8 files changed, 71 insertions(+), 41 deletions(-)

The test 'push --no-progress silences progress but not status' runs 'test_i18ngrep' without specifying a filename parameter. This has remained unnoticed since its introduction in e304aeba2 (t5541: test more combinations of --progress, 2012-05-01), because that 'test_i18ngrep' is supposed to check that the given pattern is not present in its input, and of course it won't find that pattern if its input is empty, (as it comes from /dev/null). This also means that this test could miss a potential breakage of 'git push --no-progress'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5541-http-push-smart.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Fri, Jan 26, 2018 at 01:36:59PM +0100, SZEDER Gábor wrote:

Oof, embarrassing. Thanks for catching.

This and other errors make me wonder if test_i18ngrep ought to take an explicit "-" for stdin, and error out if no file argument is given. That may be overkill, though (and it's not like we wouldn't have the same problem with regular "grep").

-Peff

On Fri, Jan 26, 2018 at 01:23:03PM -0500, Jeff King wrote:

....and I really ought to start reading your entire sets of patches before commenting on the early ones. :-/

-Peff

The primary purpose of three tests in 't4001-diff-rename.sh' is to check rename detection in 'git status', but all three do so by running 'git status' upstream of a pipe, hiding its exit code. Consequently, the test could continue even if 'git status' exited with error.

Use an intermediate file between 'git status' and 'test_i18ngrep' to catch a potential failure of the former.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t4001-diff-rename.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed to be called from our test scripts, so they should be in 'test-lib-functions.sh'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++
 t/test-lib.sh           | 26 --------------------------
 2 files changed, 26 insertions(+), 26 deletions(-)

SZEDER Gábor <szeder.dev@gmail.com> writes:

Hmph. I do not care too much either way, but I had an impression that test-lib-functions.sh is meant to be more generic (i.e. those who want can steal it from us and use it in their project without dragging too much of the local convention we employ in this project) than what is in test-lib.sh, which can heavily be specific to Git, and I also had an impression that gettext-poison build is quite a local convention we use in this project, not applicable to other people.

On Fri, Jan 26, 2018 at 10:19:17AM -0800, Junio C Hamano wrote:

I had a similar notion, but I thought it was the other way around: test-lib.sh was supposed to be the harness, and test-lib-functions.sh was our own stuff. But I do not think we have really kept to that over the years. TBH I have generally been confused by the distinction and just use ctags to find the right source file. ;)

-Peff

On Fri, Jan 26, 2018 at 7:19 PM, Junio C Hamano <gitster@pobox.com> wrote:

Well, there are a lot of Git-specific functions in 'test-lib-functions.sh' already:

test_set_index_version test_tick debug test_commit test_merge test_chmod test_unconfig test_config{,_global} test_cmp_rev test_create_repo test_ln_s_add test_normalize_bool nongit

When 'test_i18ngrep' can't find the expected pattern, it exits completely silently; when its negated form does find the pattern that shouldn't be there, it prints the matching line(s) but otherwise exits without any error message. This leaves the developer puzzled about what could have gone wrong.

Make 'test_i18ngrep' more informative on failure by printing an error message including the invoked 'grep' command and the contents of the file it had to scan through.

Note that this "dump the scanned file" part is not quite perfect, as it dumps only the file specified as the function's last positional parameter, thus assuming that there is only a single file parameter. I think that's a reasonable assumption to make, one that holds true in the current code base. And even if someone were to scan multiple files at once in the future, the worst thing that could happen is that the verbose error message won't include the contents of all those files, only the last one. Alas, we can't really do any better than this, because checking whether the other positional parameters match a filename can result in false positives: 't3400-rebase.sh' and 't3404-rebase-interactive.sh' contain one test each, where the 'test_i18ngrep's pattern verbatimely matches a file in the trash directory. Note that the absence of a file parameter is not an issue, because the lint check added in the previous commit ensures that 'test_i18ngrep' never reads from its standard input, consequently there must be a file parameter.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

On Fri, Jan 26, 2018 at 01:37:08PM +0100, SZEDER Gábor wrote:

I think this is an improvement. You can also use "-x" to get a better sense of exactly which command failed, but I have never been sad to see more verbose output from failing tests by default. :)

Heh, this makes me support even more the "last one must be a file" rule that Junio suggested for the linting check.

-Peff

On Fri, Jan 26, 2018 at 7:50 PM, Jeff King <peff@peff.net> wrote:

Yeah, I know... but I have some issues with running tests with '-x'; I suspect PEBKAC, but haven't yet got around to investigate.

On Fri, Jan 26, 2018 at 08:23:24PM +0100, SZEDER Gábor wrote:

Some tests absolutely fail with "-x", due to them caring about the stderr output of shell functions. But with the BASH_XTRACEFD stuff, if you run suite under bash it should al Just Work (and I recently added TEST_SHELL_PATH to use bash just for the test suite without building all of the scripts with it).

-Peff

On Fri, Jan 26, 2018 at 8:25 PM, Jeff King <peff@peff.net> wrote:

Yeah, I knew about TEST_SHELL_PATH, but still:

  $ make -j4 TEST_SHELL_PATH=/bin/bash
  <...>
  $ cd t/
  $ for t in t[0-9][0-9][0-9][0-9]-*.sh ; do "./$t" -x ; done >/dev/null 2>&1
  $ grep '^failed [^0]$' test-results/*.counts |wc -l
  44

The worst offender is t0008-ignores with 208 tests failing with '-x'... I suspect a setup test fails for some reason, and (most of) the other failed tests are just fallout; haven't dared to look yet :)

On Fri, Jan 26, 2018 at 09:26:38PM +0100, SZEDER Gábor wrote:

You cannot run "./$t" and expect TEST_SHELL_PATH to kick in; that starts the test with the #! header, which is always /bin/sh (we don't "build" the test scripts like we do regular scripts).

You need to run either:

  - make TEST_SHELL_PATH=/bin/bash test

or

  - bash $t -x

There _is_ one exception where it sometimes works, which is if you use --tee or --verbose-log, in which case the shell script has to re-exec itself, in which case it always pulls the value from GIT-BUILD-OPTIONS to re-exec.

-Peff

Redirecting 'test_i18ngrep's standard input from a file will interfere with the linting that will be added in a later patch.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5536-fetch-conflicts.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Two of the previous patches in this series fixed two bogus 'test_i18ngrep' invocations that had neither a filename parameter not anything piped into their standard input, yet both managed to remain unnoticed for years. A third similarly bogus invocation is currently lurking in 'pu' for a couple of weeks now.

Try to catch similar mistakes in the future by ensuring that 'test_i18ngrep' has at least two parameters, not including an optional '!' to negate the pattern. Perform these checks after we made sure that there is no data on the 'test_i18ngrep's standard input, so if the filename parameter is missing because someone is piping a git command's output into this function, then they would get the more relevant error message.

Note that this is not quite perfect, as it doesn't account for any 'grep --options' given as parameters. However, doing so would be far too complicated, considering that patters can start with dashes as well, and in the majority of the cases we don't use any such options anyway.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

About that third one in 'pu': it's test '3b-check: Avoid implicit rename if involved as source on current side' introduced in commit fcd649216 (directory rename detection: testcases to avoid taking detection too far, 2018-01-05) in branch 'en/rename-directory-detection'.

  https://public-inbox.org/git/CAM0VKj=qhJQJ7uJWbBouSTYD0frA1zp1gwXzMVXuTiF+C6GH+g@mail.gmail.com/T/#u

 t/test-lib-functions.sh | 6 ++++++
 1 file changed, 6 insertions(+)

On Fri, Jan 26, 2018 at 01:37:07PM +0100, SZEDER Gábor wrote:

Hrm. At first I thought this was redundant with the stdin thing in the previous one. But that is only checking "did you _try_ to use stdin". This is checking "did you accidentally use stdin, which was empty".

But I think maybe it's the opposite; the other one is redundant with this one, since it would be hard to convince grep to read from stdin anyway with this.

Yeah, I agree this would help most cases, but not hurt in others. I do think Junio's "see if the final argument is a file" approach seems like it would cover us pretty accurately, though, without having to get too intimate with grep options.

-Peff

On Fri, Jan 26, 2018 at 7:37 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

s/not/nor/

One of the tests in 't5510-fetch.sh' checks the output of 'git fetch' using 'test_i18ngrep', and while doing so it prefilters the output with 'grep' before piping the result into 'test_i18ngrep'.

This prefiltering is unnecessary, with the appropriate pattern 'test_i18ngrep' can do it all by itself. Furthermore, piping data into 'test_i18ngrep' will interfere with the linting that will be added in a later patch.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5510-fetch.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

SZEDER Gábor <szeder.dev@gmail.com> writes:

It is very likely that the prefiltering "grep" will not even see what it is looking for under GETTEXT_POISON build in the first place, so this conversion is the right thing to do from that point of view as well.

On Fri, Jan 26, 2018 at 7:16 PM, Junio C Hamano <gitster@pobox.com> wrote:

No, GETTEXT_POISON only affects the translated messages, but those 'grep' invocations looked only at refnames and formatting.

This is the GETTEXT_POISON-ed output of 'git fetch' in that test (probably will get line-wrapped):

# GETTEXT POISON # * # GETTEXT POISON # descriptive-branch ->
refs/crazyheads/descriptive-branch
 * # GETTEXT POISON # refs/others/crazy  -> crazy
 * # GETTEXT POISON # descriptive-tag    -> descriptive-tag

SZEDER Gábor <szeder.dev@gmail.com> writes:

You are right for this specific case, but I was talking more from general principle---running test_i18ngrep on an output from grep should be flagged as anti-pattern (I recall vaguly finding an instance not in too distant past).

When checking a git command's output with 'test_i18ngrep', it's tempting to conveniently pipe the git command's standard output into 'test_i18ngrep'. Unfortunately, this is an anti-pattern, because it hides the git command's exit code, and the test could continue even if the command exited with error.

Add a bit of linting to 'test_i18ngrep' to detect when data is fed to its standard input and to error out with a "bug in the test script" message.

Note that this change will also forbid cases where 'test_i18ngrep' would legitimately read its standard input, e.g.

  - when its standard input is redirected from a file, or

  - when a git command's standard output is first written to an
    intermediate file, which is then preprocessed by a non-git command
    before the results are piped into 'test_i18ngrep'.

See two of the previous patches for the only such cases we had in our test suite. However, reliably preventing this antipattern is arguably more important than supporting these cases, which can be worked around by only minor inconveniences.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 4 ++++
 1 file changed, 4 insertions(+)

SZEDER Gábor <szeder.dev@gmail.com> writes:

I am not sure if that inconveniences will be minor. Is this too contrived an example, for example?

  check () {
        pattern=$1 file=$2 script=./runme

        test_i18ngrep "$pattern" "$file" &&
        write_script "$script" &&
        test_expect_success "check $pattern" '
                "$script"
        '
  }

  check foo file <<-EOF
  ... test script comes here ...
  EOF

Junio C Hamano <gitster@pobox.com> writes:

Is there a case where test_i18ngrep (after your clean-ups in this series up to 06/10) needs to read from more than one file?

I actually think that the kind of inconveniences we *can* work with, without risking breakage to legitimate test, would be to allow and require test_i18ngrep to name and read only from one file that appears at the end of its command line. IOW, instead of doing a probing "read" that you cannot undo and break legitimate test, I think it is OK to see if the last token names a file that is on the filesystem, e.g.

	test_i18ngrep () {
		eval test -f \"\${$#}\" ||
		error "bug in the test sript: test_i18ngrep must" \
		      "name a file to read as the last token on the command line"
		...

On Fri, Jan 26, 2018 at 10:39:05AM -0800, Junio C Hamano wrote:

FWIW I like that much more than the weird "read" thing. It would also disallow "-", but we could make an exception for that if we choose to.

-Peff

On Fri, Jan 26, 2018 at 7:24 PM, Junio C Hamano <gitster@pobox.com> wrote:

With 'test_i18ngrep' outside the 'test_expect_success' block!? Definitely too contrived. :)

OTOH, what about flipping the order of 'test_i18ngrep' and 'write_script'? If we can't do that, then I wonder what the reason might be that is not too contrived.

SZEDER Gábor <szeder.dev@gmail.com> writes:

Well, I think you got the idea. The point is that test_i18ngrep may not be the only thing that is redirected into, but can just be a part of a block of commands, and the "probing" read will hurt.

On Fri, Jan 26, 2018 at 01:37:06PM +0100, SZEDER Gábor wrote:

The idea seems reasonable to me. Let's think about what the escape hatch looks like to work around it if you need to.

I guess you've got:

  cat >file &&
  test_i18ngrep ... file

which is not too bad.

You've also got:

  test_i18ngrep ... -

though that relies on the underlying grep understanding "-" (which is in POSIX, though with a rather vague "if the implementations supports it"). And it wouldn't work with the "read" test in this patch.

This seems kind of hacky compared to just seeing if there is a file argument. But I suppose that is hard to do, since we just pass through the arguments to grep.

Though looking at our test_18ngrep invocations, they are simple enough that would just ask "are there two non-option arguments at the end of the command line". The exception is "-e", but IMHO we could just drop that. It serves no purpose unless you're trying to hide a "-" at the start of your pattern, and in fact we used to ban it since sysv grep didn't understand it (e.g., aadbe44f883).

-Peff

The second 'test_i18ngrep' invocation in the test 'curl redirects respect whitelist' is missing its filename parameter. This has remained unnoticed since its introduction in f4113cac0 (http: limit redirection to protocol-whitelist, 2015-09-22), because it would only cause the test to fail if Git was built with a sufficiently old libcurl version. The test's two ||-chained 'test_i18ngrep' invocations are supposed to check that either one of the two patterns is present in 'git clone's error message. As it happens, the first invocation covers the error message from any reasonably up-to-date libcurl, thus the second invocation, the one without the filename parameter, isn't executed at all. Apparently no one has run the test suite's httpd tests with such an old libcurl in the last 2+ years, or at least they haven't bothered to notify us about the failed test.

Fix this by consolidating the two patterns into a single extended regexp, eliminating the need for an ||-chained second 'test_i18ngrep' invocation.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5812-proto-disable-http.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

On Fri, Jan 26, 2018 at 01:37:00PM +0100, SZEDER Gábor wrote:

Interesting find.

The "too old" curl is older than 7.19.4, which we actually fail to build with since v2.12.0. So they probably did not even get as far as the tests. ;)

OK. Once upon a time I think we had trouble with "grep -E", since some older systems had only "egrep". But I see we've introduced some "grep -E" invocations as far back as 2013 and nobody has complained, so it's probably fine.

-Peff

On Fri, Jan 26, 2018 at 7:27 PM, Jeff King <peff@peff.net> wrote:

Oh, OK, I was not aware of that. The oldest non-maintenance release with the missing filename parameter is v2.7.0, so that's still a 5 releases time frame to notice it.

Anyway, I'm preparing v2 of this series, and I'm not sure what to do about this.

  - Should I simply drop the "your curl version is too old" pattern?  It
    would make sense, but it just doesn't feel quite right to remove it
    while the corresponding printf() is still there, even if it can't be
    triggered anymore.  However, cleaning up the curl version checks in
    http.c to remove this message is beyond the scope of this patch
    series.

  - Or leave it almost-as-is, only dropping the now unnecessary curly
    braces as Simon pointed out.  And perhaps a bit of update to the
    commit message.

I'd prefer the second option.

Yeah, first I went with the more traditional "\(this\|that\)" pattern, but then noticed that 'grep -E' is already used in a couple of places, and picked the format that uses less escape characters.

On Wed, Feb 07, 2018 at 02:53:17PM +0100, SZEDER Gábor wrote:

Actually, I'm wrong. It looks like we did finally fix it in f18777ba6e (http: fix handling of missing CURLPROTO_*, 2017-08-11), which is in v2.15. So:

Yeah, I think just leave it as-is. Thanks.

-Peff

On Fri, Jan 26, 2018 at 01:37:00PM +0100, SZEDER Gábor wrote:

I think we can drop the curly braces as well, as they were only used to group the ||; leaving only:

Regards Simon

The primary purpose of 't6022-merge-rename.sh' is to test 'git merge', but one of the tests runs it upstream of a pipe, hiding its exit code. Consequently, the test could continue even if 'git merge' exited with error.

Use an intermediate file between 'git merge' and 'test_i18ngrep' to catch a potential failure of the former.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t6022-merge-rename.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

On Fri, Jan 26, 2018 at 01:36:58PM +0100, SZEDER Gábor wrote:

I know the feeling. :)

The series overall looks good to me. I left some comments on the approach in the final few patches, but I could live with it as-is, or with the approach Junio suggested.

-Peff

This is the second version of 'sg/test-i18ngrep'.

To recap, this patch series fixes a couple of bogus 'test_i18ngrep' invocations (patches 1-4), tries to prevent similar bugs in the future (patch 8), teaches 'test_i18ngrep' to be more informative on failure (patch 9), and a bit of cleanups in between (patches 5-7).

Changes since the previous version [1]:

  - Use Junio's "last parameter must be file" suggestion instead of
    trying to read stdin in patch 8.
  - Squashed together the patches validating 'test_i18ngrep's
    parameters (patches 8 and 9), in the hope that this way I can
    better explain that the two checks are not redundant but
    complement each other.
  - Followed Simon's suggestion and dropped the now unnecessary curly
    brackets in patch 2.
  - Dropped a subshell in the last patch.  I initially used it to
    prevent the variable $f from leaking into the tests, since we
    can't use the 'local' keyword (yet), but other test helper
    function don't seem to care.
  - Fixed the placements of single quotes and '!' in error messages
    and redirected one more error message to stderr in the last patch.
  - Fixed a couple of typos in commit messages (the one Eric pointed
    out, but later noticed maybe 2-3 more).

[1] - https://public-inbox.org/git/20180126123708.21722-1-szeder.dev@gmail.com/T/

SZEDER Gábor (9):
  t5541: add 'test_i18ngrep's missing filename parameter
  t5812: add 'test_i18ngrep's missing filename parameter
  t6022: don't run 'git merge' upstream of a pipe
  t4001: don't run 'git status' upstream of a pipe
  t5510: consolidate 'grep' and 'test_i18ngrep' patterns
  t5536: let 'test_i18ngrep' read the file without redirection
  t: move 'test_i18ncmp' and 'test_i18ngrep' to 'test-lib-functions.sh'
  t: validate 'test_i18ngrep's parameters
  t: make 'test_i18ngrep' more informative on failure

 t/t4001-diff-rename.sh        | 11 ++++++---
 t/t5510-fetch.sh              |  9 +++-----
 t/t5536-fetch-conflicts.sh    |  2 +-
 t/t5541-http-push-smart.sh    |  2 +-
 t/t5812-proto-disable-http.sh |  5 +---
 t/t6022-merge-rename.sh       |  6 +++--
 t/test-lib-functions.sh       | 54 +++++++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh                 | 26 ---------------------
 8 files changed, 72 insertions(+), 43 deletions(-)

The test 'push --no-progress silences progress but not status' runs 'test_i18ngrep' without specifying a filename parameter. This has remained unnoticed since its introduction in e304aeba2 (t5541: test more combinations of --progress, 2012-05-01), because that 'test_i18ngrep' is supposed to check that the given pattern is not present in its input, and of course it won't find that pattern if its input is empty (as it comes from /dev/null). This also means that this test could miss a potential breakage of 'git push --no-progress'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5541-http-push-smart.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The primary purpose of three tests in 't4001-diff-rename.sh' is to check rename detection in 'git status', but all three do so by running 'git status' upstream of a pipe, hiding its exit code. Consequently, the test could continue even if 'git status' exited with error.

Use an intermediate file between 'git status' and 'test_i18ngrep' to catch a potential failure of the former.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t4001-diff-rename.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Some of the previous patches in this series fixed bogus 'test_i18ngrep' invocations:

  - Two invocations where the tested git command's standard output is
    directly piped into 'test_i18ngrep'.  While convenient, this is an
    antipattern, because the pipe hides the git command's exit code,
    and the test could continue even if the command exited with error.

  - Two invocations that had neither a filename parameter nor anything
    piped into their standard input, yet both managed to remain
    unnoticed for years.  A third similarly bogus invocation is
    currently lurking in 'pu' for a couple of weeks now.

Prevent similar mistakes in the future by validating 'test_i18ngrep's parameters requiring that

  - The last parameter names an existing file to be read, effectively
    forbiding piping into 'test_i18ngrep'.

    Note that this change will also forbid cases where 'test_i18ngrep'
    would legitimately read its standard input, e.g. when its standard
    input is redirected from a file, or when a git command's standard
    output is first written to an intermediate file, which is then
    preprocessed by a non-git command before the results are piped
    into 'test_i18ngrep'.  See two of the previous patches for the
    only such cases we had in our test suite.  However, reliably
    preventing the piping antipattern is arguably more important than
    supporting these cases, which can be easily worked around by
    opening the file directly or using an intermediate file anyway.

  - There are at least two parameters, not including the optional '!'
    to negate the pattern.  This ought to catch corner cases when
    'test_i18ngrep' looks for the name of an existing file on its
    standard input; the above check would miss this case becase the
    filename as pattern would be the last parameter.

    Note that this is not quite perfect, as it doesn't account for any
    'grep --options' given as parameters.  However, doing so would be
    far too complicated, considering that patterns can start with
    dashes as well, and in the majority of the cases we don't use any
    such options anyway.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

On Thu, Feb 08, 2018 at 04:56:55PM +0100, SZEDER Gábor wrote:

s/forbiding/forbidding/

And most importantly, we never err on the side of complaining unnecessarily. So our safety might not kick in, but as long as it kicks in most of the time, we're fine.

I like this approach much better than the previous round.

These embedded double-quotes are unnecessary, I think, because it's a variable assignment: E.g.:

  set -- one two 'foo bar'
  eval "last_arg=\${$#}"
  echo $last_arg

should produce "foo bar".

Usually not a big deal, but because of the extra quoting it may make the whole thing a bit more readable to drop them.

-Peff

When 'test_i18ngrep' can't find the expected pattern, it exits completely silently; when its negated form does find the pattern that shouldn't be there, it prints the matching line(s) but otherwise exits without any error message. This leaves the developer puzzled about what could have gone wrong.

Make 'test_i18ngrep' more informative on failure by printing an error message including the invoked 'grep' command and the contents of the file it had to scan through.

Note that this "dump the scanned file" part is not quite perfect, as it dumps only the file specified as the function's last positional parameter, thus assuming that there is only a single file parameter. I think that's a reasonable assumption to make, one that holds true in the current code base. And even if someone were to scan multiple files at once in the future, the worst thing that could happen is that the verbose error message won't include the contents of all those files, only the last one. Alas, we can't really do any better than this, because checking whether the other positional parameters match a filename can result in false positives: 't3400-rebase.sh' and 't3404-rebase-interactive.sh' contain one test each, where the 'test_i18ngrep's pattern verbatimly matches a file in the trash directory.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

One of the tests in 't5510-fetch.sh' checks the output of 'git fetch' using 'test_i18ngrep', and while doing so it prefilters the output with 'grep' before piping the result into 'test_i18ngrep'.

This prefiltering is unnecessary, with the appropriate pattern 'test_i18ngrep' can do it all by itself. Furthermore, piping data into 'test_i18ngrep' will interfere with the linting that will be added in a later patch.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5510-fetch.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Redirecting 'test_i18ngrep's standard input from a file will interfere with the linting that will be added in a later patch.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5536-fetch-conflicts.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Both 'test_i18ncmp' and 'test_i18ngrep' helper functions are supposed to be called from our test scripts, so they should be in 'test-lib-functions.sh'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 26 ++++++++++++++++++++++++++
 t/test-lib.sh           | 26 --------------------------
 2 files changed, 26 insertions(+), 26 deletions(-)

The second 'test_i18ngrep' invocation in the test 'curl redirects respect whitelist' is missing its filename parameter. This has remained unnoticed since its introduction in f4113cac0 (http: limit redirection to protocol-whitelist, 2015-09-22), because it would only cause the test to fail if Git was built with a sufficiently old libcurl version. The test's two ||-chained 'test_i18ngrep' invocations are supposed to check that either one of the two patterns is present in 'git clone's error message. As it happens, the first invocation covers the error message from any reasonably up-to-date libcurl, thus the second invocation, the one without the filename parameter, isn't executed at all. Apparently no one has run the test suite's httpd tests with such an old libcurl in the last 2+ years, or at least they haven't bothered to notify us about the failed test.

Fix this by consolidating the two patterns into a single extended regexp, eliminating the need for an ||-chained second 'test_i18ngrep' invocation.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t5812-proto-disable-http.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

The primary purpose of 't6022-merge-rename.sh' is to test 'git merge', but one of the tests runs it upstream of a pipe, hiding its exit code. Consequently, the test could continue even if 'git merge' exited with error.

Use an intermediate file between 'git merge' and 'test_i18ngrep' to catch a potential failure of the former.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t6022-merge-rename.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

On Thu, Feb 08, 2018 at 04:56:47PM +0100, SZEDER Gábor wrote:

This round looks good to me. I left a very minor comment on patch 8, but otherwise didn't find anything wrong. Thanks.

-Peff