Re: [RFC][GSoC][PATCH] t9160: Change test -(d | f) to test_path_is_*

From: Vinayak Dev <vinayakdev.sci@gmail.com>
Date: Feb 10, 2023, 11:32 UTC
Message-ID: <CADE8Naoq4N9EpSfb8=ADPRLeAsmjVhL+ayOzhS=syaZ0PQh71w@mail.gmail.com>
In-Reply-To: <CAPig+cS-9gbxb3tWqaWtsjfiQR60GQXpfCKM7-neu6AH8rUvcA@mail.gmail.com>

On Fri, 10 Feb 2023 at 16:38, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 5 quoted lines

>
> On Fri, Feb 10, 2023 at 3:05 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:
> > Changes test -d and test -f commands in
>
> s/Changes/Change/

Sorry for this error, I actually sent the patch before I got your review of the prior one.

Show 13 quoted lines

> > t/t9160-git-svn-preserve-empty-dirs.sh to test_path_is_dir and
> > test_path_is_file respectively, which are helper functions defined in
> > t/test-lib-functions.sh
>
> This summarizes the changes made by the patch, but readers also want
> to know _why_ the changes are being made; in fact, "why" is more
> important. Previous patches similar to this one have explained that
> test_path_is_dir() and test_path_is_file() are superior to `test -d`
> and `test -f` because the functions provide diagnostic output when
> they fail, and that diagnostic output can make it easier to debug a
> failing tests. So, if you reroll the patch, focus on explaining the
> benefits of the functions rather than explaining the mechanical
> changes made by the patch.

This is correct, I will make the message more informative about the relevant aspects of the changes.

Show 9 quoted lines

> > Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>
> >
> > vinayakdsci (1):
> >   Change test -(d | f) to corresponding test_path_*
>
> As with the other GSoC patch you submitted[1], this one is also
> missing the "---" line below your sign-off, which tells git-am where
> the commit message ends. As mentioned in [2] you may need to adjust
> your tools or workflow to prevent the "---" line from being stripped.

I apologise for this mistake again.

> The actual changes made by the patch are probably reasonable (though I
> don't have CVS libraries installed presently, so I wasn't able to
> actually test the changes).

If by testing you mean running the test file to ensure there are no errors, I did do it thoroughly before sending the patch, so I can vouch for the changes :)

Thanks a lot! Vinayak