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

Commit Message

Joey Salazar Oct. 19, 2020, 4:26 a.m. UTC

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.

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/[e-mail]/


Modernized the test by replacing 'test -e' instances with
test_path_is_file helper functions.

Signed-off-by: JoeyS <[e-mail]>
---
 t/t7006-pager.sh | 84 ++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 42 deletions(-)

--
2.29.0.rc2

Comments

Phillip Wood Oct. 19, 2020, 11:28 a.m. UTC | #1

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

> 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/[e-mail]/
> 

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.

> Modernized the test by replacing 'test -e' instances with
> test_path_is_file helper functions.

s/Modernized/Modernize/

I've got some comments about the conversions of `! test -e` below


> Signed-off-by: JoeyS <[e-mail]>
> ---
>   t/t7006-pager.sh | 84 ++++++++++++++++++++++++------------------------
>   1 file changed, 42 insertions(+), 42 deletions(-)
> 
> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
> index 00e09a375c..1d0f75e34e 100755
> --- a/t/t7006-pager.sh
> +++ b/t/t7006-pager.sh
> @@ -19,7 +19,7 @@ test_expect_success 'setup' '
>   test_expect_success TTY 'some commands use a pager' '
>   	rm -f paginated.out &&
>   	test_terminal git log &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '

This and all the other conversions of `test -e` look fine

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

Best Wishes

Phillip

>   '
> 
>   test_expect_success 'no pager when stdout is a pipe' '
>   	rm -f paginated.out &&
>   	git log | cat &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success 'no pager when stdout is a regular file' '
>   	rm -f paginated.out &&
>   	git log >file &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git --paginate rev-list uses a pager' '
>   	rm -f paginated.out &&
>   	test_terminal git --paginate rev-list HEAD &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success 'no pager even with --paginate when stdout is a pipe' '
>   	rm -f file paginated.out &&
>   	git --paginate log | cat &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'no pager with --no-pager' '
>   	rm -f paginated.out &&
>   	test_terminal git --no-pager log &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'configuration can disable pager' '
>   	rm -f paginated.out &&
>   	test_unconfig pager.grep &&
>   	test_terminal git grep initial &&
> -	test -e paginated.out &&
> +	test_path_is_file paginated.out &&
> 
>   	rm -f paginated.out &&
>   	test_config pager.grep false &&
>   	test_terminal git grep initial &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'configuration can enable pager (from subdir)' '
> @@ -122,107 +122,107 @@ test_expect_success TTY 'configuration can enable pager (from subdir)' '
>   		test_terminal git bundle unbundle ../test.bundle
>   	) &&
>   	{
> -		test -e paginated.out ||
> -		test -e subdir/paginated.out
> +		test_path_is_file paginated.out ||
> +		test_path_is_file subdir/paginated.out
>   	}
>   '
> 
>   test_expect_success TTY 'git tag -l defaults to paging' '
>   	rm -f paginated.out &&
>   	test_terminal git tag -l &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag -l respects pager.tag' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag=false tag -l &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag -l respects --no-pager' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag --no-pager tag -l &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag with no args defaults to paging' '
>   	# no args implies -l so this should page like -l
>   	rm -f paginated.out &&
>   	test_terminal git tag &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag with no args respects pager.tag' '
>   	# no args implies -l so this should page like -l
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag=false tag &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag --contains defaults to paging' '
>   	# --contains implies -l so this should page like -l
>   	rm -f paginated.out &&
>   	test_terminal git tag --contains &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag --contains respects pager.tag' '
>   	# --contains implies -l so this should page like -l
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag=false tag --contains &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag -a defaults to not paging' '
>   	test_when_finished "git tag -d newtag" &&
>   	rm -f paginated.out &&
>   	test_terminal git tag -am message newtag &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag -a ignores pager.tag' '
>   	test_when_finished "git tag -d newtag" &&
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag tag -am message newtag &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag -a respects --paginate' '
>   	test_when_finished "git tag -d newtag" &&
>   	rm -f paginated.out &&
>   	test_terminal git --paginate tag -am message newtag &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
>   	test_when_finished "git tag -d newtag" &&
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.tag=false -c alias.t=tag t -l &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git branch defaults to paging' '
>   	rm -f paginated.out &&
>   	test_terminal git branch &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git branch respects pager.branch' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.branch=false branch &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git branch respects --no-pager' '
>   	rm -f paginated.out &&
>   	test_terminal git --no-pager branch &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git branch --edit-description ignores pager.branch' '
> @@ -232,8 +232,8 @@ test_expect_success TTY 'git branch --edit-description ignores pager.branch' '
>   		touch editor.used
>   	EOF
>   	EDITOR=./editor test_terminal git -c pager.branch branch --edit-description &&
> -	! test -e paginated.out &&
> -	test -e editor.used
> +	! test_path_is_file paginated.out &&
> +	test_path_is_file editor.used
>   '
> 
>   test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
> @@ -242,13 +242,13 @@ test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
>   	test_when_finished "git branch -D other" &&
>   	test_terminal git -c pager.branch branch --set-upstream-to=other &&
>   	test_when_finished "git branch --unset-upstream" &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git config ignores pager.config when setting' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.config config foo.bar bar &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git config --edit ignores pager.config' '
> @@ -257,33 +257,33 @@ test_expect_success TTY 'git config --edit ignores pager.config' '
>   		touch editor.used
>   	EOF
>   	EDITOR=./editor test_terminal git -c pager.config config --edit &&
> -	! test -e paginated.out &&
> -	test -e editor.used
> +	! test_path_is_file paginated.out &&
> +	test_path_is_file editor.used
>   '
> 
>   test_expect_success TTY 'git config --get ignores pager.config' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.config config --get foo.bar &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git config --get-urlmatch defaults to paging' '
>   	rm -f paginated.out &&
>   	test_terminal git -c http."https://foo.com/".bar=foo \
>   			  config --get-urlmatch http https://foo.com &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git config --get-all respects pager.config' '
>   	rm -f paginated.out &&
>   	test_terminal git -c pager.config=false config --get-all foo.bar &&
> -	! test -e paginated.out
> +	! test_path_is_file paginated.out
>   '
> 
>   test_expect_success TTY 'git config --list defaults to paging' '
>   	rm -f paginated.out &&
>   	test_terminal git config --list &&
> -	test -e paginated.out
> +	test_path_is_file paginated.out
>   '
> 
> 
> @@ -392,7 +392,7 @@ test_default_pager() {
>   			export PATH &&
>   			$full_command
>   		) &&
> -		test -e default_pager_used
> +		test_path_is_file default_pager_used
>   	"
>   }
> 
> @@ -406,7 +406,7 @@ test_PAGER_overrides() {
>   		PAGER='wc >PAGER_used' &&
>   		export PAGER &&
>   		$full_command &&
> -		test -e PAGER_used
> +		test_path_is_file PAGER_used
>   	"
>   }
> 
> @@ -432,7 +432,7 @@ test_core_pager() {
>   		export PAGER &&
>   		test_config core.pager 'wc >core.pager_used' &&
>   		$full_command &&
> -		${if_local_config}test -e core.pager_used
> +		${if_local_config}test_path_is_file core.pager_used
>   	"
>   }
> 
> @@ -464,7 +464,7 @@ test_pager_subdir_helper() {
>   			cd sub &&
>   			$full_command
>   		) &&
> -		${if_local_config}test -e core.pager_used
> +		${if_local_config}test_path_is_file core.pager_used
>   	"
>   }
> 
> @@ -477,7 +477,7 @@ test_GIT_PAGER_overrides() {
>   		GIT_PAGER='wc >GIT_PAGER_used' &&
>   		export GIT_PAGER &&
>   		$full_command &&
> -		test -e GIT_PAGER_used
> +		test_path_is_file GIT_PAGER_used
>   	"
>   }
> 
> @@ -489,7 +489,7 @@ test_doesnt_paginate() {
>   		GIT_PAGER='wc >GIT_PAGER_used' &&
>   		export GIT_PAGER &&
>   		$full_command &&
> -		! test -e GIT_PAGER_used
> +		! test_path_is_file GIT_PAGER_used
>   	"
>   }
> 
> --
> 2.29.0.rc2
>

Taylor Blau Oct. 19, 2020, 8:28 p.m. UTC | #2

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/[e-mail]/
> >
>
> 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

Emily Shaffer Oct. 20, 2020, 12:11 a.m. UTC | #3

On Mon, Oct 19, 2020 at 04:26:07AM +0000, Joey S wrote:
> 
> Hi everyone,
Hi Joey and welcome.

> 
> Signed-off-by: JoeyS <[e-mail]>

One thing missed by other commenters: the Developer's Certificate of
Origin line - which is what this indicates - should have your "full
name". So in my case, I sign my patches 'Emily Shaffer
<[e-mail]>'. If I'm wrong that's fine, but JoeyS sounds
like a name and initial rather than a full name.

The rest of the patch looks okay to me, though.

 - Emily

Junio C Hamano Oct. 20, 2020, 1:59 a.m. UTC | #4

Emily Shaffer <[e-mail]> writes:

> On Mon, Oct 19, 2020 at 04:26:07AM +0000, Joey S wrote:
>> 
>> Hi everyone,
> Hi Joey and welcome.
>
>> 
>> Signed-off-by: JoeyS <[e-mail]>
>
> One thing missed by other commenters: the Developer's Certificate of
> Origin line - which is what this indicates - should have your "full
> name".

... and it must match the authorship.

> So in my case, I sign my patches 'Emily Shaffer
> <[e-mail]>'. If I'm wrong that's fine, but JoeyS sounds
> like a name and initial rather than a full name.

Thanks for pointing it out.

If somebody from the "mentoring" group is taking a tally, it might
not be a bad idea to identify which style and procedure rules are
often failed to be followed by new contributors so that we can
figure out ways to make them stand out in our documentation set
(e.g. Documentation/SubmittingPatches but maybe a separate cheat
sheet might be worth having).

Thanks.

Joey Salazar Oct. 20, 2020, 7:24 a.m. UTC | #5

Hi everyone,

Thank you very much for the input and feedback, it's much appreciated.

> 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.
>
Understood, thank you.

> > Modernized the test by replacing 'test -e' instances with
> > test_path_is_file helper functions.
>
> s/Modernized/Modernize/
Will do in the amended commit next.

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

Thank you for the explanation : )

After replacing `! test -e` with `! test_path_is_missing paginated.out` however, the changed test cases are failing;
```
$ cd t/ && prove t7006-pager.sht7006-pager.sh .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 3/101 subtests

Test Summary Report
-------------------
t7006-pager.sh (Wstat: 256 Tests: 101 Failed: 3)
  Failed tests:  7-9
  Non-zero exit status: 1
Files=1, Tests=101,  5 wallclock secs ( 0.03 usr  0.00 sys +  3.49 cusr  0.65 csys =  4.17 CPU)
Result: FAIL
```
Is this the behavior I should be expecting?


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

Will keep this in mind, thank you Taylor.

> > One thing missed by other commenters: the Developer's Certificate of
> > Origin line - which is what this indicates - should have your "full
> > name".
>
> ... and it must match the authorship.

Changed, thank you both for catching that.

Thank you all,
Joey

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, October 19, 2020 7:59 PM, Junio C Hamano <[e-mail]> wrote:

> Emily Shaffer [e-mail] writes:
>
> > On Mon, Oct 19, 2020 at 04:26:07AM +0000, Joey S wrote:
> >
> > > Hi everyone,
> > > Hi Joey and welcome.
> >
> > > Signed-off-by: JoeyS [e-mail]
> >
> > One thing missed by other commenters: the Developer's Certificate of
> > Origin line - which is what this indicates - should have your "full
> > name".
>
> ... and it must match the authorship.
>
> > So in my case, I sign my patches 'Emily Shaffer
> > [e-mail]'. If I'm wrong that's fine, but JoeyS sounds
> > like a name and initial rather than a full name.
>
> Thanks for pointing it out.
>
> If somebody from the "mentoring" group is taking a tally, it might
> not be a bad idea to identify which style and procedure rules are
> often failed to be followed by new contributors so that we can
> figure out ways to make them stand out in our documentation set
> (e.g. Documentation/SubmittingPatches but maybe a separate cheat
> sheet might be worth having).
>
> Thanks.

Phillip Wood Oct. 20, 2020, 12:19 p.m. UTC | #6

Hi Joey

On 20/10/2020 08:24, Joey S wrote:
> Hi everyone,
> 
> Thank you very much for the input and feedback, it's much appreciated.
> 
>> 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.
>>
> Understood, thank you.
> 
>>> Modernized the test by replacing 'test -e' instances with
>>> test_path_is_file helper functions.
>>
>> s/Modernized/Modernize/
> Will do in the amended commit next.
> 
>>> -   ! 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.
> 
> Thank you for the explanation : )
> 
> After replacing `! test -e` with `! test_path_is_missing paginated.out` however, the changed test cases are failing;
> ```
> $ cd t/ && prove t7006-pager.sht7006-pager.sh .. Dubious, test returned 1 (wstat 256, 0x100)
> Failed 3/101 subtests
> 
> Test Summary Report
> -------------------
> t7006-pager.sh (Wstat: 256 Tests: 101 Failed: 3)
>    Failed tests:  7-9
>    Non-zero exit status: 1
> Files=1, Tests=101,  5 wallclock secs ( 0.03 usr  0.00 sys +  3.49 cusr  0.65 csys =  4.17 CPU)
> Result: FAIL
> ```
> Is this the behavior I should be expecting?

No it's not! As one aspect of this process is to help candidates improve 
their understanding I'll give you a hint rather than the whole answer. 
`test -e <path>` checks whether <path> exists and exits 0 if it does and 
the shell treats an exit code of 0 as success. `!` inverts the 
success/failure of the command  that follows it. Using that and looking 
at the definition of test_file_is_missing in t/test-lib-functions.sh see 
if you can fix the conversion so that the tests pass. If you get stuck 
do let me know and I'll try and help some more.

Best Wishes

Phillip

>> ...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.
> 
> Will keep this in mind, thank you Taylor.
> 
>>> One thing missed by other commenters: the Developer's Certificate of
>>> Origin line - which is what this indicates - should have your "full
>>> name".
>>
>> ... and it must match the authorship.
> 
> Changed, thank you both for catching that.
> 
> Thank you all,
> Joey
> 
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Monday, October 19, 2020 7:59 PM, Junio C Hamano <[e-mail]> wrote:
> 
>> Emily Shaffer [e-mail] writes:
>>
>>> On Mon, Oct 19, 2020 at 04:26:07AM +0000, Joey S wrote:
>>>
>>>> Hi everyone,
>>>> Hi Joey and welcome.
>>>
>>>> Signed-off-by: JoeyS [e-mail]
>>>
>>> One thing missed by other commenters: the Developer's Certificate of
>>> Origin line - which is what this indicates - should have your "full
>>> name".
>>
>> ... and it must match the authorship.
>>
>>> So in my case, I sign my patches 'Emily Shaffer
>>> [e-mail]'. If I'm wrong that's fine, but JoeyS sounds
>>> like a name and initial rather than a full name.
>>
>> Thanks for pointing it out.
>>
>> If somebody from the "mentoring" group is taking a tally, it might
>> not be a bad idea to identify which style and procedure rules are
>> often failed to be followed by new contributors so that we can
>> figure out ways to make them stand out in our documentation set
>> (e.g. Documentation/SubmittingPatches but maybe a separate cheat
>> sheet might be worth having).
>>
>> Thanks.
> 
>

Joey Salazar Oct. 20, 2020, 7:33 p.m. UTC | #7

Hi Phillip,

> `test -e <path>` checks whether <path> exists and exits 0 if it does and
> the shell treats an exit code of 0 as success. `!` inverts the
> success/failure of the command that follows it. Using that and looking
> at the definition of test_file_is_missing in t/test-lib-functions.sh see
> if you can fix the conversion so that the tests pass.

Thank you, that makes sense, now the test cases with `! test -e` replaced with `test_path_is_missing paginated.out` all pass.

Sending PATCH v2 next.

Thank you for your help,

Joey



‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, October 20, 2020 6:19 AM, Phillip Wood <[e-mail]> wrote:

> Hi Joey
>
> On 20/10/2020 08:24, Joey S wrote:
>
> > Hi everyone,
> > Thank you very much for the input and feedback, it's much appreciated.
> >
> > > 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.
> >
> > Understood, thank you.
> >
> > > > Modernized the test by replacing 'test -e' instances with
> > > > test_path_is_file helper functions.
> > >
> > > s/Modernized/Modernize/
> > > Will do in the amended commit next.
> >
> > > > -   ! 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.
> >
> > Thank you for the explanation : )
> > After replacing `! test -e` with `! test_path_is_missing paginated.out` however, the changed test cases are failing;
> >
> >     $ cd t/ && prove t7006-pager.sht7006-pager.sh .. Dubious, test returned 1 (wstat 256, 0x100)
> >     Failed 3/101 subtests
> >
> >     Test Summary Report
> >     -------------------
> >     t7006-pager.sh (Wstat: 256 Tests: 101 Failed: 3)
> >        Failed tests:  7-9
> >        Non-zero exit status: 1
> >     Files=1, Tests=101,  5 wallclock secs ( 0.03 usr  0.00 sys +  3.49 cusr  0.65 csys =  4.17 CPU)
> >     Result: FAIL
> >
> >
> > Is this the behavior I should be expecting?
>
> No it's not! As one aspect of this process is to help candidates improve
> their understanding I'll give you a hint rather than the whole answer.
> `test -e <path>` checks whether <path> exists and exits 0 if it does and
> the shell treats an exit code of 0 as success. `!` inverts the
> success/failure of the command that follows it. Using that and looking
> at the definition of test_file_is_missing in t/test-lib-functions.sh see
> if you can fix the conversion so that the tests pass. If you get stuck
> do let me know and I'll try and help some more.
>
> Best Wishes
>
> Phillip
>
> > > ...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.
> >
> > Will keep this in mind, thank you Taylor.
> >
> > > > One thing missed by other commenters: the Developer's Certificate of
> > > > Origin line - which is what this indicates - should have your "full
> > > > name".
> > >
> > > ... and it must match the authorship.
> >
> > Changed, thank you both for catching that.
> > Thank you all,
> > Joey
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Monday, October 19, 2020 7:59 PM, Junio C Hamano [e-mail] wrote:
> >
> > > Emily Shaffer [e-mail] writes:
> > >
> > > > On Mon, Oct 19, 2020 at 04:26:07AM +0000, Joey S wrote:
> > > >
> > > > > Hi everyone,
> > > > > Hi Joey and welcome.
> > > >
> > > > > Signed-off-by: JoeyS [e-mail]
> > > >
> > > > One thing missed by other commenters: the Developer's Certificate of
> > > > Origin line - which is what this indicates - should have your "full
> > > > name".
> > >
> > > ... and it must match the authorship.
> > >
> > > > So in my case, I sign my patches 'Emily Shaffer
> > > > [e-mail]'. If I'm wrong that's fine, but JoeyS sounds
> > > > like a name and initial rather than a full name.
> > >
> > > Thanks for pointing it out.
> > > If somebody from the "mentoring" group is taking a tally, it might
> > > not be a bad idea to identify which style and procedure rules are
> > > often failed to be followed by new contributors so that we can
> > > figure out ways to make them stand out in our documentation set
> > > (e.g. Documentation/SubmittingPatches but maybe a separate cheat
> > > sheet might be worth having).
> > > Thanks.

Patch

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 00e09a375c..1d0f75e34e 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -19,7 +19,7 @@  test_expect_success 'setup' '
 test_expect_success TTY 'some commands use a pager' '
 	rm -f paginated.out &&
 	test_terminal git log &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 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
 '

 test_expect_success 'no pager when stdout is a pipe' '
 	rm -f paginated.out &&
 	git log | cat &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success 'no pager when stdout is a regular file' '
 	rm -f paginated.out &&
 	git log >file &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git --paginate rev-list uses a pager' '
 	rm -f paginated.out &&
 	test_terminal git --paginate rev-list HEAD &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success 'no pager even with --paginate when stdout is a pipe' '
 	rm -f file paginated.out &&
 	git --paginate log | cat &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'no pager with --no-pager' '
 	rm -f paginated.out &&
 	test_terminal git --no-pager log &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'configuration can disable pager' '
 	rm -f paginated.out &&
 	test_unconfig pager.grep &&
 	test_terminal git grep initial &&
-	test -e paginated.out &&
+	test_path_is_file paginated.out &&

 	rm -f paginated.out &&
 	test_config pager.grep false &&
 	test_terminal git grep initial &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'configuration can enable pager (from subdir)' '
@@ -122,107 +122,107 @@  test_expect_success TTY 'configuration can enable pager (from subdir)' '
 		test_terminal git bundle unbundle ../test.bundle
 	) &&
 	{
-		test -e paginated.out ||
-		test -e subdir/paginated.out
+		test_path_is_file paginated.out ||
+		test_path_is_file subdir/paginated.out
 	}
 '

 test_expect_success TTY 'git tag -l defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git tag -l &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag -l respects pager.tag' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag=false tag -l &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag -l respects --no-pager' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag --no-pager tag -l &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag with no args defaults to paging' '
 	# no args implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git tag &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag with no args respects pager.tag' '
 	# no args implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag=false tag &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag --contains defaults to paging' '
 	# --contains implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git tag --contains &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag --contains respects pager.tag' '
 	# --contains implies -l so this should page like -l
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag=false tag --contains &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag -a defaults to not paging' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git tag -am message newtag &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag -a ignores pager.tag' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag tag -am message newtag &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag -a respects --paginate' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git --paginate tag -am message newtag &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag as alias ignores pager.tag with -a' '
 	test_when_finished "git tag -d newtag" &&
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag -c alias.t=tag t -am message newtag &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git tag as alias respects pager.tag with -l' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.tag=false -c alias.t=tag t -l &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git branch defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git branch &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git branch respects pager.branch' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.branch=false branch &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git branch respects --no-pager' '
 	rm -f paginated.out &&
 	test_terminal git --no-pager branch &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git branch --edit-description ignores pager.branch' '
@@ -232,8 +232,8 @@  test_expect_success TTY 'git branch --edit-description ignores pager.branch' '
 		touch editor.used
 	EOF
 	EDITOR=./editor test_terminal git -c pager.branch branch --edit-description &&
-	! test -e paginated.out &&
-	test -e editor.used
+	! test_path_is_file paginated.out &&
+	test_path_is_file editor.used
 '

 test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
@@ -242,13 +242,13 @@  test_expect_success TTY 'git branch --set-upstream-to ignores pager.branch' '
 	test_when_finished "git branch -D other" &&
 	test_terminal git -c pager.branch branch --set-upstream-to=other &&
 	test_when_finished "git branch --unset-upstream" &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git config ignores pager.config when setting' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.config config foo.bar bar &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git config --edit ignores pager.config' '
@@ -257,33 +257,33 @@  test_expect_success TTY 'git config --edit ignores pager.config' '
 		touch editor.used
 	EOF
 	EDITOR=./editor test_terminal git -c pager.config config --edit &&
-	! test -e paginated.out &&
-	test -e editor.used
+	! test_path_is_file paginated.out &&
+	test_path_is_file editor.used
 '

 test_expect_success TTY 'git config --get ignores pager.config' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.config config --get foo.bar &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git config --get-urlmatch defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git -c http."https://foo.com/".bar=foo \
 			  config --get-urlmatch http https://foo.com &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '

 test_expect_success TTY 'git config --get-all respects pager.config' '
 	rm -f paginated.out &&
 	test_terminal git -c pager.config=false config --get-all foo.bar &&
-	! test -e paginated.out
+	! test_path_is_file paginated.out
 '

 test_expect_success TTY 'git config --list defaults to paging' '
 	rm -f paginated.out &&
 	test_terminal git config --list &&
-	test -e paginated.out
+	test_path_is_file paginated.out
 '


@@ -392,7 +392,7 @@  test_default_pager() {
 			export PATH &&
 			$full_command
 		) &&
-		test -e default_pager_used
+		test_path_is_file default_pager_used
 	"
 }

@@ -406,7 +406,7 @@  test_PAGER_overrides() {
 		PAGER='wc >PAGER_used' &&
 		export PAGER &&
 		$full_command &&
-		test -e PAGER_used
+		test_path_is_file PAGER_used
 	"
 }

@@ -432,7 +432,7 @@  test_core_pager() {
 		export PAGER &&
 		test_config core.pager 'wc >core.pager_used' &&
 		$full_command &&
-		${if_local_config}test -e core.pager_used
+		${if_local_config}test_path_is_file core.pager_used
 	"
 }

@@ -464,7 +464,7 @@  test_pager_subdir_helper() {
 			cd sub &&
 			$full_command
 		) &&
-		${if_local_config}test -e core.pager_used
+		${if_local_config}test_path_is_file core.pager_used
 	"
 }

@@ -477,7 +477,7 @@  test_GIT_PAGER_overrides() {
 		GIT_PAGER='wc >GIT_PAGER_used' &&
 		export GIT_PAGER &&
 		$full_command &&
-		test -e GIT_PAGER_used
+		test_path_is_file GIT_PAGER_used
 	"
 }

@@ -489,7 +489,7 @@  test_doesnt_paginate() {
 		GIT_PAGER='wc >GIT_PAGER_used' &&
 		export GIT_PAGER &&
 		$full_command &&
-		! test -e GIT_PAGER_used
+		! test_path_is_file GIT_PAGER_used
 	"
 }