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

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

[SZEDER Gábor]

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
 '
 
-- 
2.18.0.720.g1f300496fc

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Jeff King]

On Tue, Aug 14, 2018 at 01:47:21PM +0200, SZEDER Gábor wrote:

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

Good catch, and nicely explained. The patch itself looks obviously
correct.

-Peff

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Andrei Rybak]

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?

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

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Junio C Hamano]

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.

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[SZEDER Gábor]

On Fri, Aug 17, 2018 at 12:36 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

I thought so as well, and I've looked into it; in fact this patch was
one of the skeletons that fell out of our test suite "while at it".
However, I had to change my mind about it, and now I don't think we
should go all the way and forbid that.

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

Completely forbidding feeding empty files to 'test_cmp' would require
us to add a bit of logic to cases like this to call 'test_cmp' or
'test_must_be_empty' based on the content of the second parameter.
Arguably it's only a tiny bit of logic, as only a single if statement
is needed, but following our coding style it would take 7 lines
instead of only 2:

  if test -n "$2"
  then
        echo "$2" >expect &&
        test_cmp expect actual
  else
        test_must_be_empty actual
  fi

I don't think it's worth it in cases like this.

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Andrei Rybak]

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

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Junio C Hamano]

Andrei Rybak <rybak.a.v@gmail.com> writes:

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

Style: SP before and after ().

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

	: >expect &&
	git cmd >actual &&
	test_cmp expect actual

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

It's the same story, and it is even worse than the above, when we
are expecting some output but the command produced no output, i.e.


	echo Everything up-to-date. >expect &&
	git cmd >actual &&
	test_cmp expect actual

We should get a complaint from test_cmp that 'actual' does not match
the string we were expecting (and even better, we show how they are
different by running them thru "diff -u"), not that 'actual' is an
empty file.


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

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[SZEDER Gábor]

On Fri, Aug 17, 2018 at 9:27 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:
>
> 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

Yeah, these conditions are what I instrumented 'test_cmp' with (except
I used 'error "bug in test script ..."') to find callsites that could
be converted to 'test_must_be_empty', that's how I found the bug fixed
in this patch.  However, it resulted in a lot of errors from the cases
mentioned in my previous email.  Then I reached out to Bash and tried
this:

  test_cmp() {
         if test -n "$BASH_VERSION" &&
            test "${FUNCNAME[1]}" = "test_eval_inner_" &&
            test "$1" != "-" &&
            test ! -s "$1"
         then
                 error "bug in test script: using test_cmp to check
empty file; use test_must_be_empty instead"
         fi
         $GIT_TEST_CMP "$@"
  }

i.e. to limit the check callsites where 'test_cmp' is called directly
from within a test_expect_{success,failure} block.  This is better,
almost all errors could be converted to 'test_must_be_empty' stright
away; I have the patches almost ready.  There are, however, a few
parametric cases that choke on this: where we run 'test_cmp' in a
loop, e.g. 'cvs update (-p)' in t9400, and I think there was a case
where the 'test_expect_success' block is within a function.


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

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Andrei Rybak]

On 17/08/18 22:09, Junio C Hamano wrote:
> 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 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.

  2. Enable this check via variable, smth like EMPTY_CMP_LINT=1

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Jeff King]

On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:

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

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

If we go that route, we should make sure that test_cmp's documentation
is updated to mention that the two sides are not symmetric (and possibly
update some callers, though I'd be OK leaving ones that aren't broken by
this, and just clean them up over time).

-Peff

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Andrei Rybak]

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:

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

That's what I meant by "will only catch cases exactly like the bogus
test in t5310".

However ...

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

 * both $1 and $2 are empty:
     bogus test:
       needs either fixing generation of both expect and actual
       or switching to test_must_be_empty
   OR
     bogus helper function, as Gábor described above:
       needs to switch to test_cmp_allow_empty
 
 * $1 is non-empty && $2 is empty
   proceeding with test
   test failure from GIT_TEST_CMP
 
 * $1 is empty && $2 is non-empty
   bogus test - needs either switching to test_must_be_empty
   (and after that test_must_be_empty will report failure)
   or fixing generation of expect (and after that test result
   depends on contents).
 
 * both $1 and $2 are non-empty
   proceeding with test
   result depends on contents

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Jeff King]

On Sun, Aug 19, 2018 at 11:37:59PM +0200, Andrei Rybak wrote:

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

Oh, you're right. Somewhere between the screen and my brain the "&&"
became an "||".

I agree that works, and has the advantage that the arguments are treated
symmetrically. We _might_ say "test failure" instead of "bug in test"
when the expectation is empty and the generated output is not (because
we do not know which is which), but I think that would be uncommon (and
the most important thing is that we do not silently consider it a pass).

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

-Peff

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

As I do find [1] to be a real concern, I'd prefer not to flag empty
input to test_cmp as special.  But if we _were_ to do something, I
agree that "$2 can be anything---that is the output from the
potentially buggy program we are testing" is the right attitude to
take.

[Footnote]

*1*
https://public-inbox.org/git/CAM0VKjkT7fBJRie_3f4B13BHT9hp9MxRhuX5r1sogh2x7KQzbg@mail.gmail.com/

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Matthew DeVore]

I would encourage use of an existing function to check for emptiness,
but require a particular argument for it to be considered "the right
way:"

test_cmp /dev/null actual

This means less vocabulary to memorize for test writers. It's usually a
code smell to have special logic for a specific value for a specific
argument - a sign that a separate function ought to be created - but
since we want to add an error or warning in test_cmp anyway when
<EXPECTED> is empty, I think this special logic is OK.

As for comparing against a file that *might* be empty, like a utility
function, might I suggest requiring the file name be formatted in a
specific way if it may be empty? Like require a certain substring. Then
the syntax for comparison would be:

# If the emptiness is unconditional
test_cmp /dev/null actual

# If the emptiness is unknown ahead of time
test_cmp maybe_empty_expected actual

Then, issue an error for something like:
> expected && test_cmp expected actual

which says: "
Use test_cmp /dev/null <ACTUAL> to verify a file is empty.
If the <EXPECTED> file may or may not be empty (as in a utility
function), include the string "maybe_empty" in the <EXPECTED> file name.
"

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Kirill Smelkov]

On Tue, Aug 14, 2018 at 01:47:21PM +0200, SZEDER Gábor wrote:
> 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
>  '

Thanks for catching and correcting this.

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.

Kirill

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Jeff King]

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

-Peff

Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

[Kirill Smelkov]

On Mon, Aug 27, 2018 at 07:04:52PM -0400, Jeff King wrote:
> 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

Thanks for the information. Oh well...