threads / patch / 7552

patcht5300-pack-object.sh: portability issue using /usr/bin/stat

Subject: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat

## tl;dr

6 messages between Apr 6, 2007 and Apr 7, 2007. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Arjen Laarhoven· Apr 6, 2007, 23:49 UTC · lore

In the test 'compare delta flavors', /usr/bin/stat is used to get file size. This isn't portable. There already is a dependency on Perl, use its '-s' operator to get the file size.

Signed-off-by: Arjen Laarhoven <arjen@yaph.org>
---
 t/t5300-pack-object.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t5300-pack-object.sh +2 −2
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 35e036a..a400e7a 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -125,8 +125,8 @@ cd "$TRASH"
 
 test_expect_success \
     'compare delta flavors' \
-    'size_2=`stat -c "%s" test-2-${packname_2}.pack` &&
-     size_3=`stat -c "%s" test-3-${packname_3}.pack` &&
+    'size_2=`perl -e "print -s q[test-2-${packname_2}.pack]"` &&
+     size_3=`perl -e "print -s q[test-3-${packname_3}.pack]"` &&
      test $size_2 -gt $size_3'
 
 rm -fr .git2
-- 
1.5.1.rc3.29.gd8b6
Junio C Hamano· Apr 7, 2007, 02:08 UTC · re: Arjen Laarhoven · lore

Re: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat

arjen@yaph.org (Arjen Laarhoven) writes:
> In the test 'compare delta flavors', /usr/bin/stat is used to get file size.
> This isn't portable.  There already is a dependency on Perl, use its '-s'
> operator to get the file size.

If you do use Perl, then you do not want to do it as two separate invocations with their result compared with test.

How about this on top of your patch?
Show changes to t/t5300-pack-object.sh +7 −5
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index a400e7a..5710a23 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -123,11 +123,13 @@ test_expect_success \
      done'
 cd "$TRASH"
 
-test_expect_success \
-    'compare delta flavors' \
-    'size_2=`perl -e "print -s q[test-2-${packname_2}.pack]"` &&
-     size_3=`perl -e "print -s q[test-3-${packname_3}.pack]"` &&
-     test $size_2 -gt $size_3'
+test_expect_success 'compare delta flavors' '
+	perl -e "
+		exit ( ((-s q[test-2-${packname_2}.pack]) >
+			(-s q[test-3-${packname_3}.pack]))
+			? 0 : 1);
+	"
+'
 
 rm -fr .git2
 mkdir .git2
Nicolas Pitre· Apr 7, 2007, 02:33 UTC · re: Junio C Hamano · lore

Re: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat

On Fri, 6 Apr 2007, Junio C Hamano wrote:
Show 10 quoted lines
> arjen@yaph.org (Arjen Laarhoven) writes:
> 
> > In the test 'compare delta flavors', /usr/bin/stat is used to get file size.
> > This isn't portable.  There already is a dependency on Perl, use its '-s'
> > operator to get the file size.
> 
> If you do use Perl, then you do not want to do it as two
> separate invocations with their result compared with test.
> 
> How about this on top of your patch?

Well... since this test already depends on wc then why not just use that instead of adding a perl dependency?

Something like:
Show changes to t/t5300-pack-object.sh +2 −2
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index 35e036a..ba785cf 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -125,8 +125,8 @@ cd "$TRASH"
 
 test_expect_success \
     'compare delta flavors' \
-    'size_2=`stat -c "%s" test-2-${packname_2}.pack` &&
-     size_3=`stat -c "%s" test-3-${packname_3}.pack` &&
+    'size_2=`wc -c < test-2-${packname_2}.pack` &&
+     size_3=`wc -c < test-3-${packname_3}.pack` &&
      test $size_2 -gt $size_3'
 
 rm -fr .git2
Junio C Hamano· Apr 7, 2007, 04:13 UTC · re: Nicolas Pitre · lore

Re: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat

Nicolas Pitre <nico@cam.org> writes:
Show 15 quoted lines
> On Fri, 6 Apr 2007, Junio C Hamano wrote:
>
>> arjen@yaph.org (Arjen Laarhoven) writes:
>> 
>> > In the test 'compare delta flavors', /usr/bin/stat is used to get file size.
>> > This isn't portable.  There already is a dependency on Perl, use its '-s'
>> > operator to get the file size.
>> 
>> If you do use Perl, then you do not want to do it as two
>> separate invocations with their result compared with test.
>> 
>> How about this on top of your patch?
>
> Well... since this test already depends on wc then why not just use that 
> instead of adding a perl dependency?

Because (1) other tests already use Perl; (2) wc -c reads pack to find out the size, "-s $file" doesn't AFAIK.

Nicolas Pitre· Apr 7, 2007, 12:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat

On Fri, 6 Apr 2007, Junio C Hamano wrote:
Show 9 quoted lines
> Nicolas Pitre <nico@cam.org> writes:
> 
> > On Fri, 6 Apr 2007, Junio C Hamano wrote:
> >
> > Well... since this test already depends on wc then why not just use that 
> > instead of adding a perl dependency?
> 
> Because (1) other tests already use Perl; (2) wc -c reads pack
> to find out the size, "-s $file" doesn't AFAIK.

Maybe. But my point is that wc is already used to find file size in other part of the test. So it should at least be consistent. And my patch has the advantage of looking much simpler.

Nicolas
Randal L. Schwartz· Apr 7, 2007, 02:45 UTC · re: Junio C Hamano · lore

Re: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat

>>>>> "Junio" == Junio C Hamano <junkio@cox.net> writes:
Junio> arjen@yaph.org (Arjen Laarhoven) writes:
>> In the test 'compare delta flavors', /usr/bin/stat is used to get file size.
>> This isn't portable.  There already is a dependency on Perl, use its '-s'
>> operator to get the file size.

Junio> If you do use Perl, then you do not want to do it as two Junio> separate invocations with their result compared with test.

Junio> How about this on top of your patch?
Junio> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
Junio> index a400e7a..5710a23 100755
Junio> --- a/t/t5300-pack-object.sh
Junio> +++ b/t/t5300-pack-object.sh
Junio> @@ -123,11 +123,13 @@ test_expect_success \
Junio>       done'
Junio>  cd "$TRASH"
 
Junio> -test_expect_success \
Junio> -    'compare delta flavors' \
Junio> -    'size_2=`perl -e "print -s q[test-2-${packname_2}.pack]"` &&
Junio> -     size_3=`perl -e "print -s q[test-3-${packname_3}.pack]"` &&
Junio> -     test $size_2 -gt $size_3'
Junio> +test_expect_success 'compare delta flavors' '
Junio> +	perl -e "
Junio> +		exit ( ((-s q[test-2-${packname_2}.pack]) >
Junio> +			(-s q[test-3-${packname_3}.pack]))
Junio> +			? 0 : 1);
Junio> +	"
Junio> +'
I'd go with:
    perl -e '
      defined($_ = -s $_) or die for @ARGV;
      exit 1 if $ARGV[0] <= $ARGV[1];
    ' test-2-$packname_2.pack test-3.$packname_3.pack

which also tests to make sure the -s returned something, and works a lot less hard to quote the filenames coming in (they come in via @ARGV instead of triple interpolation). I'm not sure how to shoehorn that into test_expect_success, but this is better Perl at least. :)

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

← back to recent threads