[PATCH 0/8] Avoid removing the current working directory, even if it becomes empty

Traditionally, if folks run git commands such as checkout or rebase from a
subdirectory, that git command could remove their current working directory
and result in subsequent git and non-git commands either getting confused or
printing messages that confuse the user (e.g. "fatal: Unable to read current
working directory: No such file or directory"). We already refuse to remove
directories that have untracked files within them[1], preferring to show an
error; with this series, we tweak that rule a bit to also refuse to remove
the current working directory even if it has no untracked files within it.

Peff endorsed the idea behind this series at [2] and even pointed out a
corner case that he suggested (and I agree) would probably be more
problematic to attempt to address than to leave be.

This series has a minor textual conflict with so/stash-staged (in next),
because both series add a new parameter to the do_push_stash() function. The
correct resolution is simple -- just to take both new parameters. However,
let me know if you'd rather I rebased onto so/stash-staged.

(Sorry for the delay; I know I promised this months ago in response to [3],
but it was blocked by some preliminary series and then by my absence during
a migration at $DAYJOB).

[1] well, with a few exceptions; see
https://lore.kernel.org/git/pull.1036.v3.git.1632760428.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/YS8eEtwQvF7TaLCb@coredump.intra.peff.net/
[3]
https://lore.kernel.org/git/c0557284-8f82-76cc-8c47-0b1bc9f2ce5a@rawbw.com/

Elijah Newren (8):
  t2501: add various tests for removing the current working directory
  repository, setup: introduce the_cwd
  unpack-trees: refuse to remove the current working directory
  unpack-trees: add special cwd handling
  symlinks: do not include current working directory in dir removal
  clean: do not attempt to remove current working directory
  stash: do not attempt to remove current working directory
  dir: avoid removing the current working directory

 builtin/clean.c      |  29 +++--
 builtin/stash.c      |  13 ++-
 dir.c                |  11 +-
 repository.c         |   1 +
 repository.h         |   1 +
 setup.c              |   2 +
 symlinks.c           |  12 +-
 t/t2501-cwd-empty.sh | 255 +++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.c       |  28 ++++-
 unpack-trees.h       |   1 +
 10 files changed, 329 insertions(+), 24 deletions(-)
 create mode 100755 t/t2501-cwd-empty.sh


base-commit: 88d915a634b449147855041d44875322de2b286d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1140%2Fnewren%2Fcwd_removal-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1140/newren/cwd_removal-v1
Pull-Request: https://github.com/git/git/pull/1140
-- 
gitgitgadget

From: Elijah Newren <redacted>

Removing the current working directory causes all subsequent git
commands (and likely a number of non-git commands) run from that
directory to get confused and fail with a message about being unable to
read the current working directory.  That confuses end users,
particularly since the command they get the error from is not the one
that caused the problem; the problem came from the side-effect of some
previous command.

We would like to avoid removing the current working directory; towards
this end, introduce a new the_cwd variable that tracks the current
working directory.  Subsequent commits will make use of this new
variable.

Signed-off-by: Elijah Newren <redacted>
---
 repository.c | 1 +
 repository.h | 1 +
 setup.c      | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/repository.c b/repository.c
index c5b90ba93ea..69a106c553c 100644
--- a/repository.c
+++ b/repository.c

@@ -17,6 +17,7 @@
 static struct repository the_repo;
 struct repository *the_repository;
 struct index_state the_index;
+char *the_cwd;
 
 void initialize_the_repository(void)
 {

diff --git a/repository.h b/repository.h
index a057653981c..45de85d18ef 100644
--- a/repository.h
+++ b/repository.h

@@ -147,6 +147,7 @@ struct repository {
 };
 
 extern struct repository *the_repository;
+extern char *the_cwd;
 
 /*
  * Define a custom repository layout. Any field can be NULL, which

diff --git a/setup.c b/setup.c
index 347d7181ae9..4466fa55af3 100644
--- a/setup.c
+++ b/setup.c

@@ -887,6 +887,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 		set_git_dir(gitdirenv, 1);
 		if (chdir(worktree))
 			die_errno(_("cannot chdir to '%s'"), worktree);
+		the_cwd = xstrdup(cwd->buf + offset);
 		strbuf_addch(cwd, '/');
 		free(gitfile);
 		return cwd->buf + offset;

@@ -940,6 +941,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
 	/* Make "offset" point past the '/' (already the case for root dirs) */
 	if (offset != offset_1st_component(cwd->buf))
 		offset++;
+	the_cwd = xstrdup(cwd->buf + offset);
 	/* Add a '/' at the end */
 	strbuf_addch(cwd, '/');
 	return cwd->buf + offset;

-- 
gitgitgadget

From: Elijah Newren <redacted>

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.  Add some tests checking for such
problems.

Signed-off-by: Elijah Newren <redacted>
---
 t/t2501-cwd-empty.sh | 255 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 255 insertions(+)
 create mode 100755 t/t2501-cwd-empty.sh

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
new file mode 100755
index 00000000000..5dfb456a691
--- /dev/null
+++ b/t/t2501-cwd-empty.sh

@@ -0,0 +1,255 @@
+#!/bin/sh
+
+test_description='Test handling of the current working directory becoming empty'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_commit init &&
+	mkdir subdir &&
+	test_commit subdir/file &&
+
+	git branch fd_conflict &&
+
+	mkdir -p foo/bar &&
+	test_commit foo/bar/baz &&
+
+	git revert HEAD &&
+	git tag reverted &&
+
+	git checkout fd_conflict &&
+	git rm subdir/file.t &&
+	echo not-a-directory >dirORfile &&
+	git add dirORfile &&
+	git commit -m dirORfile
+'
+
+test_expect_failure 'checkout does not clean cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_path_is_dir foo/bar &&
+
+	(
+		cd foo &&
+		git checkout init &&
+		cd ..
+	) &&
+	test_path_is_missing foo/bar/baz &&
+	test_path_is_missing foo/bar &&
+	test_path_is_dir foo
+'
+
+test_expect_failure 'checkout fails 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 checkout fd_conflict 2>../error &&
+		grep "Refusing to remove the current working directory" ../error
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'reset --hard does not clean cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_path_is_dir foo/bar &&
+
+	(
+		cd foo &&
+		git reset --hard init &&
+		cd ..
+	) &&
+	test_path_is_missing foo/bar/baz &&
+	test_path_is_missing foo/bar &&
+	test_path_is_dir foo
+'
+
+test_expect_failure 'reset --hard fails 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 reset --hard fd_conflict 2>../error &&
+		grep "Refusing to remove.*the current working directory" ../error
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'merge does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	(
+		cd subdir &&
+		git merge fd_conflict
+	) &&
+
+	test_path_is_missing subdir/file.t &&
+	test_path_is_dir subdir
+'
+
+test_expect_failure 'merge fails 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
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'cherry-pick does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	(
+		cd subdir &&
+		git cherry-pick fd_conflict
+	) &&
+
+	test_path_is_missing subdir/file.t &&
+	test_path_is_dir subdir
+'
+
+test_expect_failure 'cherry-pick fails 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
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'rebase does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	(
+		cd subdir &&
+		git rebase foo/bar/baz fd_conflict
+	) &&
+
+	test_path_is_missing subdir/file.t &&
+	test_path_is_dir subdir
+'
+
+test_expect_failure 'rebase fails 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
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'revert does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	(
+		cd subdir &&
+		git revert subdir/file
+	) &&
+
+	test_path_is_missing subdir/file.t &&
+	test_path_is_dir subdir
+'
+
+test_expect_failure 'revert fails if cwd needs to be removed' '
+	git checkout fd_conflict &&
+	git revert HEAD &&
+	test_when_finished "git clean -fdx" &&
+
+	mkdir dirORfile &&
+	(
+		cd dirORfile &&
+		test_must_fail git revert HEAD 2>../error &&
+		grep "Refusing to remove the current working directory" ../error
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'rm does not remove cwd incidentally' '
+	test_when_finished "git reset --hard" &&
+	git checkout foo/bar/baz &&
+
+	(
+		cd foo &&
+		git rm bar/baz.t
+	) &&
+
+	test_path_is_missing foo/bar/baz &&
+	test_path_is_missing foo/bar &&
+	test_path_is_dir foo
+'
+
+test_expect_failure 'apply does not remove cwd incidentally' '
+	test_when_finished "git reset --hard" &&
+	git checkout foo/bar/baz &&
+
+	(
+		cd subdir &&
+		git diff subdir/file init | git apply
+	) &&
+
+	test_path_is_missing subdir/file.t &&
+	test_path_is_dir subdir
+'
+
+test_expect_failure 'clean does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	mkdir empty &&
+	mkdir untracked &&
+	>untracked/random &&
+	(
+		cd untracked &&
+		git clean -fd -e warnings :/ >../warnings &&
+		grep "Refusing to remove current working directory" ../warnings
+	) &&
+
+	test_path_is_missing empty &&
+	test_path_is_missing untracked/random &&
+	test_path_is_dir untracked
+'
+
+test_expect_failure 'stash does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	mkdir untracked &&
+	>untracked/random &&
+	(
+		cd untracked &&
+		git stash --include-untracked &&
+		git status
+	) &&
+
+	test_path_is_missing untracked/random &&
+	test_path_is_dir untracked
+'
+
+test_done

-- 
gitgitgadget

From: Elijah Newren <redacted>

When running commands such as `git reset --hard` from a subdirectory, if
that subdirectory is in the way of adding needed files, bail with an
error message.

Note that this change looks kind of like it duplicates the new lines of
code from the previous commit in verify_clean_subdirectory().  However,
when we are preserving untracked files, we would rather any error
messages about untracked files being in the way take precedence over
error messages about a subdirectory that happens to be the current
working directory being in the way.  But in the
UNPACK_RESET_OVERWRITE_UNTRACKED case, there is no untracked checking to
be done, so we simply add a special case near the top of
verify_absent_1.

Signed-off-by: Elijah Newren <redacted>
---
 t/t2501-cwd-empty.sh |  2 +-
 unpack-trees.c       | 12 ++++++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index 212676d71c3..526d8ec2ee3 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh

@@ -67,7 +67,7 @@ test_expect_failure 'reset --hard does not clean cwd incidentally' '
 	test_path_is_dir foo
 '
 
-test_expect_failure 'reset --hard fails if cwd needs to be removed' '
+test_expect_success 'reset --hard fails if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

diff --git a/unpack-trees.c b/unpack-trees.c
index 8879b8d6c8c..d1ce4f4a35b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -2260,10 +2260,18 @@ static int verify_absent_1(const struct cache_entry *ce,
 	int len;
 	struct stat st;
 
-	if (o->index_only || !o->update ||
-	    o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED)
+	if (o->index_only || !o->update)
 		return 0;
 
+	if (o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED) {
+		/* Avoid nuking cwd... */
+		if (the_cwd && !strcmp(the_cwd, ce->name))
+			return add_rejected_path(o, ERROR_CWD_IN_THE_WAY,
+						 ce->name);
+		/* ...but nuke anything else. */
+		return 0;
+	}
+
 	len = check_leading_path(ce->name, ce_namelen(ce), 0);
 	if (!len)
 		return 0;

-- 
gitgitgadget

From: Elijah Newren <redacted>

In the past, when a directory needs to be removed to make room for a
file, we have always errored out when that directory contains any
untracked (but not ignored) files.  Add an extra condition on that: also
error out if the directory is the current working directory.

Signed-off-by: Elijah Newren <redacted>
---
 t/t2501-cwd-empty.sh | 10 +++++-----
 unpack-trees.c       | 16 ++++++++++++----
 unpack-trees.h       |  1 +
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index 5dfb456a691..212676d71c3 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh

@@ -38,7 +38,7 @@ test_expect_failure 'checkout does not clean cwd incidentally' '
 	test_path_is_dir foo
 '
 
-test_expect_failure 'checkout fails if cwd needs to be removed' '
+test_expect_success 'checkout fails if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -95,7 +95,7 @@ test_expect_failure 'merge does not remove cwd incidentally' '
 	test_path_is_dir subdir
 '
 
-test_expect_failure 'merge fails if cwd needs to be removed' '
+test_expect_success 'merge fails if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -122,7 +122,7 @@ test_expect_failure 'cherry-pick does not remove cwd incidentally' '
 	test_path_is_dir subdir
 '
 
-test_expect_failure 'cherry-pick fails if cwd needs to be removed' '
+test_expect_success 'cherry-pick fails if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -149,7 +149,7 @@ test_expect_failure 'rebase does not remove cwd incidentally' '
 	test_path_is_dir subdir
 '
 
-test_expect_failure 'rebase fails if cwd needs to be removed' '
+test_expect_success 'rebase fails if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -176,7 +176,7 @@ test_expect_failure 'revert does not remove cwd incidentally' '
 	test_path_is_dir subdir
 '
 
-test_expect_failure 'revert fails if cwd needs to be removed' '
+test_expect_success 'revert fails if cwd needs to be removed' '
 	git checkout fd_conflict &&
 	git revert HEAD &&
 	test_when_finished "git clean -fdx" &&

diff --git a/unpack-trees.c b/unpack-trees.c
index 89ca95ce90b..8879b8d6c8c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -36,6 +36,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_WARNING_TYPES] = {
 	/* ERROR_NOT_UPTODATE_DIR */
 	"Updating '%s' would lose untracked files in it",
 
+	/* ERROR_CWD_IN_THE_WAY */
+	"Refusing to remove '%s' since it is the current working directory.",
+
 	/* ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN */
 	"Untracked working tree file '%s' would be overwritten by merge.",
 

@@ -131,6 +134,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 	msgs[ERROR_NOT_UPTODATE_DIR] =
 		_("Updating the following directories would lose untracked files in them:\n%s");
 
+	msgs[ERROR_CWD_IN_THE_WAY] =
+		_("Refusing to remove the current working directory:\n%s");
+
 	if (!strcmp(cmd, "checkout"))
 		msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
 		      ? _("The following untracked working tree files would be removed by checkout:\n%%s"

@@ -2146,10 +2152,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
 		cnt++;
 	}
 
-	/*
-	 * Then we need to make sure that we do not lose a locally
-	 * present file that is not ignored.
-	 */
+	/* Do not lose a locally present file that is not ignored. */
 	pathbuf = xstrfmt("%.*s/", namelen, ce->name);
 
 	memset(&d, 0, sizeof(d));

@@ -2160,6 +2163,11 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
 	free(pathbuf);
 	if (i)
 		return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
+
+	/* Do not lose the current working directory. */
+	if (the_cwd && !strcmp(the_cwd, ce->name))
+		return add_rejected_path(o, ERROR_CWD_IN_THE_WAY, ce->name);
+
 	return cnt;
 }
 

diff --git a/unpack-trees.h b/unpack-trees.h
index 71ffb7eeb0c..efb9edfbb27 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h

@@ -19,6 +19,7 @@ enum unpack_trees_error_types {
 	ERROR_WOULD_OVERWRITE = 0,
 	ERROR_NOT_UPTODATE_FILE,
 	ERROR_NOT_UPTODATE_DIR,
+	ERROR_CWD_IN_THE_WAY,
 	ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN,
 	ERROR_WOULD_LOSE_UNTRACKED_REMOVED,
 	ERROR_BIND_OVERLAP,

-- 
gitgitgadget

From: Elijah Newren <redacted>

Signed-off-by: Elijah Newren <redacted>
---
 builtin/stash.c      | 13 ++++++++-----
 t/t2501-cwd-empty.sh |  2 +-
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index a0ccc8654df..556287cdb56 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c

@@ -1404,7 +1404,8 @@ static int create_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
-static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int quiet,
+static int do_push_stash(const struct pathspec *ps, const char *stash_msg,
+			 const char *prefix, int quiet,
 			 int keep_index, int patch_mode, int include_untracked)
 {
 	int ret = 0;

@@ -1485,8 +1486,10 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 			struct child_process cp = CHILD_PROCESS_INIT;
 
 			cp.git_cmd = 1;
+			if (prefix)
+				strvec_pushl(&cp.args, "-C", prefix, NULL);
 			strvec_pushl(&cp.args, "clean", "--force",
-				     "--quiet", "-d", NULL);
+				     "--quiet", "-d", ":/", NULL);
 			if (include_untracked == INCLUDE_ALL_FILES)
 				strvec_push(&cp.args, "-x");
 			if (run_command(&cp)) {

@@ -1656,8 +1659,8 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 		die(_("--pathspec-file-nul requires --pathspec-from-file"));
 	}
 
-	return do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,
-			     include_untracked);
+	return do_push_stash(&ps, stash_msg, prefix, quiet, keep_index,
+			     patch_mode, include_untracked);
 }
 
 static int save_stash(int argc, const char **argv, const char *prefix)

@@ -1693,7 +1696,7 @@ static int save_stash(int argc, const char **argv, const char *prefix)
 		stash_msg = strbuf_join_argv(&stash_msg_buf, argc, argv, ' ');
 
 	memset(&ps, 0, sizeof(ps));
-	ret = do_push_stash(&ps, stash_msg, quiet, keep_index,
+	ret = do_push_stash(&ps, stash_msg, prefix, quiet, keep_index,
 			    patch_mode, include_untracked);
 
 	strbuf_release(&stash_msg_buf);

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index 20e1b6adede..ff4e7cd89fa 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh

@@ -236,7 +236,7 @@ test_expect_success 'clean does not remove cwd incidentally' '
 	test_path_is_dir untracked
 '
 
-test_expect_failure 'stash does not remove cwd incidentally' '
+test_expect_success 'stash does not remove cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

-- 
gitgitgadget

From: Elijah Newren <redacted>

Signed-off-by: Elijah Newren <redacted>
---
 builtin/clean.c      | 29 ++++++++++++++++++++---------
 t/t2501-cwd-empty.sh |  2 +-
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 98a2860409b..17d54be57b7 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c

@@ -36,6 +36,8 @@ static const char *msg_skip_git_dir = N_("Skipping repository %s\n");
 static const char *msg_would_skip_git_dir = N_("Would skip repository %s\n");
 static const char *msg_warn_remove_failed = N_("failed to remove %s");
 static const char *msg_warn_lstat_failed = N_("could not lstat %s\n");
+static const char *msg_skip_cwd = N_("Refusing to remove current working directory\n");
+static const char *msg_would_skip_cwd = N_("Would refuse to remove current working directory\n");
 
 enum color_clean {
 	CLEAN_COLOR_RESET = 0,

@@ -231,16 +233,25 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	strbuf_setlen(path, original_len);
 
 	if (*dir_gone) {
-		res = dry_run ? 0 : rmdir(path->buf);
-		if (!res)
-			*dir_gone = 1;
-		else {
-			int saved_errno = errno;
-			quote_path(path->buf, prefix, &quoted, 0);
-			errno = saved_errno;
-			warning_errno(_(msg_warn_remove_failed), quoted.buf);
+		int prefixlen = prefix ? strlen(prefix) : 0;
+		if (prefix &&
+		    path->len == prefixlen + 2 &&
+		    !strncmp(path->buf, prefix, prefixlen) &&
+		    !strcmp(path->buf + prefixlen, "./")) {
+			printf("%s", dry_run ? _(msg_would_skip_cwd) : _(msg_skip_cwd));
 			*dir_gone = 0;
-			ret = 1;
+		} else {
+			res = dry_run ? 0 : rmdir(path->buf);
+			if (!res)
+				*dir_gone = 1;
+			else {
+				int saved_errno = errno;
+				quote_path(path->buf, prefix, &quoted, 0);
+				errno = saved_errno;
+				warning_errno(_(msg_warn_remove_failed), quoted.buf);
+				*dir_gone = 0;
+				ret = 1;
+			}
 		}
 	}
 

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index b92e1a9bb16..20e1b6adede 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh

@@ -218,7 +218,7 @@ test_expect_failure 'apply does not remove cwd incidentally' '
 	test_path_is_dir subdir
 '
 
-test_expect_failure 'clean does not remove cwd incidentally' '
+test_expect_success 'clean does not remove cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

-- 
gitgitgadget

From: Elijah Newren <redacted>

symlinks has a pair of schedule_dir_for_removal() and
remove_scheduled_dirs() functions that ensure that directories made
empty by removing other files also themselves get removed.  However, we
want to exclude the current working directory and leave it around so
that subsequent git commands (and non-git commands) that the user runs
afterwards don't cause the user to get confused.

Signed-off-by: Elijah Newren <redacted>
---
 symlinks.c           | 12 +++++++++++-
 t/t2501-cwd-empty.sh | 12 ++++++------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/symlinks.c b/symlinks.c
index 5232d02020c..84622bedcde 100644
--- a/symlinks.c
+++ b/symlinks.c

@@ -275,11 +275,18 @@ static int threaded_has_dirs_only_path(struct cache_def *cache, const char *name
 
 static struct strbuf removal = STRBUF_INIT;
 
+static int cant_remove(char *dirname)
+{
+	if (the_cwd && !strcmp(dirname, the_cwd))
+		return 1;
+	return rmdir(dirname);
+}
+
 static void do_remove_scheduled_dirs(int new_len)
 {
 	while (removal.len > new_len) {
 		removal.buf[removal.len] = '\0';
-		if (rmdir(removal.buf))
+		if (cant_remove(removal.buf))
 			break;
 		do {
 			removal.len--;

@@ -293,6 +300,9 @@ void schedule_dir_for_removal(const char *name, int len)
 {
 	int match_len, last_slash, i, previous_slash;
 
+	if (the_cwd && !strcmp(name, the_cwd))
+		return;	/* Do not remove the current working directory */
+
 	match_len = last_slash = i =
 		longest_path_match(name, len, removal.buf, removal.len,
 				   &previous_slash);

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index 526d8ec2ee3..b92e1a9bb16 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 does not clean cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_path_is_dir foo/bar &&
 

@@ -53,7 +53,7 @@ test_expect_success 'checkout fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'reset --hard does not clean cwd incidentally' '
+test_expect_success 'reset --hard does not clean cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_path_is_dir foo/bar &&
 

@@ -82,7 +82,7 @@ test_expect_success 'reset --hard fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'merge does not remove cwd incidentally' '
+test_expect_success 'merge does not remove cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -109,7 +109,7 @@ test_expect_success 'merge fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'cherry-pick does not remove cwd incidentally' '
+test_expect_success 'cherry-pick does not remove cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -136,7 +136,7 @@ test_expect_success 'cherry-pick fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'rebase does not remove cwd incidentally' '
+test_expect_success 'rebase does not remove cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -163,7 +163,7 @@ test_expect_success 'rebase fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'revert does not remove cwd incidentally' '
+test_expect_success 'revert does not remove cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

-- 
gitgitgadget

From: Elijah Newren <redacted>

dir has a convenient remove_path() helper that will both remove a file
in a directory and remove its containing directory if it becomes empty
as a result of the removal, recursing all the way up.  However, we do
not want the current working directory to be removed, even if it becomes
empty.

dir also has a remove_dir_recursively() function which appears to mostly
be used to remove metadata directories or temporary directories or
submodules or worktrees.  I am not sure if it needs to be protected
against removing the current working directory, but did so for good
measure.

Signed-off-by: Elijah Newren <redacted>
---
 dir.c                | 11 ++++++++---
 t/t2501-cwd-empty.sh |  4 ++--
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index 94489298f4c..15e7639a158 100644
--- a/dir.c
+++ b/dir.c

@@ -3259,9 +3259,12 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 	closedir(dir);
 
 	strbuf_setlen(path, original_len);
-	if (!ret && !keep_toplevel && !kept_down)
-		ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
-	else if (kept_up)
+	if (!ret && !keep_toplevel && !kept_down) {
+		if (the_cwd && !strcmp(the_cwd, path->buf))
+			ret = -1; /* Do not remove current working directory */
+		else
+			ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
+	} else if (kept_up)
 		/*
 		 * report the uplevel that it is not an error that we
 		 * did not rmdir() our directory.

@@ -3327,6 +3330,8 @@ int remove_path(const char *name)
 		slash = dirs + (slash - name);
 		do {
 			*slash = '\0';
+			if (the_cwd && !strcmp(the_cwd, dirs))
+				break;
 		} while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));
 		free(dirs);
 	}

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index ff4e7cd89fa..4362e7b15e5 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh

@@ -191,7 +191,7 @@ test_expect_success 'revert fails if cwd needs to be removed' '
 	test_path_is_dir dirORfile
 '
 
-test_expect_failure 'rm does not remove cwd incidentally' '
+test_expect_success 'rm does not remove cwd incidentally' '
 	test_when_finished "git reset --hard" &&
 	git checkout foo/bar/baz &&
 

@@ -205,7 +205,7 @@ test_expect_failure 'rm does not remove cwd incidentally' '
 	test_path_is_dir foo
 '
 
-test_expect_failure 'apply does not remove cwd incidentally' '
+test_expect_success 'apply does not remove cwd incidentally' '
 	test_when_finished "git reset --hard" &&
 	git checkout foo/bar/baz &&
 

-- 
gitgitgadget

Am 21.11.21 um 01:46 schrieb Elijah Newren via GitGitGadget:

From: Elijah Newren <redacted>

Removing the current working directory causes all subsequent git
commands (and likely a number of non-git commands) run from that
directory to get confused and fail with a message about being unable to
read the current working directory.  That confuses end users,
particularly since the command they get the error from is not the one
that caused the problem; the problem came from the side-effect of some
previous command.

We would like to avoid removing the current working directory;

A worthy goal.

towards
this end, introduce a new the_cwd variable that tracks the current
working directory.  Subsequent commits will make use of this new
variable.

Why make it a global variable instead of getting the working directory
in the places that try to delete directories?  (Honest question, not a
suggestion.)

Am 21.11.21 um 01:46 schrieb Elijah Newren via GitGitGadget:

Initializing the_cwd to an empty string would allow removing the NULL check
everywhere.

Is strcmp() sufficient or do we need fspathcmp() in these kinds of checks?
Do we need to worry about normalizing directory separators?

+		return 1;
+	return rmdir(dirname);
+}

I wouldn't expect a function of that name to actually try to remove
the directory.  Or with that body to require a non-const dirname.
It's used only once, perhaps inline it?

On Sun, Nov 21 2021, Elijah Newren via GitGitGadget wrote:

+		int prefixlen = prefix ? strlen(prefix) : 0;

nit: s/int/size_t/

On Sun, Nov 21 2021, Elijah Newren via GitGitGadget wrote:

From: Elijah Newren <redacted>

+test_expect_failure 'checkout fails 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 checkout fd_conflict 2>../error &&
+		grep "Refusing to remove the current working directory" ../error
+	) &&
+
+	test_path_is_dir dirORfile

I'd find this & the rest of this series much easier to understand if we
started out by positively asserting the current behavior here, and
didn't test_cmp/grep for erro r messages that don't exist anymore.

It would also help to show how exactly operations that currently "work"
behave, e.g. if you git checkout a revision within "t/" which is a
subdir, and it turns into "t" the file etc.

I'm also generally knee-jerk reactionary to test_expect_failure for its
fragility[1], but in this case more because it makes explaining this
step-by-step harder & not as obvious.

1. https://lore.kernel.org/git/87tuhmk19c.fsf@evledraar.gmail.com/

On Sun, Nov 21, 2021 at 12:56 AM René Scharfe [off-list ref] wrote:

Am 21.11.21 um 01:46 schrieb Elijah Newren via GitGitGadget:

quoted

From: Elijah Newren <redacted>

Removing the current working directory causes all subsequent git
commands (and likely a number of non-git commands) run from that
directory to get confused and fail with a message about being unable to
read the current working directory.  That confuses end users,
particularly since the command they get the error from is not the one
that caused the problem; the problem came from the side-effect of some
previous command.

We would like to avoid removing the current working directory;

A worthy goal.

quoted

towards
this end, introduce a new the_cwd variable that tracks the current
working directory.  Subsequent commits will make use of this new
variable.

Why make it a global variable instead of getting the working directory
in the places that try to delete directories?  (Honest question, not a
suggestion.)

As I mentioned in my response to Junio, I need to be clearer that what
I want to protect is the current working directory as of the startup
of the git process, as a proxy for the current working directory of
the parent process, so that subsequent commands started from the
parent process don't get confused.

As such, we don't want to get the working directory again later,
because that'd give us the wrong thing.  Also, setup.c has some nice
massaging of getcwd() from an absolute path into a relative path that
is much more convenient for us to do our later comparisons, and we'd
rather not do those additional tweaks with every check.  Since the
relative path from the project root to the _original_ current working
directory is a single value, a global seemed to make sense for saving
it.  Perhaps I should tweak it to be const as well.

quoted

Signed-off-by: Elijah Newren <redacted>
---
 repository.c | 1 +
 repository.h | 1 +
 setup.c      | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/repository.c b/repository.c
index c5b90ba93ea..69a106c553c 100644
--- a/repository.c
+++ b/repository.c

@@ -17,6 +17,7 @@
 static struct repository the_repo;
 struct repository *the_repository;
 struct index_state the_index;
+char *the_cwd;

 void initialize_the_repository(void)
 {

diff --git a/repository.h b/repository.h
index a057653981c..45de85d18ef 100644
--- a/repository.h
+++ b/repository.h

@@ -147,6 +147,7 @@ struct repository {
 };

 extern struct repository *the_repository;
+extern char *the_cwd;

 /*
  * Define a custom repository layout. Any field can be NULL, which

diff --git a/setup.c b/setup.c
index 347d7181ae9..4466fa55af3 100644
--- a/setup.c
+++ b/setup.c

@@ -887,6 +887,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
              set_git_dir(gitdirenv, 1);
              if (chdir(worktree))
                      die_errno(_("cannot chdir to '%s'"), worktree);
+             the_cwd = xstrdup(cwd->buf + offset);
              strbuf_addch(cwd, '/');
              free(gitfile);
              return cwd->buf + offset;

@@ -940,6 +941,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
      /* Make "offset" point past the '/' (already the case for root dirs) */
      if (offset != offset_1st_component(cwd->buf))
              offset++;
+     the_cwd = xstrdup(cwd->buf + offset);
      /* Add a '/' at the end */
      strbuf_addch(cwd, '/');
      return cwd->buf + offset;

On Sun, Nov 21, 2021 at 12:56 AM René Scharfe [off-list ref] wrote:

Am 21.11.21 um 01:46 schrieb Elijah Newren via GitGitGadget:

quoted

From: Elijah Newren <redacted>

symlinks has a pair of schedule_dir_for_removal() and
remove_scheduled_dirs() functions that ensure that directories made
empty by removing other files also themselves get removed.  However, we
want to exclude the current working directory and leave it around so
that subsequent git commands (and non-git commands) that the user runs
afterwards don't cause the user to get confused.

Signed-off-by: Elijah Newren <redacted>
---
 symlinks.c           | 12 +++++++++++-
 t/t2501-cwd-empty.sh | 12 ++++++------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/symlinks.c b/symlinks.c
index 5232d02020c..84622bedcde 100644
--- a/symlinks.c
+++ b/symlinks.c

@@ -275,11 +275,18 @@ static int threaded_has_dirs_only_path(struct cache_def *cache, const char *name

 static struct strbuf removal = STRBUF_INIT;

+static int cant_remove(char *dirname)
+{
+     if (the_cwd && !strcmp(dirname, the_cwd))

Initializing the_cwd to an empty string would allow removing the NULL check
everywhere.

I actually went and made this change, because although it'd add
protection to the toplevel directory (the_cwd is relative to the
toplevel), we usually want it protected.  However, dir.c's
remove_dir_recursively() can be used to remove the toplevel directory
as well and has an explicit flag for that, so I need to be able to
distinguish between uninitialized and explicitly set to the toplevel
directory.

Is strcmp() sufficient or do we need fspathcmp() in these kinds of checks?
Do we need to worry about normalizing directory separators?

Good catch, I should normalize the_cwd when I create it; I was
essentially creating it to match "prefix", but it appears that isn't
pre-emptively normalized, and instead later callers normalize any
combination of prefix plus another path.

quoted

+             return 1;
+     return rmdir(dirname);
+}

I wouldn't expect a function of that name to actually try to remove
the directory.  Or with that body to require a non-const dirname.
It's used only once, perhaps inline it?

Sure.

quoted

+
 static void do_remove_scheduled_dirs(int new_len)
 {
      while (removal.len > new_len) {
              removal.buf[removal.len] = '\0';
-             if (rmdir(removal.buf))
+             if (cant_remove(removal.buf))
                      break;
              do {
                      removal.len--;

@@ -293,6 +300,9 @@ void schedule_dir_for_removal(const char *name, int len)
 {
      int match_len, last_slash, i, previous_slash;

+     if (the_cwd && !strcmp(name, the_cwd))
+             return; /* Do not remove the current working directory */
+
      match_len = last_slash = i =
              longest_path_match(name, len, removal.buf, removal.len,
                                 &previous_slash);

I don't think it's appropriate to implement user-facing concern (don't remove 
cwd because this will confuse users) in library functions like remove_path().
remove_path() has other callers e.g. midx.c, and possible future callers e.g. 
we're working on adding a command to delete corrupted commit-graphs and this 
library function would be extremely handy.

It seems more appropriate to check the_cwd from builtin/add.c and builtin/rm.c 
instead.

On Mon, Nov 22, 2021 at 4:40 PM Glen Choo [off-list ref] wrote:

quoted

@@ -3259,9 +3259,12 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
      closedir(dir);

      strbuf_setlen(path, original_len);
-     if (!ret && !keep_toplevel && !kept_down)
-             ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
-     else if (kept_up)
+     if (!ret && !keep_toplevel && !kept_down) {
+             if (the_cwd && !strcmp(the_cwd, path->buf))
+                     ret = -1; /* Do not remove current working directory */
+             else
+                     ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
+     } else if (kept_up)
              /*
               * report the uplevel that it is not an error that we
               * did not rmdir() our directory.

@@ -3327,6 +3330,8 @@ int remove_path(const char *name)
              slash = dirs + (slash - name);
              do {
                      *slash = '\0';
+                     if (the_cwd && !strcmp(the_cwd, dirs))
+                             break;
              } while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));
              free(dirs);
      }

I don't think it's appropriate to implement user-facing concern (don't remove
cwd because this will confuse users) in library functions like remove_path().
remove_path() has other callers e.g. midx.c, and possible future callers e.g.
we're working on adding a command to delete corrupted commit-graphs and this
library function would be extremely handy.

I think we'd want this code change for those cases too.  Said another
way, why wouldn't these callers want to avoid deleting the original
current working directory of the git process (which is likely still
the current working directory of the parent process)?  Deleting that
directory causes problems regardless of whether it's a user-facing
command (rm, merge, stash, etc.) or something more internal (midx or
commit-graphs stuff being called by gc) that is doing the deleting.

Putting it in this helper function means we protect all current and
future callers without developers having to remember which
"remove_path()" variant they need and why.

It seems more appropriate to check the_cwd from builtin/add.c and builtin/rm.c
instead.

Not sure how you determined that those two paths are affected or that
those are the only two.

On Sun, Nov 21, 2021 at 9:51 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Sun, Nov 21 2021, Elijah Newren via GitGitGadget wrote:

quoted

+             int prefixlen = prefix ? strlen(prefix) : 0;

nit: s/int/size_t/

Thanks; will fix.

On Sun, Nov 21, 2021 at 9:59 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Sun, Nov 21 2021, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

quoted

+test_expect_failure 'checkout fails 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 checkout fd_conflict 2>../error &&
+             grep "Refusing to remove the current working directory" ../error
+     ) &&
+
+     test_path_is_dir dirORfile

I'd find this & the rest of this series much easier to understand if we
started out by positively asserting the current behavior here, and
didn't test_cmp/grep for erro r messages that don't exist anymore.

Yeah, this is my fault for my bad commit message.  I stated I was
adding tests checking for the problems of interest, making it sound
like I was testing existing behavior, but I should have stated I was
adding tests with the behavior we'd prefer to have (i.e. basically a
test-driven-development) setup.

Also, there really wouldn't need to be so many tests for describing
the existing behavior.  It's basically just `git
$OPERATION_THAT_REMOVES_CWD_AS_SIDE_EFFECT` followed by nearly any
other git command will cause the second and later commands to fail
with:

shell-init: error retrieving current directory: getcwd: cannot access
parent directories: No such file or directory
fatal: Unable to read current working directory: No such file or directory

However, we do need a lot of tests for corrected behavior, because
there are so many different codepaths we can follow which will lead to
deletion of the current working directory.

On Mon, Nov 22 2021, Elijah Newren wrote:

On Sun, Nov 21, 2021 at 9:59 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

On Sun, Nov 21 2021, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

quoted

+test_expect_failure 'checkout fails 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 checkout fd_conflict 2>../error &&
+             grep "Refusing to remove the current working directory" ../error
+     ) &&
+
+     test_path_is_dir dirORfile

I'd find this & the rest of this series much easier to understand if we
started out by positively asserting the current behavior here, and
didn't test_cmp/grep for erro r messages that don't exist anymore.

Yeah, this is my fault for my bad commit message.  I stated I was
adding tests checking for the problems of interest, making it sound
like I was testing existing behavior, but I should have stated I was
adding tests with the behavior we'd prefer to have (i.e. basically a
test-driven-development) setup.

Also, there really wouldn't need to be so many tests for describing
the existing behavior.  It's basically just `git
$OPERATION_THAT_REMOVES_CWD_AS_SIDE_EFFECT` followed by nearly any
other git command will cause the second and later commands to fail
with:

shell-init: error retrieving current directory: getcwd: cannot access
parent directories: No such file or directory
fatal: Unable to read current working directory: No such file or directory

However, we do need a lot of tests for corrected behavior, because
there are so many different codepaths we can follow which will lead to
deletion of the current working directory.

Currently if I do e.g.:

    git checkout master
    git clean -dxf
    cd perl
    git checkout v0.99
    cd ../
    git clean -dxfn

Nothing breaks and I don't end up with an empty perl/ to remove. With
these patches we'd either die on the "checkout" (I think) keep the
"perl" and have an empty perl/ to report in the "git clean -dxfn" at the
end (I'm not sure which, I forgot and haven't re-read this series just
now).

I think changing it anyway might be justifiable, but changing the
behavior of things like that tickles my spidey sense a bit. I.e. I can
see people having written scripts like that which would break (it's
often easier to cd around after globbing than staying at the top-level,
then jump back).

So I wonder (especially with Glen's comment in
[off-list ref]) if this is being done at
the right API level. E.g. maybe it would be better for some commands to
ease into this with an advise() or warning() and not a die() or error(),
or have the die() be in the likes of "git switch" but not "reset
--hard".

Or maybe not, just food for thought...

On Mon, Nov 22, 2021 at 6:27 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Mon, Nov 22 2021, Elijah Newren wrote:

quoted

On Sun, Nov 21, 2021 at 9:59 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

On Sun, Nov 21 2021, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>

quoted

+test_expect_failure 'checkout fails 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 checkout fd_conflict 2>../error &&
+             grep "Refusing to remove the current working directory" ../error
+     ) &&
+
+     test_path_is_dir dirORfile

I'd find this & the rest of this series much easier to understand if we
started out by positively asserting the current behavior here, and
didn't test_cmp/grep for erro r messages that don't exist anymore.

Yeah, this is my fault for my bad commit message.  I stated I was
adding tests checking for the problems of interest, making it sound
like I was testing existing behavior, but I should have stated I was
adding tests with the behavior we'd prefer to have (i.e. basically a
test-driven-development) setup.

Also, there really wouldn't need to be so many tests for describing
the existing behavior.  It's basically just `git
$OPERATION_THAT_REMOVES_CWD_AS_SIDE_EFFECT` followed by nearly any
other git command will cause the second and later commands to fail
with:

shell-init: error retrieving current directory: getcwd: cannot access
parent directories: No such file or directory
fatal: Unable to read current working directory: No such file or directory

However, we do need a lot of tests for corrected behavior, because
there are so many different codepaths we can follow which will lead to
deletion of the current working directory.

Currently if I do e.g.:

    git checkout master
    git clean -dxf
    cd perl
    git checkout v0.99
    cd ../
    git clean -dxfn

Nothing breaks and I don't end up with an empty perl/ to remove. With
these patches we'd either die on the "checkout" (I think) keep the
"perl" and have an empty perl/ to report in the "git clean -dxfn" at the
end (I'm not sure which, I forgot and haven't re-read this series just
now).

You'd have an empty perl/ left after the checkout, which would be
cleaned up by your final git clean command.

I think changing it anyway might be justifiable, but changing the
behavior of things like that tickles my spidey sense a bit. I.e. I can
see people having written scripts like that which would break (it's
often easier to cd around after globbing than staying at the top-level,
then jump back).

I disagree this would break any user scripts.  If people expect a 'git
checkout' or 'git rebase' to always work, their script is _already_
broken.  The presence of any untracked files within the directory
already results in a hard error -- we refuse to remove non-empty
directories (unless all files are tracked and unmodified).  This rule
deserves a clarification: treat the current working directory as
non-empty since the parent process is likely still parked there..

Further, our own commands are broken/misbehaving due to us not
erroring out; see e.g.
https://lore.kernel.org/git/xmqqv93n7q1v.fsf@gitster.g/ and its
grandparent.  User scripts likely have lurking problems too.

So I wonder (especially with Glen's comment in
[off-list ref]) if this is being done at
the right API level.

Glen's comment was interesting, but provided no specifics about how
the changes I made could cause any possible harm in his case.
Further, the fact that others are adding extra places doing cleanup
sound like additional codepaths that should be protected for the exact
same reasons.  I think we absolutely want my changes affecting his new
codepaths.

E.g. maybe it would be better for some commands to
ease into this with an advise() or warning() and not a die() or error(),
or have the die() be in the likes of "git switch" but not "reset
--hard".

The commands that don't need to remove the current working directory
but just were as a convenience, no longer do and continue on just
fine.  Commands that need to remove the current working directory in
order to place a file there will error out, just as they would have
when trying to remove a directory with untracked files.  I see no need
to ease into anything here.

Or maybe not, just food for thought...

You may also be interested in reading more of the other thread I
linked to from my cover letter; all these cases were discussed in good
detail over there.  For example, look at
https://lore.kernel.org/git/CABPp-BFmU+RaAjq4_0-PSfRgH1Jc63nN0fMuDWk2+iDbdz7CCA@mail.gmail.com/.
Peff's previous suggestion was to just make the commands error out if
they'd normally remove the current working directory and require the
user to run from a different directory instead.  My version lightened
that requirement so it only errors out if the current working
directory needed to be removed in order to place something else there
(and if nothing else was needed to be placed there, then just leaving
the directory around).

Elijah Newren [off-list ref] writes:

quoted

I don't think it's appropriate to implement user-facing concern (don't remove
cwd because this will confuse users) in library functions like remove_path().
remove_path() has other callers e.g. midx.c, and possible future callers e.g.
we're working on adding a command to delete corrupted commit-graphs and this
library function would be extremely handy.

I think we'd want this code change for those cases too.  Said another
way, why wouldn't these callers want to avoid deleting the original
current working directory of the git process (which is likely still
the current working directory of the parent process)?  Deleting that
directory causes problems regardless of whether it's a user-facing
command (rm, merge, stash, etc.) or something more internal (midx or
commit-graphs stuff being called by gc) that is doing the deleting.

I agree that most, possibly all, of our commands should prefer to die
than to remove the cwd, but that doesn't justify adding
application-level concerns to a general-purpose utility function. Even
if it sounds overly defensive, having an obviously correct utility
function makes it easier for future authors to know exactly what their
code is doing and why. And surely if we're imaginative enough, we can
definitely dream up some possible use cases for remove_path() that don't
want this dying behavior e.g. other applications that link to our
libraries, or some new merge strategy that may need to remove + restore
the cwd.

I'm not going to say that we'll *definitely* need remove_path()
in its current form, but mixing concerns like this is an invitation to
unexpected behavior. An (imperfect) example that demonstrates this
principle is https://lore.kernel.org/git/24bffdab139435173712101aaf72f7277298c99d.1632497954.git.gitgitgadget@gmail.com/,
where we made a change to a generic path matching function in order to
speed up unpack_trees(), but accidentally ended up breaking gitignore.

Putting it in this helper function means we protect all current and
future callers without developers having to remember which
"remove_path()" variant they need and why.

Code comments sound like adequate protection to me. Fudging the names a
little..

  /*
   * Pretend we have utility function that generalizes
   * check-then-delete (though we probably won't need it).
   */
  static int remove_path_conditionally(const char *name, check_path_fn can_delete_path);

  /**
   * This is identical to remove_path(), except that it will die if
   * attempting to remove the_cwd. When writing Git commands, prefer
   * using this over remove_path() so that we don't delete the cwd and
   * leave the user in a confusing state.
   */
  int remove_path_except_cwd(const char *name)
  {
    return remove_path_conditionally(name, die_on_cwd);
  }

  /*
   * Tries to remove the path with empty directories along it, ignores
   * ENOENT. Unless you really need to remove the path unconditionally,
   * consider using remove_path_except_cwd() instead.
   */
  int remove_path(const char *name);

quoted

It seems more appropriate to check the_cwd from builtin/add.c and builtin/rm.c
instead.

Not sure how you determined that those two paths are affected or that
those are the only two.

Typo: s/add/apply.

I took the example from your test cases:

  diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
  index ff4e7cd89fa..4362e7b15e5 100755
  --- a/t/t2501-cwd-empty.sh
  +++ b/t/t2501-cwd-empty.sh
  @@ -191,7 +191,7 @@ test_expect_success 'revert fails if cwd needs to be removed' '
    test_path_is_dir dirORfile
   '
 
  -test_expect_failure 'rm does not remove cwd incidentally' '
  +test_expect_success 'rm does not remove cwd incidentally' '
    test_when_finished "git reset --hard" &&
    git checkout foo/bar/baz &&
 
  @@ -205,7 +205,7 @@ test_expect_failure 'rm does not remove cwd incidentally' '
    test_path_is_dir foo
   '
 
  -test_expect_failure 'apply does not remove cwd incidentally' '
  +test_expect_success 'apply does not remove cwd incidentally' '
    test_when_finished "git reset --hard" &&
    git checkout foo/bar/baz &&

I read this as "I made these changes in order to make these tests pass".
I really like the 'TDD-ish' approach you used in this series; as a
reader, it gave me a clear idea of the expected outcome of your changes.
From that perspective, the fact that there are certainly untested paths
which are affected takes away some of the benefits of this approach.

On Tue, Nov 23, 2021 at 10:19 AM Glen Choo [off-list ref] wrote:

Elijah Newren [off-list ref] writes:

quoted

quoted

I don't think it's appropriate to implement user-facing concern (don't remove
cwd because this will confuse users) in library functions like remove_path().
remove_path() has other callers e.g. midx.c, and possible future callers e.g.
we're working on adding a command to delete corrupted commit-graphs and this
library function would be extremely handy.

I think we'd want this code change for those cases too.  Said another
way, why wouldn't these callers want to avoid deleting the original
current working directory of the git process (which is likely still
the current working directory of the parent process)?  Deleting that
directory causes problems regardless of whether it's a user-facing
command (rm, merge, stash, etc.) or something more internal (midx or
commit-graphs stuff being called by gc) that is doing the deleting.

I agree that most, possibly all, of our commands should prefer to die
than to remove the cwd, but that doesn't justify adding
application-level concerns to a general-purpose utility function. Even
if it sounds overly defensive, having an obviously correct utility
function makes it easier for future authors to know exactly what their
code is doing and why. And surely if we're imaginative enough, we can
definitely dream up some possible use cases for remove_path() that don't
want this dying behavior e.g. other applications that link to our
libraries, or some new merge strategy that may need to remove + restore
the cwd.

Sounds like your objections here are based on a misunderstanding.  I
totally agree with you that adding dying behavior to these functions
would be wrong.

My patch doesn't do that.

I'm not going to say that we'll *definitely* need remove_path()
in its current form, but mixing concerns like this is an invitation to
unexpected behavior. An (imperfect) example that demonstrates this
principle is https://lore.kernel.org/git/24bffdab139435173712101aaf72f7277298c99d.1632497954.git.gitgitgadget@gmail.com/,
where we made a change to a generic path matching function in order to
speed up unpack_trees(), but accidentally ended up breaking gitignore.

There's no mixture of concerns; my patch is correcting this library
function to more fully match its documented intent; from dir.h:

    /* tries to remove the path with empty directories along it,
ignores ENOENT */
    int remove_path(const char *path);

Since the parent process's current working directory is still likely
parked in that directory, there is a good reason to treat it as
non-empty.  Thus the cwd should not be one of those directories
removed along with the specified path.  No need to die, just stop
removing the leading directories once it hits the cwd (much like it'd
stop once it hit a directory that had files left in it).

Elijah Newren [off-list ref] writes:

quoted

I agree that most, possibly all, of our commands should prefer to die
than to remove the cwd, but that doesn't justify adding
application-level concerns to a general-purpose utility function. Even
if it sounds overly defensive, having an obviously correct utility
function makes it easier for future authors to know exactly what their
code is doing and why. And surely if we're imaginative enough, we can
definitely dream up some possible use cases for remove_path() that don't
want this dying behavior e.g. other applications that link to our
libraries, or some new merge strategy that may need to remove + restore
the cwd.

Sounds like your objections here are based on a misunderstanding.  I
totally agree with you that adding dying behavior to these functions
would be wrong.

My patch doesn't do that.

Ah my mistake, that should be s/die/'stop gently'. Even so, that is not
at the core of my objection, mixing of concerns is.

quoted

I'm not going to say that we'll *definitely* need remove_path()
in its current form, but mixing concerns like this is an invitation to
unexpected behavior. An (imperfect) example that demonstrates this
principle is https://lore.kernel.org/git/24bffdab139435173712101aaf72f7277298c99d.1632497954.git.gitgitgadget@gmail.com/,
where we made a change to a generic path matching function in order to
speed up unpack_trees(), but accidentally ended up breaking gitignore.

There's no mixture of concerns; my patch is correcting this library
function to more fully match its documented intent; from dir.h:

    /* tries to remove the path with empty directories along it,
ignores ENOENT */
    int remove_path(const char *path);

I don't think that there is a mismatch; reading the implementation +
documented intent seems to make it clear that 'emptiness' is defined by
directory contents, not the presence of any processes using it as its
current working directory.

Since the parent process's current working directory is still likely
parked in that directory, there is a good reason to treat it as
non-empty.  Thus the cwd should not be one of those directories
removed along with the specified path.  No need to die, just stop
removing the leading directories once it hits the cwd (much like it'd
stop once it hit a directory that had files left in it).

This doesn't sound like a typical definition of 'emptiness' to me, but I
can accept it if others also find it compelling. IOW if your definition
of 'emptiness' is compelling enough, then I'll be convinced that there
is no mixing of concerns and there would be no objection.

On Tue, Nov 23, 2021 at 12:33 PM Glen Choo [off-list ref] wrote:

Elijah Newren [off-list ref] writes:

quoted

quoted

I agree that most, possibly all, of our commands should prefer to die
than to remove the cwd, but that doesn't justify adding
application-level concerns to a general-purpose utility function. Even
if it sounds overly defensive, having an obviously correct utility
function makes it easier for future authors to know exactly what their
code is doing and why. And surely if we're imaginative enough, we can
definitely dream up some possible use cases for remove_path() that don't
want this dying behavior e.g. other applications that link to our
libraries, or some new merge strategy that may need to remove + restore
the cwd.

Sounds like your objections here are based on a misunderstanding.  I
totally agree with you that adding dying behavior to these functions
would be wrong.

My patch doesn't do that.

Ah my mistake, that should be s/die/'stop gently'. Even so, that is not
at the core of my objection, mixing of concerns is.

If I were to introduce a new function, say remove_path_not_cwd(), to
avoid this claimed mixing of concerns, what would that buy us?

I've looked at every single caller of remove_path() in the git
codebase.  If I did introduce a new function, as you seem to want, my
series would include two more commits: one that would replace _every_
call of remove_path() in the codebase with a call to the new function,
and one that would delete the remove_path() declaration and definition
in dir.[ch] since they would be unused.  The net effect would be
merely forcing git developers to learn a different function name.
(I'd probably also follow it up later with another commit to rename
remove_path_not_cwd() to remove_path(), for simplicity, and so I don't
have to remember the longer name anymore.)


I haven't yet found or heard of any potential callers, even
hypothetical, that would be harmed by the modified behavior.  Every
case suggested so far actually sounds like a good candidate for the
modified behavior.

Elijah Newren [off-list ref] writes:

quoted

quoted

quoted

I agree that most, possibly all, of our commands should prefer to die
than to remove the cwd, but that doesn't justify adding
application-level concerns to a general-purpose utility function. Even
if it sounds overly defensive, having an obviously correct utility
function makes it easier for future authors to know exactly what their
code is doing and why. And surely if we're imaginative enough, we can
definitely dream up some possible use cases for remove_path() that don't
want this dying behavior e.g. other applications that link to our
libraries, or some new merge strategy that may need to remove + restore
the cwd.

Sounds like your objections here are based on a misunderstanding.  I
totally agree with you that adding dying behavior to these functions
would be wrong.

My patch doesn't do that.

Ah my mistake, that should be s/die/'stop gently'. Even so, that is not
at the core of my objection, mixing of concerns is.

If I were to introduce a new function, say remove_path_not_cwd(), to
avoid this claimed mixing of concerns, what would that buy us?
I've looked at every single caller of remove_path() in the git
codebase.  If I did introduce a new function, as you seem to want, my
series would include two more commits: one that would replace _every_
call of remove_path() in the codebase with a call to the new function,
and one that would delete the remove_path() declaration and definition
in dir.[ch] since they would be unused.

There is at least one other possible outcome, which is that
remove_path() is replaced with remove_path_not_cwd() in all callers that
obviously want it e.g. builtins, but not replaced in other callers. My
mental model of this is that the two functions serve two different use
cases:

1) remove_path(): Remove a path and all empty directories
2) remove_path_not_cwd(): Remove a path and all empty directories,
   except cwd

I haven't yet found or heard of any potential callers, even
hypothetical, that would be harmed by the modified behavior.  Every
case suggested so far actually sounds like a good candidate for the
modified behavior.

I trust that you have considered this change carefully, so I will
downgrade my objection to a nitpick. remove_path() seems nice to have as
a low-level function but I certainly can't imagine any non-contrived
use cases that *wouldn't* benefit.

To me, a more compelling argument is that protecting cwd is important
in order to ensure correctness, and user experience is an incidental
benefit. AFAICT that is not the argument you are making, but perhaps
there is some correctness benefit as well?

On Tue, Nov 23 2021, Elijah Newren wrote:

On Tue, Nov 23, 2021 at 10:19 AM Glen Choo [off-list ref] wrote:

quoted

Elijah Newren [off-list ref] writes:
[...]
I'm not going to say that we'll *definitely* need remove_path()
in its current form, but mixing concerns like this is an invitation to
unexpected behavior. An (imperfect) example that demonstrates this
principle is https://lore.kernel.org/git/24bffdab139435173712101aaf72f7277298c99d.1632497954.git.gitgitgadget@gmail.com/,
where we made a change to a generic path matching function in order to
speed up unpack_trees(), but accidentally ended up breaking gitignore.

There's no mixture of concerns; my patch is correcting this library
function to more fully match its documented intent; from dir.h:

    /* tries to remove the path with empty directories along it,
ignores ENOENT */
    int remove_path(const char *path);

Since the parent process's current working directory is still likely
parked in that directory, there is a good reason to treat it as
non-empty.  Thus the cwd should not be one of those directories
removed along with the specified path.  No need to die, just stop
removing the leading directories once it hits the cwd (much like it'd
stop once it hit a directory that had files left in it).

I can buy that we'd pick this new behavior as a worthwhile trade-off,
but not that anyone intended for this to be the behavior all along.

I don't think "a process is sitting in it" has ever been anyone's idea
of a "non-empty directory". Rather it's what rmdir() returning EEXIST or
ENOTEMPTY maps to.

Doesn't this series also change the behavior of e.g.:

    cd contrib/subtree
    git rm -r ../subtree

If so then re the "spidey sense" comment I had earlier: There's no rm
codepaths or tests changed by this series, so the implementation of
doing it at this lower level might be casting too wide a net. Wouldn't
e.g. changing callers that use "remove_dir_recursively()" to use a
"remove_dir_recursively_not_cwd()" (or whatever) be a gentler way of
introducing this, and make sure that each step of the way we grok what's
being changed, that there's test coverage etc.

On Tue, Nov 23, 2021 at 5:19 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Tue, Nov 23 2021, Elijah Newren wrote:

quoted

On Tue, Nov 23, 2021 at 10:19 AM Glen Choo [off-list ref] wrote:

quoted

Elijah Newren [off-list ref] writes:
[...]
I'm not going to say that we'll *definitely* need remove_path()
in its current form, but mixing concerns like this is an invitation to
unexpected behavior. An (imperfect) example that demonstrates this
principle is https://lore.kernel.org/git/24bffdab139435173712101aaf72f7277298c99d.1632497954.git.gitgitgadget@gmail.com/,
where we made a change to a generic path matching function in order to
speed up unpack_trees(), but accidentally ended up breaking gitignore.

There's no mixture of concerns; my patch is correcting this library
function to more fully match its documented intent; from dir.h:

    /* tries to remove the path with empty directories along it,
ignores ENOENT */
    int remove_path(const char *path);

Since the parent process's current working directory is still likely
parked in that directory, there is a good reason to treat it as
non-empty.  Thus the cwd should not be one of those directories
removed along with the specified path.  No need to die, just stop
removing the leading directories once it hits the cwd (much like it'd
stop once it hit a directory that had files left in it).

I can buy that we'd pick this new behavior as a worthwhile trade-off,
but not that anyone intended for this to be the behavior all along.

I don't think "a process is sitting in it" has ever been anyone's idea
of a "non-empty directory". Rather it's what rmdir() returning EEXIST or
ENOTEMPTY maps to.

Yeah, Junio commented on my reasoning in that same paragraph of mine.
Bad reasoning on my part, and you were both right to call it out.

But that reasoning wasn't the underlying motivation for Peff to
suggest the behavior behind this series[1] and this patch, nor the
rationale Junio used to say that the overall behavioral change behind
this series "makes sense".[2]

[1] https://lore.kernel.org/git/YS8eEtwQvF7TaLCb@coredump.intra.peff.net/
[2] https://lore.kernel.org/git/xmqqo86elyht.fsf@gitster.g/

Doesn't this series also change the behavior of e.g.:

    cd contrib/subtree
    git rm -r ../subtree

Yes, of course!

Before:

    $ cd contrib/subtree
    $ git rm -r -q ../subtree/
    $ ls -ld ../subtree/
    ls: cannot access '../subtree/': No such file or directory
    $ git status --porcelain
    fatal: Unable to read current working directory: No such file or directory
    $ git checkout HEAD ../subtree/
    fatal: Unable to read current working directory: No such file or directory

After:

    $ cd contrib/subtree
    $ git rm -r -q ../subtree/
    $ ls -ld ../subtree/
    drwxrwxr-x. 1 newren newren 0 Nov 23 19:18 ../subtree/
    $ git status --porcelain
    D  contrib/subtree/.gitignore
    D  contrib/subtree/COPYING
    D  contrib/subtree/INSTALL
    D  contrib/subtree/Makefile
    D  contrib/subtree/README
    D  contrib/subtree/git-subtree.sh
    D  contrib/subtree/git-subtree.txt
    D  contrib/subtree/t/Makefile
    D  contrib/subtree/t/t7900-subtree.sh
    D  contrib/subtree/todo
    $ git checkout HEAD ../subtree/
    Updated 10 paths from c557be478e

Very nice fix, don't you think?

If so then re the "spidey sense" comment I had earlier: There's no rm
codepaths or tests changed by this series,

That's not correct; I explicitly added a new rm test in the first
patch in my series.  Further, that same test was modified to mark it
as passing by this particular patch you are commenting on.

so the implementation of
doing it at this lower level might be casting too wide a net.

I'm getting the vibe that you are assuming I'm changing these two
functions without realizing what places might be calling them;
basically, that I'm just flippantly changing them.  Ignoring the
ramifications of such an assumption, if this vibe is correct, then let
me inform you that I've read over each and every caller (as well as
searched for other callers of unlink() and rmdir() throughout the tree
to see if they needed similar changes).  In my opinion, *each* *and*
*every* *single* *one* of the calls to remove_path() and
remove_dir_recursively() should take the behavioral change suggested
in this patch.

It's also not clear to me that you understand the point of the change
behind the series.  Clearly, I'm not doing well explaining it, but
have you read Peff's or Junio's comments on why they thought
protecting the_original_cwd makes sense?  Again, see the links [1] and
[2] above.  I think it'd help me understand how to respond to you
better if you could clarify to me whether you disagree with them, or
whether you agree with them but think I've gone wrong in the
implementation of their high level explanation somehow in this
particular patch.

e.g. changing callers that use "remove_dir_recursively()" to use a
"remove_dir_recursively_not_cwd()" (or whatever) be a gentler way of
introducing this, and make sure that each step of the way we grok what's
being changed, that there's test coverage etc.

I'll ask you the same two questions I asked Glen when he suggested
basically the same thing; if you can provide an answer to either one
of my questions that is compelling to me, I'd be totally onboard with
your suggested change:

(1) Can you point to any concrete example caller anywhere in the code
tree (or even provide a future hypothetical caller) that would in fact
be harmed by the change in this patch?

...and no, I'm not asking you to do my work for me; I've done that
same work -- in fact looking at all callsites -- and came up
empty-handed.  Since this and your previous email are essentially
claiming that I've probably missed things, I think it's a fair
question.

(2) What benefit would there be to introducing these new functions?

In particular, if I were to introduce these new functions, it would
look like this:

  * add new remove_path_not_cwd() and remove_dir_recursively_not_cwd()
function in one patch
  * convert all relevant callsites to use these new functions in the
subsequent patch(es)
  * delete the existing remove_path() and remove_dir_recursively()
functions for two reasons: (1) they are now unused, and (2) future
potential callers of these old functions would more than likely
reintroduce the bugs this series is trying to fix if they were to use
them and should be discouraged from doing so
  * rename {remove_path,remove_dir_recursively}_not_cwd() to remove
the "_not_cwd" suffix in order to have more memorable and less ugly
names

Once finished, the end result would be identical to this patch.  Well,
identical other than taking more patches to get to the same end result
and using up more reviewer time.  If there's some benefit to taking
this circuitous route, though, I'm more than willing to do so.

On Tue, Nov 23 2021, Elijah Newren wrote:

On Tue, Nov 23, 2021 at 5:19 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

Doesn't this series also change the behavior of e.g.:

    cd contrib/subtree
    git rm -r ../subtree

Yes, of course!

Before:

    $ cd contrib/subtree
    $ git rm -r -q ../subtree/
    $ ls -ld ../subtree/
    ls: cannot access '../subtree/': No such file or directory
    $ git status --porcelain
    fatal: Unable to read current working directory: No such file or directory
    $ git checkout HEAD ../subtree/
    fatal: Unable to read current working directory: No such file or directory

After:

    $ cd contrib/subtree
    $ git rm -r -q ../subtree/
    $ ls -ld ../subtree/
    drwxrwxr-x. 1 newren newren 0 Nov 23 19:18 ../subtree/
    $ git status --porcelain
    D  contrib/subtree/.gitignore
    D  contrib/subtree/COPYING
    D  contrib/subtree/INSTALL
    D  contrib/subtree/Makefile
    D  contrib/subtree/README
    D  contrib/subtree/git-subtree.sh
    D  contrib/subtree/git-subtree.txt
    D  contrib/subtree/t/Makefile
    D  contrib/subtree/t/t7900-subtree.sh
    D  contrib/subtree/todo
    $ git checkout HEAD ../subtree/
    Updated 10 paths from c557be478e

Very nice fix, don't you think?

I'd be more sympathetic to this for the "checkout" etc. commands, but
once I add a "-f" to that "rm" I'm *really* expecting it to rm the
directory, but it won't anymore because it's in the underlying library
function.

But if the goal is to get "git status" and the like working isn't a much
more pointed fix to have setup.c handle the case of getting ENOENT from
getcwd() more gracefully. I.e. currently (and even with your patches):

    $ (mkdir blah && cd blah && rmdir ../blah && git status)
    fatal: Unable to read current working directory: No such file or directory

Whereas if we do e.g.:
	
	diff --git a/strbuf.c b/strbuf.c
	index b22e9816559..3f9a957ff9d 100644
	--- a/strbuf.c
	+++ b/strbuf.c
	@@ -600,6 +600,16 @@ int strbuf_getcwd(struct strbuf *sb)
	                        return 0;
	                }
	
	+               if (errno == ENOENT){
	+                       const char *pwd = getenv("PWD");
	+
	+                       if (pwd) {
	+                               warning(_("unable to getcwd(), but can read PWD, limping along with that..."));
	+                               strbuf_addstr(sb, pwd);
	+                               return 0;
	+                       }
	+               }
	+
	                /*
	                 * If getcwd(3) is implemented as a syscall that falls
	                 * back to a regular lookup using readdir(3) etc. then

We'll get:
	
	$ (mkdir blah && cd blah && rmdir ../blah && GIT_DISCOVERY_ACROSS_FILESYSTEM=1 ~/g/git/git status)
	warning: unable to getcwd(), but can read PWD, limping along with that...
	On branch master
	Your branch is up to date with 'origin/master'.
	
	Changes not staged for commit:
	  (use "git add <file>..." to update what will be committed)
	  (use "git restore <file>..." to discard changes in working directory)
	        modified:   ../strbuf.c
	
	no changes added to commit (use "git add" and/or "git commit -a")

I think that getenv("PWD") trick is widely supported, and once we get
past that we seem OK. The relative path to strbuf.c is even correct.

Currently you'd need to set GIT_DISCOVERY_ACROSS_FILESYSTEM=1 because we
run into another case in setup.c where we're not carrying that ENOENT
forward, but we could just patch strbuf_getcwd() or that subsequent code
to handle this edge case.

quoted

If so then re the "spidey sense" comment I had earlier: There's no rm
codepaths or tests changed by this series,

That's not correct; I explicitly added a new rm test in the first
patch in my series.  Further, that same test was modified to mark it
as passing by this particular patch you are commenting on.

Sorry about that, I didn't look carefully enough.

quoted

so the implementation of
doing it at this lower level might be casting too wide a net.

I'm getting the vibe that you are assuming I'm changing these two
functions without realizing what places might be calling them;
basically, that I'm just flippantly changing them.  Ignoring the
ramifications of such an assumption, if this vibe is correct[...]

Sorry no, I didn't mean to imply that. I snipped the rest, but hopefully
this answers the questions you had well enough (and in the time I have
for this reply):

I'm not concerned that you didn't research this change well enough, I
just find it a bit iffy to introduce semantics in git around FS
operations that don't conform with that of POSIX & the underlying OS. My
*nix system happily accepts an "rm -rf" or an "rmdir" of the directory
I'm in, I'd expect git to do the same.

But whatever research we do on in-tree users we're left with changing
behavior for users in the wild, e.g. a script that cd's to each subdir
in a repo, inspects something, and if it wants to remove that does an
"git rm -r" of the directory it's in, commits, and expects the repo to
be clean afterwards.

I did follow/read at least one of the the original discussions[1] a bit,
and some earlier discussion I'm vaguely recalling around bisect in a
subdir.

If the underlying goal is to address the UX problem in git of e.g. "git
status" and the like hard-dying I wonder if something in the direction
of the setup.c/strbuf.c change above might be more of a gentle change.

That approach of a more gentler setup.c also has the benefit of having
git work when it ends up in this situation without the git commands
having landed it there, as in the above "rmdir" example.

Anyway, I really didn't have time to look at this very carefully. I just
remember looking into this with bisect/status etc. in the past, and
thinking that these problems were solvable in those cases, i.e. they
were just being overly anal about ENOENT, and not falling back on "PWD"
etc.

1. https://lore.kernel.org/git/YS3Tv7UfNkF+adry@coredump.intra.peff.net/

On 24/11/2021 11:14, Ævar Arnfjörð Bjarmason wrote:

I'm not concerned that you didn't research this change well enough, I
just find it a bit iffy to introduce semantics in git around FS
operations that don't conform with that of POSIX & the underlying OS. My
*nix system happily accepts an "rm -rf" or an "rmdir" of the directory
I'm in, I'd expect git to do the same.

Isn't this the same, conceptually, as trying to remove the root
directory, but from a Git perspective?

i.e. Something along the lines of
https://superuser.com/questions/542978/is-it-possible-to-remove-the-root-directory
(their answer is 'no' without a special option, default since 2006)

If I read the arguments correctly, Elijah is saying that Git shouldn't
delete it's own root (cwd) directory, and that it is already implicit
within the current Git code.

Philip

On Wed, Nov 24 2021, Ævar Arnfjörð Bjarmason wrote:

On Tue, Nov 23 2021, Elijah Newren wrote:

quoted

On Tue, Nov 23, 2021 at 5:19 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

quoted

Doesn't this series also change the behavior of e.g.:

    cd contrib/subtree
    git rm -r ../subtree

Yes, of course!

Before:

    $ cd contrib/subtree
    $ git rm -r -q ../subtree/
    $ ls -ld ../subtree/
    ls: cannot access '../subtree/': No such file or directory
    $ git status --porcelain
    fatal: Unable to read current working directory: No such file or directory
    $ git checkout HEAD ../subtree/
    fatal: Unable to read current working directory: No such file or directory

After:

    $ cd contrib/subtree
    $ git rm -r -q ../subtree/
    $ ls -ld ../subtree/
    drwxrwxr-x. 1 newren newren 0 Nov 23 19:18 ../subtree/
    $ git status --porcelain
    D  contrib/subtree/.gitignore
    D  contrib/subtree/COPYING
    D  contrib/subtree/INSTALL
    D  contrib/subtree/Makefile
    D  contrib/subtree/README
    D  contrib/subtree/git-subtree.sh
    D  contrib/subtree/git-subtree.txt
    D  contrib/subtree/t/Makefile
    D  contrib/subtree/t/t7900-subtree.sh
    D  contrib/subtree/todo
    $ git checkout HEAD ../subtree/
    Updated 10 paths from c557be478e

Very nice fix, don't you think?

I'd be more sympathetic to this for the "checkout" etc. commands, but
once I add a "-f" to that "rm" I'm *really* expecting it to rm the
directory, but it won't anymore because it's in the underlying library
function.

But if the goal is to get "git status" and the like working isn't a much
more pointed fix to have setup.c handle the case of getting ENOENT from
getcwd() more gracefully. I.e. currently (and even with your patches):

    $ (mkdir blah && cd blah && rmdir ../blah && git status)
    fatal: Unable to read current working directory: No such file or directory

Whereas if we do e.g.:
	
	diff --git a/strbuf.c b/strbuf.c
	index b22e9816559..3f9a957ff9d 100644
	--- a/strbuf.c
	+++ b/strbuf.c
	@@ -600,6 +600,16 @@ int strbuf_getcwd(struct strbuf *sb)
	                        return 0;
	                }
	
	+               if (errno == ENOENT){
	+                       const char *pwd = getenv("PWD");
	+
	+                       if (pwd) {
	+                               warning(_("unable to getcwd(), but can read PWD, limping along with that..."));
	+                               strbuf_addstr(sb, pwd);
	+                               return 0;
	+                       }
	+               }
	+
	                /*
	                 * If getcwd(3) is implemented as a syscall that falls
	                 * back to a regular lookup using readdir(3) etc. then

We'll get:
	
	$ (mkdir blah && cd blah && rmdir ../blah && GIT_DISCOVERY_ACROSS_FILESYSTEM=1 ~/g/git/git status)
	warning: unable to getcwd(), but can read PWD, limping along with that...
	On branch master
	Your branch is up to date with 'origin/master'.
	
	Changes not staged for commit:
	  (use "git add <file>..." to update what will be committed)
	  (use "git restore <file>..." to discard changes in working directory)
	        modified:   ../strbuf.c
	
	no changes added to commit (use "git add" and/or "git commit -a")

I think that getenv("PWD") trick is widely supported, and once we get
past that we seem OK. The relative path to strbuf.c is even correct.

Currently you'd need to set GIT_DISCOVERY_ACROSS_FILESYSTEM=1 because we
run into another case in setup.c where we're not carrying that ENOENT
forward, but we could just patch strbuf_getcwd() or that subsequent code
to handle this edge case.

quoted

quoted

If so then re the "spidey sense" comment I had earlier: There's no rm
codepaths or tests changed by this series,

That's not correct; I explicitly added a new rm test in the first
patch in my series.  Further, that same test was modified to mark it
as passing by this particular patch you are commenting on.

Sorry about that, I didn't look carefully enough.

quoted

quoted

so the implementation of
doing it at this lower level might be casting too wide a net.

I'm getting the vibe that you are assuming I'm changing these two
functions without realizing what places might be calling them;
basically, that I'm just flippantly changing them.  Ignoring the
ramifications of such an assumption, if this vibe is correct[...]

Sorry no, I didn't mean to imply that. I snipped the rest, but hopefully
this answers the questions you had well enough (and in the time I have
for this reply):

I'm not concerned that you didn't research this change well enough, I
just find it a bit iffy to introduce semantics in git around FS
operations that don't conform with that of POSIX & the underlying OS. My
*nix system happily accepts an "rm -rf" or an "rmdir" of the directory
I'm in, I'd expect git to do the same.

But whatever research we do on in-tree users we're left with changing
behavior for users in the wild, e.g. a script that cd's to each subdir
in a repo, inspects something, and if it wants to remove that does an
"git rm -r" of the directory it's in, commits, and expects the repo to
be clean afterwards.

I did follow/read at least one of the the original discussions[1] a bit,
and some earlier discussion I'm vaguely recalling around bisect in a
subdir.

If the underlying goal is to address the UX problem in git of e.g. "git
status" and the like hard-dying I wonder if something in the direction
of the setup.c/strbuf.c change above might be more of a gentle change.

That approach of a more gentler setup.c also has the benefit of having
git work when it ends up in this situation without the git commands
having landed it there, as in the above "rmdir" example.

Anyway, I really didn't have time to look at this very carefully. I just
remember looking into this with bisect/status etc. in the past, and
thinking that these problems were solvable in those cases, i.e. they
were just being overly anal about ENOENT, and not falling back on "PWD"
etc.

1. https://lore.kernel.org/git/YS3Tv7UfNkF+adry@coredump.intra.peff.net/

I fleshened this out a bit in this WIP change:
https://github.com/avar/git/tree/avar/setup-handle-gone-directory +
commit:
https://github.com/avar/git/commit/97968518909eef88edba44973b7885d154b7a273

As noted there there's some caveats, but so far nothing I spotted that
can't be overcome. It's particularly painful to test it because of an
implementation detail of our test suite, the bin-wrappers are
shellscripts, and the very first thing they do is reset $PWD (none of
which happens if you run the real "git" binary).

That's b.t.w. the "shell-init" error you noted in
https://lore.kernel.org/git/CABPp-BEp3OL7F2J_LzqtC-x-8pBUPO8ZR1fTx_6XbqZeOH1kRw@mail.gmail.com/,
it's from the bin-wrapper.

I really wish we didn't have the bin-wrappers...

On Wed, Nov 24, 2021 at 3:33 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Tue, Nov 23 2021, Elijah Newren wrote:

quoted

On Tue, Nov 23, 2021 at 5:19 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

quoted

Doesn't this series also change the behavior of e.g.:

    cd contrib/subtree
    git rm -r ../subtree

Yes, of course!

Before:

    $ cd contrib/subtree
    $ git rm -r -q ../subtree/
    $ ls -ld ../subtree/
    ls: cannot access '../subtree/': No such file or directory
    $ git status --porcelain
    fatal: Unable to read current working directory: No such file or directory
    $ git checkout HEAD ../subtree/
    fatal: Unable to read current working directory: No such file or directory

After:

    $ cd contrib/subtree
    $ git rm -r -q ../subtree/
    $ ls -ld ../subtree/
    drwxrwxr-x. 1 newren newren 0 Nov 23 19:18 ../subtree/
    $ git status --porcelain
    D  contrib/subtree/.gitignore
    D  contrib/subtree/COPYING
    D  contrib/subtree/INSTALL
    D  contrib/subtree/Makefile
    D  contrib/subtree/README
    D  contrib/subtree/git-subtree.sh
    D  contrib/subtree/git-subtree.txt
    D  contrib/subtree/t/Makefile
    D  contrib/subtree/t/t7900-subtree.sh
    D  contrib/subtree/todo
    $ git checkout HEAD ../subtree/
    Updated 10 paths from c557be478e

Very nice fix, don't you think?

I'd be more sympathetic to this for the "checkout" etc. commands, but
once I add a "-f" to that "rm" I'm *really* expecting it to rm the
directory, but it won't anymore because it's in the underlying library
function.

Please note that the command `git rm -rf dir` does not mean remove
`dir`.  It means forcibly and recursively remove all tracked paths
matching the pathspec `dir`.  There is certainly a difference, because
while it sometimes might end up deleting `dir`, it also might only
delete a subset of dir, and in some cases -- due to our added
conveniences -- it may also delete things outside of dir/.  It's been
that way for a decade and a half.  As such, the complaint that "`git
rm -rf dir` sometimes doesn't rm the directory" is not something new
being introduced by this series; that was already a possibility.  Yes,
I'm adding another case where that happens, but it's not some entirely
new qualitative difference.  More on this below.

But if the goal is to get "git status" and the like working isn't a much
more pointed fix to have setup.c handle the case of getting ENOENT from
getcwd() more gracefully. I.e. currently (and even with your patches):

    $ (mkdir blah && cd blah && rmdir ../blah && git status)
    fatal: Unable to read current working directory: No such file or directory

Whereas if we do e.g.:

        diff --git a/strbuf.c b/strbuf.c
        index b22e9816559..3f9a957ff9d 100644
        --- a/strbuf.c
        +++ b/strbuf.c
        @@ -600,6 +600,16 @@ int strbuf_getcwd(struct strbuf *sb)
                                return 0;
                        }

        +               if (errno == ENOENT){
        +                       const char *pwd = getenv("PWD");
        +
        +                       if (pwd) {
        +                               warning(_("unable to getcwd(), but can read PWD, limping along with that..."));
        +                               strbuf_addstr(sb, pwd);
        +                               return 0;
        +                       }
        +               }
        +
                        /*
                         * If getcwd(3) is implemented as a syscall that falls
                         * back to a regular lookup using readdir(3) etc. then

We'll get:

        $ (mkdir blah && cd blah && rmdir ../blah && GIT_DISCOVERY_ACROSS_FILESYSTEM=1 ~/g/git/git status)
        warning: unable to getcwd(), but can read PWD, limping along with that...
        On branch master
        Your branch is up to date with 'origin/master'.

        Changes not staged for commit:
          (use "git add <file>..." to update what will be committed)
          (use "git restore <file>..." to discard changes in working directory)
                modified:   ../strbuf.c

        no changes added to commit (use "git add" and/or "git commit -a")

I think that getenv("PWD") trick is widely supported, and once we get
past that we seem OK. The relative path to strbuf.c is even correct.

Currently you'd need to set GIT_DISCOVERY_ACROSS_FILESYSTEM=1 because we
run into another case in setup.c where we're not carrying that ENOENT
forward, but we could just patch strbuf_getcwd() or that subsequent code
to handle this edge case.

Ooh, nifty.  This sounds like a useful complement to this series; I'd
be in favor of it.

...

I'm not concerned that you didn't research this change well enough, I
just find it a bit iffy to introduce semantics in git around FS
operations that don't conform with that of POSIX & the underlying OS. My
*nix system happily accepts an "rm -rf" or an "rmdir" of the directory
I'm in, I'd expect git to do the same.

This seems like an odd objection.  Semantics in git around FS
operations that don't conform with that of POSIX and the underlying OS
have been with us for a decade and a half now:

For example, if we ran the following:
  mkdir -p a/b/c
  touch a/b/c/{tracked,untracked}
  git commit -m "whatever" a/b/c/tracked
  cd a/b

Then we'd see the following:

`rm -rf ../b`: leaves a/ as an empty directory
`git rm -rf ../b`: does not nuke a/b/; in fact, it leaves
a/b/c/untracked around afterward

Also, if left a/b/c/untracked out of our setup, then we'd instead see
the following:

`rm -rf ../b`: leaves a/ as an empty directory
`git rm -rf ../b`: not only deletes a/b, but also deletes a/ -- the
parent of what was specified

Also, prior to d9b814cc97f1 (Add builtin "git rm" command,
2006-05-19), git rm would never delete the directories, so I guess you
can take our non-conforming behavior back even further than a decade
and a half.

But whatever research we do on in-tree users we're left with changing
behavior for users in the wild, e.g. a script that cd's to each subdir
in a repo, inspects something, and if it wants to remove that does an
"git rm -r" of the directory it's in, commits, and expects the repo to
be clean afterwards.

Yes, this series is about changing behavior.  It's not an
optimization, or a code cleanup, or new command or anything.  It's
very specifically about git's behavior of deleting leading empty
directories after removing other files being limited to when those
empty directories are not the original current working directory of
the process.

However, although I still didn't know of any concrete or even
theoretical examples of problems this series would cause, I decided
after your email to do a bit more work; perhaps some of it will help:

  * I split this final patch in two, one for each affected function,
with much more detailed history and writeup.
  * remove_dir_recursively() already takes a flags parameter; I added
one for specifying whether the caller wants the traditional behavior
or this new behavior.
  * I revisited each and every caller of remove_path() and
remove_dir_recursively() and re-evaluated each one in case I had
missed something.  Of the 32 callers, most are unaffected by the
change.  Of those that are affected, most should take it, but I did
end up finding one that sometimes, in a very specific corner case,
should get the old behavior (namely when a submodule is present and
cwd is within the submodule and you pass all of --git-dir and
--work-tree and -f to rm when removing the submodule).  I added some
testcases and ensured it worked.

Since the flag is used, it's not at risk of being deleted, and thus
library callers can pick their preferred behavior.  Does that address,
or at least ameliorate, your concerns?

I'll submit the new series soon.

I did follow/read at least one of the the original discussions[1] a bit,
and some earlier discussion I'm vaguely recalling around bisect in a
subdir.

If the underlying goal is to address the UX problem in git of e.g. "git
status" and the like hard-dying I wonder if something in the direction
of the setup.c/strbuf.c change above might be more of a gentle change.

That approach of a more gentler setup.c also has the benefit of having
git work when it ends up in this situation without the git commands
having landed it there, as in the above "rmdir" example.

Anyway, I really didn't have time to look at this very carefully. I just
remember looking into this with bisect/status etc. in the past, and
thinking that these problems were solvable in those cases, i.e. they
were just being overly anal about ENOENT, and not falling back on "PWD"
etc.

The fact that git aborts in such directories is one obvious
manifestation of the problem, but my commit messages and cover letter
both called out that it can happen with non-git commands too.  I
certainly think your suggestion is very intriguing and worth pursuing,
but I think it complements rather than supplants this series.

1. https://lore.kernel.org/git/YS3Tv7UfNkF+adry@coredump.intra.peff.net/

On Wed, Nov 24, 2021 at 6:18 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

I fleshened this out a bit in this WIP change:
https://github.com/avar/git/tree/avar/setup-handle-gone-directory +
commit:
https://github.com/avar/git/commit/97968518909eef88edba44973b7885d154b7a273

Sweet!

As noted there there's some caveats, but so far nothing I spotted that
can't be overcome. It's particularly painful to test it because of an
implementation detail of our test suite, the bin-wrappers are
shellscripts, and the very first thing they do is reset $PWD (none of
which happens if you run the real "git" binary).

That's b.t.w. the "shell-init" error you noted in
https://lore.kernel.org/git/CABPp-BEp3OL7F2J_LzqtC-x-8pBUPO8ZR1fTx_6XbqZeOH1kRw@mail.gmail.com/,
it's from the bin-wrapper.

To be a bit more precise, the error/warning is from bash rather than
the bin-wrappers specifically; for example, you can also trigger the
same shell-init message with a simple `bash -c 'echo hello'` when your
cwd has been deleted.

I really wish we didn't have the bin-wrappers...

I'm curious: is this issue the only reason, or am I reading between
the lines correctly that you have additional reasons for that?

From: Elijah Newren <redacted>

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 <redacted>
---
 t/t2501-cwd-empty.sh | 255 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 255 insertions(+)
 create mode 100755 t/t2501-cwd-empty.sh

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
new file mode 100755
index 00000000000..5dfb456a691
--- /dev/null
+++ b/t/t2501-cwd-empty.sh

@@ -0,0 +1,255 @@
+#!/bin/sh
+
+test_description='Test handling of the current working directory becoming empty'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_commit init &&
+	mkdir subdir &&
+	test_commit subdir/file &&
+
+	git branch fd_conflict &&
+
+	mkdir -p foo/bar &&
+	test_commit foo/bar/baz &&
+
+	git revert HEAD &&
+	git tag reverted &&
+
+	git checkout fd_conflict &&
+	git rm subdir/file.t &&
+	echo not-a-directory >dirORfile &&
+	git add dirORfile &&
+	git commit -m dirORfile
+'
+
+test_expect_failure 'checkout does not clean cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_path_is_dir foo/bar &&
+
+	(
+		cd foo &&
+		git checkout init &&
+		cd ..
+	) &&
+	test_path_is_missing foo/bar/baz &&
+	test_path_is_missing foo/bar &&
+	test_path_is_dir foo
+'
+
+test_expect_failure 'checkout fails 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 checkout fd_conflict 2>../error &&
+		grep "Refusing to remove the current working directory" ../error
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'reset --hard does not clean cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_path_is_dir foo/bar &&
+
+	(
+		cd foo &&
+		git reset --hard init &&
+		cd ..
+	) &&
+	test_path_is_missing foo/bar/baz &&
+	test_path_is_missing foo/bar &&
+	test_path_is_dir foo
+'
+
+test_expect_failure 'reset --hard fails 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 reset --hard fd_conflict 2>../error &&
+		grep "Refusing to remove.*the current working directory" ../error
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'merge does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	(
+		cd subdir &&
+		git merge fd_conflict
+	) &&
+
+	test_path_is_missing subdir/file.t &&
+	test_path_is_dir subdir
+'
+
+test_expect_failure 'merge fails 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
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'cherry-pick does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	(
+		cd subdir &&
+		git cherry-pick fd_conflict
+	) &&
+
+	test_path_is_missing subdir/file.t &&
+	test_path_is_dir subdir
+'
+
+test_expect_failure 'cherry-pick fails 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
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'rebase does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	(
+		cd subdir &&
+		git rebase foo/bar/baz fd_conflict
+	) &&
+
+	test_path_is_missing subdir/file.t &&
+	test_path_is_dir subdir
+'
+
+test_expect_failure 'rebase fails 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
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'revert does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	(
+		cd subdir &&
+		git revert subdir/file
+	) &&
+
+	test_path_is_missing subdir/file.t &&
+	test_path_is_dir subdir
+'
+
+test_expect_failure 'revert fails if cwd needs to be removed' '
+	git checkout fd_conflict &&
+	git revert HEAD &&
+	test_when_finished "git clean -fdx" &&
+
+	mkdir dirORfile &&
+	(
+		cd dirORfile &&
+		test_must_fail git revert HEAD 2>../error &&
+		grep "Refusing to remove the current working directory" ../error
+	) &&
+
+	test_path_is_dir dirORfile
+'
+
+test_expect_failure 'rm does not remove cwd incidentally' '
+	test_when_finished "git reset --hard" &&
+	git checkout foo/bar/baz &&
+
+	(
+		cd foo &&
+		git rm bar/baz.t
+	) &&
+
+	test_path_is_missing foo/bar/baz &&
+	test_path_is_missing foo/bar &&
+	test_path_is_dir foo
+'
+
+test_expect_failure 'apply does not remove cwd incidentally' '
+	test_when_finished "git reset --hard" &&
+	git checkout foo/bar/baz &&
+
+	(
+		cd subdir &&
+		git diff subdir/file init | git apply
+	) &&
+
+	test_path_is_missing subdir/file.t &&
+	test_path_is_dir subdir
+'
+
+test_expect_failure 'clean does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	mkdir empty &&
+	mkdir untracked &&
+	>untracked/random &&
+	(
+		cd untracked &&
+		git clean -fd -e warnings :/ >../warnings &&
+		grep "Refusing to remove current working directory" ../warnings
+	) &&
+
+	test_path_is_missing empty &&
+	test_path_is_missing untracked/random &&
+	test_path_is_dir untracked
+'
+
+test_expect_failure 'stash does not remove cwd incidentally' '
+	git checkout foo/bar/baz &&
+	test_when_finished "git clean -fdx" &&
+
+	mkdir untracked &&
+	>untracked/random &&
+	(
+		cd untracked &&
+		git stash --include-untracked &&
+		git status
+	) &&
+
+	test_path_is_missing untracked/random &&
+	test_path_is_dir untracked
+'
+
+test_done

-- 
gitgitgadget

Traditionally, if folks run git commands such as checkout or rebase from a
subdirectory, that git command could remove their current working directory
and result in subsequent git and non-git commands either getting confused or
printing messages that confuse the user (e.g. "fatal: Unable to read current
working directory: No such file or directory"). Many commands either
silently avoid removing directories that are not empty (i.e. those that have
untracked or modified files in them)[1], or show an error and abort,
depending on which is more appropriate for the command in question. With
this series, we augment the reasons to avoid removing directories to include
not just has-untracked-or-modified-files, but also to avoid removing the
original_cwd as well.

Peff and Junio provided some good pros/cons, if it helps:

 * Pros: Peff (original suggester of the idea)[2], and Junio[3]
 * Cons: Peff [2, again -- see the "P.S."], and Junio[4]

[1] well, with a few exceptions; see
https://lore.kernel.org/git/pull.1036.v3.git.1632760428.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/YS8eEtwQvF7TaLCb@coredump.intra.peff.net/
[3] https://lore.kernel.org/git/xmqqo86elyht.fsf@gitster.g/ [4]
https://lore.kernel.org/git/xmqqo8691gr8.fsf@gitster.g/

Changes since v1:

 * clarified multiple commit messages
 * renamed the_cwd to startup_info->original_cwd to make it clearer that
   it's our parent process'es cwd that really matters, which we inherited at
   program startup. Also pulls it out of the global namespace.
 * Normalize the path for startup_info->original_cwd, and ensure that it's
   actually the original cwd even if -C is passed to git.
 * small code cleanups suggested by René and Ævar
 * split the final patch, which got the most comments into two, one for each
   function being modified; significantly extending the first of the two
   commit messages with a lot of history
 * no longer has a content conflict with so/stash-staged
 * add another value for the flags parameter that remove_dir_recursively()
   takes so that it can opt into either the old or the new behavior. Use
   that for the one special corner case I could find where it matters, and
   add a few tests around it to highlight the utility of the flag.

Elijah Newren (9):
  t2501: add various tests for removing the current working directory
  setup: introduce startup_info->original_cwd
  unpack-trees: refuse to remove startup_info->original_cwd
  unpack-trees: add special cwd handling
  symlinks: do not include startup_info->original_cwd in dir removal
  clean: do not attempt to remove startup_info->original_cwd
  stash: do not attempt to remove startup_info->original_cwd
  dir: avoid incidentally removing the original_cwd in remove_path()
  dir: new flag to remove_dir_recurse() to spare the original_cwd

 builtin/clean.c      |  47 +++++--
 builtin/rm.c         |   3 +-
 builtin/stash.c      |   6 +-
 cache.h              |   1 +
 dir.c                |  15 ++-
 dir.h                |   9 +-
 git.c                |   2 +
 setup.c              |  49 +++++++
 symlinks.c           |   8 +-
 t/t2501-cwd-empty.sh | 297 +++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.c       |  30 ++++-
 unpack-trees.h       |   1 +
 12 files changed, 446 insertions(+), 22 deletions(-)
 create mode 100755 t/t2501-cwd-empty.sh


base-commit: 88d915a634b449147855041d44875322de2b286d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1140%2Fnewren%2Fcwd_removal-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1140/newren/cwd_removal-v2
Pull-Request: https://github.com/git/git/pull/1140

Range-diff vs v1:

  1:  0b71996a3b4 !  1:  38a120f5c03 t2501: add various tests for removing the current working directory
     @@ Commit message
          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.  Add some tests checking for such
     -    problems.
     +    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 [off-list ref]
      
  2:  7b0c665fb75 <  -:  ----------- repository, setup: introduce the_cwd
  -:  ----------- >  2:  f6129a8ac9c setup: introduce startup_info->original_cwd
  3:  94bf468be09 !  3:  e74975e83cc unpack-trees: refuse to remove the current working directory
     @@ Metadata
      Author: Elijah Newren [off-list ref]
      
       ## Commit message ##
     -    unpack-trees: refuse to remove the current working directory
     +    unpack-trees: refuse to remove startup_info->original_cwd
      
          In the past, when a directory needs to be removed to make room for a
          file, we have always errored out when that directory contains any
          untracked (but not ignored) files.  Add an extra condition on that: also
     -    error out if the directory is the current working directory.
     +    error out if the directory is the current working directory we inherited
     +    from our parent process.
      
          Signed-off-by: Elijah Newren [off-list ref]
      
     @@ unpack-trees.c: static int verify_clean_subdirectory(const struct cache_entry *c
       	if (i)
       		return add_rejected_path(o, ERROR_NOT_UPTODATE_DIR, ce->name);
      +
     -+	/* Do not lose the current working directory. */
     -+	if (the_cwd && !strcmp(the_cwd, ce->name))
     ++	/* Do not lose startup_info->original_cwd */
     ++	if (startup_info->original_cwd &&
     ++	    !strcmp(startup_info->original_cwd, ce->name))
      +		return add_rejected_path(o, ERROR_CWD_IN_THE_WAY, ce->name);
      +
       	return cnt;
  4:  39830fffd45 !  4:  e06806e3a32 unpack-trees: add special cwd handling
     @@ Commit message
          code from the previous commit in verify_clean_subdirectory().  However,
          when we are preserving untracked files, we would rather any error
          messages about untracked files being in the way take precedence over
     -    error messages about a subdirectory that happens to be the current
     -    working directory being in the way.  But in the
     -    UNPACK_RESET_OVERWRITE_UNTRACKED case, there is no untracked checking to
     -    be done, so we simply add a special case near the top of
     -    verify_absent_1.
     +    error messages about a subdirectory that happens to be the_original_cwd
     +    being in the way.  But in the UNPACK_RESET_OVERWRITE_UNTRACKED case,
     +    there is no untracked checking to be done, so we simply add a special
     +    case near the top of verify_absent_1.
      
          Signed-off-by: Elijah Newren [off-list ref]
      
     @@ unpack-trees.c: static int verify_absent_1(const struct cache_entry *ce,
       		return 0;
       
      +	if (o->reset == UNPACK_RESET_OVERWRITE_UNTRACKED) {
     -+		/* Avoid nuking cwd... */
     -+		if (the_cwd && !strcmp(the_cwd, ce->name))
     ++		/* Avoid nuking startup_info->original_cwd... */
     ++		if (startup_info->original_cwd &&
     ++		    !strcmp(startup_info->original_cwd, ce->name))
      +			return add_rejected_path(o, ERROR_CWD_IN_THE_WAY,
      +						 ce->name);
      +		/* ...but nuke anything else. */
  5:  8a69d2878c9 !  5:  46728f74ea1 symlinks: do not include current working directory in dir removal
     @@ Metadata
      Author: Elijah Newren [off-list ref]
      
       ## Commit message ##
     -    symlinks: do not include current working directory in dir removal
     +    symlinks: do not include startup_info->original_cwd in dir removal
      
          symlinks has a pair of schedule_dir_for_removal() and
          remove_scheduled_dirs() functions that ensure that directories made
          empty by removing other files also themselves get removed.  However, we
     -    want to exclude the current working directory and leave it around so
     -    that subsequent git commands (and non-git commands) that the user runs
     -    afterwards don't cause the user to get confused.
     +    want to exclude startup_info->original_cwd and leave it around.  This
     +    avoids the user getting confused by subsequent git commands (and non-git
     +    commands) that would otherwise report confusing messages about being
     +    unable to read the current working directory.
      
          Signed-off-by: Elijah Newren [off-list ref]
      
       ## symlinks.c ##
     -@@ symlinks.c: static int threaded_has_dirs_only_path(struct cache_def *cache, const char *name
     - 
     - static struct strbuf removal = STRBUF_INIT;
     - 
     -+static int cant_remove(char *dirname)
     -+{
     -+	if (the_cwd && !strcmp(dirname, the_cwd))
     -+		return 1;
     -+	return rmdir(dirname);
     -+}
     -+
     - static void do_remove_scheduled_dirs(int new_len)
     +@@ symlinks.c: static void do_remove_scheduled_dirs(int new_len)
       {
       	while (removal.len > new_len) {
       		removal.buf[removal.len] = '\0';
      -		if (rmdir(removal.buf))
     -+		if (cant_remove(removal.buf))
     ++		if ((startup_info->original_cwd &&
     ++		     !strcmp(removal.buf, startup_info->original_cwd)) ||
     ++		    rmdir(removal.buf))
       			break;
       		do {
       			removal.len--;
     @@ symlinks.c: void schedule_dir_for_removal(const char *name, int len)
       {
       	int match_len, last_slash, i, previous_slash;
       
     -+	if (the_cwd && !strcmp(name, the_cwd))
     ++	if (startup_info->original_cwd &&
     ++	    !strcmp(name, startup_info->original_cwd))
      +		return;	/* Do not remove the current working directory */
      +
       	match_len = last_slash = i =
  6:  69bcaf0aab2 !  6:  01ce9444dae clean: do not attempt to remove current working directory
     @@ Metadata
      Author: Elijah Newren [off-list ref]
      
       ## Commit message ##
     -    clean: do not attempt to remove current working directory
     +    clean: do not attempt to remove startup_info->original_cwd
      
          Signed-off-by: Elijah Newren [off-list ref]
      
     @@ builtin/clean.c: static const char *msg_skip_git_dir = N_("Skipping repository %
       
       enum color_clean {
       	CLEAN_COLOR_RESET = 0,
     +@@ builtin/clean.c: static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
     + {
     + 	DIR *dir;
     + 	struct strbuf quoted = STRBUF_INIT;
     ++	struct strbuf realpath = STRBUF_INIT;
     ++	struct strbuf real_ocwd = STRBUF_INIT;
     + 	struct dirent *e;
     + 	int res = 0, ret = 0, gone = 1, original_len = path->len, len;
     + 	struct string_list dels = STRING_LIST_INIT_DUP;
      @@ builtin/clean.c: static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
       	strbuf_setlen(path, original_len);
       
     @@ builtin/clean.c: static int remove_dirs(struct strbuf *path, const char *prefix,
      -			quote_path(path->buf, prefix, &quoted, 0);
      -			errno = saved_errno;
      -			warning_errno(_(msg_warn_remove_failed), quoted.buf);
     -+		int prefixlen = prefix ? strlen(prefix) : 0;
     -+		if (prefix &&
     -+		    path->len == prefixlen + 2 &&
     -+		    !strncmp(path->buf, prefix, prefixlen) &&
     -+		    !strcmp(path->buf + prefixlen, "./")) {
     ++		/*
     ++		 * Normalize path components in path->buf, e.g. change '\' to
     ++		 * '/' on Windows.
     ++		 */
     ++		strbuf_realpath(&realpath, path->buf, 1);
     ++
     ++		/*
     ++		 * path and realpath are absolute; for comparison, we want
     ++		 * startup_info->original_cwd to be an absolute path too.  We
     ++		 * can use strbuf_realpath for this.  Also, if original_cwd
     ++		 * started out as the empty string, then it corresponded to
     ++		 * the top of the worktree, which is protected by other means
     ++		 * so we just leave it blank.
     ++		 */
     ++		 if (*startup_info->original_cwd)
     ++			 strbuf_realpath(&real_ocwd,
     ++					 startup_info->original_cwd, 1);
     ++
     ++		if (!strbuf_cmp(&realpath, &real_ocwd)) {
      +			printf("%s", dry_run ? _(msg_would_skip_cwd) : _(msg_skip_cwd));
       			*dir_gone = 0;
      -			ret = 1;
     @@ builtin/clean.c: static int remove_dirs(struct strbuf *path, const char *prefix,
       		}
       	}
       
     +@@ builtin/clean.c: static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
     + 			printf(dry_run ?  _(msg_would_remove) : _(msg_remove), dels.items[i].string);
     + 	}
     + out:
     ++	strbuf_release(&realpath);
     ++	strbuf_release(&real_ocwd);
     + 	strbuf_release(&quoted);
     + 	string_list_clear(&dels, 0);
     + 	return ret;
      
       ## t/t2501-cwd-empty.sh ##
      @@ t/t2501-cwd-empty.sh: test_expect_failure 'apply does not remove cwd incidentally' '
  7:  93005b17c28 !  7:  edec0894ca2 stash: do not attempt to remove current working directory
     @@ Metadata
      Author: Elijah Newren [off-list ref]
      
       ## Commit message ##
     -    stash: do not attempt to remove current working directory
     +    stash: do not attempt to remove startup_info->original_cwd
      
          Signed-off-by: Elijah Newren [off-list ref]
      
       ## builtin/stash.c ##
     -@@ builtin/stash.c: static int create_stash(int argc, const char **argv, const char *prefix)
     - 	return ret;
     - }
     - 
     --static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int quiet,
     -+static int do_push_stash(const struct pathspec *ps, const char *stash_msg,
     -+			 const char *prefix, int quiet,
     - 			 int keep_index, int patch_mode, int include_untracked)
     - {
     - 	int ret = 0;
      @@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
       			struct child_process cp = CHILD_PROCESS_INIT;
       
       			cp.git_cmd = 1;
     -+			if (prefix)
     -+				strvec_pushl(&cp.args, "-C", prefix, NULL);
     ++			if (startup_info->original_cwd &&
     ++			    *startup_info->original_cwd &&
     ++			    !is_absolute_path(startup_info->original_cwd))
     ++				cp.dir = startup_info->original_cwd;
       			strvec_pushl(&cp.args, "clean", "--force",
      -				     "--quiet", "-d", NULL);
      +				     "--quiet", "-d", ":/", NULL);
       			if (include_untracked == INCLUDE_ALL_FILES)
       				strvec_push(&cp.args, "-x");
       			if (run_command(&cp)) {
     -@@ builtin/stash.c: static int push_stash(int argc, const char **argv, const char *prefix,
     - 		die(_("--pathspec-file-nul requires --pathspec-from-file"));
     - 	}
     - 
     --	return do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,
     --			     include_untracked);
     -+	return do_push_stash(&ps, stash_msg, prefix, quiet, keep_index,
     -+			     patch_mode, include_untracked);
     - }
     - 
     - static int save_stash(int argc, const char **argv, const char *prefix)
     -@@ builtin/stash.c: static int save_stash(int argc, const char **argv, const char *prefix)
     - 		stash_msg = strbuf_join_argv(&stash_msg_buf, argc, argv, ' ');
     - 
     - 	memset(&ps, 0, sizeof(ps));
     --	ret = do_push_stash(&ps, stash_msg, quiet, keep_index,
     -+	ret = do_push_stash(&ps, stash_msg, prefix, quiet, keep_index,
     - 			    patch_mode, include_untracked);
     - 
     - 	strbuf_release(&stash_msg_buf);
      
       ## t/t2501-cwd-empty.sh ##
      @@ t/t2501-cwd-empty.sh: test_expect_success 'clean does not remove cwd incidentally' '
  8:  a5528cbb14d !  8:  1815f18592b dir: avoid removing the current working directory
     @@ Metadata
      Author: Elijah Newren [off-list ref]
      
       ## Commit message ##
     -    dir: avoid removing the current working directory
     +    dir: avoid incidentally removing the original_cwd in remove_path()
      
     -    dir has a convenient remove_path() helper that will both remove a file
     -    in a directory and remove its containing directory if it becomes empty
     -    as a result of the removal, recursing all the way up.  However, we do
     -    not want the current working directory to be removed, even if it becomes
     -    empty.
     +    Modern git often tries to avoid leaving empty directories around when
     +    removing files.  Originally, it did not bother.  This behavior started
     +    with commit 80e21a9ed809 (merge-recursive::removeFile: remove empty
     +    directories, 2005-11-19), stating the reason simply as:
      
     -    dir also has a remove_dir_recursively() function which appears to mostly
     -    be used to remove metadata directories or temporary directories or
     -    submodules or worktrees.  I am not sure if it needs to be protected
     -    against removing the current working directory, but did so for good
     -    measure.
     +        When the last file in a directory is removed as the result of a
     +        merge, try to rmdir the now-empty directory.
     +
     +    This was reimplemented in C and renamed to remove_path() in commit
     +    e1b3a2cad7 ("Build-in merge-recursive", 2008-02-07), but was still
     +    internal to merge-recursive.
     +
     +    This trend towards removing leading empty directories continued with
     +    commit d9b814cc97f1 (Add builtin "git rm" command, 2006-05-19), which
     +    stated the reasoning as:
     +
     +        The other question is what to do with leading directories. The old
     +        "git rm" script didn't do anything, which is somewhat inconsistent.
     +        This one will actually clean up directories that have become empty
     +        as a result of removing the last file, but maybe we want to have a
     +        flag to decide the behaviour?
     +
     +    remove_path() in dir.c was added in 4a92d1bfb784 (Add remove_path: a
     +    function to remove as much as possible of a path, 2008-09-27), because
     +    it was noted that we had two separate implementations of the same idea
     +    AND both were buggy.  It described the purpose of the function as
     +
     +        a function to remove as much as possible of a path
     +
     +    Why remove as much as possible?  Well, at the time we probably would
     +    have said something like:
     +
     +      * removing leading directories makes things feel tidy
     +      * removing leading directories doesn't hurt anything so long as they
     +        had no files in them.
     +
     +    But I don't believe those reasons hold when the empty directory happens
     +    to be the current working directory we inherited from our parent
     +    process.  Leaving the parent process in a deleted directory can cause
     +    user confusion when subsequent processes fail: any git command, for
     +    example, will immediately fail with
     +
     +        fatal: Unable to read current working directory: No such file or directory
     +
     +    Other commands may similarly get confused.  Modify remove_path() so that
     +    the empty leading directories it also deletes does not include the
     +    current working directory we inherited from our parent process.  I have
     +    looked through every caller of remove_path() in the current codebase to
     +    make sure that all should take this change.
      
          Signed-off-by: Elijah Newren [off-list ref]
      
       ## dir.c ##
     -@@ dir.c: static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
     - 	closedir(dir);
     - 
     - 	strbuf_setlen(path, original_len);
     --	if (!ret && !keep_toplevel && !kept_down)
     --		ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
     --	else if (kept_up)
     -+	if (!ret && !keep_toplevel && !kept_down) {
     -+		if (the_cwd && !strcmp(the_cwd, path->buf))
     -+			ret = -1; /* Do not remove current working directory */
     -+		else
     -+			ret = (!rmdir(path->buf) || errno == ENOENT) ? 0 : -1;
     -+	} else if (kept_up)
     - 		/*
     - 		 * report the uplevel that it is not an error that we
     - 		 * did not rmdir() our directory.
      @@ dir.c: int remove_path(const char *name)
       		slash = dirs + (slash - name);
       		do {
       			*slash = '\0';
     -+			if (the_cwd && !strcmp(the_cwd, dirs))
     ++			if (startup_info->original_cwd &&
     ++			    !strcmp(startup_info->original_cwd, dirs))
      +				break;
       		} while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));
       		free(dirs);
       	}
      
     + ## dir.h ##
     +@@ dir.h: int get_sparse_checkout_patterns(struct pattern_list *pl);
     +  */
     + int remove_dir_recursively(struct strbuf *path, int flag);
     + 
     +-/* tries to remove the path with empty directories along it, ignores ENOENT */
     ++/*
     ++ * Tries to remove the path, along with leading empty directories so long as
     ++ * those empty directories are not startup_info->original_cwd.  Ignores
     ++ * ENOENT.
     ++ */
     + int remove_path(const char *path);
     + 
     + int fspathcmp(const char *a, const char *b);
     +
       ## t/t2501-cwd-empty.sh ##
      @@ t/t2501-cwd-empty.sh: test_expect_success 'revert fails if cwd needs to be removed' '
       	test_path_is_dir dirORfile
  -:  ----------- >  9:  adaad7aeaac dir: new flag to remove_dir_recurse() to spare the original_cwd

-- 
gitgitgadget

From: Elijah Newren <redacted>

Removing the current working directory causes all subsequent git
commands run from that directory to get confused and fail with a message
about being unable to read the current working directory:

    $ git status
    fatal: Unable to read current working directory: No such file or directory

Non-git commands likely have similar warnings or even errors, e.g.

    $ bash -c 'echo hello'
    shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory
    hello

This confuses end users, particularly since the command they get the
error from is not the one that caused the problem; the problem came from
the side-effect of some previous command.

We would like to avoid removing the current working directory of our
parent process; towards this end, introduce a new variable,
startup_info->original_cwd, that tracks the current working directory
that we inherited from our parent process.  For convenience of later
comparisons, we prefer that this new variable store a path relative to
the toplevel working directory (thus much like 'prefix'), except without
the trailing slash.

Subsequent commits will make use of this new variable.

Signed-off-by: Elijah Newren <redacted>
---
 cache.h |  1 +
 git.c   |  2 ++
 setup.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/cache.h b/cache.h
index eba12487b99..d7903c65b57 100644
--- a/cache.h
+++ b/cache.h

@@ -1834,6 +1834,7 @@ void overlay_tree_on_index(struct index_state *istate,
 struct startup_info {
 	int have_repository;
 	const char *prefix;
+	const char *original_cwd;
 };
 extern struct startup_info *startup_info;
 

diff --git a/git.c b/git.c
index 5ff21be21f3..2c98ab48936 100644
--- a/git.c
+++ b/git.c

@@ -866,6 +866,8 @@ int cmd_main(int argc, const char **argv)
 
 	trace_command_performance(argv);
 
+	startup_info->original_cwd = xgetcwd();
+
 	/*
 	 * "git-xxxx" is the same as "git xxxx", but we obviously:
 	 *

diff --git a/setup.c b/setup.c
index 347d7181ae9..f30657723ea 100644
--- a/setup.c
+++ b/setup.c

@@ -432,6 +432,54 @@ void setup_work_tree(void)
 	initialized = 1;
 }
 
+static void setup_original_cwd(void)
+{
+	struct strbuf tmp = STRBUF_INIT;
+	const char *worktree = NULL;
+	int offset = -1;
+
+	/*
+	 * startup_info->original_cwd wass set early on in cmd_main(), unless
+	 * we're an auxiliary tool like git-remote-http or test-tool.
+	 */
+	if (!startup_info->original_cwd)
+		return;
+
+	/*
+	 * startup_info->original_cwd points to the current working
+	 * directory we inherited from our parent process, which is a
+	 * directory we want to avoid incidentally removing.
+	 *
+	 * For convience, we would like to have the path relative to the
+	 * worktree instead of an absolute path.
+	 *
+	 * Yes, startup_info->original_cwd is usually the same as 'prefix',
+	 * but differs in two ways:
+	 *   - prefix has a trailing '/'
+	 *   - if the user passes '-C' to git, that modifies the prefix but
+	 *     not startup_info->original_cwd.
+	 */
+
+	/* Normalize the directory */
+	strbuf_realpath(&tmp, startup_info->original_cwd, 1);
+	free((char*)startup_info->original_cwd);
+	startup_info->original_cwd = strbuf_detach(&tmp, NULL);
+
+	/* Find out if this is in the worktree */
+	worktree = get_git_work_tree();
+	if (worktree)
+		offset = dir_inside_of(startup_info->original_cwd, worktree);
+	if (offset >= 0) {
+		/*
+		 * original_cwd was inside worktree; precompose it just as
+		 * we do prefix so that built up paths will match
+		 */
+		startup_info->original_cwd = \
+			precompose_string_if_needed(startup_info->original_cwd
+						    + offset);
+	}
+}
+
 static int read_worktree_config(const char *var, const char *value, void *vdata)
 {
 	struct repository_format *data = vdata;

@@ -1330,6 +1378,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
 	}
 
+	setup_original_cwd();
 
 	strbuf_release(&dir);
 	strbuf_release(&gitdir);

-- 
gitgitgadget

From: Elijah Newren <redacted>

In the past, when a directory needs to be removed to make room for a
file, we have always errored out when that directory contains any
untracked (but not ignored) files.  Add an extra condition on that: also
error out if the directory is the current working directory we inherited
from our parent process.

Signed-off-by: Elijah Newren <redacted>
---
 t/t2501-cwd-empty.sh | 10 +++++-----
 unpack-trees.c       | 17 +++++++++++++----
 unpack-trees.h       |  1 +
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index 5dfb456a691..212676d71c3 100755
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh

@@ -38,7 +38,7 @@ test_expect_failure 'checkout does not clean cwd incidentally' '
 	test_path_is_dir foo
 '
 
-test_expect_failure 'checkout fails if cwd needs to be removed' '
+test_expect_success 'checkout fails if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&