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

[Taylor Blau <me@ttaylorr.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