t7001-mv.sh:modernizing test script using function

Commit Message

Debra Obondo Oct. 30, 2022, 5:20 p.m. UTC

From: Debra Obondo <[e-mail]>

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_*'

Signed-off-by: Debra Obondo <[e-mail]>
---
    [PATCH] [OUTREACHY] t7001-mv.sh : Use test_path_is_* functions in test
    script
    
    Older test scripts use the command 'test -' to verify the presence or
    absence of features such as files, directories and symbolic links. This
    however requires slightly complicated uses, such as 'test ! -f '. The
    helper functions 'test_path_is_' located in t/test-lib-functions.sh have
    taken into account all scenarios of the 'test -*' to reduce errors. This
    was a microproject to replace them with the helper functions.
    
    Test script to verify the presence/absence of files, paths,
    directories,symboliclinks and many other features in mv command are
    using the command format
    
    'test -(e|f|s|h|...).
    
    Replace them with helper functions of format
    
    'test_path_is_*'
    
    Signed-off-by: Debra Obondo [e-mail]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1372%2Ffobiasic07%2Ft7001-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1372/fobiasic07/t7001-v1
Pull-Request: https://github.com/git/git/pull/1372

 t/t7001-mv.sh | 62 +++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)


base-commit: 1fc3c0ad407008c2f71dd9ae1241d8b75f8ef886

Comments

Taylor Blau Oct. 30, 2022, 6 p.m. UTC | #1

On Sun, Oct 30, 2022 at 05:20:41PM +0000, Debra Obondo via GitGitGadget wrote:
>  t/t7001-mv.sh | 62 +++++++++++++++++++++++++--------------------------
>  1 file changed, 31 insertions(+), 31 deletions(-)

Looks good. Will queue, thanks.

Thanks,
Taylor

Martin Ågren Oct. 31, 2022, 6:04 p.m. UTC | #2

Hi Debra,

On Sun, 30 Oct 2022 at 18:35, Debra Obondo via GitGitGadget
<[e-mail]> wrote:
>
> From: Debra Obondo <[e-mail]>
>
> 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?


Martin

Taylor Blau Nov. 1, 2022, 1:14 a.m. UTC | #3

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
> <[e-mail]> wrote:
> >
> > From: Debra Obondo <[e-mail]>
> >
> > 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

Patch

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 8c37bceb336..aa97a2f8e1c 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -60,8 +60,8 @@  test_expect_success 'checking the commit' '
 
 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
 '
 
 test_expect_success 'checking -k on non-existing file' '
@@ -71,25 +71,25 @@  test_expect_success 'checking -k on non-existing file' '
 test_expect_success 'checking -k on untracked file' '
 	>untracked1 &&
 	git mv -k untracked1 path0 &&
-	test -f untracked1 &&
-	test ! -f path0/untracked1
+	test_path_is_file untracked1 &&
+	! test_path_is_file path0/untracked1
 '
 
 test_expect_success 'checking -k on multiple untracked files' '
 	>untracked2 &&
 	git mv -k untracked1 untracked2 path0 &&
-	test -f untracked1 &&
-	test -f untracked2 &&
-	test ! -f path0/untracked1 &&
-	test ! -f path0/untracked2
+	test_path_is_file untracked1 &&
+	test_path_is_file untracked2 &&
+	! test_path_is_file path0/untracked1 &&
+	! test_path_is_file path0/untracked2
 '
 
 test_expect_success 'checking -f on untracked file with existing target' '
 	>path0/untracked1 &&
 	test_must_fail git mv -f untracked1 path0 &&
-	test ! -f .git/index.lock &&
-	test -f untracked1 &&
-	test -f path0/untracked1
+	! test_path_is_file .git/index.lock &&
+	test_path_is_file untracked1 &&
+	test_path_is_file path0/untracked1
 '
 
 # clean up the mess in case bad things happen
@@ -215,8 +215,8 @@  test_expect_success 'absolute pathname' '
 		git add sub/file &&
 
 		git mv sub "$(pwd)/in" &&
-		! test -d sub &&
-		test -d in &&
+		! test_path_is_dir sub &&
+		test_path_is_dir in &&
 		git ls-files --error-unmatch in/file
 	)
 '
@@ -234,8 +234,8 @@  test_expect_success 'absolute pathname outside should fail' '
 		git add sub/file &&
 
 		test_must_fail git mv sub "$out/out" &&
-		test -d sub &&
-		! test -d ../in &&
+		test_path_is_dir sub &&
+		! test_path_is_dir ../in &&
 		git ls-files --error-unmatch sub/file
 	)
 '
@@ -295,8 +295,8 @@  test_expect_success 'git mv should overwrite symlink to a file' '
 	git add moved &&
 	test_must_fail git mv moved symlink &&
 	git mv -f moved symlink &&
-	! test -e moved &&
-	test -f symlink &&
+	! test_path_exists moved &&
+	test_path_is_file symlink &&
 	test "$(cat symlink)" = 1 &&
 	git update-index --refresh &&
 	git diff-files --quiet
@@ -312,13 +312,13 @@  test_expect_success 'git mv should overwrite file with a symlink' '
 	git add moved &&
 	test_must_fail git mv symlink moved &&
 	git mv -f symlink moved &&
-	! test -e symlink &&
+	! test_path_exists symlink &&
 	git update-index --refresh &&
 	git diff-files --quiet
 '
 
 test_expect_success SYMLINKS 'check moved symlink' '
-	test -h moved
+	test_path_is_symlink moved
 '
 
 rm -f moved symlink
@@ -352,7 +352,7 @@  test_expect_success 'git mv moves a submodule with a .git directory and no .gitm
 	) &&
 	mkdir mod &&
 	git mv sub mod/sub &&
-	! test -e sub &&
+	! test_path_exists sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
@@ -372,7 +372,7 @@  test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu
 	) &&
 	mkdir mod &&
 	git mv sub mod/sub &&
-	! test -e sub &&
+	! test_path_exists sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	echo mod/sub >expected &&
@@ -389,7 +389,7 @@  test_expect_success 'git mv moves a submodule with gitfile' '
 	entry="$(git ls-files --stage sub | cut -f 1)" &&
 	mkdir mod &&
 	git -C mod mv ../sub/ . &&
-	! test -e sub &&
+	! test_path_exists sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	echo mod/sub >expected &&
@@ -408,7 +408,7 @@  test_expect_success 'mv does not complain when no .gitmodules file is found' '
 	mkdir mod &&
 	git mv sub mod/sub 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -e sub &&
+	! test_path_exists sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
@@ -423,13 +423,13 @@  test_expect_success 'mv will error out on a modified .gitmodules file unless sta
 	entry="$(git ls-files --stage sub | cut -f 1)" &&
 	mkdir mod &&
 	test_must_fail git mv sub mod/sub 2>actual.err &&
-	test -s actual.err &&
-	test -e sub &&
+	test_file_not_empty actual.err &&
+	test_path_exists sub &&
 	git diff-files --quiet -- sub &&
 	git add .gitmodules &&
 	git mv sub mod/sub 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -e sub &&
+	! test_path_exists sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
@@ -447,7 +447,7 @@  test_expect_success 'mv issues a warning when section is not found in .gitmodule
 	mkdir mod &&
 	git mv sub mod/sub 2>actual.err &&
 	test_cmp expect.err actual.err &&
-	! test -e sub &&
+	! test_path_exists sub &&
 	test "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" &&
 	git -C mod/sub status &&
 	git update-index --refresh &&
@@ -460,7 +460,7 @@  test_expect_success 'mv --dry-run does not touch the submodule or .gitmodules' '
 	git submodule update &&
 	mkdir mod &&
 	git mv -n sub mod/sub 2>actual.err &&
-	test -f sub/.git &&
+	test_path_is_file sub/.git &&
 	git diff-index --exit-code HEAD &&
 	git update-index --refresh &&
 	git diff-files --quiet -- sub .gitmodules
@@ -474,10 +474,10 @@  test_expect_success 'checking out a commit before submodule moved needs manual u
 	git status -s sub2 >actual &&
 	echo "?? sub2/" >expected &&
 	test_cmp expected actual &&
-	! test -f sub/.git &&
-	test -f sub2/.git &&
+	! test_path_is_file sub/.git &&
+	test_path_is_file sub2/.git &&
 	git submodule update &&
-	test -f sub/.git &&
+	test_path_is_file sub/.git &&
 	rm -rf sub2 &&
 	git diff-index --exit-code HEAD &&
 	git update-index --refresh &&