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@xxxxxxxxxxxxxx/ > > > > 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
