| Message ID | patch-15.16-0cd511206c4-20210412T110456Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib.sh: new test_commit args, simplification & fixes | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > test_path_is_file () { > - test "$#" -ne 1 && BUG "1 param" > - if ! test -f "$1" > + if ! test -f "$@" > then > - echo "File $1 doesn't exist" > + echo "File $@ doesn't exist" > return 1 What does it even mean to call test_path_is_file Documentation/ Makefile with this patch applied? If there were three files "COPYING Makefile", "COPYING", and "Makefile", what would happen when you did test_path_is_file COPYING Makefile (without dq around them)? I think this particular medicine is far worse than the symptom it tries to cure.
On Mon, Apr 12 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> test_path_is_file () { >> - test "$#" -ne 1 && BUG "1 param" >> - if ! test -f "$1" >> + if ! test -f "$@" >> then >> - echo "File $1 doesn't exist" >> + echo "File $@ doesn't exist" >> return 1 > > What does it even mean to call > > test_path_is_file Documentation/ Makefile > > with this patch applied? > > If there were three files "COPYING Makefile", "COPYING", and > "Makefile", what would happen when you did > > test_path_is_file COPYING Makefile > > (without dq around them)? > > I think this particular medicine is far worse than the symptom it > tries to cure. We'll error with: test: foo: unexpected operator And then say: File COPYING Makefile doesn't exist I thought guarding just for the one-off development error of not using the function correctly wasn't worth it, but I thought it made sense not to litter all of this with: diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 28b8826e565..0bd7367a07e 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -690,6 +690,7 @@ test_expect_success () { test_path_is_file () { if ! test -f "$@" then + test $# -eq 1 || BUG "Do not call test_path_is_file() with more than one argument!" echo "File $@ doesn't exist" return 1 fi But I could do that if you think it's better. We could also just emit $1 in the "echo". I don't feel strongly about that, but think that's arguably worse, since having it be $@ means the developer is more likely to notice the invalid usage right away.
On Apr 15 2021, Ævar Arnfjörð Bjarmason wrote: > On Mon, Apr 12 2021, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> test_path_is_file () { >>> - test "$#" -ne 1 && BUG "1 param" >>> - if ! test -f "$1" >>> + if ! test -f "$@" >>> then >>> - echo "File $1 doesn't exist" >>> + echo "File $@ doesn't exist" >>> return 1 >> >> What does it even mean to call >> >> test_path_is_file Documentation/ Makefile >> >> with this patch applied? >> >> If there were three files "COPYING Makefile", "COPYING", and >> "Makefile", what would happen when you did >> >> test_path_is_file COPYING Makefile >> >> (without dq around them)? >> >> I think this particular medicine is far worse than the symptom it >> tries to cure. > > We'll error with: > > test: foo: unexpected operator If you want a single argument you should use "$*", not "$@". Andreas.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Mon, Apr 12 2021, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> test_path_is_file () { >>> - test "$#" -ne 1 && BUG "1 param" >>> - if ! test -f "$1" >>> + if ! test -f "$@" >>> then >>> - echo "File $1 doesn't exist" >>> + echo "File $@ doesn't exist" >>> return 1 >> >> What does it even mean to call >> >> test_path_is_file Documentation/ Makefile >> >> with this patch applied? >> >> If there were three files "COPYING Makefile", "COPYING", and >> "Makefile", what would happen when you did >> >> test_path_is_file COPYING Makefile >> >> (without dq around them)? >> >> I think this particular medicine is far worse than the symptom it >> tries to cure. > > We'll error with: > > test: foo: unexpected operator Ah, so use of "$@" was intentional. That's clever (I thought it was a common typo people make when they mean "$*"). Of course, it would not work if the caller did a nonsense like so: test_path_is_file foo -o ok but as long as we trust that the callers would not make stupid mistakes, this is OK. Is that the reasoning behind this removal of the BUG? > I thought guarding just for the one-off development error of not using > the function correctly wasn't worth it, but I thought it made sense not > to litter all of this with: > > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index 28b8826e565..0bd7367a07e 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -690,6 +690,7 @@ test_expect_success () { > test_path_is_file () { > if ! test -f "$@" > then > + test $# -eq 1 || BUG "Do not call test_path_is_file() with more than one argument!" But this breaks our assumption that the caller would not be making stupid mistakes, so I am not sure if it is worth it. If we were to have a sanity check, shouldn't we do the check upfront, like the original? Thanks.
On Mon, Apr 12, 2021 at 01:09:04PM +0200, Ævar Arnfjörð Bjarmason wrote: > This reverts and amends my my own e7884b353b7 (test-lib-functions: > assert correct parameter count, 2021-02-12) in order to improve the -x > output. > > When I added these BUG assertions in e7884b353b7 I missed that this > made the -x output much more verbose. > > E.g. for each test_cmp invocation we'd now emit: > > + test_cmp expect actual > + test 2 -ne 2 > + eval diff -u "$@" > + diff -u expect actual > > That "test 2 -ne 2" line is new in e7884b353b7. As noted in > 45a2686441b (test-lib-functions: remove bug-inducing "diagnostics" > helper param, 2021-02-12) we had buggy invocations of some of these > functions with too many parameters. > > Let's instead use "$@" instead of "$1" to achieve the same ends with > no extra -x output verbosity. The "test" operator will die with an > error if given more than one argument in these contexts, so using "$@" > achieves the same goal. I prefer the current check for its explicitness over the implicit and somewhat cryptic approach introduced in this patch. I hope that sooner or later I'll finish up my patch series to suppress '-x' output from test helper functions, and then this issue will become moot anyway. > The same goes for "cmp" and "diff -u" (which we typically use for > test_cmp). > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/test-lib-functions.sh | 41 ++++++++++++++++------------------------- > 1 file changed, 16 insertions(+), 25 deletions(-) > > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index c46bf0ff09c..2cf72b56851 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -759,39 +759,35 @@ test_external_without_stderr () { > # debugging-friendly alternatives to "test [-f|-d|-e]" > # The commands test the existence or non-existence of $1 > test_path_is_file () { > - test "$#" -ne 1 && BUG "1 param" > - if ! test -f "$1" > + if ! test -f "$@" > then > - echo "File $1 doesn't exist" > + echo "File $@ doesn't exist" > return 1 > fi > } > > test_path_is_dir () { > - test "$#" -ne 1 && BUG "1 param" > - if ! test -d "$1" > + if ! test -d "$@" > then > - echo "Directory $1 doesn't exist" > + echo "Directory $@ doesn't exist" > return 1 > fi > } > > test_path_exists () { > - test "$#" -ne 1 && BUG "1 param" > - if ! test -e "$1" > + if ! test -e "$@" > then > - echo "Path $1 doesn't exist" > + echo "Path $@ doesn't exist" > return 1 > fi > } > > # Check if the directory exists and is empty as expected, barf otherwise. > test_dir_is_empty () { > - test "$#" -ne 1 && BUG "1 param" > - test_path_is_dir "$1" && > - if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')" > + test_path_is_dir "$@" && > + if test -n "$(ls -a1 "$@" | egrep -v '^\.\.?$')" > then > - echo "Directory '$1' is not empty, it contains:" > + echo "Directory '$@' is not empty, it contains:" > ls -la "$1" > return 1 > fi > @@ -799,17 +795,15 @@ test_dir_is_empty () { > > # Check if the file exists and has a size greater than zero > test_file_not_empty () { > - test "$#" = 2 && BUG "2 param" > - if ! test -s "$1" > + if ! test -s "$@" > then > - echo "'$1' is not a non-empty file." > + echo "'$@' is not a non-empty file." > return 1 > fi > } > > test_path_is_missing () { > - test "$#" -ne 1 && BUG "1 param" > - if test -e "$1" > + if test -e "$@" > then > echo "Path $1 exists!" > false > @@ -1013,7 +1007,6 @@ test_expect_code () { > # - not all diff versions understand "-u" > > test_cmp () { > - test "$#" -ne 2 && BUG "2 param" > eval "$GIT_TEST_CMP" '"$@"' > } > > @@ -1043,7 +1036,6 @@ test_cmp_config () { > # test_cmp_bin - helper to compare binary files > > test_cmp_bin () { > - test "$#" -ne 2 && BUG "2 param" > cmp "$@" > } > > @@ -1104,12 +1096,11 @@ verbose () { > # otherwise. > > test_must_be_empty () { > - test "$#" -ne 1 && BUG "1 param" > - test_path_is_file "$1" && > - if test -s "$1" > + test_path_is_file "$@" && > + if test -s "$@" > then > - echo "'$1' is not empty, it contains:" > - cat "$1" > + echo "'$@' is not empty, it contains:" > + cat "$@" > return 1 > fi > } > -- > 2.31.1.634.gb41287a30b0 >
SZEDER Gábor <szeder.dev@gmail.com> writes: > ... suppress '-x' output > from test helper functions, and then this issue will become moot > anyway. That is certainly a better approach than tweaking each call site of BUG. The interface to BUG is "write a code to determine condition and then die by calling BUG", which means under '-x' you are bound to see the trace from "code to determine condition" part. I wonder if introducing a BUG_ON helper function that - turns off '-x' trace upon entry; - takes a condition as one of its arguments and evals it; - issues a message and dies if needed; - otherwise arranges to turn '-x' trace on and return. would solve it well? Thanks.
On Thu, Apr 15 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Mon, Apr 12 2021, Junio C Hamano wrote: >> >>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> >>>> test_path_is_file () { >>>> - test "$#" -ne 1 && BUG "1 param" >>>> - if ! test -f "$1" >>>> + if ! test -f "$@" >>>> then >>>> - echo "File $1 doesn't exist" >>>> + echo "File $@ doesn't exist" >>>> return 1 >>> >>> What does it even mean to call >>> >>> test_path_is_file Documentation/ Makefile >>> >>> with this patch applied? >>> >>> If there were three files "COPYING Makefile", "COPYING", and >>> "Makefile", what would happen when you did >>> >>> test_path_is_file COPYING Makefile >>> >>> (without dq around them)? >>> >>> I think this particular medicine is far worse than the symptom it >>> tries to cure. >> >> We'll error with: >> >> test: foo: unexpected operator > > Ah, so use of "$@" was intentional. That's clever (I thought it was > a common typo people make when they mean "$*"). > > Of course, it would not work if the caller did a nonsense like so: > > test_path_is_file foo -o ok > > but as long as we trust that the callers would not make stupid > mistakes, this is OK. Is that the reasoning behind this removal of > the BUG? The reasoning is to get rid of verbosity in the trace output, while still effectively retaining the error checking. Yes you could do "foo -o ok", but as my already-on-master fixes to the few misuses showed we only realistically have to worry about them being used with many normal looking file names (if that). >> I thought guarding just for the one-off development error of not using >> the function correctly wasn't worth it, but I thought it made sense not >> to litter all of this with: >> >> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh >> index 28b8826e565..0bd7367a07e 100644 >> --- a/t/test-lib-functions.sh >> +++ b/t/test-lib-functions.sh >> @@ -690,6 +690,7 @@ test_expect_success () { >> test_path_is_file () { >> if ! test -f "$@" >> then >> + test $# -eq 1 || BUG "Do not call test_path_is_file() with more than one argument!" > > But this breaks our assumption that the caller would not be making > stupid mistakes, so I am not sure if it is worth it. If we were to > have a sanity check, shouldn't we do the check upfront, like the > original? It's fine to do sanity checking if it's in the "else" branch where we're already emitting an error, and we only do so in the BUG case. I.e. if it's written like e.g. this: test_path_is_file () { if test $# -ne 1 then BUG "Do not call test_path_is_file() with more than one argument!" elif ! test -f "$@" then echo "File $@ doesn't exist" Then with e.g.: ./t3600-rm.sh --run=1-3 -vx We get: + test_path_is_file foo + test 1 -ne 1 + test -f foo + git ls-files --error-unmatch foo But if we, as my patch does, piggy-pack on the test-built in to panic on too many arguments we get the much more succinct: + test_path_is_file foo + test -f foo + git ls-files --error-unmatch foo I think that's the only trace output that matters, having "test 2 -ne 1" or whatever in the case where we're just about to invoke BUG anyway is fine.
On Fri, Apr 16 2021, SZEDER Gábor wrote: > On Mon, Apr 12, 2021 at 01:09:04PM +0200, Ævar Arnfjörð Bjarmason wrote: >> This reverts and amends my my own e7884b353b7 (test-lib-functions: >> assert correct parameter count, 2021-02-12) in order to improve the -x >> output. >> >> When I added these BUG assertions in e7884b353b7 I missed that this >> made the -x output much more verbose. >> >> E.g. for each test_cmp invocation we'd now emit: >> >> + test_cmp expect actual >> + test 2 -ne 2 >> + eval diff -u "$@" >> + diff -u expect actual >> >> That "test 2 -ne 2" line is new in e7884b353b7. As noted in >> 45a2686441b (test-lib-functions: remove bug-inducing "diagnostics" >> helper param, 2021-02-12) we had buggy invocations of some of these >> functions with too many parameters. >> >> Let's instead use "$@" instead of "$1" to achieve the same ends with >> no extra -x output verbosity. The "test" operator will die with an >> error if given more than one argument in these contexts, so using "$@" >> achieves the same goal. > > I prefer the current check for its explicitness over the implicit and > somewhat cryptic approach introduced in this patch. Fair enough, I think it's worth it to have a bit of a non-obvious pattern there for less trace verbosity across the board. > I hope that sooner or later I'll finish up my patch series to suppress > '-x' output from test helper functions, and then this issue will > become moot anyway. That sounds like an interesting feature for those who want it, but it's entirely orthagonal to the direction this patch is taking. I'd like to have trace output for when tests descend into test-lib-functions.sh and friends, I'd just like the most frequently used code there to not be needlessly verbose. That's not the same as wanting to hide it entirely, i.e. treat test_path_is_file et al as though they were shell built-ins.
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index c46bf0ff09c..2cf72b56851 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -759,39 +759,35 @@ test_external_without_stderr () { # debugging-friendly alternatives to "test [-f|-d|-e]" # The commands test the existence or non-existence of $1 test_path_is_file () { - test "$#" -ne 1 && BUG "1 param" - if ! test -f "$1" + if ! test -f "$@" then - echo "File $1 doesn't exist" + echo "File $@ doesn't exist" return 1 fi } test_path_is_dir () { - test "$#" -ne 1 && BUG "1 param" - if ! test -d "$1" + if ! test -d "$@" then - echo "Directory $1 doesn't exist" + echo "Directory $@ doesn't exist" return 1 fi } test_path_exists () { - test "$#" -ne 1 && BUG "1 param" - if ! test -e "$1" + if ! test -e "$@" then - echo "Path $1 doesn't exist" + echo "Path $@ doesn't exist" return 1 fi } # Check if the directory exists and is empty as expected, barf otherwise. test_dir_is_empty () { - test "$#" -ne 1 && BUG "1 param" - test_path_is_dir "$1" && - if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')" + test_path_is_dir "$@" && + if test -n "$(ls -a1 "$@" | egrep -v '^\.\.?$')" then - echo "Directory '$1' is not empty, it contains:" + echo "Directory '$@' is not empty, it contains:" ls -la "$1" return 1 fi @@ -799,17 +795,15 @@ test_dir_is_empty () { # Check if the file exists and has a size greater than zero test_file_not_empty () { - test "$#" = 2 && BUG "2 param" - if ! test -s "$1" + if ! test -s "$@" then - echo "'$1' is not a non-empty file." + echo "'$@' is not a non-empty file." return 1 fi } test_path_is_missing () { - test "$#" -ne 1 && BUG "1 param" - if test -e "$1" + if test -e "$@" then echo "Path $1 exists!" false @@ -1013,7 +1007,6 @@ test_expect_code () { # - not all diff versions understand "-u" test_cmp () { - test "$#" -ne 2 && BUG "2 param" eval "$GIT_TEST_CMP" '"$@"' } @@ -1043,7 +1036,6 @@ test_cmp_config () { # test_cmp_bin - helper to compare binary files test_cmp_bin () { - test "$#" -ne 2 && BUG "2 param" cmp "$@" } @@ -1104,12 +1096,11 @@ verbose () { # otherwise. test_must_be_empty () { - test "$#" -ne 1 && BUG "1 param" - test_path_is_file "$1" && - if test -s "$1" + test_path_is_file "$@" && + if test -s "$@" then - echo "'$1' is not empty, it contains:" - cat "$1" + echo "'$@' is not empty, it contains:" + cat "$@" return 1 fi }
This reverts and amends my my own e7884b353b7 (test-lib-functions: assert correct parameter count, 2021-02-12) in order to improve the -x output. When I added these BUG assertions in e7884b353b7 I missed that this made the -x output much more verbose. E.g. for each test_cmp invocation we'd now emit: + test_cmp expect actual + test 2 -ne 2 + eval diff -u "$@" + diff -u expect actual That "test 2 -ne 2" line is new in e7884b353b7. As noted in 45a2686441b (test-lib-functions: remove bug-inducing "diagnostics" helper param, 2021-02-12) we had buggy invocations of some of these functions with too many parameters. Let's instead use "$@" instead of "$1" to achieve the same ends with no extra -x output verbosity. The "test" operator will die with an error if given more than one argument in these contexts, so using "$@" achieves the same goal. The same goes for "cmp" and "diff -u" (which we typically use for test_cmp). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/test-lib-functions.sh | 41 ++++++++++++++++------------------------- 1 file changed, 16 insertions(+), 25 deletions(-)