{"thread":{"id":"54456","subject":"[Outreachy-Microproject][PATCH 1/1] t0000: replace 'test -[def]' with helpers","startedAt":"2020-10-18T06:11:04Z","lastAt":"2020-10-18T23:36:17Z","messageCount":5,"participants":["Caleb Tillman","Shourya Shukla","Christian Couder","Eric Sunshine","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"407838","messageId":"20201018061052.32350-1-caleb.tillman@gmail.com","threadId":"54456","inReplyTo":null,"subject":"[Outreachy-Microproject][PATCH 1/1] t0000: replace 'test -[def]' with helpers","fromName":"Caleb Tillman","fromEmail":"caleb.tillman@gmail.com","sentAt":"2020-10-18T06:10:52Z","receivedAt":"2020-10-18T06:11:04Z","isPatch":true,"sender":{"key":"caleb.tillman@gmail.com","avatar":null},"body":"The test_path_is* functions provide debug-friendly upon failure.\n\nSigned-off-by: Caleb Tillman <caleb.tillman@gmail.com>\n---\nOutreachy microproject, revised submission.\n t/t0000-basic.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 923281af93..eb99892a87 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1191,7 +1191,7 @@ test_expect_success 'writing this tree with --missing-ok' '\n test_expect_success 'git read-tree followed by write-tree should be idempotent' '\n \trm -f .git/index &&\n \tgit read-tree $tree &&\n-\ttest -f .git/index &&\n+\ttest_path_is_file .git/index &&\n \tnewtree=$(git write-tree) &&\n \ttest \"$newtree\" = \"$tree\"\n '\n-- \n2.25.1\n\n"},{"id":"407844","messageId":"20201018130219.GA6749@konoha","threadId":"54456","inReplyTo":"20201018061052.32350-1-caleb.tillman@gmail.com","subject":"Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace 'test -[def]' with helpers","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-10-18T13:02:19Z","receivedAt":"2020-10-18T13:02:30Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Hello Caleb,\n\nI have some comments.\n\nFirst of all, I notice that this is a v2 of this PATCH:\nhttps://lore.kernel.org/git/20201018005522.217397-1-caleb.tillman@gmail.com/\n\nSo, I think that the subject of the mail should reflect the same. I\nbelieve that you have used 'git format-patch' to generate this mail\ntherefore what you can do is:\n\n'git format-patch -v2 @~n', where 'n' is the number of commits which you\nwant to include in the patch. So in your case it will be:\n'git format-patch -v2 @~1' and a patch mail will be generated.\n\nAlso, you need not put the '[Outreachy-Microproject]' tag in the\nsubject, '[OUTREACHY]' will suffice.\n\nNow, coming to the meat of the patch.\n\n> The test_path_is* functions provide debug-friendly upon failure.\n\nThis commit can be redone to be even more better. This does not exactly\nreflect what has been done. I understand that yes 'test_patch_is_*'\nfunctions are better and why they are better. But where did you replace\nthem, this is left unanswered.\n\nThis is one example of how the commit messages can be, not too verbose\nand not too short, somewhere in the middle:\nhttps://lore.kernel.org/git/20200118083326.9643-6-shouryashukla.oo@gmail.com/\n\n> Signed-off-by: Caleb Tillman <caleb.tillman@gmail.com>\n---\n> Outreachy microproject, revised submission.\n>  t/t0000-basic.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 923281af93..eb99892a87 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -1191,7 +1191,7 @@ test_expect_success 'writing this tree with --missing-ok' '\n>  test_expect_success 'git read-tree followed by write-tree should be idempotent' '\n> \trm -f .git/index &&\n> \tgit read-tree $tree &&\n> -\ttest -f .git/index &&\n> +\ttest_path_is_file .git/index &&\n>  \tnewtree=$(git write-tree) &&\n> \ttest \"$newtree\" = \"$tree\"\n\nThe change is fine but I feel you can easily find files in which you can\ndo the same type of change but in a large quantity. This way you will\nget an even better idea of how the tests work at Git. To find such\nfiles, one way can be to look here:\nhttps://github.com/git/git/tree/master/t\n\nHere if you try finding files which had commits over 11-12+ years ago,\nyou will find some ancient relics to modernise too! Great that you took\nTaylor's advice ;)\n\nBest of luck,\nShourya Shukla\n\n"},{"id":"407851","messageId":"CAP8UFD1Ux7uu637_0NF4TCPJq4++KARkRm+g26ou9AdZb06OcA@mail.gmail.com","threadId":"54456","inReplyTo":"20201018130219.GA6749@konoha","subject":"Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace 'test -[def]' with helpers","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-10-18T18:38:44Z","receivedAt":"2020-10-18T18:39:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Shourya and Caleb,\n\nOn Sun, Oct 18, 2020 at 4:12 PM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n>\n> Hello Caleb,\n>\n> I have some comments.\n>\n> First of all, I notice that this is a v2 of this PATCH:\n> https://lore.kernel.org/git/20201018005522.217397-1-caleb.tillman@gmail.com/\n>\n> So, I think that the subject of the mail should reflect the same. I\n> believe that you have used 'git format-patch' to generate this mail\n> therefore what you can do is:\n>\n> 'git format-patch -v2 @~n', where 'n' is the number of commits which you\n> want to include in the patch. So in your case it will be:\n> 'git format-patch -v2 @~1' and a patch mail will be generated.\n\nYeah, using \"-v2\" is definitely needed. It will put \"[PATCH v2]\" or\n\"[PATCH v2 1/1]\" in the subject.\n\n> Also, you need not put the '[Outreachy-Microproject]' tag in the\n> subject, '[OUTREACHY]' will suffice.\n\nI am ok with '[Outreachy-Microproject]' even if it's a bit longer.\n\n> Now, coming to the meat of the patch.\n>\n> > The test_path_is* functions provide debug-friendly upon failure.\n\ns/debug-friendly/debug-friendly output/ would be more clear.\n\n> This commit can be redone to be even more better. This does not exactly\n> reflect what has been done. I understand that yes 'test_patch_is_*'\n> functions are better and why they are better. But where did you replace\n> them, this is left unanswered.\n\nThere is \"t0000\" in the subject which is enough.\n\n> This is one example of how the commit messages can be, not too verbose\n> and not too short, somewhere in the middle:\n> https://lore.kernel.org/git/20200118083326.9643-6-shouryashukla.oo@gmail.com/\n\nI am not sure it is a very good example. I would be ok with the commit\nbeing a bit more verbose though.\n\n> > Signed-off-by: Caleb Tillman <caleb.tillman@gmail.com>\n> ---\n> > Outreachy microproject, revised submission.\n> >  t/t0000-basic.sh | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> > index 923281af93..eb99892a87 100755\n> > --- a/t/t0000-basic.sh\n> > +++ b/t/t0000-basic.sh\n> > @@ -1191,7 +1191,7 @@ test_expect_success 'writing this tree with --missing-ok' '\n> >  test_expect_success 'git read-tree followed by write-tree should be idempotent' '\n> >       rm -f .git/index &&\n> >       git read-tree $tree &&\n> > -     test -f .git/index &&\n> > +     test_path_is_file .git/index &&\n> >       newtree=$(git write-tree) &&\n> >       test \"$newtree\" = \"$tree\"\n>\n> The change is fine but I feel you can easily find files in which you can\n> do the same type of change but in a large quantity. This way you will\n> get an even better idea of how the tests work at Git. To find such\n> files, one way can be to look here:\n> https://github.com/git/git/tree/master/t\n\nWe actually don't want that for most microprojects. On\nhttps://git.github.io/Outreachy-21-Microprojects/ we ask it to be done\non only one test script.\n\n> Here if you try finding files which had commits over 11-12+ years ago,\n> you will find some ancient relics to modernise too! Great that you took\n> Taylor's advice ;)\n\nNo need to find a really old test script for this microproject as I\nthink some 'test -[def]' uses have been introduced not too long ago.\n\nThanks both,\nChristian.\n"},{"id":"407852","messageId":"CAPig+cR+Jkg3ymFGBXtmLUEB7+XbgX0HvkjV3Z_9Lvk6qsY0qA@mail.gmail.com","threadId":"54456","inReplyTo":"20201018130219.GA6749@konoha","subject":"Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace 'test -[def]' with helpers","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-18T18:42:53Z","receivedAt":"2020-10-18T18:43:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 18, 2020 at 9:02 AM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n> First of all, I notice that this is a v2 of this PATCH:\n>\n> So, I think that the subject of the mail should reflect the same. I\n> believe that you have used 'git format-patch' to generate this mail\n> therefore what you can do is:\n>\n> 'git format-patch -v2 @~n', where 'n' is the number of commits which you\n> want to include in the patch. So in your case it will be:\n> 'git format-patch -v2 @~1' and a patch mail will be generated.\n\nEven simpler is to use the short form -<n> where <n> is the number of\npatches you want to format, so this can become:\n\n    git format-patch -v2 -1\n\n> Also, you need not put the '[Outreachy-Microproject]' tag in the\n> subject, '[OUTREACHY]' will suffice.\n\nGood advice.\n\n> > t0000: replace 'test -[def]' with helpers\n> >\n> > The test_path_is* functions provide debug-friendly upon failure.\n\nSince this patch is replacing only a single `test -f` (and not\ntouching anything of the form `test -d` or `test -e`), it would be\nmore accurate and reviewer-friendly to be explicit and say only `test\n-f` in the subject, and `test_path_is_file` in the body rather than\nmaking the commit message unnecessarily and overly generic.\n\n> This commit can be redone to be even more better. This does not exactly\n> reflect what has been done. I understand that yes 'test_patch_is_*'\n> functions are better and why they are better. But where did you replace\n> them, this is left unanswered.\n\nI'm having trouble parsing this. It is unclear what has been left unanswered.\n\n> This is one example of how the commit messages can be, not too verbose\n> and not too short, somewhere in the middle:\n> https://lore.kernel.org/git/20200118083326.9643-6-shouryashukla.oo@gmail.com/\n\nIt doesn't hurt to add a little back history as in the example you\ncite, but the commit message of this patch already does a reasonable\njob of explaining why this change is a good idea (specifically because\n`test_path_is_file` makes for a better debugging experience), so it\ndoesn't necessarily need more explanation.\n\n> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> > @@ -1191,7 +1191,7 @@ test_expect_success 'writing this tree with --missing-ok' '\n> >  test_expect_success 'git read-tree followed by write-tree should be idempotent' '\n> > -     test -f .git/index &&\n> > +     test_path_is_file .git/index &&\n>\n> The change is fine but I feel you can easily find files in which you can\n> do the same type of change but in a large quantity. This way you will\n> get an even better idea of how the tests work at Git. [...]\n>\n> Here if you try finding files which had commits over 11-12+ years ago,\n> you will find some ancient relics to modernise too!\n\nIf we consider that a microproject is meant to give a newcomer a taste\nof what it is like to contribute to the Git project -- submitting\npatches via email, interacting with reviewers, re-rolling a patch\nseries, etc. -- then keeping the submissions small and focussed is\npreferable to making them large and wide-ranging. This is especially\nso with changes like this which are primarily mechanical in nature;\nthey can easily lead to reviewer-fatigue when done in large numbers.\nSo, this is a case of smaller-is-better. As such, this submission is a\ngood size already.\n"},{"id":"407868","messageId":"20201018233612.GB4204@nand.local","threadId":"54456","inReplyTo":"20201018130219.GA6749@konoha","subject":"Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace 'test -[def]' with helpers","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-10-18T23:36:12Z","receivedAt":"2020-10-18T23:36:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Oct 18, 2020 at 06:32:19PM +0530, Shourya Shukla wrote:\n> 'git format-patch -v2 @~n', where 'n' is the number of commits which you\n> want to include in the patch. So in your case it will be:\n> 'git format-patch -v2 @~1' and a patch mail will be generated.\n\n(Note also that providing the name of the branch that yours is based off\nof is just as good, i.e., 'git format-patch -v2 master').\n\n> Also, you need not put the '[Outreachy-Microproject]' tag in the\n> subject, '[OUTREACHY]' will suffice.\n\nThanks for saying this.\n\n> Now, coming to the meat of the patch.\n>\n> > The test_path_is* functions provide debug-friendly upon failure.\n>\n> This commit can be redone to be even more better. This does not exactly\n> reflect what has been done. I understand that yes 'test_patch_is_*'\n> functions are better and why they are better. But where did you replace\n> them, this is left unanswered.\n>\n> This is one example of how the commit messages can be, not too verbose\n> and not too short, somewhere in the middle:\n> https://lore.kernel.org/git/20200118083326.9643-6-shouryashukla.oo@gmail.com/\n\nI'm actually perfectly happy with the patch text; let's not\novercomplicate something as straightforward as using a built-in test\nhelper instead of 'test -f'.\n\n> The change is fine but I feel you can easily find files in which you can\n> do the same type of change but in a large quantity. This way you will\n> get an even better idea of how the tests work at Git. To find such\n> files, one way can be to look here:\n> https://github.com/git/git/tree/master/t\n\nI'm also fine with Caleb just working on t0000; sending this patch on\nits own would usually look like unnecessary churn unless it was either\n(a) preparation for some other modification to t0000, and we want to\nstart from a modern-looking base, or (b) it is in the name of removing\n'test -f' from 't' en-masse, in which case I'd expect this to cover all\nof our tests.\n\nSince this is just to get Caleb's feet wet, I'm fine with them starting\nsmall :). Sending this patch to the mailing list in a good format with a\nwell-written commit message is exercise enough.\n\n> Here if you try finding files which had commits over 11-12+ years ago,\n> you will find some ancient relics to modernise too! Great that you took\n> Taylor's advice ;)\n\n:-)\n\nThanks,\nTaylor\n"}]}