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

From: Shourya Shukla <shouryashukla.oo@gmail.com>
Date: Oct 18, 2020, 13:02 UTC
Message-ID: <20201018130219.GA6749@konoha>
In-Reply-To: <20201018061052.32350-1-caleb.tillman@gmail.com>

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

Show 12 quoted lines

> 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