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

From: adrian wijaya <adrianwijaya100@gmail.com>
Date: Mar 20, 2020, 15:22 UTC
Message-ID: <CA+0Uiy-EkT1BHSgL4R783Qh=c4-W_FZwB=LwnQG7OLx92YN=ww@mail.gmail.com>
In-Reply-To: <20200320055225.GG499858@coredump.intra.peff.net>

On Fri, Mar 20, 2020 at 12:52 PM Jeff King <peff@peff.net> wrote:

Show 13 quoted lines

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

Show 17 quoted lines

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

Show 5 quoted lines

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

Show 18 quoted lines

>
> > 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 :)

Show 8 quoted lines

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