| Message ID | 20201018061052.32350-1-caleb.tillman@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ac9b5475487c95c33ebd66a9c2207be63f07cc30 |
| Headers | show |
| Series | [Outreachy-Microproject,1/1] t0000: replace 'test -[def]' with helpers | expand |
Hello Caleb, I have some comments. First of all, I notice that this is a v2 of this PATCH: https://lore.kernel.org/git/20201018005522.217397-1-caleb.tillman@gmail.com/ So, I think that the subject of the mail should reflect the same. I believe that you have used 'git format-patch' to generate this mail therefore what you can do is: 'git format-patch -v2 @~n', where 'n' is the number of commits which you want to include in the patch. So in your case it will be: 'git format-patch -v2 @~1' and a patch mail will be generated. Also, you need not put the '[Outreachy-Microproject]' tag in the subject, '[OUTREACHY]' will suffice. Now, coming to the meat of the patch. > The test_path_is* functions provide debug-friendly upon failure. This commit can be redone to be even more better. This does not exactly reflect what has been done. I understand that yes 'test_patch_is_*' functions are better and why they are better. But where did you replace them, this is left unanswered. This is one example of how the commit messages can be, not too verbose and not too short, somewhere in the middle: https://lore.kernel.org/git/20200118083326.9643-6-shouryashukla.oo@gmail.com/ > Signed-off-by: Caleb Tillman <caleb.tillman@gmail.com> --- > Outreachy microproject, revised submission. > t/t0000-basic.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh > index 923281af93..eb99892a87 100755 > --- a/t/t0000-basic.sh > +++ b/t/t0000-basic.sh > @@ -1191,7 +1191,7 @@ test_expect_success 'writing this tree with --missing-ok' ' > test_expect_success 'git read-tree followed by write-tree should be idempotent' ' > rm -f .git/index && > git read-tree $tree && > - test -f .git/index && > + test_path_is_file .git/index && > newtree=$(git write-tree) && > test "$newtree" = "$tree" The change is fine but I feel you can easily find files in which you can do the same type of change but in a large quantity. This way you will get an even better idea of how the tests work at Git. To find such files, one way can be to look here: https://github.com/git/git/tree/master/t Here if you try finding files which had commits over 11-12+ years ago, you will find some ancient relics to modernise too! Great that you took Taylor's advice ;) Best of luck, Shourya Shukla
Hi Shourya and Caleb, On Sun, Oct 18, 2020 at 4:12 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > > Hello Caleb, > > I have some comments. > > First of all, I notice that this is a v2 of this PATCH: > https://lore.kernel.org/git/20201018005522.217397-1-caleb.tillman@gmail.com/ > > So, I think that the subject of the mail should reflect the same. I > believe that you have used 'git format-patch' to generate this mail > therefore what you can do is: > > 'git format-patch -v2 @~n', where 'n' is the number of commits which you > want to include in the patch. So in your case it will be: > 'git format-patch -v2 @~1' and a patch mail will be generated. Yeah, using "-v2" is definitely needed. It will put "[PATCH v2]" or "[PATCH v2 1/1]" in the subject. > Also, you need not put the '[Outreachy-Microproject]' tag in the > subject, '[OUTREACHY]' will suffice. I am ok with '[Outreachy-Microproject]' even if it's a bit longer. > Now, coming to the meat of the patch. > > > The test_path_is* functions provide debug-friendly upon failure. s/debug-friendly/debug-friendly output/ would be more clear. > This commit can be redone to be even more better. This does not exactly > reflect what has been done. I understand that yes 'test_patch_is_*' > functions are better and why they are better. But where did you replace > them, this is left unanswered. There is "t0000" in the subject which is enough. > This is one example of how the commit messages can be, not too verbose > and not too short, somewhere in the middle: > https://lore.kernel.org/git/20200118083326.9643-6-shouryashukla.oo@gmail.com/ I am not sure it is a very good example. I would be ok with the commit being a bit more verbose though. > > Signed-off-by: Caleb Tillman <caleb.tillman@gmail.com> > --- > > Outreachy microproject, revised submission. > > t/t0000-basic.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh > > index 923281af93..eb99892a87 100755 > > --- a/t/t0000-basic.sh > > +++ b/t/t0000-basic.sh > > @@ -1191,7 +1191,7 @@ test_expect_success 'writing this tree with --missing-ok' ' > > test_expect_success 'git read-tree followed by write-tree should be idempotent' ' > > rm -f .git/index && > > git read-tree $tree && > > - test -f .git/index && > > + test_path_is_file .git/index && > > newtree=$(git write-tree) && > > test "$newtree" = "$tree" > > The change is fine but I feel you can easily find files in which you can > do the same type of change but in a large quantity. This way you will > get an even better idea of how the tests work at Git. To find such > files, one way can be to look here: > https://github.com/git/git/tree/master/t We actually don't want that for most microprojects. On https://git.github.io/Outreachy-21-Microprojects/ we ask it to be done on only one test script. > Here if you try finding files which had commits over 11-12+ years ago, > you will find some ancient relics to modernise too! Great that you took > Taylor's advice ;) No need to find a really old test script for this microproject as I think some 'test -[def]' uses have been introduced not too long ago. Thanks both, Christian.
On Sun, Oct 18, 2020 at 9:02 AM Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > First of all, I notice that this is a v2 of this PATCH: > > So, I think that the subject of the mail should reflect the same. I > believe that you have used 'git format-patch' to generate this mail > therefore what you can do is: > > 'git format-patch -v2 @~n', where 'n' is the number of commits which you > want to include in the patch. So in your case it will be: > 'git format-patch -v2 @~1' and a patch mail will be generated. Even simpler is to use the short form -<n> where <n> is the number of patches you want to format, so this can become: git format-patch -v2 -1 > Also, you need not put the '[Outreachy-Microproject]' tag in the > subject, '[OUTREACHY]' will suffice. Good advice. > > t0000: replace 'test -[def]' with helpers > > > > The test_path_is* functions provide debug-friendly upon failure. Since this patch is replacing only a single `test -f` (and not touching anything of the form `test -d` or `test -e`), it would be more accurate and reviewer-friendly to be explicit and say only `test -f` in the subject, and `test_path_is_file` in the body rather than making the commit message unnecessarily and overly generic. > This commit can be redone to be even more better. This does not exactly > reflect what has been done. I understand that yes 'test_patch_is_*' > functions are better and why they are better. But where did you replace > them, this is left unanswered. I'm having trouble parsing this. It is unclear what has been left unanswered. > This is one example of how the commit messages can be, not too verbose > and not too short, somewhere in the middle: > https://lore.kernel.org/git/20200118083326.9643-6-shouryashukla.oo@gmail.com/ It doesn't hurt to add a little back history as in the example you cite, but the commit message of this patch already does a reasonable job of explaining why this change is a good idea (specifically because `test_path_is_file` makes for a better debugging experience), so it doesn't necessarily need more explanation. > > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh > > @@ -1191,7 +1191,7 @@ test_expect_success 'writing this tree with --missing-ok' ' > > test_expect_success 'git read-tree followed by write-tree should be idempotent' ' > > - test -f .git/index && > > + test_path_is_file .git/index && > > The change is fine but I feel you can easily find files in which you can > do the same type of change but in a large quantity. This way you will > get an even better idea of how the tests work at Git. [...] > > Here if you try finding files which had commits over 11-12+ years ago, > you will find some ancient relics to modernise too! If we consider that a microproject is meant to give a newcomer a taste of what it is like to contribute to the Git project -- submitting patches via email, interacting with reviewers, re-rolling a patch series, etc. -- then keeping the submissions small and focussed is preferable to making them large and wide-ranging. This is especially so with changes like this which are primarily mechanical in nature; they can easily lead to reviewer-fatigue when done in large numbers. So, this is a case of smaller-is-better. As such, this submission is a good size already.
On Sun, Oct 18, 2020 at 06:32:19PM +0530, Shourya Shukla wrote: > 'git format-patch -v2 @~n', where 'n' is the number of commits which you > want to include in the patch. So in your case it will be: > 'git format-patch -v2 @~1' and a patch mail will be generated. (Note also that providing the name of the branch that yours is based off of is just as good, i.e., 'git format-patch -v2 master'). > Also, you need not put the '[Outreachy-Microproject]' tag in the > subject, '[OUTREACHY]' will suffice. Thanks for saying this. > Now, coming to the meat of the patch. > > > The test_path_is* functions provide debug-friendly upon failure. > > This commit can be redone to be even more better. This does not exactly > reflect what has been done. I understand that yes 'test_patch_is_*' > functions are better and why they are better. But where did you replace > them, this is left unanswered. > > This is one example of how the commit messages can be, not too verbose > and not too short, somewhere in the middle: > https://lore.kernel.org/git/20200118083326.9643-6-shouryashukla.oo@gmail.com/ I'm actually perfectly happy with the patch text; let's not overcomplicate something as straightforward as using a built-in test helper instead of 'test -f'. > The change is fine but I feel you can easily find files in which you can > do the same type of change but in a large quantity. This way you will > get an even better idea of how the tests work at Git. To find such > files, one way can be to look here: > https://github.com/git/git/tree/master/t I'm also fine with Caleb just working on t0000; sending this patch on its own would usually look like unnecessary churn unless it was either (a) preparation for some other modification to t0000, and we want to start from a modern-looking base, or (b) it is in the name of removing 'test -f' from 't' en-masse, in which case I'd expect this to cover all of our tests. Since this is just to get Caleb's feet wet, I'm fine with them starting small :). Sending this patch to the mailing list in a good format with a well-written commit message is exercise enough. > Here if you try finding files which had commits over 11-12+ years ago, > you will find some ancient relics to modernise too! Great that you took > Taylor's advice ;) :-) Thanks, Taylor
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 923281af93..eb99892a87 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -1191,7 +1191,7 @@ test_expect_success 'writing this tree with --missing-ok' ' test_expect_success 'git read-tree followed by write-tree should be idempotent' ' rm -f .git/index && git read-tree $tree && - test -f .git/index && + test_path_is_file .git/index && newtree=$(git write-tree) && test "$newtree" = "$tree" '
The test_path_is* functions provide debug-friendly upon failure. Signed-off-by: Caleb Tillman <caleb.tillman@gmail.com> --- Outreachy microproject, revised submission. t/t0000-basic.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)