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@xxxxxxxxx> wrote: > > > > From: Debra Obondo <debraobondo@xxxxxxxxx> > > > > 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
