(no subject)

[Dorcas Litunya]

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@gmail.com> 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?
> > 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.

Re:[PATCH] t/t7601: Modernize test scripts using functions

[Dorcas Litunya]

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@gmail.com> 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.

Re: none

[Junio C Hamano]

Dorcas Litunya <anonolitunya@gmail.com> writes:

> Bcc: 
> Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
> Reply-To: 
> In-Reply-To: <xmqq1qdumrto.fsf@gitster.g>

What are these lines doing here?

> So should I replace this in the next version or leave this as is?

Perhaps I was not clear enough, but I found the commit title and
description need to be updated to clearly record the intent of the
change with a handful of points, so I will not be accepting the
patch as-is.

These two sections may be of help.

Documentation/MyFirstContribution.txt::now-what
Documentation/MyFirstContribution.txt::reviewing

Thanks.

Re: [PATCH] t/t7601: Modernize test scripts using functions

[Dorcas Litunya]

On Tue, Oct 17, 2023 at 01:21:54PM -0700, Junio C Hamano wrote:
> Dorcas Litunya <anonolitunya@gmail.com> writes:
> 
> > Bcc: 
> > Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions
> > Reply-To: 
> > In-Reply-To: <xmqq1qdumrto.fsf@gitster.g>
> 
> What are these lines doing here?
> 
Sorry, I formatted the email wrongly.
> > So should I replace this in the next version or leave this as is?
> 
> Perhaps I was not clear enough, but I found the commit title and
> description need to be updated to clearly record the intent of the
> change with a handful of points, so I will not be accepting the
> patch as-is.
> 
> These two sections may be of help.
> 
> Documentation/MyFirstContribution.txt::now-what
> Documentation/MyFirstContribution.txt::reviewing
> 
Thanks for the resources and feedback. Ihave edited the patch based on
it and sent v2.
> Thanks.