{"thread":{"id":"7552","subject":"[PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat","startedAt":"2007-04-06T23:49:03Z","lastAt":"2007-04-07T12:39:12Z","messageCount":6,"participants":["Arjen Laarhoven","Junio C Hamano","Nicolas Pitre","Randal L. Schwartz"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"38823","messageId":"20070406234903.GJ3854@regex.yaph.org","threadId":"7552","inReplyTo":null,"subject":"[PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat","fromName":"Arjen Laarhoven","fromEmail":"arjen@yaph.org","sentAt":"2007-04-06T23:49:03Z","receivedAt":"2007-04-06T23:49:03Z","isPatch":true,"sender":{"key":"arjen@yaph.org","avatar":"https://gravatar.com/avatar/f776c2c0c5ea62d70827b942eb7d95ce85661a3d70bc3f03cf9773815c599c01?d=mp&s=160"},"body":"In the test 'compare delta flavors', /usr/bin/stat is used to get file size.\nThis isn't portable.  There already is a dependency on Perl, use its '-s'\noperator to get the file size.\n\nSigned-off-by: Arjen Laarhoven <arjen@yaph.org>\n---\n t/t5300-pack-object.sh |    4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 35e036a..a400e7a 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -125,8 +125,8 @@ cd \"$TRASH\"\n \n test_expect_success \\\n     'compare delta flavors' \\\n-    'size_2=`stat -c \"%s\" test-2-${packname_2}.pack` &&\n-     size_3=`stat -c \"%s\" test-3-${packname_3}.pack` &&\n+    'size_2=`perl -e \"print -s q[test-2-${packname_2}.pack]\"` &&\n+     size_3=`perl -e \"print -s q[test-3-${packname_3}.pack]\"` &&\n      test $size_2 -gt $size_3'\n \n rm -fr .git2\n-- \n1.5.1.rc3.29.gd8b6\n"},{"id":"38832","messageId":"7vfy7dgcn1.fsf@assigned-by-dhcp.cox.net","threadId":"7552","inReplyTo":"20070406234903.GJ3854@regex.yaph.org","subject":"Re: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-07T02:08:02Z","receivedAt":"2007-04-07T02:08:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"arjen@yaph.org (Arjen Laarhoven) writes:\n\n> In the test 'compare delta flavors', /usr/bin/stat is used to get file size.\n> This isn't portable.  There already is a dependency on Perl, use its '-s'\n> operator to get the file size.\n\nIf you do use Perl, then you do not want to do it as two\nseparate invocations with their result compared with test.\n\nHow about this on top of your patch?\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex a400e7a..5710a23 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -123,11 +123,13 @@ test_expect_success \\\n      done'\n cd \"$TRASH\"\n \n-test_expect_success \\\n-    'compare delta flavors' \\\n-    'size_2=`perl -e \"print -s q[test-2-${packname_2}.pack]\"` &&\n-     size_3=`perl -e \"print -s q[test-3-${packname_3}.pack]\"` &&\n-     test $size_2 -gt $size_3'\n+test_expect_success 'compare delta flavors' '\n+\tperl -e \"\n+\t\texit ( ((-s q[test-2-${packname_2}.pack]) >\n+\t\t\t(-s q[test-3-${packname_3}.pack]))\n+\t\t\t? 0 : 1);\n+\t\"\n+'\n \n rm -fr .git2\n mkdir .git2\n"},{"id":"38834","messageId":"alpine.LFD.0.98.0704062227430.28181@xanadu.home","threadId":"7552","inReplyTo":"7vfy7dgcn1.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-07T02:33:34Z","receivedAt":"2007-04-07T02:33:34Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 6 Apr 2007, Junio C Hamano wrote:\n\n> arjen@yaph.org (Arjen Laarhoven) writes:\n> \n> > In the test 'compare delta flavors', /usr/bin/stat is used to get file size.\n> > This isn't portable.  There already is a dependency on Perl, use its '-s'\n> > operator to get the file size.\n> \n> If you do use Perl, then you do not want to do it as two\n> separate invocations with their result compared with test.\n> \n> How about this on top of your patch?\n\nWell... since this test already depends on wc then why not just use that \ninstead of adding a perl dependency?\n\nSomething like:\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 35e036a..ba785cf 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -125,8 +125,8 @@ cd \"$TRASH\"\n \n test_expect_success \\\n     'compare delta flavors' \\\n-    'size_2=`stat -c \"%s\" test-2-${packname_2}.pack` &&\n-     size_3=`stat -c \"%s\" test-3-${packname_3}.pack` &&\n+    'size_2=`wc -c < test-2-${packname_2}.pack` &&\n+     size_3=`wc -c < test-3-${packname_3}.pack` &&\n      test $size_2 -gt $size_3'\n \n rm -fr .git2\n"},{"id":"38835","messageId":"86odm0sy19.fsf@blue.stonehenge.com","threadId":"7552","inReplyTo":"7vfy7dgcn1.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat","fromName":"Randal L. Schwartz","fromEmail":"merlyn@stonehenge.com","sentAt":"2007-04-07T02:45:06Z","receivedAt":"2007-04-07T02:45:06Z","isPatch":true,"sender":{"key":"merlyn@stonehenge.com","avatar":"https://gravatar.com/avatar/dc528d210743ff0333e6213f9ee7b33b23f1b7bc1f3c5a8c2d819074ecd7ab19?d=mp&s=160"},"body":">>>>> \"Junio\" == Junio C Hamano <junkio@cox.net> writes:\n\nJunio> arjen@yaph.org (Arjen Laarhoven) writes:\n>> In the test 'compare delta flavors', /usr/bin/stat is used to get file size.\n>> This isn't portable.  There already is a dependency on Perl, use its '-s'\n>> operator to get the file size.\n\nJunio> If you do use Perl, then you do not want to do it as two\nJunio> separate invocations with their result compared with test.\n\nJunio> How about this on top of your patch?\n\nJunio> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nJunio> index a400e7a..5710a23 100755\nJunio> --- a/t/t5300-pack-object.sh\nJunio> +++ b/t/t5300-pack-object.sh\nJunio> @@ -123,11 +123,13 @@ test_expect_success \\\nJunio>       done'\nJunio>  cd \"$TRASH\"\n \nJunio> -test_expect_success \\\nJunio> -    'compare delta flavors' \\\nJunio> -    'size_2=`perl -e \"print -s q[test-2-${packname_2}.pack]\"` &&\nJunio> -     size_3=`perl -e \"print -s q[test-3-${packname_3}.pack]\"` &&\nJunio> -     test $size_2 -gt $size_3'\nJunio> +test_expect_success 'compare delta flavors' '\nJunio> +\tperl -e \"\nJunio> +\t\texit ( ((-s q[test-2-${packname_2}.pack]) >\nJunio> +\t\t\t(-s q[test-3-${packname_3}.pack]))\nJunio> +\t\t\t? 0 : 1);\nJunio> +\t\"\nJunio> +'\n\nI'd go with:\n\n    perl -e '\n      defined($_ = -s $_) or die for @ARGV;\n      exit 1 if $ARGV[0] <= $ARGV[1];\n    ' test-2-$packname_2.pack test-3.$packname_3.pack\n\nwhich also tests to make sure the -s returned something, and works a lot less\nhard to quote the filenames coming in (they come in via @ARGV instead of\ntriple interpolation).  I'm not sure how to shoehorn that into\ntest_expect_success, but this is better Perl at least. :)\n\n-- \nRandal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095\n<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>\nPerl/Unix/security consulting, Technical writing, Comedy, etc. etc.\nSee PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!\n"},{"id":"38838","messageId":"7vabxkhleh.fsf@assigned-by-dhcp.cox.net","threadId":"7552","inReplyTo":"alpine.LFD.0.98.0704062227430.28181@xanadu.home","subject":"Re: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-07T04:13:26Z","receivedAt":"2007-04-07T04:13:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> On Fri, 6 Apr 2007, Junio C Hamano wrote:\n>\n>> arjen@yaph.org (Arjen Laarhoven) writes:\n>> \n>> > In the test 'compare delta flavors', /usr/bin/stat is used to get file size.\n>> > This isn't portable.  There already is a dependency on Perl, use its '-s'\n>> > operator to get the file size.\n>> \n>> If you do use Perl, then you do not want to do it as two\n>> separate invocations with their result compared with test.\n>> \n>> How about this on top of your patch?\n>\n> Well... since this test already depends on wc then why not just use that \n> instead of adding a perl dependency?\n\nBecause (1) other tests already use Perl; (2) wc -c reads pack\nto find out the size, \"-s $file\" doesn't AFAIK.\n"},{"id":"38842","messageId":"alpine.LFD.0.98.0704070831140.28181@xanadu.home","threadId":"7552","inReplyTo":"7vabxkhleh.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] t5300-pack-object.sh: portability issue using /usr/bin/stat","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-07T12:39:12Z","receivedAt":"2007-04-07T12:39:12Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 6 Apr 2007, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@cam.org> writes:\n> \n> > On Fri, 6 Apr 2007, Junio C Hamano wrote:\n> >\n> > Well... since this test already depends on wc then why not just use that \n> > instead of adding a perl dependency?\n> \n> Because (1) other tests already use Perl; (2) wc -c reads pack\n> to find out the size, \"-s $file\" doesn't AFAIK.\n\nMaybe.  But my point is that wc is already used to find file size in \nother part of the test.  So it should at least be consistent.  And my \npatch has the advantage of looking much simpler.\n\n\nNicolas\n"}]}