Re: [PATCH] t6041: do not compress backup tar file

From: Stefan Beller <sbeller@google.com>
Date: May 9, 2016, 19:04 UTC
Message-ID: <CAGZ79kZ0gVzb7aqtdFFGt1mNxgNwX+xaWOtHOQt8kB84Yxhwsg@mail.gmail.com>
In-Reply-To: <xmqq8tzjul0p.fsf@gitster.mtv.corp.google.com>

+ cc Jens as he authored both t6041 as well as t3513 in
the series leading to ad25da009e2a3730 (2014-07-21,
Merge branch 'jl/submodule-tests')

On Mon, May 9, 2016 at 11:46 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 33 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
>> index c6b7aa6..62b8a2e 100755
>> --- a/t/t6041-bisect-submodule.sh
>> +++ b/t/t6041-bisect-submodule.sh
>> @@ -8,7 +8,7 @@ test_description='bisect can handle submodules'
>>  git_bisect () {
>>       git status -su >expect &&
>>       ls -1pR * >>expect &&
>> -     tar czf "$TRASH_DIRECTORY/tmp.tgz" * &&
>> +     tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
>>       GOOD=$(git rev-parse --verify HEAD) &&
>>       git checkout "$1" &&
>>       echo "foo" >bar &&
>> @@ -20,7 +20,7 @@ git_bisect () {
>>       git bisect start &&
>>       git bisect good $GOOD &&
>>       rm -rf * &&
>> -     tar xzf "$TRASH_DIRECTORY/tmp.tgz" &&
>> +     tar xf "$TRASH_DIRECTORY/tmp.tar" &&
>>       git status -su >actual &&
>>       ls -1pR * >>actual &&
>>       test_cmp expect actual &&
>
> While I am fine with taking this (and the other) change, which are
> the only sensible response to these bug reports, this makes me
> wonder two things and a half.
>
>  * Why do we even run "tar", especially without having a
>    test_when_finished clean-up?  Can't there be better ways to test
>    this and the other one without creating a copy of everything in
>    the test directory?

I think some of the submodule testing is a test machinery on its own. Any submodule test that is not in t74* follows the pattern to provide a short function for its testing and then call test_submodule_switch with some long option, e.g. KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1

I haven't attempted to look into these tests yet as they seem to test fundamentals ("submodules as files" in other commands), whereas I am more interested in some new features currently.

>
>  * Are we sure the trash directory contents is kept manageable size?

The testing in lib-submodule-update.sh do not seem to take care of these tarballs. And the small testing scripts do not either, but there we could inject a

    test_when_finished "rm ..."

snippet IIUC.

Show 10 quoted lines

>    I am primarily worried about this "we are not sure what we will
>    be clobbering, so let's blindly tar up everything and restore it
>    later" pattern spreading and people mistakenly use it in tests
>    that simulate our behaviour on a huge blob with a sparse but
>    gigantic file.
>
>  * Is an addition of 'test_snapshot $tarball *' and 'test_restore
>    $tarball' pair too much unnecessary refactoring to cater to only
>    two users of this "let's tar up everything" pattern, given that
>    we want to _discourage_ use of this pattern in the longer term?

As said before, I did not yet dive into these test areas myself.

>From a birds eye, there was not a lot of discussion around that series

(as compared to submodule groups for example). Maybe I am missing prior or later series though.

http://thread.gmane.org/gmane.comp.version-control.git/251682

Show 5 quoted lines

>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html