threads / patch / 42265

patcht6041: do not compress backup tar file

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

## tl;dr

4 messages between May 9, 2016 and May 9, 2016. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Stefan Beller· May 9, 2016, 17:09 UTC · lore

The test uses the 'z' option, i.e. "compress the output while at it", which is GNUism and not portable.

Reported-by: Armin Kunaschik <megabreit@googlemail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Thanks Armin for reporting these GNUism!
 Are there any more? (So we can do these patches as a
 series instead of one by one:)
 
 developed on top of origin/sb/z-is-gnutar-ism
 
 Thanks,
 Stefan
 t/t6041-bisect-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t6041-bisect-submodule.sh +2 −2
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 &&
-- 
2.8.0.37.g63b3e6f.dirty
Armin Kunaschik· May 9, 2016, 17:20 UTC · re: Stefan Beller · lore

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

Hi Stefan,

I'm currently in the process of skipping through the failed tests on my AIX box. There are more tests which require GNU tools like mktemp (t7610-mergetool.sh) or readlink (t7800-difftool.sh). But I don't have a solution or workaround for these two. But at least there are not more failing compressed tar tests :-)

Thanks, Armin

On Mon, May 9, 2016 at 7:09 PM, Stefan Beller <sbeller@google.com> wrote:
Show 15 quoted lines
> The test uses the 'z' option, i.e. "compress the output while at
> it", which is GNUism and not portable.
>
> Reported-by: Armin Kunaschik <megabreit@googlemail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>  Thanks Armin for reporting these GNUism!
>  Are there any more? (So we can do these patches as a
>  series instead of one by one:)
>
>  developed on top of origin/sb/z-is-gnutar-ism
>
>  Thanks,
>  Stefan
Junio C Hamano· May 9, 2016, 18:46 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 22 quoted lines
> 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?
 * Are we sure the trash directory contents is kept manageable size?
   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?
Stefan Beller· May 9, 2016, 19:04 UTC · re: Junio C Hamano · lore

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

+ 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

← back to recent threads