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

> ---
>  t/t5310-pack-bitmaps.sh | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 6ee4d3f2d9..557bd0d0c0 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -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
>  '
>  
> 

> On 14/08/18 13:47, SZEDER Gábor wrote:
>> ... 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?

>
> Andrei Rybak <rybak.a.v@gmail.com> writes:
>
> > On 14/08/18 13:47, SZEDER Gábor wrote:
> >> ... 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.)

> Andrei Rybak <rybak.a.v@gmail.com> writes:
>>
>> 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'.

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

> On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:
> 
>>   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.
> 
> 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).

> On 19/08/18 22:32, Jeff King wrote:
> > On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:
> > 
> >>   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.
> > 
> > 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:

> > 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).
> 
> ... this is indeed a better solution. I written out the cases for
> updated test_cmp to straighten out my thinking:

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

>
> On 17/08/18 19:39, SZEDER Gábor wrote:
> >
> > 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'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.)

> 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 <szeder.dev@gmail.com>
> ---
>  t/t5310-pack-bitmaps.sh | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 6ee4d3f2d9..557bd0d0c0 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -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
>  '

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

> On Mon, Aug 27, 2018 at 10:22:46AM +0000, Kirill Smelkov wrote:
> 
> > 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