Re: [OUTREACHY][PATCH v1] t7006: Use test_path_is_* functions in test script

[Taylor Blau <me@ttaylorr.com>]

On Mon, Oct 19, 2020 at 12:28:33PM +0100, Phillip Wood wrote:
> Hi Joey
>
> On 19/10/2020 05:26, Joey S wrote:
> > Hi everyone,
> >
> > This is my first contribution to Git's public repo and, after using Git for several years, I'm very looking forward to becoming an active member of the community.
>
> Welcome to the list

Indeed :-).

> > In this patch for test t7006-pager, I have:
> >
> >    - ensured the guidelines[1] were followed
> >    - used the helper function 'test_path_is_file()' to replace all found instances of 'test -e'
> >
> > Please find the output of 'git format-patch' below.
> >
> > Thank you all, looking forward to your feedback and observations,
> >
> > Joey
> >
> > [1] lore.kernel.org/git/CAPig+cQpUu2UO-+jWn1nTaDykWnxwuEitzVB7PnW2SS_b7V8Hg@mail.gmail.com/
> >
>
> All this text above is useful context for reviewers but appears as part of
> the commit message which is not what you want. If you add notes after the
> `---` line below then they will not end up in the commit message.

...Alternatively, this would fit just fine in a cover letter. Usually
cover letters are not necessary for single patches (where the patch
message itself conveys the full message, or a little bit of additional
context below the triple-dash line is all that's necessary to clarify
the intent). But, if you want to introduce yourself, a 0/1 cover letter
is fine, too.

> >   test_expect_failure TTY 'pager runs from subdir' '
> > @@ -65,49 +65,49 @@ test_expect_success !MINGW,TTY 'LESS and LV envvars set by git-sh-setup' '
> >   test_expect_success TTY 'some commands do not use a pager' '
> >   	rm -f paginated.out &&
> >   	test_terminal git rev-list HEAD &&
> > -	! test -e paginated.out
> > +	! test_path_is_file paginated.out
>
> It would be better to replace `! test -e` this with `test_path_is_missing`
> as the modified test will pass if paginated.out exists but is not a file.
> `test_path_is_missing` will print an appropriate diagnostic message as well.

Yup, great catch.

Thanks,
Taylor