patcht0000: replace 'test -[def]' with helpers

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(-)

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.

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/

---

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:

Yeah, using "-v2" is definitely needed. It will put "[PATCH v2]" or "[PATCH v2 1/1]" in the subject.

I am ok with '[Outreachy-Microproject]' even if it's a bit longer.

s/debug-friendly/debug-friendly output/ would be more clear.

There is "t0000" in the subject which is enough.

I am not sure it is a very good example. I would be ok with the commit being a bit more verbose though.

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.

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:

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

Good advice.

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.

I'm having trouble parsing this. It is unclear what has been left unanswered.

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.

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:

(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').

Thanks for saying this.

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

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.

:-)

Thanks, Taylor