Re: [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty`

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 4, 2019, 03:45 UTC
Message-ID: <xmqqo96rxpyi.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190303233750.6500-2-rohit.ashiwal265@gmail.com>

Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:

> test-lib-functions: add a helper function that checks for a file and that
> the file is not empty. The helper function will provide better error message
> in case of failure and improve readability

Avoid making the log message into an enumerated list, when there aren't that many things to enumerate to begin with (specifically, the "test-lib-functions:" label is unsightly here). Finish the sentence with a full stop.

	Add a helper function to ensure that a given path is a
	non-empty file, and give an error message when it is not.

	Give separate messages for the case when the path is missing
	or a non-file, and for the case when the path is a file but
	is empty.

should be sufficient.

I still do not see why the posted code is better than this

	if ! test -s "$1"
	then
		echo "'$1' is not a non-empty file.'
	fi
 
which is more to the point.  After all, if we are truly aiming for
finer-grained diagnosis, there is no good reason to accept a single
error message "does not exist or not a file" for these two cases,
but you'd be writing more like

	if ! test -e "$1"
	then
		echo "'$1' does not exist"
	elif ! test -f "$1"
	then
		echo "'$1' is not a file"
	elif ! test -s "$1"
	then
		echo "'$1' is not empty"
	else
		: happy
		return
	fi
	false

But I do not see much point in doing so, and I do not see much point in the version that makes an extra check only for "test -f", either.

Show 22 quoted lines

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 80402a428f..f9fcd2e013 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -593,6 +593,21 @@ test_dir_is_empty () {
>  	fi
>  }
>  
> +# Check if the file exists and has a size greater than zero
> +test_file_not_empty () {
> +	if ! test -f "$1"
> +	then
> +		echo "'$1' does not exist or not a file."
> +		false
> +	else
> +		if ! test -s "$1"
> +		then
> +			echo "'$1' is an empty file."
> +			false
> +		fi
> +	fi
> +}

If I were writing this, I'd dedent it by turning this into

	if ! test -f ...
	then
		echo ...
	elif ! test -s ...
	then
		echo ...
	else
		: happy
		return
	fi
	false

But as I said, I do not see much point in the extra "test -f", so more likely this is what I would write, if I were doing this step myself:

	if test -s "$1"
	then
		: happy
	else
		echo "'$1' is not a non-empty file"
		false
	fi

> +
>  test_path_is_missing () {
>  	if test -e "$1"
>  	then