threads / patch / 48152

patchtest_must_be_empty: simplify file existence check

Subject: [PATCH] test_must_be_empty: simplify file existence check

## tl;dr

6 messages between Mar 26, 2018 and Mar 28, 2018. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

SZEDER Gábor· Mar 26, 2018, 11:58 UTC · lore

Commit ea3c87d0b7 (test_must_be_empty: make sure the file exists, not just empty, 2018-02-27) basically duplicated the 'test_path_is_file' helper function in 'test_must_be_empty'.

Just call 'test_path_is_file' to avoid this code duplication.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/test-lib-functions.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Show changes to t/test-lib-functions.sh +2 −5

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index d2eaf5ab67..36ad8accdd 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -718,11 +718,8 @@ verbose () {
 # otherwise.
 
 test_must_be_empty () {
-	if ! test -f "$1"
-	then
-		echo "'$1' is missing"
-		return 1
-	elif test -s "$1"
+	test_path_is_file "$1" &&
+	if test -s "$1"
 	then
 		echo "'$1' is not empty, it contains:"
 		cat "$1"

-- 
2.17.0.rc1.145.gfac1bf9500

Jeff King· Mar 26, 2018, 12:48 UTC · re: SZEDER Gábor · lore

Re: [PATCH] test_must_be_empty: simplify file existence check

On Mon, Mar 26, 2018 at 01:58:15PM +0200, SZEDER Gábor wrote:

Show 5 quoted lines

> Commit ea3c87d0b7 (test_must_be_empty: make sure the file exists, not
> just empty, 2018-02-27) basically duplicated the 'test_path_is_file'
> helper function in 'test_must_be_empty'.
> 
> Just call 'test_path_is_file' to avoid this code duplication.

This looks like an improvement to me.

I think you meant to reference 11395a3b4b16d9fc637ca2e41a6892ea2e6289ce, though (I don't have ea3c87d0b7 at all).

-Peff

SZEDER Gábor· Mar 26, 2018, 13:09 UTC · re: Jeff King · lore

Re: [PATCH] test_must_be_empty: simplify file existence check

On Mon, Mar 26, 2018 at 2:48 PM, Jeff King <peff@peff.net> wrote:

Show 12 quoted lines

> On Mon, Mar 26, 2018 at 01:58:15PM +0200, SZEDER Gábor wrote:
>
>> Commit ea3c87d0b7 (test_must_be_empty: make sure the file exists, not
>> just empty, 2018-02-27) basically duplicated the 'test_path_is_file'
>> helper function in 'test_must_be_empty'.
>>
>> Just call 'test_path_is_file' to avoid this code duplication.
>
> This looks like an improvement to me.
>
> I think you meant to reference 11395a3b4b16d9fc637ca2e41a6892ea2e6289ce,
> though (I don't have ea3c87d0b7 at all).

Indeed. I remember picking up that patch from the mailing list, which then became ea3c87d0b7 in my repo. What I don't remember is how I managed to build on top of this hand-picked commit instead of what has been in git.git for a month...

SZEDER Gábor· Mar 26, 2018, 13:11 UTC · re: Jeff King · lore

[PATCH v2] test_must_be_empty: simplify file existence check

Commit 11395a3b4b (test_must_be_empty: make sure the file exists, not just empty, 2018-02-27) basically duplicated the 'test_path_is_file' helper function in 'test_must_be_empty'.

Just call 'test_path_is_file' to avoid this code duplication.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

The only change is to refer to the right commit in the log message.

 t/test-lib-functions.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Show changes to t/test-lib-functions.sh +2 −5

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index d2eaf5ab67..36ad8accdd 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -718,11 +718,8 @@ verbose () {
 # otherwise.
 
 test_must_be_empty () {
-	if ! test -f "$1"
-	then
-		echo "'$1' is missing"
-		return 1
-	elif test -s "$1"
+	test_path_is_file "$1" &&
+	if test -s "$1"
 	then
 		echo "'$1' is not empty, it contains:"
 		cat "$1"

-- 
2.17.0.rc1.148.gc92dc354c5

Junio C Hamano· Mar 28, 2018, 00:10 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2] test_must_be_empty: simplify file existence check

SZEDER Gábor <szeder.dev@gmail.com> writes:

Show 32 quoted lines

> Commit 11395a3b4b (test_must_be_empty: make sure the file exists, not
> just empty, 2018-02-27) basically duplicated the 'test_path_is_file'
> helper function in 'test_must_be_empty'.
>
> Just call 'test_path_is_file' to avoid this code duplication.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>
> The only change is to refer to the right commit in the log message.
>
>  t/test-lib-functions.sh | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index d2eaf5ab67..36ad8accdd 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -718,11 +718,8 @@ verbose () {
>  # otherwise.
>  
>  test_must_be_empty () {
> -	if ! test -f "$1"
> -	then
> -		echo "'$1' is missing"
> -		return 1
> -	elif test -s "$1"
> +	test_path_is_file "$1" &&
> +	if test -s "$1"
>  	then
>  		echo "'$1' is not empty, it contains:"
>  		cat "$1"

"Just call it" is fine as an idea but

	A &&
	if B
	then
		...
	fi

is somewhat questionable.  Shouldn't we make it

	if A && B
	then
		...
	fi

instead? That way, if we ever need to add an else clause, the logic flow would be more obvious, no?

Junio C Hamano· Mar 28, 2018, 05:20 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] test_must_be_empty: simplify file existence check

Junio C Hamano <gitster@pobox.com> writes:

Show 28 quoted lines

>>  test_must_be_empty () {
>> -	if ! test -f "$1"
>> -	then
>> -		echo "'$1' is missing"
>> -		return 1
>> -	elif test -s "$1"
>> +	test_path_is_file "$1" &&
>> +	if test -s "$1"
>>  	then
>>  		echo "'$1' is not empty, it contains:"
>>  		cat "$1"
>
> "Just call it" is fine as an idea but
>
> 	A &&
> 	if B
> 	then
> 		...
> 	fi
>
> is somewhat questionable.  Shouldn't we make it
>
> 	if A && B
> 	then
> 		...
> 	fi
>
> instead?

Nah, you want to treat A's success as a condition *not* to enter the "then" clause in this case, so my rewrite is bogus. SOrry for the noise.