On Mon, Oct 16, 2023 at 09:43:24PM +0300, Dorcas Litunya wrote: > Bcc: > Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions > Reply-To: > In-Reply-To: <xmqq1qdumrto.fsf@gitster.g> > > On Mon, Oct 16, 2023 at 09:53:55AM -0700, Junio C Hamano wrote: > > Dorcas AnonoLitunya <anonolitunya@xxxxxxxxx> writes: > > > > > Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions > > > > Let's try if we can pack a bit more information. For example > > > > Subject: [PATCH] t7601: use "test_path_is_file" etc. instead of "test -f" > > > > would clarify what kind of modernization is done by this patch. > > > > > The test script is currently using the command format 'test -f' to > > > check for existence or absence of files. > > > > "is currently using" -> "uses". > > > > > Replace it with new helper functions following the format > > > 'test_path_is_file'. > > > > I am not sure what role "the format" plays in this picture. > > test_path_is_file is not new---it has been around for quite a while. > > > > > Consequently, the patch also replaces the inverse command '! test -f' or > > > 'test ! -f' with new helper function following the format > > > 'test_path_is_missing' > > > > A bit more on this later. > > > So should I replace this in the next version or leave this as is? Hello Junio, Following up on this? What are your thoughts on it? Thanks! Dorcas > > > This adjustment using helper functions makes the code more readable and > > > easier to understand. > > > > Looking good. If I were writing this, I'll make the whole thing > > more like this, though: > > > > t7601: use "test_path_is_file" etc. instead of "test -f" > > > > Some tests in t7601 use "test -f" and "test ! -f" to see if a > > path exists or is missing. Use test_path_is_file and > > test_path_is_missing helper functions to clarify these tests a > > bit better. This especially matters for the "missing" case, > > because "test ! -f F" will be happy if "F" exists as a > > directory, but the intent of the test is that "F" should not > > exist, even as a directory. > > > > > > > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh > > > index bd238d89b0..e08767df66 100755 > > > --- a/t/t7601-merge-pull-config.sh > > > +++ b/t/t7601-merge-pull-config.sh > > > @@ -349,13 +349,13 @@ test_expect_success 'Cannot rebase with multiple heads' ' > > > > > > test_expect_success 'merge c1 with c2' ' > > > git reset --hard c1 && > > > - test -f c0.c && > > > - test -f c1.c && > > > - test ! -f c2.c && > > > - test ! -f c3.c && > > > + test_path_is_file c0.c && > > > + test_path_is_file c1.c && > > > + test_path_is_missing c2.c && > > > + test_path_is_missing c3.c && > > > > The original says "We are happy if c2.c is not a file", so it would > > have been happy if by some mistake "git reset" created a directory > > there. But the _intent_ of the test is that we do not have anything > > at c2.c, and the updated code expresses it better.
