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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 18, 2020, 18:42 UTC
Message-ID: <CAPig+cR+Jkg3ymFGBXtmLUEB7+XbgX0HvkjV3Z_9Lvk6qsY0qA@mail.gmail.com>
In-Reply-To: <20201018130219.GA6749@konoha>

On Sun, Oct 18, 2020 at 9:02 AM Shourya Shukla <shouryashukla.oo@gmail.com> wrote:

Show 9 quoted lines

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

Show 12 quoted lines

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