t5319: don't trip over a user name with whitespace
diff mbox series

Message ID d4a3f56b-5eaa-1325-f8c1-be6797a9ac03@kdbg.org
State New
Headers show
Series
  • t5319: don't trip over a user name with whitespace
Related show

Commit Message

Johannes Sixt June 30, 2019, 6:57 p.m. UTC
On my Windows system where the POSIX commands are provided by MSYS2,
I observe this output:

$ ls -l Makefile
-rw-r--r-- 1 Johannes Sixt 197121 101780 Jun 30 09:33 Makefile

Notice the blank in the user name. Obviously, extracting the size
of a file by counting columns won't work. But two tests in t5319
do that. Use the stat command to print just the file size.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Notice further that the group ID is presented numerically. The
actual output used in the tests is:

-r--r--r-- 1 Johannes Sixt 197121 2239 Jan  1 01:03 .git/objects/pack/pack-A-40289997979ee9f2d7198a5b4ac9fe14e27cbe4b.pack
-r--r--r-- 1 Johannes Sixt 197121 1385 Jan  1 01:02 .git/objects/pack/pack-B-926e20b0e641003b2f6bcddd6ccf53e7741c8642.pack
-r--r--r-- 1 Johannes Sixt 197121  838 Jan  1 01:01 .git/objects/pack/pack-C-b33c048d7202659249bb0cea7bbc997ce448c75d.pack
-r--r--r-- 1 Johannes Sixt 197121  560 Jan  1 01:00 .git/objects/pack/pack-D-49ca36507a52bbba6d1768e28666eb0f4e5f7d95.pack
-r--r--r-- 1 Johannes Sixt 197121 4587 Jun 30 16:17 .git/objects/pack/pack-E-31ea0966bb15206fbfd1b4c2f5be4b24a03050f5.pack

In both tests, this group ID was passed as --batch-size, but actually
the first of the two tests does not fail.

Now I have to wonder: Why that? The test carefully attempts to extract
the smallest pack size to pass as --batch-size argument. But when it
passes some large number, the test still passes. Is this the intent?

That is, both a small and a large --batch-size satisfy 'does not alter
existing packs', but some medium sized --batch-size satisfy the next
test 'repack creates a new pack'. That is very curious.

Disclaimer: I've no clue about this multi-pack-index thing.

 t/t5319-multi-pack-index.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Eric Sunshine June 30, 2019, 7:48 p.m. UTC | #1
On Sun, Jun 30, 2019 at 2:57 PM Johannes Sixt <j6t@kdbg.org> wrote:
> On my Windows system where the POSIX commands are provided by MSYS2,
> I observe this output:
>
> $ ls -l Makefile
> -rw-r--r-- 1 Johannes Sixt 197121 101780 Jun 30 09:33 Makefile
>
> Notice the blank in the user name. Obviously, extracting the size
> of a file by counting columns won't work. But two tests in t5319
> do that. Use the stat command to print just the file size.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> @@ -443,7 +443,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
> -               MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
> +               MINSIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 1) &&

Unfortunately, this is not portable. While "stat -c %s" works on Linux
and MSYS2, neither that option nor the format directive are recognized
on BSD-like platforms (I tested Mac OS and FreeBSD), which instead
need "stat -f %z".
Johannes Sixt June 30, 2019, 8:59 p.m. UTC | #2
Am 30.06.19 um 21:48 schrieb Eric Sunshine:
> On Sun, Jun 30, 2019 at 2:57 PM Johannes Sixt <j6t@kdbg.org> wrote:
>> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
>> @@ -443,7 +443,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
>> -               MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
>> +               MINSIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 1) &&
> 
> Unfortunately, this is not portable. While "stat -c %s" works on Linux
> and MSYS2, neither that option nor the format directive are recognized
> on BSD-like platforms (I tested Mac OS and FreeBSD), which instead
> need "stat -f %z".

Ouch! I did notice that stat(1) is not in POSIX, but hoped that it was
sufficiently portable. I need a new idea...

-- Hannes
Jeff King June 30, 2019, 10:25 p.m. UTC | #3
On Sun, Jun 30, 2019 at 10:59:34PM +0200, Johannes Sixt wrote:

> Am 30.06.19 um 21:48 schrieb Eric Sunshine:
> > On Sun, Jun 30, 2019 at 2:57 PM Johannes Sixt <j6t@kdbg.org> wrote:
> >> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> >> @@ -443,7 +443,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
> >> -               MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
> >> +               MINSIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 1) &&
> > 
> > Unfortunately, this is not portable. While "stat -c %s" works on Linux
> > and MSYS2, neither that option nor the format directive are recognized
> > on BSD-like platforms (I tested Mac OS and FreeBSD), which instead
> > need "stat -f %z".
> 
> Ouch! I did notice that stat(1) is not in POSIX, but hoped that it was
> sufficiently portable. I need a new idea...

If we are OK relying on rudimentary perl[1], then:

  perl -le "print((stat)[7]) for @ARGV"

works. If you want it more readable, then maybe:

  perl -MFile::stat -le "print stat(\$_)->size for @ARGV"

Both of those should work with even antique perl versions.

-Peff

[1] I'd also be fine with implementing a test-tool helper in C that
    behaves like stat(1). Or we could punt on that until somebody feels
    like trying to eradicate perl (because this is far from the first
    perl one-liner in the test suite).
Johannes Sixt July 1, 2019, 6:33 a.m. UTC | #4
Am 01.07.19 um 00:25 schrieb Jeff King:
> On Sun, Jun 30, 2019 at 10:59:34PM +0200, Johannes Sixt wrote:
> 
>> Am 30.06.19 um 21:48 schrieb Eric Sunshine:
>>> On Sun, Jun 30, 2019 at 2:57 PM Johannes Sixt <j6t@kdbg.org> wrote:
>>>> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
>>>> @@ -443,7 +443,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
>>>> -               MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
>>>> +               MINSIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 1) &&
>>>
>>> Unfortunately, this is not portable. While "stat -c %s" works on Linux
>>> and MSYS2, neither that option nor the format directive are recognized
>>> on BSD-like platforms (I tested Mac OS and FreeBSD), which instead
>>> need "stat -f %z".
>>
>> Ouch! I did notice that stat(1) is not in POSIX, but hoped that it was
>> sufficiently portable. I need a new idea...
> 
> If we are OK relying on rudimentary perl[1], then:
> 
>   perl -le "print((stat)[7]) for @ARGV"

I'm fine with that. But then we should do the sort + head -n 1 at the
same time. I can do that with a small script, but I'm sure it's possible
in a one-liner...

-- Hannes
SZEDER Gábor July 1, 2019, 8:36 a.m. UTC | #5
On Sun, Jun 30, 2019 at 10:59:34PM +0200, Johannes Sixt wrote:
> Am 30.06.19 um 21:48 schrieb Eric Sunshine:
> > On Sun, Jun 30, 2019 at 2:57 PM Johannes Sixt <j6t@kdbg.org> wrote:
> >> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> >> @@ -443,7 +443,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
> >> -               MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
> >> +               MINSIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 1) &&
> > 
> > Unfortunately, this is not portable. While "stat -c %s" works on Linux
> > and MSYS2, neither that option nor the format directive are recognized
> > on BSD-like platforms (I tested Mac OS and FreeBSD), which instead
> > need "stat -f %z".
> 
> Ouch! I did notice that stat(1) is not in POSIX, but hoped that it was
> sufficiently portable. I need a new idea...

'wc -c' perhaps?  We already use it in a couple of places in the test
suite to get file size.

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 79bfaeafa9..ddba862114 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -443,7 +443,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
 		touch -m -t 201901010002 .git/objects/pack/pack-B* &&
 		touch -m -t 201901010003 .git/objects/pack/pack-A* &&
 		ls .git/objects/pack >expect &&
-		MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
+		MINSIZE=$(wc -c .git/objects/pack/*pack | sort -n | sed -n -e "s/^ *//" -e "1 s/[^ ]*$//p") &&
 		git multi-pack-index repack --batch-size=$MINSIZE &&
 		ls .git/objects/pack >actual &&
 		test_cmp expect actual
@@ -455,7 +455,7 @@ test_expect_success 'repack creates a new pack' '
 		cd dup &&
 		ls .git/objects/pack/*idx >idx-list &&
 		test_line_count = 5 idx-list &&
-		THIRD_SMALLEST_SIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 3 | tail -n 1) &&
+		THIRD_SMALLEST_SIZE=$(wc -c .git/objects/pack/*pack | sort -n | sed -n -e "s/^ *//" -e "3 s/[^ ]*$//p") &&
 		BATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&
 		git multi-pack-index repack --batch-size=$BATCH_SIZE &&
 		ls .git/objects/pack/*idx >idx-list &&

Note that 'wc -c' prints the numbers in its output left padded, hence
the 'sed "s/^ *//"' above, and once 'sed' is already up and running,
it might as well take over the role of that 'head -n 3 | tail -n 1'.
Jeff King July 1, 2019, 9:16 a.m. UTC | #6
On Mon, Jul 01, 2019 at 08:33:18AM +0200, Johannes Sixt wrote:

> > If we are OK relying on rudimentary perl[1], then:
> > 
> >   perl -le "print((stat)[7]) for @ARGV"
> 
> I'm fine with that. But then we should do the sort + head -n 1 at the
> same time. I can do that with a small script, but I'm sure it's possible
> in a one-liner...

Something like:

  perl -le 'print((sort { $a <=> $b } map { (stat)[7] } @ARGV)[0])'

but that is getting pretty unreadable. If we rely on List::Util, it's a
bit nicer:

  perl -MList::Util=min -e 'print min(map { (stat)[7] } @ARGV)'

but that implies perl v5.7.3. Which is from 2002, and older than our
usual minimum-perl version, but we've typically been very conservative
with these one-liners, since they do not respect NO_PERL.

Probably writing it out like:

  perl -le '
    my @sizes = map { (stat)[7] } @ARGV;
    @sizes = sort { $a <=> $b } @sizes;
    print $size[0];
  '

would be better (and makes "3rd-smallest" a trivial change).

I see Gábor suggested using "wc -c" elsewhere in the thread. That would
be fine with me, too, though I think the required sed there may be
getting pretty unreadable, too. :)

-Peff
SZEDER Gábor July 1, 2019, 11:33 a.m. UTC | #7
On Mon, Jul 01, 2019 at 05:16:02AM -0400, Jeff King wrote:
> I see Gábor suggested using "wc -c" elsewhere in the thread. That would
> be fine with me, too, though I think the required sed there may be
> getting pretty unreadable, too. :)

It could be done even without 'sed', though at the expense of running
a coupe more 'wc -c's in a loop:

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 79bfaeafa9..bacec5e2e4 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -443,7 +443,12 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
 		touch -m -t 201901010002 .git/objects/pack/pack-B* &&
 		touch -m -t 201901010003 .git/objects/pack/pack-A* &&
 		ls .git/objects/pack >expect &&
-		MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
+		MINSIZE=$(
+			for pack in .git/objects/pack/*pack
+			do
+				wc -c <"$pack"
+			done | sort -n | head -n 1
+		) &&
 		git multi-pack-index repack --batch-size=$MINSIZE &&
 		ls .git/objects/pack >actual &&
 		test_cmp expect actual
@@ -455,7 +460,12 @@ test_expect_success 'repack creates a new pack' '
 		cd dup &&
 		ls .git/objects/pack/*idx >idx-list &&
 		test_line_count = 5 idx-list &&
-		THIRD_SMALLEST_SIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 3 | tail -n 1) &&
+		THIRD_SMALLEST_SIZE=$(
+			for pack in .git/objects/pack/*pack
+			do
+				wc -c <"$pack"
+			done | sort -n | head -n 3 | tail -n 1
+		) &&
 		BATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&
 		git multi-pack-index repack --batch-size=$BATCH_SIZE &&
 		ls .git/objects/pack/*idx >idx-list &&

Is it really better?  Dunno, but at least there is no subtlety with
the leading padding spaces.
Derrick Stolee July 1, 2019, 12:03 p.m. UTC | #8
Sorry I'm late to the thread. Thanks for reporting this issue, Johannes.

On 7/1/2019 7:33 AM, SZEDER Gábor wrote:
> On Mon, Jul 01, 2019 at 05:16:02AM -0400, Jeff King wrote:
>> I see Gábor suggested using "wc -c" elsewhere in the thread. That would
>> be fine with me, too, though I think the required sed there may be
>> getting pretty unreadable, too. :)
> 
> It could be done even without 'sed', though at the expense of running
> a coupe more 'wc -c's in a loop:>
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 79bfaeafa9..bacec5e2e4 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -443,7 +443,12 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
>  		touch -m -t 201901010002 .git/objects/pack/pack-B* &&
>  		touch -m -t 201901010003 .git/objects/pack/pack-A* &&
>  		ls .git/objects/pack >expect &&
> -		MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
> +		MINSIZE=$(
> +			for pack in .git/objects/pack/*pack
> +			do
> +				wc -c <"$pack"
> +			done | sort -n | head -n 1
> +		) &&
>  		git multi-pack-index repack --batch-size=$MINSIZE &&
>  		ls .git/objects/pack >actual &&
>  		test_cmp expect actual
> @@ -455,7 +460,12 @@ test_expect_success 'repack creates a new pack' '
>  		cd dup &&
>  		ls .git/objects/pack/*idx >idx-list &&
>  		test_line_count = 5 idx-list &&
> -		THIRD_SMALLEST_SIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 3 | tail -n 1) &&
> +		THIRD_SMALLEST_SIZE=$(
> +			for pack in .git/objects/pack/*pack
> +			do
> +				wc -c <"$pack"
> +			done | sort -n | head -n 3 | tail -n 1
> +		) &&
>  		BATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&
>  		git multi-pack-index repack --batch-size=$BATCH_SIZE &&
>  		ls .git/objects/pack/*idx >idx-list &&
> 
> Is it really better?  Dunno, but at least there is no subtlety with
> the leading padding spaces.

Your change is certainly more straight-forward, and avoids all issues
around Perl compatibility.

It does have the issue of more 'wc' processes, which is a downside. The
count here is not too bad, but if we need to duplicate this pattern elsewhere
we may be better off creating a stat(1) replacement in test-lib.sh.

Thanks,
-Stolee
Johannes Schindelin July 1, 2019, 12:11 p.m. UTC | #9
Hi Peff,

On Sun, 30 Jun 2019, Jeff King wrote:

> On Sun, Jun 30, 2019 at 10:59:34PM +0200, Johannes Sixt wrote:
>
> > Am 30.06.19 um 21:48 schrieb Eric Sunshine:
> > > On Sun, Jun 30, 2019 at 2:57 PM Johannes Sixt <j6t@kdbg.org> wrote:
> > >> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> > >> @@ -443,7 +443,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
> > >> -               MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
> > >> +               MINSIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 1) &&
> > >
> > > Unfortunately, this is not portable. While "stat -c %s" works on Linux
> > > and MSYS2, neither that option nor the format directive are recognized
> > > on BSD-like platforms (I tested Mac OS and FreeBSD), which instead
> > > need "stat -f %z".
> >
> > Ouch! I did notice that stat(1) is not in POSIX, but hoped that it was
> > sufficiently portable. I need a new idea...
>
> If we are OK relying on rudimentary perl[1], then:
>
>   perl -le "print((stat)[7]) for @ARGV"
>
> works. If you want it more readable, then maybe:
>
>   perl -MFile::stat -le "print stat(\$_)->size for @ARGV"

Or we stop introducing new Perl calls, and use the perfectly fine
`test-tool path-utils file-size` command:

https://github.com/git/git/blob/v2.22.0/t/helper/test-path-utils.c#L302-L312

This solves not only portability problems but also avoids yet another
obstacle into making a `NO_PERL` test suite run really work without Perl.

Ciao,
Dscho
Derrick Stolee July 1, 2019, 12:30 p.m. UTC | #10
On 7/1/2019 8:11 AM, Johannes Schindelin wrote:
> Hi Peff,
> 
> On Sun, 30 Jun 2019, Jeff King wrote:
> 
>> On Sun, Jun 30, 2019 at 10:59:34PM +0200, Johannes Sixt wrote:
>>
>>> Am 30.06.19 um 21:48 schrieb Eric Sunshine:
>>>> On Sun, Jun 30, 2019 at 2:57 PM Johannes Sixt <j6t@kdbg.org> wrote:
>>>>> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
>>>>> @@ -443,7 +443,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
>>>>> -               MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
>>>>> +               MINSIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 1) &&
>>>>
>>>> Unfortunately, this is not portable. While "stat -c %s" works on Linux
>>>> and MSYS2, neither that option nor the format directive are recognized
>>>> on BSD-like platforms (I tested Mac OS and FreeBSD), which instead
>>>> need "stat -f %z".
>>>
>>> Ouch! I did notice that stat(1) is not in POSIX, but hoped that it was
>>> sufficiently portable. I need a new idea...
>>
>> If we are OK relying on rudimentary perl[1], then:
>>
>>   perl -le "print((stat)[7]) for @ARGV"
>>
>> works. If you want it more readable, then maybe:
>>
>>   perl -MFile::stat -le "print stat(\$_)->size for @ARGV"
> 
> Or we stop introducing new Perl calls, and use the perfectly fine
> `test-tool path-utils file-size` command:
> 
> https://github.com/git/git/blob/v2.22.0/t/helper/test-path-utils.c#L302-L312
> 
> This solves not only portability problems but also avoids yet another
> obstacle into making a `NO_PERL` test suite run really work without Perl.
Thanks! This does seem like the best option. Thanks for bringing this to our
attention. Here is a diff, and I'll prepare a full patch:

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index dd6083e61a2..5379e59168a 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -447,7 +447,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
                touch -m -t 201901010002 .git/objects/pack/pack-B* &&
                touch -m -t 201901010003 .git/objects/pack/pack-A* &&
                ls .git/objects/pack >expect &&
-               MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
+               MINSIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 1) &&
                git multi-pack-index repack --batch-size=$MINSIZE &&
                ls .git/objects/pack >actual &&
                test_cmp expect actual
@@ -459,7 +459,7 @@ test_expect_success 'repack creates a new pack' '
                cd dup &&
                ls .git/objects/pack/*idx >idx-list &&
                test_line_count = 5 idx-list &&
-               THIRD_SMALLEST_SIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 3 | tail -n 1) &&
+               THIRD_SMALLEST_SIZE=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&
                BATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&
                git multi-pack-index repack --batch-size=$BATCH_SIZE &&
                ls .git/objects/pack/*idx >idx-list &&

Thanks,
-Stolee
Jeff King July 1, 2019, 12:53 p.m. UTC | #11
On Mon, Jul 01, 2019 at 02:11:43PM +0200, Johannes Schindelin wrote:

> Or we stop introducing new Perl calls, and use the perfectly fine
> `test-tool path-utils file-size` command:
> 
> https://github.com/git/git/blob/v2.22.0/t/helper/test-path-utils.c#L302-L312

Ah, thanks, I missed that we had already added it. Certainly that seems
like the right solution then.

-Peff
Andreas Schwab July 1, 2019, 5:17 p.m. UTC | #12
On Jun 30 2019, Johannes Sixt <j6t@kdbg.org> wrote:

> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 79bfaeafa9..4b4d06a1c8 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -443,7 +443,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
>  		touch -m -t 201901010002 .git/objects/pack/pack-B* &&
>  		touch -m -t 201901010003 .git/objects/pack/pack-A* &&
>  		ls .git/objects/pack >expect &&
> -		MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
> +		MINSIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 1) &&

You could also use ls -n.

Andreas.
Johannes Sixt July 1, 2019, 6:22 p.m. UTC | #13
Am 01.07.19 um 14:30 schrieb Derrick Stolee:
> On 7/1/2019 8:11 AM, Johannes Schindelin wrote:
>> Or we stop introducing new Perl calls, and use the perfectly fine
>> `test-tool path-utils file-size` command:
>>
>> https://github.com/git/git/blob/v2.22.0/t/helper/test-path-utils.c#L302-L312
>>
>> This solves not only portability problems but also avoids yet another
>> obstacle into making a `NO_PERL` test suite run really work without Perl.
> Thanks! This does seem like the best option. Thanks for bringing this to our
> attention. Here is a diff, and I'll prepare a full patch:

Thanks. Please also explain why the first of the two tests does not fail
with a large --batch-size (unless it is obvious for people who know a
bit about multi-pack-index, of course).

-- Hannes
Derrick Stolee July 1, 2019, 6:47 p.m. UTC | #14
On 7/1/2019 2:22 PM, Johannes Sixt wrote:
> Am 01.07.19 um 14:30 schrieb Derrick Stolee:
>> On 7/1/2019 8:11 AM, Johannes Schindelin wrote:
>>> Or we stop introducing new Perl calls, and use the perfectly fine
>>> `test-tool path-utils file-size` command:
>>>
>>> https://github.com/git/git/blob/v2.22.0/t/helper/test-path-utils.c#L302-L312
>>>
>>> This solves not only portability problems but also avoids yet another
>>> obstacle into making a `NO_PERL` test suite run really work without Perl.
>> Thanks! This does seem like the best option. Thanks for bringing this to our
>> attention. Here is a diff, and I'll prepare a full patch:
> 
> Thanks. Please also explain why the first of the two tests does not fail
> with a large --batch-size (unless it is obvious for people who know a
> bit about multi-pack-index, of course).

Sorry about missing that. Here is an explanation:

The --batch-size=X option determines how much data we want to move in
a single operation. Based on this value, we select packs greedily as
follows:

  1. If a pack has size at most X, include it.
  2. If our total pack size is greater than X, stop.

At the end of this process, if we have zero or one packs we do nothing.
That is why a small size does nothing.

Also, if our total pack size is smaller than X, we do nothing. Our
batch is not "full" enough to merit the work. This is why a large size
does nothing.

Thanks,
-Stolee
Junio C Hamano July 1, 2019, 7:24 p.m. UTC | #15
Andreas Schwab <schwab@linux-m68k.org> writes:

> On Jun 30 2019, Johannes Sixt <j6t@kdbg.org> wrote:
>
>> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
>> index 79bfaeafa9..4b4d06a1c8 100755
>> --- a/t/t5319-multi-pack-index.sh
>> +++ b/t/t5319-multi-pack-index.sh
>> @@ -443,7 +443,7 @@ test_expect_success 'repack with minimum size does not alter existing packs' '
>>  		touch -m -t 201901010002 .git/objects/pack/pack-B* &&
>>  		touch -m -t 201901010003 .git/objects/pack/pack-A* &&
>>  		ls .git/objects/pack >expect &&
>> -		MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
>> +		MINSIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 1) &&
>
> You could also use ls -n.

Nice ;-)

Patch
diff mbox series

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 79bfaeafa9..4b4d06a1c8 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -443,7 +443,7 @@  test_expect_success 'repack with minimum size does not alter existing packs' '
 		touch -m -t 201901010002 .git/objects/pack/pack-B* &&
 		touch -m -t 201901010003 .git/objects/pack/pack-A* &&
 		ls .git/objects/pack >expect &&
-		MINSIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 1) &&
+		MINSIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 1) &&
 		git multi-pack-index repack --batch-size=$MINSIZE &&
 		ls .git/objects/pack >actual &&
 		test_cmp expect actual
@@ -455,7 +455,7 @@  test_expect_success 'repack creates a new pack' '
 		cd dup &&
 		ls .git/objects/pack/*idx >idx-list &&
 		test_line_count = 5 idx-list &&
-		THIRD_SMALLEST_SIZE=$(ls -l .git/objects/pack/*pack | awk "{print \$5;}" | sort -n | head -n 3 | tail -n 1) &&
+		THIRD_SMALLEST_SIZE=$(stat -c %s .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&
 		BATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&
 		git multi-pack-index repack --batch-size=$BATCH_SIZE &&
 		ls .git/objects/pack/*idx >idx-list &&