From: Junio C Hamano <gitster@pobox.com>
To: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Cc: Johannes.Schindelin@gmx.de, christian.couder@gmail.com,
git@vger.kernel.org, t.gummerer@gmail.com
Subject: Re: [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty`
Date: Mon, 04 Mar 2019 12:45:25 +0900 [thread overview]
Message-ID: <xmqqo96rxpyi.fsf@gitster-ct.c.googlers.com> (raw)
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.
> 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
next prev parent reply other threads:[~2019-03-04 3:45 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-03 12:28 [GSoC][PATCH 0/3] Use helper functions in test script Rohit Ashiwal
2019-03-03 12:28 ` [PATCH 1/3] test functions: Add new function `test_file_not_empty` Rohit Ashiwal
2019-03-03 13:20 ` Junio C Hamano
2019-03-03 13:29 ` Rohit Ashiwal
2019-03-03 13:33 ` none Junio C Hamano
2019-03-03 14:07 ` Clearing logic Rohit Ashiwal
2019-03-03 16:19 ` Thomas Gummerer
2019-03-03 12:28 ` [PATCH 2/3] t3600: refactor code according to contemporary guidelines Rohit Ashiwal
2019-03-03 13:30 ` Junio C Hamano
2019-03-03 14:13 ` t3600: refactor code according to comtemporary guidelines Rohit Ashiwal
2019-03-03 12:28 ` [PATCH 3/3] t3600: use helper functions from test-lib-functions Rohit Ashiwal
2019-03-03 13:32 ` Junio C Hamano
2019-03-03 23:37 ` [GSoC][PATCH v2 0/3] Use helper functions in test script Rohit Ashiwal
2019-03-03 23:37 ` [GSoC][PATCH v2 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal
2019-03-04 3:45 ` Junio C Hamano [this message]
2019-03-03 23:37 ` [GSoC][PATCH v2 2/3] t3600: restructure code according to contemporary guidelines Rohit Ashiwal
2019-03-04 4:17 ` Junio C Hamano
2019-03-03 23:37 ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal
2019-03-04 12:07 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Rohit Ashiwal
2019-03-04 12:07 ` [GSoC][PATCH v3 1/3] test functions: add function `test_file_not_empty` Rohit Ashiwal
2019-03-05 0:17 ` Eric Sunshine
2019-03-05 12:43 ` Junio C Hamano
2019-03-05 13:27 ` [GSoc][PATCH " Rohit Ashiwal
2019-03-04 12:08 ` [GSoC][PATCH v3 2/3] t3600: modernize style Rohit Ashiwal
2019-03-05 0:36 ` Eric Sunshine
2019-03-05 12:44 ` Junio C Hamano
2019-03-04 12:08 ` [GSoC][PATCH v3 3/3] t3600: use helpers to replace test -d/f/e/s <path> Rohit Ashiwal
2019-03-05 0:42 ` Eric Sunshine
2019-03-05 13:42 ` Rohit Ashiwal
2019-03-05 14:03 ` Eric Sunshine
2019-03-05 14:21 ` [GSoC][PATCH v2 " Rohit Ashiwal
2019-03-05 14:57 ` Eric Sunshine
2019-03-05 23:38 ` Rohit Ashiwal
2019-03-08 5:38 ` [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path> Junio C Hamano
2019-03-08 9:51 ` Eric Sunshine
2019-03-11 1:54 ` Junio C Hamano
2019-03-05 0:09 ` [GSoC][PATCH v3 0/3] Use helper functions in test script Eric Sunshine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqo96rxpyi.fsf@gitster-ct.c.googlers.com \
--to=gitster@pobox.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=rohit.ashiwal265@gmail.com \
--cc=t.gummerer@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.