[PATCH] p5310: Fix broken && chain in performance test

[PATCH] p5310: Fix broken && chain in performance test

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/perf/p5310-pack-bitmaps.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
index f8ed857..de2a224 100755
--- a/t/perf/p5310-pack-bitmaps.sh
+++ b/t/perf/p5310-pack-bitmaps.sh
@@ -39,14 +39,14 @@ test_expect_success 'create partial bitmap state' '
 
 	# now kill off all of the refs and pretend we had
 	# just the one tip
-	rm -rf .git/logs .git/refs/* .git/packed-refs
-	git update-ref HEAD $cutoff
+	rm -rf .git/logs .git/refs/* .git/packed-refs &&
+	git update-ref HEAD $cutoff &&
 
 	# and then repack, which will leave us with a nice
 	# big bitmap pack of the "old" history, and all of
 	# the new history will be loose, as if it had been pushed
 	# up incrementally and exploded via unpack-objects
-	git repack -Ad
+	git repack -Ad &&
 
 	# and now restore our original tip, as if the pushes
 	# had happened
-- 
2.4.1.345.gab207b6.dirty

Re: [PATCH] p5310: Fix broken && chain in performance test

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Signed-off-by: Stefan Beller <sbeller@google.com>

Thanks.  How did you find this (does the auto &&-chain test apply to
t/perf stuff as well)?

Will queue.

> ---
>  t/perf/p5310-pack-bitmaps.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
> index f8ed857..de2a224 100755
> --- a/t/perf/p5310-pack-bitmaps.sh
> +++ b/t/perf/p5310-pack-bitmaps.sh
> @@ -39,14 +39,14 @@ test_expect_success 'create partial bitmap state' '
>  
>  	# now kill off all of the refs and pretend we had
>  	# just the one tip
> -	rm -rf .git/logs .git/refs/* .git/packed-refs
> -	git update-ref HEAD $cutoff
> +	rm -rf .git/logs .git/refs/* .git/packed-refs &&
> +	git update-ref HEAD $cutoff &&
>  
>  	# and then repack, which will leave us with a nice
>  	# big bitmap pack of the "old" history, and all of
>  	# the new history will be loose, as if it had been pushed
>  	# up incrementally and exploded via unpack-objects
> -	git repack -Ad
> +	git repack -Ad &&
>  
>  	# and now restore our original tip, as if the pushes
>  	# had happened

Re: [PATCH] p5310: Fix broken && chain in performance test

[Stefan Beller]

On Fri, Jun 26, 2015 at 3:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>
> Thanks.  How did you find this (does the auto &&-chain test apply to
> t/perf stuff as well)?

Apparently the &&-chain tests for it as I got a warning for it while
benchmarking some changes in ALLOC_GROW. (which originally
should have fixed the coverity false positives, but I was side tracked
wondering about performance)

>
> Will queue.
>
>> ---
>>  t/perf/p5310-pack-bitmaps.sh | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
>> index f8ed857..de2a224 100755
>> --- a/t/perf/p5310-pack-bitmaps.sh
>> +++ b/t/perf/p5310-pack-bitmaps.sh
>> @@ -39,14 +39,14 @@ test_expect_success 'create partial bitmap state' '
>>
>>       # now kill off all of the refs and pretend we had
>>       # just the one tip
>> -     rm -rf .git/logs .git/refs/* .git/packed-refs
>> -     git update-ref HEAD $cutoff
>> +     rm -rf .git/logs .git/refs/* .git/packed-refs &&
>> +     git update-ref HEAD $cutoff &&
>>
>>       # and then repack, which will leave us with a nice
>>       # big bitmap pack of the "old" history, and all of
>>       # the new history will be loose, as if it had been pushed
>>       # up incrementally and exploded via unpack-objects
>> -     git repack -Ad
>> +     git repack -Ad &&
>>
>>       # and now restore our original tip, as if the pushes
>>       # had happened

Re: [PATCH] p5310: Fix broken && chain in performance test

[Jeff King]

On Fri, Jun 26, 2015 at 03:34:19PM -0700, Stefan Beller wrote:

> > Thanks.  How did you find this (does the auto &&-chain test apply to
> > t/perf stuff as well)?
> 
> Apparently the &&-chain tests for it as I got a warning for it while
> benchmarking some changes in ALLOC_GROW. (which originally
> should have fixed the coverity false positives, but I was side tracked
> wondering about performance)

That makes sense; we use test_expect_success here, so I think it is good
for it to check that we are actually creating a sane exit status.

It looks like we do not extend the same protection to test_perf (it uses
test_eval_, bnot test_run_). That is probably OK, as those tests are
more about measuring the time than about correctness. OTOH, we do notice
when a test_perf reports failure, so perhaps it would make sense to
extend &&-chaining checks there.

-Peff