Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace 'test -[def]' with helpers

From: Christian Couder <christian.couder@gmail.com>
Date: Oct 18, 2020, 18:38 UTC
Message-ID: <CAP8UFD1Ux7uu637_0NF4TCPJq4++KARkRm+g26ou9AdZb06OcA@mail.gmail.com>
In-Reply-To: <20201018130219.GA6749@konoha>

Hi Shourya and Caleb,

On Sun, Oct 18, 2020 at 4:12 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote:

Show 15 quoted lines

>
> 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.

Show 24 quoted lines

> > 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.