Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2020, 17:43 UTC
Message-ID: <xmqqtv3i5xhx.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20200222071335.27292-2-wasmus@zom.bi>

Rasmus Jonsson <wasmus@zom.bi> writes:

Show 19 quoted lines

> Replace "test -f" with test_path_is_file, which gives more verbose
> and accurate output.
>
> Signed-off-by: Rasmus Jonsson <wasmus@zom.bi>
> ---
>  t/t1050-large.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index d3b2adb28b..667fc2a745 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -53,7 +53,8 @@ test_expect_success 'add a large file or two' '
>  	for p in .git/objects/pack/pack-*.pack
>  	do
>  		count=$(( $count + 1 ))
> -		if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
> +		if test_path_is_file "$p" && idx=${p%.pack}.idx &&
> +		   test_path_is_file "$idx"

This is not wrong per-se, but assignment to idx logically is tied stronger to its use (i.e. the first test_path_is_file is about the .pack half of the packfile, the assignment to idx *and* the second test_path_is_file is about the corresponding .idx half), so either write it like so:

        if test_path_is_file "$p" &&
           idx=${p%.pack}.idx && test_path_is_file "$idx"

or each piece on its own line, if the result becomes overly long:

        if test_path_is_file "$p" &&
           idx=${p%.pack}.idx &&
	   test_path_is_file "$idx"

All the changes in this patch make sense, otherwise.  Thanks.

> @@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '
>  	test $cnt = 2 &&
>  	for l in .git/objects/??/??????????????????????????????????????

It is totally an unrelated tangent, but brian, are the lines of this kind on your radar? The object names in SHA-256 world would not be caught with the pattern right? The fix probably belongs to next to where OID_REGEX is defined in test-lib.sh (this is a glob and not a regex, though). Perhaps the original should have been written like

	# somewhere in test-lib.sh
	HEXGLOB='[0-9a-f]'
	HEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB ;# 6
	HEXGLOB38=$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38 ;# 36
	HEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB38

	OBJFANOUTGLOB=$HEXGLOB$HEXGLOB
	OBJFILEGLOB=$HEXGLOB38

...

	for l in .git/objects/$OBJFANOUTGLOB/$OBJFILEGLOB

and then SHA-256 series would just update OBJFANOUTGLOB and OBJFILEGLOB patterns, or something like that?

Show 24 quoted lines

>  	do
> -		test -f "$l" || continue
> +		test_path_is_file "$l" || continue
>  		bad=t
>  	done &&
>  	test -z "$bad" &&
> @@ -76,7 +77,8 @@ test_expect_success 'add a large file or two' '
>  	for p in .git/objects/pack/pack-*.pack
>  	do
>  		count=$(( $count + 1 ))
> -		if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
> +		if test_path_is_file "$p" && idx=${p%.pack}.idx &&
> +		   test_path_is_file "$idx"
>  		then
>  			continue
>  		fi
> @@ -111,7 +113,7 @@ test_expect_success 'packsize limit' '
>  		count=0 &&
>  		for pi in .git/objects/pack/pack-*.idx
>  		do
> -			test -f "$pi" && count=$(( $count + 1 ))
> +			test_path_is_file "$pi" && count=$(( $count + 1 ))
>  		done &&
>  		test $count = 2 &&