From: Jeff King <firstname.lastname@example.org>
To: Adrian Wijaya <email@example.com>
Subject: Re: [GSOC][PATCH 2/2] t1300: replace "test -f" into "test_path_is_file"
Date: Fri, 20 Mar 2020 01:52:25 -0400 [thread overview]
Message-ID: <20200320055225.GG499858@coredump.intra.peff.net> (raw)
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
The general form of the subject line looks good, and follows our
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 <firstname.lastname@example.org>
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.
next prev parent reply other threads:[~2020-03-20 5:52 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-19 23:47 [GSOC][PATCH 1/2] t1300: replace "test -f" into "test_path_is_file" Adrian Wijaya
2020-03-19 23:47 ` [GSOC][PATCH 2/2] " Adrian Wijaya
2020-03-20 5:52 ` Jeff King [this message]
2020-03-20 15:22 ` adrian wijaya
2020-03-20 15:45 ` [GSOC][PATCH 1/2] " adrian wijaya
2020-03-20 16:07 ` [GSOC][PATCH v2] t1300: convert "test -f" with "test_path_is_file" Adrian Wijaya
2020-03-20 18:04 ` Junio C Hamano
2020-03-20 18:08 ` Junio C Hamano
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:
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 \
* 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
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).