git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [Outreachy-Microproject][PATCH 1/1] t0000: replace an instant of test -f with test_path_is_file functions.
@ 2020-10-18  0:55 Caleb Tillman
  2020-10-18  3:55 ` Taylor Blau
  0 siblings, 1 reply; 3+ messages in thread
From: Caleb Tillman @ 2020-10-18  0:55 UTC (permalink / raw)
  To: git; +Cc: Caleb Tillman

The test_path_is* functions provide debug-friendly messages upon failure.

Signed-off-by: Caleb Tillman <caleb.tillman@gmail.com>
--- Outrachy 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"
 '
-- 
2.25.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace an instant of test -f with test_path_is_file functions.
  2020-10-18  0:55 [Outreachy-Microproject][PATCH 1/1] t0000: replace an instant of test -f with test_path_is_file functions Caleb Tillman
@ 2020-10-18  3:55 ` Taylor Blau
  2020-10-18 20:11   ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Taylor Blau @ 2020-10-18  3:55 UTC (permalink / raw)
  To: Caleb Tillman; +Cc: git

Hi Caleb,

On Sun, Oct 18, 2020 at 12:55:22AM +0000, Caleb Tillman wrote:
> The test_path_is* functions provide debug-friendly messages upon failure.

The body looks fine to me. Your subject is getting a little long,
however. Typical guidance would be somewhere around 50 (at least in my
opinion, I thought we had something in Documentation/CodingGuidelines,
but I couldn't find anything).

Maybe something instead like:

  t0000: replace 'test -f' with helpers

or:

  t0000: modernize test style

If you're looking for inspiration, you can use `git log`'s `-S` flag to
look for anything that mentions 'test_path_is_file' to see how similar
patches have been written in the past. (When I was recommending
alternatives, I ran "git log --oneline -Stest_path_is_file -- t").

> Signed-off-by: Caleb Tillman <caleb.tillman@gmail.com>
> --- Outrachy Microproject, revised submission

I think that you meant to put this "Outrachy ..." _below_ the
triple-dash line. Incidentally, Git still applies this patch just fine,
but it is easier for reviewers to pick it up if the "---" line is left
alone.

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

This looks totally correct to me.

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace an instant of test -f with test_path_is_file functions.
  2020-10-18  3:55 ` Taylor Blau
@ 2020-10-18 20:11   ` Junio C Hamano
  0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2020-10-18 20:11 UTC (permalink / raw)
  To: Taylor Blau; +Cc: Caleb Tillman, git

Taylor Blau <me@ttaylorr.com> writes:

> Hi Caleb,
>
> On Sun, Oct 18, 2020 at 12:55:22AM +0000, Caleb Tillman wrote:
>> The test_path_is* functions provide debug-friendly messages upon failure.
>
> The body looks fine to me. Your subject is getting a little long,
> however. Typical guidance would be somewhere around 50 (at least in my
> opinion, I thought we had something in Documentation/CodingGuidelines,
> but I couldn't find anything).
>
> Maybe something instead like:
>
>   t0000: replace 'test -f' with helpers
>
> or:
>
>   t0000: modernize test style
>
> If you're looking for inspiration, you can use `git log`'s `-S` flag to
> look for anything that mentions 'test_path_is_file' to see how similar
> patches have been written in the past. (When I was recommending
> alternatives, I ran "git log --oneline -Stest_path_is_file -- t").

Thanks for a great educational input.

>> 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 &&
>
> This looks totally correct to me.

By nature of "microproject" exchange, it is almost trivial to get
the patch text right after an exchange.  The problems are typically
so easy that there is only one way to write the code part correctly.

Polishing proposed log message is much harder ;-)

Thanks.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-10-18 20:11 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-18  0:55 [Outreachy-Microproject][PATCH 1/1] t0000: replace an instant of test -f with test_path_is_file functions Caleb Tillman
2020-10-18  3:55 ` Taylor Blau
2020-10-18 20:11   ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git