On Fri, Mar 20, 2020 at 12:52 PM Jeff King <peff@xxxxxxxx> 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@xxxxxxxxx> > > --- > > 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.
