[GSOC][PATCH 1/2] t1300: replace "test -f" into "test_path_is_file"

[GSOC][PATCH 1/2] t1300: replace "test -f" into "test_path_is_file"

[Adrian Wijaya]

Dear all,

In this year, I intend to apply in git for GSoC 2020. This is actually my 
first contribution to the git community. I am a sophomore Computer Science 
student at University of Indonesia.

In this patch, I have replaced 'test -f' into 'test_path_is_file' function 
(can be found in test-lib-functions).

Suggestions and advice are very welcome. 

Regards,
Adrian Wijaya

[GSOC][PATCH 2/2] t1300: replace "test -f" into "test_path_is_file"

[Adrian Wijaya]

Replace "test -f" into "test_path_is_file" to give more verbose
test output.

Signed-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>
---
 t/t1300-config.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 97ebfe1f9d..d74554fc09 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '
 	ln -s notyet myconfig &&
 	git config --file=myconfig test.frotz nitfol &&
 	test -h myconfig &&
-	test -f notyet &&
+	test_path_is_file notyet &&
 	test "z$(git config --file=notyet test.frotz)" = znitfol &&
 	git config --file=myconfig test.xyzzy rezrov &&
 	test -h myconfig &&
-	test -f notyet &&
+	test_path_is_file notyet &&
 	cat >expect <<-\EOF &&
 	nitfol
 	rezrov
-- 
2.26.0.rc1.11.g30e9940356

Re: [GSOC][PATCH 2/2] t1300: replace "test -f" into "test_path_is_file"

[Jeff King]

On Fri, Mar 20, 2020 at 06:47:23AM +0700, Adrian Wijaya wrote:

> [...]

Thanks, and welcome to the Git community. The patch looks pretty good to
me. A few minor nits:

> Subject: Re: [GSOC][PATCH 2/2] t1300: replace "test -f" into "test_path_is_file"

The subject says 2/2, but I think there is only one patch. :) Looks like
you used send-email; the --cover-letter option is probably what you
wanted to generate the first message. Though for a single-patch series,
I'd generally suggest just sending one email total, and putting any
comments below the "---" line (which would then not be included in the
commit message).

The general form of the subject line looks good, and follows our
conventions.

I'd suggest s/into/with/ in the subject line as a minor English fixup.
We'd often assume the maintainer will just fix up something small like
that while applying (or if he doesn't, that it's not too big a deal).
But since the point of the microproject is to get comfortable with the
patch submission process, maybe it would be good practice for you to fix
it up yourself (using "commit --amend" or "rebase -i") and re-send (try
git-send-email's "-v" option).

> Replace "test -f" into "test_path_is_file" to give more verbose
> test output.

Same s/into/with/ here, too (or perhaps s/Replace/Convert/).

Maybe worth saying "to give more verbose test output on failure", though
now I am really nit-picking (sorry, you avoided so many of the usual
first-time-patch pitfalls I have to stretch :) ).

> Signed-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>
> ---

You remembered your signoff. Good.

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 97ebfe1f9d..d74554fc09 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '
>  	ln -s notyet myconfig &&
>  	git config --file=myconfig test.frotz nitfol &&
>  	test -h myconfig &&
> -	test -f notyet &&
> +	test_path_is_file notyet &&

And the patch itself looks obviously correct.

The "test -h" in the context sticks out now, but we don't have a
test_path_is_symlink(). I think adding it goes beyond the scope of this
patch, and beyond what's needed for a microproject. But if you or
anybody wants to add it (modeled after test_path_is_file), it seems like
a reasonable thing for us to have.

-Peff

Re: [GSOC][PATCH 2/2] t1300: replace "test -f" into "test_path_is_file"

[adrian wijaya]

On Fri, Mar 20, 2020 at 12:52 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Mar 20, 2020 at 06:47:23AM +0700, Adrian Wijaya wrote:
>
> > [...]
>
> Thanks, and welcome to the Git community. The patch looks pretty good to
> me. A few minor nits:
>
> > Subject: Re: [GSOC][PATCH 2/2] t1300: replace "test -f" into "test_path_is_file"
>
> The subject says 2/2, but I think there is only one patch. :) Looks like
> you used send-email; the --cover-letter option is probably what you
> wanted to generate the first message. Though for a single-patch series,

Thanks for letting me know. Hmm, looks like I didn't get to see that part
when I looked at the documentation.

> I'd generally suggest just sending one email total, and putting any
> comments below the "---" line (which would then not be included in the
> commit message).

Got it.

>
> The general form of the subject line looks good, and follows our
> conventions.
>
> I'd suggest s/into/with/ in the subject line as a minor English fixup.
> We'd often assume the maintainer will just fix up something small like
> that while applying (or if he doesn't, that it's not too big a deal).
> But since the point of the microproject is to get comfortable with the
> patch submission process, maybe it would be good practice for you to fix
> it up yourself (using "commit --amend" or "rebase -i") and re-send (try
> git-send-email's "-v" option).
>
> > Replace "test -f" into "test_path_is_file" to give more verbose
> > test output.
>
> Same s/into/with/ here, too (or perhaps s/Replace/Convert/).
>

Sounds good. I will make a second version of this patch.

>
> Maybe worth saying "to give more verbose test output on failure", though
> now I am really nit-picking (sorry, you avoided so many of the usual
> first-time-patch pitfalls I have to stretch :) ).
>

No worries. Actually, I can learn something that will be useful for my next
contribution.

>
> > Signed-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>
> > ---
>
> You remembered your signoff. Good.
>
> > diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> > index 97ebfe1f9d..d74554fc09 100755
> > --- a/t/t1300-config.sh
> > +++ b/t/t1300-config.sh
> > @@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '
> >       ln -s notyet myconfig &&
> >       git config --file=myconfig test.frotz nitfol &&
> >       test -h myconfig &&
> > -     test -f notyet &&
> > +     test_path_is_file notyet &&
>
> And the patch itself looks obviously correct.

Thanks :)

>
> The "test -h" in the context sticks out now, but we don't have a
> test_path_is_symlink(). I think adding it goes beyond the scope of this
> patch, and beyond what's needed for a microproject. But if you or
> anybody wants to add it (modeled after test_path_is_file), it seems like
> a reasonable thing for us to have.
>
> -Peff


Never thought of that. I think I will make a feature request about it when
I have enough time.

Re: [GSOC][PATCH 1/2] t1300: replace "test -f" into "test_path_is_file"

[adrian wijaya]

From 5fb689207ebe06b5f4c506b0b184c95a8a2e529d Mon Sep 17 00:00:00 2001
From: Adrian Wijaya <adrianwijaya100@gmail.com>
Date: Thu, 19 Mar 2020 21:13:44 +0700
Subject: [GSOC][PATCH v2] t1300: convert "test -f" with "test_path_is_file"

Convert "test -f" with "test_path_is_file" to give more verbose
test output on failure.

Signed-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>
---
 t/t1300-config.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 97ebfe1f9d..d74554fc09 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '
     ln -s notyet myconfig &&
     git config --file=myconfig test.frotz nitfol &&
     test -h myconfig &&
-    test -f notyet &&
+    test_path_is_file notyet &&
     test "z$(git config --file=notyet test.frotz)" = znitfol &&
     git config --file=myconfig test.xyzzy rezrov &&
     test -h myconfig &&
-    test -f notyet &&
+    test_path_is_file notyet &&
     cat >expect <<-\EOF &&
     nitfol
     rezrov
-- 
2.26.0.rc1.11.g30e9940356

On Fri, Mar 20, 2020 at 6:47 AM Adrian Wijaya <adrianwijaya100@gmail.com> wrote:
>
> Dear all,
>
> In this year, I intend to apply in git for GSoC 2020. This is actually my
> first contribution to the git community. I am a sophomore Computer Science
> student at University of Indonesia.
>
> In this patch, I have replaced 'test -f' into 'test_path_is_file' function
> (can be found in test-lib-functions).
>
> Suggestions and advice are very welcome.
>
> Regards,
> Adrian Wijaya

[GSOC][PATCH v2] t1300: convert "test -f" with "test_path_is_file"

[Adrian Wijaya]

Convert "test -f" with "test_path_is_file" to give more verbose
test output on failure.

Signed-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>
---
 t/t1300-config.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 97ebfe1f9d..d74554fc09 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '
 	ln -s notyet myconfig &&
 	git config --file=myconfig test.frotz nitfol &&
 	test -h myconfig &&
-	test -f notyet &&
+	test_path_is_file notyet &&
 	test "z$(git config --file=notyet test.frotz)" = znitfol &&
 	git config --file=myconfig test.xyzzy rezrov &&
 	test -h myconfig &&
-	test -f notyet &&
+	test_path_is_file notyet &&
 	cat >expect <<-\EOF &&
 	nitfol
 	rezrov
-- 
2.26.0.rc1.11.g30e9940356

Re: [GSOC][PATCH v2] t1300: convert "test -f" with "test_path_is_file"

[Junio C Hamano]

Adrian Wijaya <adrianwijaya100@gmail.com> writes:

> Subject: Re: [GSOC][PATCH v2] t1300: convert "test -f" with "test_path_is_file"

I thought the suggestion was either "replace with" or "convert
into", but the above looks a bit funny cross between them.  I'd
probably go with "replace".

> Convert "test -f" with "test_path_is_file" to give more verbose
> test output on failure.

Likewise, but the first part is already said in the title.  So

	Subject: t1300: replace "test -f" with "test_path_is_file"

	This way, the test will not fail silently, making it easier
	to diagnose.

or something like that, perhaps.

> Signed-off-by: Adrian Wijaya <adrianwijaya100@gmail.com>
> ---
>  t/t1300-config.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 97ebfe1f9d..d74554fc09 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1020,11 +1020,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '
>  	ln -s notyet myconfig &&
>  	git config --file=myconfig test.frotz nitfol &&
>  	test -h myconfig &&
> -	test -f notyet &&
> +	test_path_is_file notyet &&
>  	test "z$(git config --file=notyet test.frotz)" = znitfol &&
>  	git config --file=myconfig test.xyzzy rezrov &&
>  	test -h myconfig &&
> -	test -f notyet &&
> +	test_path_is_file notyet &&
>  	cat >expect <<-\EOF &&
>  	nitfol
>  	rezrov

Re: [GSOC][PATCH v2] t1300: convert "test -f" with "test_path_is_file"

[Junio C Hamano]

Adrian Wijaya <adrianwijaya100@gmail.com> writes:

>  	test "z$(git config --file=notyet test.frotz)" = znitfol &&

We see 130+ instances of this rather antiquated construct that
presumably avoids to compare an empty string with something else
using "test X = Y" tool.  It might have been a good idea in 2005 (I
do not remember why we started doing this), but perhaps it is time
to clean them up by listing it as another microproject idea for the
next year?