Re: [msysGit] Re: [PATCH v2] t5000, t5003: do not use test_cmp to compare binary files

2014-06-04 Thread Michael Geddes 
I have the problem that the overridden test_cmp crashes on a couple of places 
where it is doing a binary compare, so this is definitely needed.

I actually used cmp -q   in my override as it's the return code that is most 
important.

//.

On Wed, 4 Jun 2014 11:22:56 AM Junio C Hamano wrote:
> Stepan Kasal  writes:
> > test_cmp() is primarily meant to compare text files (and display the
> > difference for debug purposes).
> > 
> > Raw "cmp" is better suited to compare binary files (tar, zip, etc.).
> > 
> > On MinGW, test_cmp is a shell function mingw_test_cmp that tries to
> > read both files into environment, stripping CR characters (introduced
> > in commit 4d715ac0).
> > 
> > This function usually speeds things up, as fork is extremly slow on
> > Windows.  But no wonder that this function is extremely slow and
> > sometimes even crashes when comparing large tar or zip files.
> > 
> > Signed-off-by: Stepan Kasal 
> > ---
> > 
> > Hi Thomas,
> > 
> > On Wed, Jun 04, 2014 at 02:59:44PM +0200, Thomas Braun wrote:
> >> Using test_cmp_bin instead of cmp would result in then four assertions
> >> for comparing arbitrary data
> >> test_cmp
> >> test_i18ncmp
> >> test_cmp_text
> >> test_cmp_bin
> >> where I think the purpose of each function is clear from its name.
> > 
> > [test_cmp_text does not exist (yet)]
> > 
> > OK, I agree, hence this modified version of the patch.
> 
> Yeah, I think the above reasoning is sound.  And I do not think we
> ever need to have test_cmp_text -- our payload and our messages
> compared by tests to make sure our expectations hold are text by
> default.
> 
> Will queue; thanks.

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

Re: [PATCH v2] t5000, t5003: do not use test_cmp to compare binary files

2014-06-04 Thread Junio C Hamano 
Stepan Kasal  writes:

> test_cmp() is primarily meant to compare text files (and display the
> difference for debug purposes).
>
> Raw "cmp" is better suited to compare binary files (tar, zip, etc.).
>
> On MinGW, test_cmp is a shell function mingw_test_cmp that tries to
> read both files into environment, stripping CR characters (introduced
> in commit 4d715ac0).
>
> This function usually speeds things up, as fork is extremly slow on
> Windows.  But no wonder that this function is extremely slow and
> sometimes even crashes when comparing large tar or zip files.
>
> Signed-off-by: Stepan Kasal 
> ---
>
> Hi Thomas,
> On Wed, Jun 04, 2014 at 02:59:44PM +0200, Thomas Braun wrote:
>> Using test_cmp_bin instead of cmp would result in then four assertions
>> for comparing arbitrary data
>> test_cmp
>> test_i18ncmp
>> test_cmp_text
>> test_cmp_bin
>> where I think the purpose of each function is clear from its name.
>
> [test_cmp_text does not exist (yet)]
>
> OK, I agree, hence this modified version of the patch.

Yeah, I think the above reasoning is sound.  And I do not think we
ever need to have test_cmp_text -- our payload and our messages
compared by tests to make sure our expectations hold are text by
default.

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

[PATCH v2] t5000, t5003: do not use test_cmp to compare binary files

2014-06-04 Thread Stepan Kasal 
test_cmp() is primarily meant to compare text files (and display the
difference for debug purposes).

Raw "cmp" is better suited to compare binary files (tar, zip, etc.).

On MinGW, test_cmp is a shell function mingw_test_cmp that tries to
read both files into environment, stripping CR characters (introduced
in commit 4d715ac0).

This function usually speeds things up, as fork is extremly slow on
Windows.  But no wonder that this function is extremely slow and
sometimes even crashes when comparing large tar or zip files.

Signed-off-by: Stepan Kasal 
---

Hi Thomas,
On Wed, Jun 04, 2014 at 02:59:44PM +0200, Thomas Braun wrote:
> Using test_cmp_bin instead of cmp would result in then four assertions
> for comparing arbitrary data
> test_cmp
> test_i18ncmp
> test_cmp_text
> test_cmp_bin
> where I think the purpose of each function is clear from its name.

[test_cmp_text does not exist (yet)]

OK, I agree, hence this modified version of the patch.

Stepan

 t/t5000-tar-tree.sh | 34 +-
 t/t5001-archive-attr.sh |  2 +-
 t/t5003-archive-zip.sh  |  6 +++---
 t/t5004-archive-corner-cases.sh |  2 +-
 t/test-lib-functions.sh |  6 ++
 5 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 1cf0a4e..4efaf8c 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -164,7 +164,7 @@ check_tar with_olde-prefix olde-
 test_expect_success 'git archive on large files' '
 test_config core.bigfilethreshold 1 &&
 git archive HEAD >b3.tar &&
-test_cmp b.tar b3.tar
+test_cmp_bin b.tar b3.tar
 '
 
 test_expect_success \
@@ -173,15 +173,15 @@ test_expect_success \
 
 test_expect_success \
 'git archive vs. the same in a bare repo' \
-'test_cmp b.tar b3.tar'
+'test_cmp_bin b.tar b3.tar'
 
 test_expect_success 'git archive with --output' \
 'git archive --output=b4.tar HEAD &&
-test_cmp b.tar b4.tar'
+test_cmp_bin b.tar b4.tar'
 
 test_expect_success 'git archive --remote' \
 'git archive --remote=. HEAD >b5.tar &&
-test_cmp b.tar b5.tar'
+test_cmp_bin b.tar b5.tar'
 
 test_expect_success \
 'validate file modification time' \
@@ -198,7 +198,7 @@ test_expect_success \
 
 test_expect_success 'git archive with --output, override inferred format' '
git archive --format=tar --output=d4.zip HEAD &&
-   test_cmp b.tar d4.zip
+   test_cmp_bin b.tar d4.zip
 '
 
 test_expect_success \
@@ -244,34 +244,34 @@ test_expect_success 'archive --list shows only enabled 
remote filters' '
 test_expect_success 'invoke tar filter by format' '
git archive --format=tar.foo HEAD >config.tar.foo &&
tr ab ba config.tar &&
-   test_cmp b.tar config.tar &&
+   test_cmp_bin b.tar config.tar &&
git archive --format=bar HEAD >config.bar &&
tr ab ba config.tar &&
-   test_cmp b.tar config.tar
+   test_cmp_bin b.tar config.tar
 '
 
 test_expect_success 'invoke tar filter by extension' '
git archive -o config-implicit.tar.foo HEAD &&
-   test_cmp config.tar.foo config-implicit.tar.foo &&
+   test_cmp_bin config.tar.foo config-implicit.tar.foo &&
git archive -o config-implicit.bar HEAD &&
-   test_cmp config.tar.foo config-implicit.bar
+   test_cmp_bin config.tar.foo config-implicit.bar
 '
 
 test_expect_success 'default output format remains tar' '
git archive -o config-implicit.baz HEAD &&
-   test_cmp b.tar config-implicit.baz
+   test_cmp_bin b.tar config-implicit.baz
 '
 
 test_expect_success 'extension matching requires dot' '
git archive -o config-implicittar.foo HEAD &&
-   test_cmp b.tar config-implicittar.foo
+   test_cmp_bin b.tar config-implicittar.foo
 '
 
 test_expect_success 'only enabled filters are available remotely' '
test_must_fail git archive --remote=. --format=tar.foo HEAD \
>remote.tar.foo &&
git archive --remote=. --format=bar >remote.bar HEAD &&
-   test_cmp remote.bar config.bar
+   test_cmp_bin remote.bar config.bar
 '
 
 test_expect_success GZIP 'git archive --format=tgz' '
@@ -280,27 +280,27 @@ test_expect_success GZIP 'git archive --format=tgz' '
 
 test_expect_success GZIP 'git archive --format=tar.gz' '
git archive --format=tar.gz HEAD >j1.tar.gz &&
-   test_cmp j.tgz j1.tar.gz
+   test_cmp_bin j.tgz j1.tar.gz
 '
 
 test_expect_success GZIP 'infer tgz from .tgz filename' '
git archive --output=j2.tgz HEAD &&
-   test_cmp j.tgz j2.tgz
+   test_cmp_bin j.tgz j2.tgz
 '
 
 test_expect_success GZIP 'infer tgz from .tar.gz filename' '
git archive --output=j3.tar.gz HEAD &&
-   test_cmp j.tgz j3.tar.gz
+   test_cmp_bin j.tgz j3.tar.gz
 '
 
 test_expect_success GZIP 'extract tgz file' '
gzip -d -c j.tar &&
-   test_cmp b.tar j.tar
+   test_cmp_bin b.tar j.tar
 '
 
 test_expect_success GZIP 'remote tar.gz is allowed by default' '