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

[Eric Sunshine <sunshine@sunshineco.com>]

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.