t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

The test 'pack-objects to file can use bitmap' added in 645c432d61 (pack-objects: use reachability bitmap index when generating non-stdout pack, 2016-09-10) is silently buggy and doesn't check what it's supposed to.

In 't5310-pack-bitmaps.sh', the 'list_packed_objects' helper function does what its name implies by running:

git show-index <"$1" | cut -d' ' -f2

The test in question invokes this function like this:

list_packed_objects <packa-$packasha1.idx >packa.objects && list_packed_objects <packb-$packbsha1.idx >packb.objects && test_cmp packa.objects packb.objects

Note how these two callsites don't specify the name of the pack index file as the function's parameter, but redirect the function's standard input from it. This triggers an error message from the shell, as it has no filename to redirect from in the function, but this error is ignored, because it happens upstream of a pipe. Consequently, both invocations produce empty 'pack{a,b}.objects' files, and the subsequent 'test_cmp' happily finds those two empty files identical.

Fix these two 'list_packed_objects' invocations by specifying the pack index files as parameters. Furthermore, eliminate the pipe in that function by replacing it with an &&-chained pair of commands using an intermediate file, so a failure of 'git show-index' or the shell redirection will fail the test.

... both
invocations produce empty 'pack{a,b}.objects' files, and the subsequent 'test_cmp' happily finds those two empty files identical.

---
 t/t5310-pack-bitmaps.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

t/t5310-pack-bitmaps.sh +4 -3

index 6ee4d3f2d9..557bd0d0c0 100755
@@ -9,7 +9,8 @@ objpath () {
 
 # show objects present in pack ($1 should be associated *.idx)
 list_packed_objects () {
-	git show-index <"$1" | cut -d' ' -f2
+	git show-index <"$1" >object-list &&
+	cut -d' ' -f2 object-list
 }
 
 # has_any pattern-file content-file

@@ -204,8 +205,8 @@ test_expect_success 'pack-objects to file can use bitmap' '
 	# verify equivalent packs are generated with/without using bitmap index
 	packasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) &&
 	packbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) &&
-	list_packed_objects <packa-$packasha1.idx >packa.objects &&
-	list_packed_objects <packb-$packbsha1.idx >packb.objects &&
+	list_packed_objects packa-$packasha1.idx >packa.objects &&
+	list_packed_objects packb-$packbsha1.idx >packb.objects &&
 	test_cmp packa.objects packb.objects
 '
 

Quoted 3 lines · SZEDER Gábor

... both
invocations produce empty 'pack{a,b}.objects' files, and the subsequent 'test_cmp' happily finds those two empty files identical.

Is test_cmp ever used for empty files? Would it make sense for test_cmp to issue warning when an empty file is being compared?

Quoted 7 lines · Andrei Rybak

Quoted 3 lines · SZEDER Gábor

... both
invocations produce empty 'pack{a,b}.objects' files, and the subsequent 'test_cmp' happily finds those two empty files identical.

Is test_cmp ever used for empty files? Would it make sense for test_cmp to issue warning when an empty file is being compared?

Typically test_cmp is used to compare the actual output from a dubious command being tested with the expected output from a procedure that is known not to be broken (e.g. a run of 'echo', or a 'cat' of here-doc), so at least one side would not be empty.

The test done here is an odd case---it compares output from two equally dubious processes that are being tested and sees if their results match.

That said, since we have test_must_be_empty, we could forbid feeding empty files to test_cmp, after telling everybody that a test that expects an empty output must use test_must_be_empty. I do not think it is a terrible idea.

See, we have quite a few tests that extract repetitive common tasks into helper functions, which sometimes includes preparing the expected results and running 'test_cmp', e.g. something like this (oversimplified) example:

  check_cmd () {
        git cmd $1 >actual &&
        echo "$2" >expect &&
        test_cmp expect actual
  }

  check_cmd --foo    FOO
  check_cmd --no-foo ""

I think it would be a good trade-off to allow these helper functions to skip checking emptiness of arguments for test_cmp. Such patch will require only s/test_cmp/&_allow_empty/ for these helper functions and it will help catch cases as bogus test in t5310.

I'll try something like the following on the weekend:

	test_cmp() {

		if test "$1" != - && ! test -s "$1"
		then
			echo >&4 "error: trying to compare empty file '$1'"
			return 1

		fi
		if test "$2" != - && ! test -s "$2"
		then
			echo >&4 "error: trying to compare empty file '$2'"
			return 1
		fi
		test_cmp_allow_empty "$@"
	}

	test_cmp_allow_empty() {
		$GIT_TEST_CMP "$@"
	}

(I'm not sure about redirections in test lib functions. The two if's would probably be in a separate function to be re-used by test_i18ncmp.)

Quoted 16 lines · Andrei Rybak

I'll try something like the following on the weekend:

	test_cmp () {
		if test "$1" != - && ! test -s "$1"
		then
			echo >&4 "error: trying to compare empty file '$1'"
			return 1
		fi
		if test "$2" != - && ! test -s "$2"
		then
			echo >&4 "error: trying to compare empty file '$2'"
			return 1
		fi
		test_cmp_allow_empty "$@"
	}

I actually think the above gives way too confusing output, when the actual output is empty and we are expecting some output.

The tester wants to hear from test_cmp "your 'git cmd' produced some output when we are expecting none" as the primary message. We are trying to find bugs in "git" under development, and diagnosing iffy tests is secondary. But with your change, the first thing that is checked is if 'expect' is an empty file and that is what we get complaints about, without even looking at what is in 'actual'.

Quoted 9 lines

I actually think the above gives way too confusing output, when the actual output is empty and we are expecting some output.

The tester wants to hear from test_cmp "your 'git cmd' produced some output when we are expecting none" as the primary message. We are trying to find bugs in "git" under development, and diagnosing iffy tests is secondary. But with your change, the first thing that is checked is if 'expect' is an empty file and that is what we get complaints about, without even looking at what is in 'actual'.

I came up with two solutions for this issue:

1. Check both files at the same time (combination with Gábor's

     function):

	test_cmp () {
		if test "$1" != - &&
		   test "$2" != - &&
		   ! test -s "$1" && 
		   ! test -s "$2"
		then
			error "bug in test script: using test_cmp to check empty file; use test_must_be_empty instead"
		fi
		test_cmp_allow_empty "$@"
	}

     This will still be reporting to the developer clearly, but
     will only catch cases exactly like the bogus test in t5310.

Quoted 14 lines · Andrei Rybak

1. Check both files at the same time (combination with Gábor's

     function):

	test_cmp () {
		if test "$1" != - &&
		   test "$2" != - &&
		   ! test -s "$1" && 
		   ! test -s "$2"
		then
			error "bug in test script: using test_cmp to check empty file; use test_must_be_empty instead"
		fi
		test_cmp_allow_empty "$@"
	}

Quoted 2 lines

This will still be reporting to the developer clearly, but will only catch cases exactly like the bogus test in t5310.

Doesn't that have the opposite issue? If we expect non-empty output but the command produces empty output, we'd say "bug in the test script". But that is not true at all; it's a failed test.

If we assume that "expect" is first (which is our convention but not necessarily guaranteed), then I think the best logic is something like:

if $1 is empty; then
bug in the test script
elif test_cmp_allow_empty "$@"
test failure

We do not need to check $2 at all. An empty one is either irrelevant (if the expectation is empty), or a test failure (because it would not match the non-empty $1).

Quoted 19 lines · Jeff King

Quoted 16 lines · Andrei Rybak

1. Check both files at the same time (combination with Gábor's

     function):

	test_cmp () {
		if test "$1" != - &&
		   test "$2" != - &&
		   ! test -s "$1" && 
		   ! test -s "$2"
		then
			error "bug in test script: using test_cmp to check empty file; use test_must_be_empty instead"
		fi
		test_cmp_allow_empty "$@"
	}

     This will still be reporting to the developer clearly, but
     will only catch cases exactly like the bogus test in t5310.

Quoted 3 lines

Doesn't that have the opposite issue? If we expect non-empty output but the command produces empty output, we'd say "bug in the test script". But that is not true at all; it's a failed test.

No. Only when both "$1" and "$2" are empty files will the function above report "bug in test script". From patch's commit message:

Quoted 8 lines

If we assume that "expect" is first (which is our convention but not necessarily guaranteed), then I think the best logic is something like:

if $1 is empty; then
bug in the test script
elif test_cmp_allow_empty "$@"
test failure

Quoted 3 lines

We do not need to check $2 at all. An empty one is either irrelevant (if the expectation is empty), or a test failure (because it would not match the non-empty $1).

... this is indeed a better solution. I written out the cases for updated test_cmp to straighten out my thinking:

Quoted 8 lines

Quoted 8 lines

If we assume that "expect" is first (which is our convention but not necessarily guaranteed), then I think the best logic is something like:

if $1 is empty; then
bug in the test script
elif test_cmp_allow_empty "$@"
test failure

Quoted 6 lines

Quoted 3 lines

We do not need to check $2 at all. An empty one is either irrelevant (if the expectation is empty), or a test failure (because it would not match the non-empty $1).

... this is indeed a better solution. I written out the cases for updated test_cmp to straighten out my thinking:

I'd be OK pursuing either this line, or what you showed originally.

Quoted 6 lines · SZEDER Gábor

See, we have quite a few tests that extract repetitive common tasks into helper functions, which sometimes includes preparing the expected results and running 'test_cmp', e.g. something like this (oversimplified) example:

Quoted 8 lines

  check_cmd () {
        git cmd $1 >actual &&
        echo "$2" >expect &&
        test_cmp expect actual
  }

  check_cmd --foo    FOO
  check_cmd --no-foo ""

I've only had time to look into this from t0001 up to t0008-ignores.sh, where test_check_ignore does this. If these helper functions need to allow comparing empty files -- how about adding special variation of cmp functions for cases like this: test_cmp_allow_empty and test_i18ncmp_allow_empty?

I think it would be a good trade-off to allow these helper functions to skip checking emptiness of arguments for test_cmp. Such patch will require only s/test_cmp/&_allow_empty/ for these helper functions and it will help catch cases as bogus test in t5310.

I'll try something like the following on the weekend:

        test_cmp() {
                if test "$1" != - && ! test -s "$1"
                then
                        echo >&4 "error: trying to compare empty file '$1'"
                        return 1
                fi
                if test "$2" != - && ! test -s "$2"
                then
                        echo >&4 "error: trying to compare empty file '$2'"
                        return 1
                fi

                test_cmp_allow_empty "$@"
        }

        test_cmp_allow_empty() {
                $GIT_TEST_CMP "$@"
        }

(I'm not sure about redirections in test lib functions. The two if's would probably be in a separate function to be re-used by test_i18ncmp.)

expected && test_cmp expected actual

The test 'pack-objects to file can use bitmap' added in 645c432d61 (pack-objects: use reachability bitmap index when generating non-stdout pack, 2016-09-10) is silently buggy and doesn't check what it's supposed to.

In 't5310-pack-bitmaps.sh', the 'list_packed_objects' helper function does what its name implies by running:

git show-index <"$1" | cut -d' ' -f2

The test in question invokes this function like this:

list_packed_objects <packa-$packasha1.idx >packa.objects && list_packed_objects <packb-$packbsha1.idx >packb.objects && test_cmp packa.objects packb.objects

Note how these two callsites don't specify the name of the pack index file as the function's parameter, but redirect the function's standard input from it. This triggers an error message from the shell, as it has no filename to redirect from in the function, but this error is ignored, because it happens upstream of a pipe. Consequently, both invocations produce empty 'pack{a,b}.objects' files, and the subsequent 'test_cmp' happily finds those two empty files identical.

Fix these two 'list_packed_objects' invocations by specifying the pack index files as parameters. Furthermore, eliminate the pipe in that function by replacing it with an &&-chained pair of commands using an intermediate file, so a failure of 'git show-index' or the shell redirection will fail the test.

Signed-off-by SZEDER Gábor <[email protected]>

---
 t/t5310-pack-bitmaps.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

t/t5310-pack-bitmaps.sh +2 -1

index 6ee4d3f2d9..557bd0d0c0 100755
@@ -9,7 +9,8 @@ objpath () {
 
 # show objects present in pack ($1 should be associated *.idx)
 list_packed_objects () {
-	git show-index <"$1" | cut -d' ' -f2
+	git show-index <"$1" >object-list &&
+	cut -d' ' -f2 object-list
 }
 
 # has_any pattern-file content-file

A minor comment from outside observer: running tests under something like

-e and -o pipefail

would automatically catch the mistake in the first place. Maybe -o pipefail is bashism (I had not checked), but git grep " | " t/ shows there are a lot of pipelines being used, and thus similar errors might be silently resting there. Something like -o pipefail would catch all such problems automatically.

Quoted 10 lines · Kirill Smelkov

A minor comment from outside observer: running tests under something like

-e and -o pipefail

would automatically catch the mistake in the first place. Maybe -o pipefail is bashism (I had not checked), but git grep " | " t/ shows there are a lot of pipelines being used, and thus similar errors might be silently resting there. Something like -o pipefail would catch all such problems automatically.

Yes, "pipefail" is a bash-ism that we can't rely on.

I will say that I have been bitten before by "set -e -o pipefail" and its subtle handling of SIGPIPE. Try this:

set -e -o pipefail
yes | head