{"thread":{"id":"60377","subject":"","startedAt":"2023-10-16T18:43:28Z","lastAt":"2023-10-18T12:52:18Z","messageCount":4,"participants":["Dorcas Litunya","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"483314","messageId":"ZS2ESFGP2H3CTJSK@dorcaslitunya-virtual-machine","threadId":"60377","inReplyTo":null,"subject":"","fromName":"Dorcas Litunya","fromEmail":"anonolitunya@gmail.com","sentAt":"2023-10-16T18:43:20Z","receivedAt":"2023-10-16T18:43:28Z","isPatch":false,"sender":{"key":"anonolitunya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/36160963?v=4"},"body":"Bcc: \nSubject: Re: [PATCH] t/t7601: Modernize test scripts using functions\nReply-To: \nIn-Reply-To: \n\nOn Mon, Oct 16, 2023 at 09:53:55AM -0700, Junio C Hamano wrote:\n> Dorcas AnonoLitunya writes:\n> \n> > Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions\n> \n> Let's try if we can pack a bit more information. For example\n> \n> Subject: [PATCH] t7601: use \"test_path_is_file\" etc. instead of \"test -f\"\n> \n> would clarify what kind of modernization is done by this patch.\n> \n> > The test script is currently using the command format 'test -f' to\n> > check for existence or absence of files.\n> \n> \"is currently using\" -> \"uses\".\n> \n> > Replace it with new helper functions following the format\n> > 'test_path_is_file'.\n> \n> I am not sure what role \"the format\" plays in this picture.\n> test_path_is_file is not new---it has been around for quite a while.\n> \n> > Consequently, the patch also replaces the inverse command '! test -f' or\n> > 'test ! -f' with new helper function following the format\n> > 'test_path_is_missing'\n> \n> A bit more on this later.\n>\nSo should I replace this in the next version or leave this as is?\n> > This adjustment using helper functions makes the code more readable and\n> > easier to understand.\n> \n> Looking good. If I were writing this, I'll make the whole thing\n> more like this, though:\n> \n> t7601: use \"test_path_is_file\" etc. instead of \"test -f\"\n> \n> Some tests in t7601 use \"test -f\" and \"test ! -f\" to see if a\n> path exists or is missing. Use test_path_is_file and\n> test_path_is_missing helper functions to clarify these tests a\n> bit better. This especially matters for the \"missing\" case,\n> because \"test ! -f F\" will be happy if \"F\" exists as a\n> directory, but the intent of the test is that \"F\" should not\n> exist, even as a directory.\n> \n> \n> > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\n> > index bd238d89b0..e08767df66 100755\n> > --- a/t/t7601-merge-pull-config.sh\n> > +++ b/t/t7601-merge-pull-config.sh\n> > @@ -349,13 +349,13 @@ test_expect_success 'Cannot rebase with multiple heads' '\n> > \n> > test_expect_success 'merge c1 with c2' '\n> > \tgit reset --hard c1 &&\n> > -\ttest -f c0.c &&\n> > -\ttest -f c1.c &&\n> > -\ttest ! -f c2.c &&\n> > -\ttest ! -f c3.c &&\n> > +\ttest_path_is_file c0.c &&\n> > +\ttest_path_is_file c1.c &&\n> > +\ttest_path_is_missing c2.c &&\n> > +\ttest_path_is_missing c3.c &&\n> \n> The original says \"We are happy if c2.c is not a file\", so it would\n> have been happy if by some mistake \"git reset\" created a directory\n> there. But the _intent_ of the test is that we do not have anything\n> at c2.c, and the updated code expresses it better.\n"},{"id":"483353","messageId":"ZS66sBosc55Q0lLp@dorcaslitunya-virtual-machine","threadId":"60377","inReplyTo":"ZS2ESFGP2H3CTJSK@dorcaslitunya-virtual-machine","subject":"Re:[PATCH] t/t7601: Modernize test scripts using functions","fromName":"Dorcas Litunya","fromEmail":"anonolitunya@gmail.com","sentAt":"2023-10-17T16:47:44Z","receivedAt":"2023-10-17T16:47:52Z","isPatch":true,"sender":{"key":"anonolitunya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/36160963?v=4"},"body":"On Mon, Oct 16, 2023 at 09:43:24PM +0300, Dorcas Litunya wrote:\n> Bcc: \n> Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions\n> Reply-To: \n> In-Reply-To: \n> \n> On Mon, Oct 16, 2023 at 09:53:55AM -0700, Junio C Hamano wrote:\n> > Dorcas AnonoLitunya writes:\n> > \n> > > Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions\n> > \n> > Let's try if we can pack a bit more information. For example\n> > \n> > Subject: [PATCH] t7601: use \"test_path_is_file\" etc. instead of \"test -f\"\n> > \n> > would clarify what kind of modernization is done by this patch.\n> > \n> > > The test script is currently using the command format 'test -f' to\n> > > check for existence or absence of files.\n> > \n> > \"is currently using\" -> \"uses\".\n> > \n> > > Replace it with new helper functions following the format\n> > > 'test_path_is_file'.\n> > \n> > I am not sure what role \"the format\" plays in this picture.\n> > test_path_is_file is not new---it has been around for quite a while.\n> > \n> > > Consequently, the patch also replaces the inverse command '! test -f' or\n> > > 'test ! -f' with new helper function following the format\n> > > 'test_path_is_missing'\n> > \n> > A bit more on this later.\n> >\n> So should I replace this in the next version or leave this as is?\nHello Junio,\n\nFollowing up on this? What are your thoughts on it?\n\nThanks!\n\nDorcas\n> > > This adjustment using helper functions makes the code more readable and\n> > > easier to understand.\n> > \n> > Looking good. If I were writing this, I'll make the whole thing\n> > more like this, though:\n> > \n> > t7601: use \"test_path_is_file\" etc. instead of \"test -f\"\n> > \n> > Some tests in t7601 use \"test -f\" and \"test ! -f\" to see if a\n> > path exists or is missing. Use test_path_is_file and\n> > test_path_is_missing helper functions to clarify these tests a\n> > bit better. This especially matters for the \"missing\" case,\n> > because \"test ! -f F\" will be happy if \"F\" exists as a\n> > directory, but the intent of the test is that \"F\" should not\n> > exist, even as a directory.\n> > \n> > \n> > > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\n> > > index bd238d89b0..e08767df66 100755\n> > > --- a/t/t7601-merge-pull-config.sh\n> > > +++ b/t/t7601-merge-pull-config.sh\n> > > @@ -349,13 +349,13 @@ test_expect_success 'Cannot rebase with multiple heads' '\n> > > \n> > > test_expect_success 'merge c1 with c2' '\n> > > \tgit reset --hard c1 &&\n> > > -\ttest -f c0.c &&\n> > > -\ttest -f c1.c &&\n> > > -\ttest ! -f c2.c &&\n> > > -\ttest ! -f c3.c &&\n> > > +\ttest_path_is_file c0.c &&\n> > > +\ttest_path_is_file c1.c &&\n> > > +\ttest_path_is_missing c2.c &&\n> > > +\ttest_path_is_missing c3.c &&\n> > \n> > The original says \"We are happy if c2.c is not a file\", so it would\n> > have been happy if by some mistake \"git reset\" created a directory\n> > there. But the _intent_ of the test is that we do not have anything\n> > at c2.c, and the updated code expresses it better.\n"},{"id":"483363","messageId":"xmqqjzrlgftp.fsf@gitster.g","threadId":"60377","inReplyTo":"ZS2ESFGP2H3CTJSK@dorcaslitunya-virtual-machine","subject":"Re: none","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-17T20:21:54Z","receivedAt":"2023-10-17T20:22:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dorcas Litunya writes:\n\n> Bcc: \n> Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions\n> Reply-To: \n> In-Reply-To: \n\nWhat are these lines doing here?\n\n> So should I replace this in the next version or leave this as is?\n\nPerhaps I was not clear enough, but I found the commit title and\ndescription need to be updated to clearly record the intent of the\nchange with a handful of points, so I will not be accepting the\npatch as-is.\n\nThese two sections may be of help.\n\nDocumentation/MyFirstContribution.txt::now-what\nDocumentation/MyFirstContribution.txt::reviewing\n\nThanks.\n"},{"id":"483401","messageId":"ZS/U+rPzVCpzGMww@dorcaslitunya-virtual-machine","threadId":"60377","inReplyTo":"xmqqjzrlgftp.fsf@gitster.g","subject":"Re: [PATCH] t/t7601: Modernize test scripts using functions","fromName":"Dorcas Litunya","fromEmail":"anonolitunya@gmail.com","sentAt":"2023-10-18T12:52:10Z","receivedAt":"2023-10-18T12:52:18Z","isPatch":true,"sender":{"key":"anonolitunya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/36160963?v=4"},"body":"On Tue, Oct 17, 2023 at 01:21:54PM -0700, Junio C Hamano wrote:\n> Dorcas Litunya writes:\n> \n> > Bcc: \n> > Subject: Re: [PATCH] t/t7601: Modernize test scripts using functions\n> > Reply-To: \n> > In-Reply-To: \n> \n> What are these lines doing here?\n> \nSorry, I formatted the email wrongly.\n> > So should I replace this in the next version or leave this as is?\n> \n> Perhaps I was not clear enough, but I found the commit title and\n> description need to be updated to clearly record the intent of the\n> change with a handful of points, so I will not be accepting the\n> patch as-is.\n> \n> These two sections may be of help.\n> \n> Documentation/MyFirstContribution.txt::now-what\n> Documentation/MyFirstContribution.txt::reviewing\n> \nThanks for the resources and feedback. Ihave edited the patch based on\nit and sent v2.\n> Thanks.\n"}]}