| Message ID | 20190303122842.30380-2-rohit.ashiwal265@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use helper functions in test script | expand |
Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: > Subject: Re: [PATCH 1/3] test functions: Add new function `test_file_not_empty` s/Add/add/. Strictly speaking, you do not need to say "new", if you are already saying "add", then that's redundant. > 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 > > Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > --- > t/test-lib-functions.sh | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index 80402a428f..1302df63b6 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -593,6 +593,16 @@ test_dir_is_empty () { > fi > } > > +# Check if the file exists and has a size greater than zero > +test_file_not_empty () { > + test_path_is_file "$1" && > + if ! test -s "$1" "test -s <path>" is true if <path> resolves to an existing directory entry for a file that has a size greater than zero. Isn't it redundant and wasteful to have test_path_is_file before it, or is there a situation where "test -s" alone won't give us what we want to check? > + then > + echo "'$1' is an empty file." > + false > + fi > +} > + > test_path_is_missing () { > if test -e "$1" > then
Hey Junio On 2019-03-03 13:20 UTC Junio C Hamano <gitster@pobox.com> wrote: > s/Add/add/. Strictly speaking, you do not need to say "new", if you > are already saying "add", then that's redundant. Oh, my mistake, I will change in coming revisions. > "test -s <path>" is true if <path> resolves to an existing directory > entry for a file that has a size greater than zero. Isn't it > redundant and wasteful to have test_path_is_file before it, or is > there a situation where "test -s" alone won't give us what we want > to check? Just to be clear of what caused the error: 1. Path not being file, or 2. File not being empty I am checking for both. Regards Rohit
Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes: > Just to be clear of what caused the error: > 1. Path not being file, or > 2. File not being empty > I am checking for both. test -s <path> makes sure <path> is file; if it is not a file, then it won't yield true. So why do you need to say test_path_is_file yourself, if you are asking "test -s"?
On 2019-03-03 13:33 UTC Junio C Hamano <gitster@pobox.com> wrote: > test -s <path> makes sure <path> is file; if it is not a file, then > it won't yield true. > On 2019-03-03 13:20 UTC Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > > test_path_is_file "$1" && > > if ! test -s "$1" According to the conditional if the path is not a file then we will get the error "file does not exist" and then we will shortcircuit without checking the second conditional, on the other hand, if path is a file then we will again check if it has a size greater than zero, then error will be different (if any). Regards Rohit
On 03/03, Rohit Ashiwal wrote: > On 2019-03-03 13:33 UTC Junio C Hamano <gitster@pobox.com> wrote: > > > test -s <path> makes sure <path> is file; if it is not a file, then > > it won't yield true. > > > On 2019-03-03 13:20 UTC Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote: > > > test_path_is_file "$1" && > > > if ! test -s "$1" > > According to the conditional if the path is not a file then we will get > the error "file does not exist" and then we will shortcircuit without checking > the second conditional, on the other hand, if path is a file then we will > again check if it has a size greater than zero, then error will be different > (if any). I do agree that the better error message is probably worth the additional 'test_path_is_file' before the 'test -s'. Although it may be better to only make that distinction in the 'if' (and then maybe just using 'test -f', which would explain better why we have an additional call. Either way it would be nice to describe that reasoning in the commit message, as it's not 100% clear from the code what is going on here, which also lead to Junio's question. > Regards > Rohit >
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 80402a428f..1302df63b6 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -593,6 +593,16 @@ test_dir_is_empty () { fi } +# Check if the file exists and has a size greater than zero +test_file_not_empty () { + test_path_is_file "$1" && + if ! test -s "$1" + then + echo "'$1' is an empty file." + false + fi +} + test_path_is_missing () { if test -e "$1" then
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 Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com> --- t/test-lib-functions.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) --