Re: [PATCH] t7001-mv.sh:modernizing test script using function

From: Taylor Blau <me@ttaylorr.com>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: Debra Obondo via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Debra Obondo <debraobondo@gmail.com>
Subject: Re: [PATCH] t7001-mv.sh:modernizing test script using function
Date: Mon, 31 Oct 2022 21:14:34 -0400	[thread overview]
Message-ID: <Y2By+vbn7dPrk+01@nand.local> (raw)
In-Reply-To: <CAN0heSpvgtLG5o9i_cSbn0zHfNN+GJFYY6oBXmHUs8FniW5oOw@mail.gmail.com>

On Mon, Oct 31, 2022 at 07:04:20PM +0100, Martin Ågren wrote:
> Hi Debra,
>
> On Sun, 30 Oct 2022 at 18:35, Debra Obondo via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Debra Obondo <debraobondo@gmail.com>
> >
> > Test script to verify the presence/absence of files, paths, directories,
> > symlinks and other features in 'git mv' command are using the command
> > format:
> >
> > 'test (-e|f|d|h|...)'
> >
> > Replace them with helper functions of format:
> >
> > 'test_path_is_*'
>
> This is a good idea.
>
> The subject of this patch could use a space after the colon. You could
> also write "modernize" to give an order to the code base. So something
> like
>
>   t7001-mv.sh: modernize test script using function
>
> perhaps. "Function" is a bit vague, perhaps.
>
> I wanted to comment on this:
>
> >  test_expect_success 'mv --dry-run does not move file' '
> >         git mv -n path0/COPYING MOVED &&
> > -       test -f path0/COPYING &&
> > -       test ! -f MOVED
> > +       test_path_is_file path0/COPYING &&
> > +       ! test_path_is_file MOVED
> >  '
>
> It is my understanding that we prefer to only use such a helper when we
> really expect the file to exist. If the path is not a file, this helper
> prints a helpful message before returning with an error.
>
> Here, this means we will emit this 'helpful'
>
>   File MOVED doesn't exist
>
> on every test run, when really everything is as it should. And if the
> file is actually there, i.e., we have a bug, we'll emit nothing -- but
> that is precisely when we would want some diagnostics such as
>
>   Path exists:
>   ... MOVED ...
>
> to show us that the file actually exists, contrary to the test's
> expectations.
>
> Such output is precisely what `test_path_is_missing` would give us. :-)
>
> My gut feeling is that where this patch adds "! test_path_foo", it
> should use "test_path_bar" instead, for various values of "foo" and
> "bar". What do you think about that?

All good suggestions, thanks. I'll hold this back while we wait for a
rerolled version.

Thanks,
Taylor