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
next prev parent reply other threads:[~2022-11-01 1:14 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-30 17:20 [PATCH] t7001-mv.sh:modernizing test script using function Debra Obondo via GitGitGadget
2022-10-30 18:00 ` Taylor Blau
2022-10-31 18:04 ` Martin Ågren
2022-11-01 1:14 ` Taylor Blau [this message]
2022-11-03 18:39 ` [PATCH v2] t7001-mv.sh: modernizing test script using functions Debra Obondo via GitGitGadget
2022-11-03 19:37 ` Martin Ågren
2022-11-04 15:05 ` [PATCH v3] " Debra Obondo via GitGitGadget
2022-11-04 22:00 ` Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y2By+vbn7dPrk+01@nand.local \
--to=me@ttaylorr.com \
--cc=debraobondo@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=martin.agren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).