# [PATCH] test_must_be_empty: simplify file existence check 6 messages from 2018-03-26 to 2018-03-28. Participants: SZEDER Gábor, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/48152 ## SZEDER Gábor, 2018-03-26 11:58 Subject: [PATCH] test_must_be_empty: simplify file existence check Message-ID: <20180326115815.10180-1-szeder.dev@gmail.com> URL: https://gitlist.dev/e/20180326115815.10180-1-szeder.dev%40gmail.com ``` 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 --- 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 ``` ## Jeff King, 2018-03-26 12:48 Subject: Re: [PATCH] test_must_be_empty: simplify file existence check Message-ID: <20180326124847.GA21993@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180326124847.GA21993%40sigill.intra.peff.net In-Reply-To: <20180326115815.10180-1-szeder.dev@gmail.com> ``` 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 ``` ## SZEDER Gábor, 2018-03-26 13:09 Subject: Re: [PATCH] test_must_be_empty: simplify file existence check Message-ID: URL: https://gitlist.dev/e/CAM0VKjmCNDAKF%3DueDBtscTUMJzVEZoFDA_CO6Oz9sTLUpyyiuQ%40mail.gmail.com In-Reply-To: <20180326124847.GA21993@sigill.intra.peff.net> ``` On Mon, Mar 26, 2018 at 2:48 PM, Jeff King 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... ``` ## SZEDER Gábor, 2018-03-26 13:11 Subject: [PATCH v2] test_must_be_empty: simplify file existence check Message-ID: <20180326131124.13003-1-szeder.dev@gmail.com> URL: https://gitlist.dev/e/20180326131124.13003-1-szeder.dev%40gmail.com In-Reply-To: <20180326124847.GA21993@sigill.intra.peff.net> ``` 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 --- 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 ``` ## Junio C Hamano, 2018-03-28 00:10 Subject: Re: [PATCH v2] test_must_be_empty: simplify file existence check Message-ID: URL: https://gitlist.dev/e/xmqqd0zp9i0d.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20180326131124.13003-1-szeder.dev@gmail.com> ``` SZEDER Gábor 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 > --- > > 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? ``` ## Junio C Hamano, 2018-03-28 05:20 Subject: Re: [PATCH v2] test_must_be_empty: simplify file existence check Message-ID: URL: https://gitlist.dev/e/xmqqzi2s93p6.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Junio C Hamano 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. ```