* (no subject)
@ 2023-10-16 18:43 Dorcas Litunya
2023-10-17 16:47 ` Re:[PATCH] t/t7601: Modernize test scripts using functions Dorcas Litunya
2023-10-17 20:21 ` none Junio C Hamano
0 siblings, 2 replies; 6+ messages in thread
From: Dorcas Litunya @ 2023-10-16 18:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: christian.couder, git
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re:[PATCH] t/t7601: Modernize test scripts using functions
2023-10-16 18:43 Dorcas Litunya
@ 2023-10-17 16:47 ` Dorcas Litunya
2023-10-17 20:21 ` none Junio C Hamano
1 sibling, 0 replies; 6+ messages in thread
From: Dorcas Litunya @ 2023-10-17 16:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: anonolitunya, git, christian.couder
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: none
2023-10-16 18:43 Dorcas Litunya
2023-10-17 16:47 ` Re:[PATCH] t/t7601: Modernize test scripts using functions Dorcas Litunya
@ 2023-10-17 20:21 ` Junio C Hamano
2023-10-18 12:52 ` [PATCH] t/t7601: Modernize test scripts using functions Dorcas Litunya
1 sibling, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2023-10-17 20:21 UTC (permalink / raw)
To: Dorcas Litunya; +Cc: christian.couder, git
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] t/t7601: Modernize test scripts using functions
2023-10-17 20:21 ` none Junio C Hamano
@ 2023-10-18 12:52 ` Dorcas Litunya
0 siblings, 0 replies; 6+ messages in thread
From: Dorcas Litunya @ 2023-10-18 12:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: christian.couder, git
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] t/t7601: Modernize test scripts using functions
@ 2023-10-16 15:21 Dorcas AnonoLitunya
2023-10-16 16:53 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Dorcas AnonoLitunya @ 2023-10-16 15:21 UTC (permalink / raw)
To: christian.couder; +Cc: anonolitunya, git, gitster
The test script is currently using the command format 'test -f' to
check for existence or absence of files.
Replace it with new helper functions following the format
'test_path_is_file'.
Consequently, the patch also replaces the inverse command '! test -f' or
'test ! -f' with new helper function following the format
'test_path_is_missing'
This adjustment using helper functions makes the code more readable and
easier to understand.
Signed-off-by: Dorcas AnonoLitunya <anonolitunya@gmail.com>
---
t/t7601-merge-pull-config.sh | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
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 &&
git merge c2 &&
- test -f c1.c &&
- test -f c2.c
+ test_path_is_file c1.c &&
+ test_path_is_file c2.c
'
test_expect_success 'fast-forward pull succeeds with "true" in pull.ff' '
@@ -411,8 +411,8 @@ test_expect_success 'merge c1 with c2 (ours in pull.twohead)' '
git reset --hard c1 &&
git config pull.twohead ours &&
git merge c2 &&
- test -f c1.c &&
- ! test -f c2.c
+ test_path_is_file c1.c &&
+ test_path_is_missing c2.c
'
test_expect_success 'merge c1 with c2 and c3 (recursive in pull.octopus)' '
@@ -431,10 +431,10 @@ test_expect_success 'merge c1 with c2 and c3 (recursive and octopus in pull.octo
test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
test "$(git rev-parse c3)" = "$(git rev-parse HEAD^3)" &&
git diff --exit-code &&
- 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_file c2.c &&
+ test_path_is_file c3.c
'
conflict_count()
--
2.42.0.345.gaab89be2eb
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] t/t7601: Modernize test scripts using functions
2023-10-16 15:21 Dorcas AnonoLitunya
@ 2023-10-16 16:53 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2023-10-16 16:53 UTC (permalink / raw)
To: Dorcas AnonoLitunya; +Cc: christian.couder, git
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.
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-18 12:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-16 18:43 Dorcas Litunya
2023-10-17 16:47 ` Re:[PATCH] t/t7601: Modernize test scripts using functions Dorcas Litunya
2023-10-17 20:21 ` none Junio C Hamano
2023-10-18 12:52 ` [PATCH] t/t7601: Modernize test scripts using functions Dorcas Litunya
-- strict thread matches above, loose matches on Subject: below --
2023-10-16 15:21 Dorcas AnonoLitunya
2023-10-16 16:53 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.