git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: Caleb Tillman <caleb.tillman@gmail.com>, git@vger.kernel.org
Subject: Re: [Outreachy-Microproject][PATCH 1/1] t0000: replace an instant of test -f with test_path_is_file functions.
Date: Sun, 18 Oct 2020 13:11:53 -0700	[thread overview]
Message-ID: <xmqqk0vn70qe.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20201018035547.GA2262492@nand.local> (Taylor Blau's message of "Sat, 17 Oct 2020 23:55:47 -0400")

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.

      reply	other threads:[~2020-10-18 20:11 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqk0vn70qe.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=caleb.tillman@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).