threads / patch / 51411

patchFix test bug with spaces in file owner

Subject: [PATCH 0/1] Fix test bug with spaces in file owner

## tl;dr

4 messages between Jul 1, 2019 and Jul 2, 2019. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Derrick Stolee via GitGitGadget· Jul 1, 2019, 13:16 UTC · lore

Thanks to Johannes Sixt for reporting this bug and Dscho for presenting the correct test-tool to use.

This applies on ds/midx-expire-repack. If we instead want to squash this into that branch, the two hunks will need to be squashed into these commits:

THIRD_SMALLEST_SIZE applies to ce1e4a105b4 (midx: implement midx_repack(), 2019-06-10).

MINSIZE applies to 2af890bb280 (multi-pack-index: prepare 'repack' subcommand, 2019-06-10).

Thanks, -Stolee
Derrick Stolee (1):
  t5319: use 'test-tool path-utils' instead of 'ls -l'
 t/t5319-multi-pack-index.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
base-commit: b526d8cbbb8740faa10411caa757c6586395fcab
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-280%2Fderrickstolee%2Fmidx-test-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-280/derrickstolee/midx-test-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/280
-- 
gitgitgadget
Derrick Stolee via GitGitGadget· Jul 1, 2019, 13:16 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 1/1] t5319: use 'test-tool path-utils' instead of 'ls -l'

From: Derrick Stolee <dstolee@microsoft.com>

Using 'ls -l' and parsing the columns to find file sizes is problematic when the platform could report the owner as a name with spaces. Instead, use the 'test-tool path-utils file-size' command to list only the sizes.

Reported-by: Johannes Sixt <j6t@kdbg.org>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t5319-multi-pack-index.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t5319-multi-pack-index.sh +2 −2
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 79bfaeafa9..c72ca04399 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=$(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
@@ -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=$(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 &&

-- 
gitgitgadget
Johannes Schindelin· Jul 2, 2019, 12:06 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 1/1] t5319: use 'test-tool path-utils' instead of 'ls -l'

Hi Stolee,
On Mon, 1 Jul 2019, Derrick Stolee via GitGitGadget wrote:
Show 6 quoted lines
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Using 'ls -l' and parsing the columns to find file sizes is
> problematic when the platform could report the owner as a name
> with spaces. Instead, use the 'test-tool path-utils file-size'
> command to list only the sizes.
Thank you!
Show 10 quoted lines
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index 79bfaeafa9..c72ca04399 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=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 1) &&
You know, we could also add a `--show-minimum` option...
Show 6 quoted lines
> @@ -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=$(test-tool path-utils file-size .git/objects/pack/*pack | sort -n | head -n 3 | tail -n 1) &&
I guess the `--show-minimum` option is no longer such a smart idea ;-)

Patch looks good, thanks for working on this, Dscho

Show 6 quoted lines
>  		BATCH_SIZE=$(($THIRD_SMALLEST_SIZE + 1)) &&
>  		git multi-pack-index repack --batch-size=$BATCH_SIZE &&
>  		ls .git/objects/pack/*idx >idx-list &&
> --
> gitgitgadget
>
Junio C Hamano· Jul 2, 2019, 17:25 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/1] t5319: use 'test-tool path-utils' instead of 'ls -l'

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
> Hi Stolee,
>
> On Mon, 1 Jul 2019, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Using 'ls -l' and parsing the columns to find file sizes is
>> problematic when the platform could report the owner as a name
>> with spaces. Instead, use the 'test-tool path-utils file-size'
>> command to list only the sizes.
>
> Thank you!
Yup.  Sounds good.

← back to recent threads