[PATCH] test_must_be_empty: simplify file existence check

[PATCH] test_must_be_empty: simplify file existence check

[SZEDER Gábor]

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(-)

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

Re: [PATCH] test_must_be_empty: simplify file existence check

[Jeff King]

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).

-Peff

Re: [PATCH] test_must_be_empty: simplify file existence check

[SZEDER Gábor]

On Mon, Mar 26, 2018 at 2:48 PM, Jeff King <peff@peff.net> wrote:
> 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...

[PATCH v2] test_must_be_empty: simplify file existence check

[SZEDER Gábor]

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"
-- 
2.17.0.rc1.148.gc92dc354c5

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

[Junio C Hamano]

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

> 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?

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

[Junio C Hamano]

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

>>  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.