* Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace 'test -[def]' with helpers
2020-10-18 13:02 ` Shourya Shukla
@ 2020-10-18 18:38 ` Christian Couder
2020-10-18 18:42 ` Eric Sunshine
2020-10-18 23:36 ` Taylor Blau
2 siblings, 0 replies; 5+ messages in thread
From: Christian Couder @ 2020-10-18 18:38 UTC (permalink / raw)
To: Shourya Shukla, Caleb Tillman; +Cc: git
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace 'test -[def]' with helpers
2020-10-18 13:02 ` Shourya Shukla
2020-10-18 18:38 ` Christian Couder
@ 2020-10-18 18:42 ` Eric Sunshine
2020-10-18 23:36 ` Taylor Blau
2 siblings, 0 replies; 5+ messages in thread
From: Eric Sunshine @ 2020-10-18 18:42 UTC (permalink / raw)
To: Shourya Shukla; +Cc: caleb.tillman, Git List
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace 'test -[def]' with helpers
2020-10-18 13:02 ` Shourya Shukla
2020-10-18 18:38 ` Christian Couder
2020-10-18 18:42 ` Eric Sunshine
@ 2020-10-18 23:36 ` Taylor Blau
2 siblings, 0 replies; 5+ messages in thread
From: Taylor Blau @ 2020-10-18 23:36 UTC (permalink / raw)
To: Shourya Shukla; +Cc: caleb.tillman, git
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
^ permalink raw reply [flat|nested] 5+ messages in thread