Hi Shourya and Caleb, On Sun, Oct 18, 2020 at 4:12 PM Shourya Shukla <shouryashukla.oo@xxxxxxxxx> 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@xxxxxxxxx/ > > 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@xxxxxxxxx/ 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@xxxxxxxxx> > --- > > 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.
