Re: [PATCH v2 1/9] t2501: add various tests for removing the current working directory

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 25, 2021, 10:21 UTC
Message-ID: <211125.86fsrkjz5t.gmgdl@evledraar.gmail.com>
In-Reply-To: <38a120f5c0379daabc1f9730039ff7166037410d.1637829556.git.gitgitgadget@gmail.com>

On Thu, Nov 25 2021, Elijah Newren via GitGitGadget wrote:

Show 24 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> Numerous commands will remove empty working directories, especially if
> they are in the way of placing needed files.  That is normally fine, but
> removing the current working directory can cause confusion for the user
> when they run subsequent commands.  For example, after one git process
> has removed the current working directory, git status/log/diff will all
> abort with the message:
>
>     fatal: Unable to read current working directory: No such file or directory
>
> Since there are several code paths that can result in the current
> working directory being removed, add several tests of various different
> codepaths that check for the behavior we would instead like to see.
> This include a number of new error messages that we will be adding in
> subsequent commits as we implement the desired checks.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  t/t2501-cwd-empty.sh | 255 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 255 insertions(+)
>  create mode 100755 t/t2501-cwd-empty.sh
>
> [...]

As noted on v1 (and particularly if we're going to have something like my proposed alternate "let's not make setup.c die then?" I really think this should positively assert our existing behavior, and we can then update this later for the behavior change.

I got that ~working locally, I think you can squash the below in, and then cherry-pick this version on top in the actual change that adds the "Refusing to remove" behavior change.

I think this really helps to explain the change, and to make sure we test existing behavior.

I added a new test for "rm" with the "-f" flag, and by the "~" in "working" I mean that this worked as I expected aside from the last couple of tests.

I think that's a bug in your existing test that's hidden by the use of test_expect_failure here. I.e. in the clean test we'll exit with:

    [...]
    + git clean -fd -e warnings :/
    warning: failed to remove ./: Invalid argument

A "bug" in the sense that this AFAICT would never have removed that directory anyway since "clean" puts out before, but with your change we'll catch that earlier and emit a new "error". Also do we need ":/" there, isn't "." more obvious? In any case, the updated test below shows that we already punt out in that case, but perhaps it's incomplete. Will "clean" remove these directories in other cases already?

The "status" then had to be removed from the "stash" test, was it leftover debugging cruft?

Finally these are quite repetitive. It would be very welcome to factor these into e.g.:

    test_expect_untracked_dir hard foo/bar/baz -- <command>

Where we just do the common case of "if $1 = hard" we do the setup with "reset --hard", otherwise the "git clean -dxf" etc. Then if it's the "foo/bar/baz" case add the 3x "test_path_is_missing" for that etc.

Another issue: You have a "git cmd | other git cmd" with "| git apply" there, should be moved into two split by a && to not potentially hide an error on the LHS of the pipe.

I think there's also probably numerous missing tests here when it comes to how other commands behaved before/after we removed the CWD. E.g. the case of:

    # in x/
    git rm -r ../x
    git reset ../some-file.txt

Which I noted in another follow-up to v1, i.e. that fails currently due to our path construction in setup.c. I.e. in terms of "selling" the change and showing the greater behavior impact, i.e. we from:

    # works
    git rm -r ../x
    # fails
    git reset ../some-file.txt

To:

    # fails (or keeps x/?)
    git rm -r ../x
    # works
    git reset ../some-file.txt

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index 5dfb456a691..f1fb8b4a872 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh
@@ -24,7 +24,7 @@ test_expect_success setup '
 	git commit -m dirORfile
 '
 
-test_expect_failure 'checkout does not clean cwd incidentally' '
+test_expect_success 'checkout cleans cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_path_is_dir foo/bar &&
 
@@ -35,10 +35,10 @@ test_expect_failure 'checkout does not clean cwd incidentally' '
 	) &&
 	test_path_is_missing foo/bar/baz &&
 	test_path_is_missing foo/bar &&
-	test_path_is_dir foo
+	test_path_is_missing foo
 '
 
-test_expect_failure 'checkout fails if cwd needs to be removed' '
+test_expect_success 'checkout if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -46,14 +46,14 @@ test_expect_failure 'checkout fails if cwd needs to be removed' '
 	(
 		cd dirORfile &&
 
-		test_must_fail git checkout fd_conflict 2>../error &&
-		grep "Refusing to remove the current working directory" ../error
+		git checkout fd_conflict 2>../error &&
+		grep "Switched to branch" ../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'reset --hard does not clean cwd incidentally' '
+test_expect_success 'reset --hard cleans cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_path_is_dir foo/bar &&
 
@@ -64,10 +64,10 @@ test_expect_failure 'reset --hard does not clean cwd incidentally' '
 	) &&
 	test_path_is_missing foo/bar/baz &&
 	test_path_is_missing foo/bar &&
-	test_path_is_dir foo
+	test_path_is_missing foo
 '
 
-test_expect_failure 'reset --hard fails if cwd needs to be removed' '
+test_expect_success 'reset --hard succeeds if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -75,14 +75,14 @@ test_expect_failure 'reset --hard fails if cwd needs to be removed' '
 	(
 		cd dirORfile &&
 
-		test_must_fail git reset --hard fd_conflict 2>../error &&
-		grep "Refusing to remove.*the current working directory" ../error
+		git reset --hard fd_conflict 2>../error &&
+		test_must_be_empty ../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'merge does not remove cwd incidentally' '
+test_expect_success 'merge removes cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -92,24 +92,24 @@ test_expect_failure 'merge does not remove cwd incidentally' '
 	) &&
 
 	test_path_is_missing subdir/file.t &&
-	test_path_is_dir subdir
+	test_path_is_missing subdir
 '
 
-test_expect_failure 'merge fails if cwd needs to be removed' '
+test_expect_success 'merge succeeds if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
 	mkdir dirORfile &&
 	(
 		cd dirORfile &&
-		test_must_fail git merge fd_conflict 2>../error &&
-		grep "Refusing to remove the current working directory" ../error
+		git merge fd_conflict 2>../error &&
+		test_must_be_empty ../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'cherry-pick does not remove cwd incidentally' '
+test_expect_success 'cherry-pick removes cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -119,24 +119,23 @@ test_expect_failure 'cherry-pick does not remove cwd incidentally' '
 	) &&
 
 	test_path_is_missing subdir/file.t &&
-	test_path_is_dir subdir
+	test_path_is_missing subdir
 '
 
-test_expect_failure 'cherry-pick fails if cwd needs to be removed' '
+test_expect_success 'cherry-pick suceeds if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
 	mkdir dirORfile &&
 	(
 		cd dirORfile &&
-		test_must_fail git cherry-pick fd_conflict 2>../error &&
-		grep "Refusing to remove the current working directory" ../error
+		git cherry-pick fd_conflict 2>../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'rebase does not remove cwd incidentally' '
+test_expect_success 'rebase does removes cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -146,24 +145,23 @@ test_expect_failure 'rebase does not remove cwd incidentally' '
 	) &&
 
 	test_path_is_missing subdir/file.t &&
-	test_path_is_dir subdir
+	test_path_is_missing subdir
 '
 
-test_expect_failure 'rebase fails if cwd needs to be removed' '
+test_expect_success 'rebase succeeds if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
 	mkdir dirORfile &&
 	(
 		cd dirORfile &&
-		test_must_fail git rebase foo/bar/baz fd_conflict 2>../error &&
-		grep "Refusing to remove the current working directory" ../error
+		git rebase foo/bar/baz fd_conflict 2>../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'revert does not remove cwd incidentally' '
+test_expect_success 'revert removes cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -173,10 +171,10 @@ test_expect_failure 'revert does not remove cwd incidentally' '
 	) &&
 
 	test_path_is_missing subdir/file.t &&
-	test_path_is_dir subdir
+	test_path_is_missing subdir
 '
 
-test_expect_failure 'revert fails if cwd needs to be removed' '
+test_expect_success 'revert succeeds if cwd needs to be removed' '
 	git checkout fd_conflict &&
 	git revert HEAD &&
 	test_when_finished "git clean -fdx" &&
@@ -184,14 +182,13 @@ test_expect_failure 'revert fails if cwd needs to be removed' '
 	mkdir dirORfile &&
 	(
 		cd dirORfile &&
-		test_must_fail git revert HEAD 2>../error &&
-		grep "Refusing to remove the current working directory" ../error
+		git revert HEAD 2>../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'rm does not remove cwd incidentally' '
+test_expect_success 'rm removes cwd incidentally' '
 	test_when_finished "git reset --hard" &&
 	git checkout foo/bar/baz &&
 
@@ -202,10 +199,24 @@ test_expect_failure 'rm does not remove cwd incidentally' '
 
 	test_path_is_missing foo/bar/baz &&
 	test_path_is_missing foo/bar &&
-	test_path_is_dir foo
+	test_path_is_missing foo
 '
 
-test_expect_failure 'apply does not remove cwd incidentally' '
+test_expect_success 'rm -f removes cwd incidentally' '
+	test_when_finished "git reset --hard" &&
+	git checkout foo/bar/baz &&
+
+	(
+		cd foo &&
+		git rm -f bar/baz.t
+	) &&
+
+	test_path_is_missing foo/bar/baz &&
+	test_path_is_missing foo/bar &&
+	test_path_is_missing foo
+'
+
+test_expect_success 'apply removes cwd incidentally' '
 	test_when_finished "git reset --hard" &&
 	git checkout foo/bar/baz &&
 
@@ -215,10 +226,10 @@ test_expect_failure 'apply does not remove cwd incidentally' '
 	) &&
 
 	test_path_is_missing subdir/file.t &&
-	test_path_is_dir subdir
+	test_path_is_missing subdir
 '
 
-test_expect_failure 'clean does not remove cwd incidentally' '
+test_expect_success 'clean does not remove cwd incidentally (cannot match pathspec)' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -227,16 +238,15 @@ test_expect_failure 'clean does not remove cwd incidentally' '
 	>untracked/random &&
 	(
 		cd untracked &&
-		git clean -fd -e warnings :/ >../warnings &&
-		grep "Refusing to remove current working directory" ../warnings
+		test_must_fail git clean -fd .
 	) &&
 
-	test_path_is_missing empty &&
+	test_path_is_dir empty &&
 	test_path_is_missing untracked/random &&
 	test_path_is_dir untracked
 '
 
-test_expect_failure 'stash does not remove cwd incidentally' '
+test_expect_success 'stash removes cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
@@ -244,12 +254,11 @@ test_expect_failure 'stash does not remove cwd incidentally' '
 	>untracked/random &&
 	(
 		cd untracked &&
-		git stash --include-untracked &&
-		git status
+		git stash --include-untracked
 	) &&
 
 	test_path_is_missing untracked/random &&
-	test_path_is_dir untracked
+	test_path_is_missing untracked
 '
 
 test_done