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

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

[Caleb Tillman]

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

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"
 '
-- 
2.25.1

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

[Shourya Shukla]

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.

> The test_path_is* functions provide debug-friendly upon failure.

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/

> 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

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

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

[Christian Couder]

Hi Shourya and Caleb,

On Sun, Oct 18, 2020 at 4:12 PM Shourya Shukla
<shouryashukla.oo@gmail.com> 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@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.

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

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

[Eric Sunshine]

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.

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

[Taylor Blau]

On Sun, Oct 18, 2020 at 06:32:19PM +0530, Shourya Shukla wrote:
> '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.

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

> Also, you need not put the '[Outreachy-Microproject]' tag in the
> subject, '[OUTREACHY]' will suffice.

Thanks for saying this.

> Now, coming to the meat of the patch.
>
> > The test_path_is* functions provide debug-friendly upon failure.
>
> 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/

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

> 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

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.

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

:-)

Thanks,
Taylor