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/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/
[3]
https://lore.kernel.org/git/[email protected]/

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 <[email protected]>

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.

---
 t/t2501-cwd-empty.sh | 255 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 255 insertions(+)
 create mode 100755 t/t2501-cwd-empty.sh

new file mode 100755
index 00000000000..5dfb456a691
@@ -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

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 <[email protected]>) 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...

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

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/[email protected]/ and its grandparent. User scripts likely have lurking problems too.

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.

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.

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).

FWIW that's with a -n, just showing that we've got things left.

I.e. just a demo of the new state, in any case not really applicable since the end-state behavior is that we die...

Broken as in they'll upgrade git and a previously working script or cronjob stops working.

Maybe it's too obscure to worry about, but saying nothing broke that wasn't working before seems to be moving goalposts to an odd place.

Anyway, I see from
https://lore.kernel.org/git/CABPp-BETpWU9Rkd6pcxh6+gav2QtYnu_5V8ji_1_3kMnVswp1Q@mail.gmail.com/
that you're not really saying that & that we've partially got a split thread here. Just replying here...

I'm not going to make this my hill to die on, and honestly wouldn't care that much if this were changed, aside from wondering how much is really the x-y problem of setup.c.

But just in the interest of clarity, what I'm talking about is that git is a *nix tool, and usually just follows *nix FS semantics.

I think we should have a strong bias towards continuing until the OS tell us to stop for any FS operations. Rather than aborting early in anticipation of an eventual error, particularly when as I've shown that eventual error is fixable.

*Nod*. Not to go on about it, but I think those issues are pointing out things that would be fixed by the the more narrow WIP patch I've got of "let's just make setup.c not suck in that case" :)

Again, "not my hill" etc., but I think the harm is pretty much that every time we start adding any sort of unexpected special-sauce on top of doing stuff until we get a syscall error we're subverting the default expectations of users.

I think about this as being in the same category as changing "checkout", "rm" etc. to error out if we detect the target file is open (Windows), or refusing to remove an executable that we detect as being in use (AIX).

If you're not on those platforms having a random tool you're scripting imposing strictures outside of what the OS would do is just another thing to explain to users. Let's just try and run into the OS error, or fix our handling of the subsequent error (if any).

Whatever direction we decide on here, I really don't think it makes sense to think about tracked and modified local content the same as untracked and unmodified content.

For "tracked and modified" we clearly "own" it, and erroring out is what everyone would expect us to do. We don't want git to shred user data.

For "untracked and unmodified" (the part after "and" being redundant :-)) we've got the "precious" edge-case, i.e. we could shred something the user cares about (which you've also been working on & I very much applaud).

But I think by any sane definition we'd still classify such content as less important (since in those cases of shredding it's matching a .gitignore).

And surely *way lower* in that foodchain is "untracked and irrelevant". I.e. it's conceivable that someone is making meaningful use of an empty directory, but it's very unlikely, and in any case your argument for this change isn't that the directory is important per-se, but that

You note in
https://lore.kernel.org/git/CABPp-BETpWU9Rkd6pcxh6+gav2QtYnu_5V8ji_1_3kMnVswp1Q@mail.gmail.com/
that for "rm -r" we already left the directory behind for a "dir" that had partially tracked/untracked content, that's correct. I'm referring to the case where "x" is entirely tracked.

I've read through those, and might have missed something, but it seems to all really be discussing the shortcomings of setup.c's handling of failing getcwd() by proxy.

That and references to other 3rd party commands potentially being equally confused.

In summary I think we should aim more towards:

    $ grep -m 1 Git ../README.md 
    Git - fast, scalable, distributed revision control system

Than:

    $ git grep --no-index -m 1 . ../README.md 
    fatal: Unable to read current working directory: No such file or directory

Which fails due to the setup.c issue I've noted, but also because we stupidly really translate that to the equivalent of a (in an x/ that doesn't exist anymore):

    $ grep -m 1 Git ../x/../README.md 
    grep: ../x/../README.md: No such file or directory

From: Elijah Newren <[email protected]>

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.

---
 repository.c | 1 +
 repository.h | 1 +
 setup.c      | 2 ++
 3 files changed, 4 insertions(+)

index c5b90ba93ea..69a106c553c 100644
@@ -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)
 {

index a057653981c..45de85d18ef 100644
@@ -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

index 347d7181ae9..4466fa55af3 100644
@@ -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

Maybe a stupid question, but how is this different from doing getcwd() and storing it away to the_cwd, or adding a check to see if the directory we are about to rmdir() is the cwd, next to the existing check that we do to see if that directory has some untracked files?

I am wondering how we are going to make sure that the_cwd is always set to, and maintained to be, the correct value, even in the future when these code paths change. I also wonder if it might be safer to learn what the value of cwd is very near the place where it will become needed (i.e. the callsites of such rmdir() of a directory inside working tree), instead of caching.

The exact output of getcwd() is an absolute path, whereas it's easier to perform later checks if we have a path that is relative to the toplevel working directory. Also, setup.c already calls getcwd() (via strbuf_getcwd()) and then massages it, and I'd have to do the exact same type of massaging. So, instead of calling getcwd() again and re-massaging its output into the format I want, I just reuse the existing call of getcwd() and its massaged output.

I need to clean up my wording a bit, to clarify this. Doing as you suggest here would protect the wrong thing. git has long done an automatic chdir() at startup to the toplevel working tree, whereas what we want to protect is the original current working directory as of the time git started. The original current working directory is likely also still the current working directory of the parent process that spawned us, and if that parent process is a shell, any subsequent commands executed from there can be somewhat confusing when its current working directory is removed.

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

A worthy goal.

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.

From: Elijah Newren <[email protected]>

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.

---
 t/t2501-cwd-empty.sh | 10 +++++-----
 unpack-trees.c       | 16 ++++++++++++----
 unpack-trees.h       |  1 +
 3 files changed, 18 insertions(+), 9 deletions(-)

index 5dfb456a691..212676d71c3 100755
@@ -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" &&

index 89ca95ce90b..8879b8d6c8c 100644
@@ -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;
 }
 

index 71ffb7eeb0c..efb9edfbb27 100644
@@ -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 <[email protected]>

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.

---
 t/t2501-cwd-empty.sh |  2 +-
 unpack-trees.c       | 12 ++++++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)

index 212676d71c3..526d8ec2ee3 100755
@@ -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" &&
 

index 8879b8d6c8c..d1ce4f4a35b 100644
@@ -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 <[email protected]>

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.

---
 symlinks.c           | 12 +++++++++++-
 t/t2501-cwd-empty.sh | 12 ++++++------
 2 files changed, 17 insertions(+), 7 deletions(-)

index 5232d02020c..84622bedcde 100644
@@ -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);

index 526d8ec2ee3..b92e1a9bb16 100755
@@ -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

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

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.

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.

Sure.

From: Elijah Newren <[email protected]>

---
 builtin/clean.c      | 29 ++++++++++++++++++++---------
 t/t2501-cwd-empty.sh |  2 +-
 2 files changed, 21 insertions(+), 10 deletions(-)

index 98a2860409b..17d54be57b7 100644
@@ -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;
+			}
 		}
 	}
 

index b92e1a9bb16..20e1b6adede 100755
@@ -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

Thanks; will fix.

From: Elijah Newren <[email protected]>

---
 builtin/stash.c      | 13 ++++++++-----
 t/t2501-cwd-empty.sh |  2 +-
 2 files changed, 9 insertions(+), 6 deletions(-)

index a0ccc8654df..556287cdb56 100644
@@ -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);

index 20e1b6adede..ff4e7cd89fa 100755
@@ -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 <[email protected]>

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.

---
 dir.c                | 11 ++++++++---
 t/t2501-cwd-empty.sh |  4 ++--
 2 files changed, 10 insertions(+), 5 deletions(-)

index 94489298f4c..15e7639a158 100644
@@ -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);
 	}

index ff4e7cd89fa..4362e7b15e5 100755
@@ -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

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.

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

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.

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);

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.

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.

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).

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.

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.

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.

FWIW, I do not find it compelling. I can grant that it might be convenient, but I do not think it is a good idea to explain the reason why the directory is protected is because it is "not empty".

Is the objection to my hand-wavy explanation? If so, point taken. However, I'm curious if you're also objecting to my commit message and/or the patch as well.

If your objection also includes my commit message, but not the patch, would the following suit your taste better? :

"""
remove_path() was added in 4a92d1bfb784 (Add remove_path: a function to remove as much as possible of a path, 2008-09-27) to, as it says, remove as much of a path as possible. 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.

However, the second reason (and perhaps also the first) do not hold when that empty directory was 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

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. Also do the same for remove_dir_recursively(). """

The objection is against the definition of 'emptiness' Glen perceived in your explanation ;-)

A directory is empty when there is no filesystem entity hangs below it. A process can have any directory as its cwd, even an empty one, but the presense of such a process does not make an empty directory suddenly non-empty. That is the objection.

I think you meant trailing, but I do not think you need to say it twice---they say pretty much the same thing. We are removing as many directories that contain no files (i.e. non-directories) underneath to make things tidy, as such a directory serves no useful purpose.

I am not saying that an empty directory should not be protected even when the current process sits there. I just do not think it is a good idea to call the protection "we protect a non-empty directory". It is something else.

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.

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 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?

I doubt there is.

It would be annoying if the command fails to remove a directory that becomes empty after an operation only because your interactive shell that spawned "git" was there, especially if your system allows interactive shell to still sit in an unlinked directory. After such a rmdir, /bin/pwd or cd .. may not work, but then the only thing you need to do to recover from this situation is to run cd /path/to/repository and we can continue working, without having to worry about a leftover directory.

That may be a more pleasant end-user experience than what is being proposed, which forces you (1) to realize that you are in a directory that was to be removed if you weren't sitting there, and
(2) to compute how many levels of otherwise empty directories were left because of your presense, and (3) to go up sufficient number of levels and manually run "rm -fr" the hierarchy, to recover.

So, I dunno. It does make the end-user experience on such a system that allows your cwd to go away equally unpleasant to the end-user experience on a system that does not allow your cwd to go away, so we might be gaining a bit in the consistency department, but as has been already discussed, "git" level protection can only notice the process immediately above us and we wouldn't be able to pay attention to the fact some other unrelated process is using a directory as its cwd, so even such a consistency argument does not go all that far.

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.

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/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/

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?

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.

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.

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.

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.

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

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/[email protected]/

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

I do not think it is about protecting "root"; the series wants

    cd t/ && git rm -r ../t

to leave an empty directory at 't/', because "git rm" was started in that directory.

My point was about where the conceptual 'root' (for Git and it's rm command) was deemed to be.

For instance, can/should we be able to elevate ourselves into a super project for the deletion? I did notice that a regular cd / && cd ../../ will happily recycle itself at /, rather than bugging out.

Whichever way is decided (cwd, GIT_WORK_TREE, or higher), ensuring that the documentation is plain and clear , and not just the code, is important for future readers, to help avoid future confusions.

Philip

Makes sense, but nothing being discussed here has to do with crossing repository boundaries so far.

This behavior explicitly standardized in POSIX:
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html:

The special filename dot shall refer to the directory specified byits predecessor. The special filename dot-dot shall refer to the parent directory of its predecessor directory. As a special case, in the root directory, dot-dot may refer to the root directory itself.c

The same goes for "foo", "./foo", "./././././foo" etc.

Yes, offhand I don't know where we canonically document our behavior of not crossing repo boundaries, or rather not going "up".

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...

Sweet!

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'm curious: is this issue the only reason, or am I reading between the lines correctly that you have additional reasons for that?

*nod*

I think it sucks in general :)

This is a much bigger change than what's needed for this topic or the proposed setup.c change I've got, but what we should be doing in general with bin-wrapper is to not have them at all under RUNTIME_PREFIX=Y (which we can make the default), and simply have a built/ directory or something, the structure of that directory will then 1=1 map to all the relative paths you'd get if you did a "make install".

Currently in a built checkout you can do:

./git version

But not:

    # need to add --exec-path=$PWD
    ./git clone <http url>

Or:

    ./git send-email ...

Which all comes down to sillyness in how we try to discover these paths, the *.mo files etc., there's also Perl-specific caveats in there.

We'd still need something like a shellscript wrapper for the case of GIT_DEBUGGER, but we could just move that to being a compile-time flag, i.e. we'd swap out the "git" binary for a shellscript that chain-loaded to the "real" binary.

It probably matters a bit for e.g. test suite performance, currently we spawn an extra shell for every single "git" invocation, but I mostly care about it for correctness, e.g. in this case of $PWD munging, and being able to have running a built binary from a checkout Just Work without the wrapper.

It also adds quite a bit of overhead to the tests, more than I vaguely recalled. It differs by test, but here's one that invokes "git" a lot". Trying with GIT_TEST_INSTALLED is a handy way to avoid the bin-wrappers:

    $ hyperfine -L e ,GIT_TEST_INSTALLED=/home/avar/g/git/installed/bin '{e} ./t3070-wildmatch.sh --root=/dev/shm/git'
    Benchmark 1:  ./t3070-wildmatch.sh --root=/dev/shm/git
      Time (mean ± σ):      4.385 s ±  0.028 s    [User: 3.730 s, System: 0.824 s]
      Range (min … max):    4.340 s …  4.430 s    10 runs

    Benchmark 2: GIT_TEST_INSTALLED=/home/avar/g/git/installed/bin ./t3070-wildmatch.sh --root=/dev/shm/git
      Time (mean ± σ):      3.737 s ±  0.036 s    [User: 3.240 s, System: 0.656 s]
      Range (min … max):    3.686 s …  3.796 s    10 runs
     
    Summary
      'GIT_TEST_INSTALLED=/home/avar/g/git/installed/bin ./t3070-wildmatch.sh --root=/dev/shm/git' ran
        1.17 ± 0.01 times faster than ' ./t3070-wildmatch.sh --root=/dev/shm/git'

So a ~15-20% speedup, a more common case of a ~10% speedup:

    $ hyperfine -L e ,GIT_TEST_INSTALLED=/home/avar/g/git/installed/bin '{e} ./t0001-init.sh --root=/dev/shm/git'
    Benchmark 1:  ./t0001-init.sh --root=/dev/shm/git
      Time (mean ± σ):     522.8 ms ±  22.8 ms    [User: 438.9 ms, System: 130.2 ms]
      Range (min … max):   502.8 ms … 581.3 ms    10 runs

    Benchmark 2: GIT_TEST_INSTALLED=/home/avar/g/git/installed/bin ./t0001-init.sh --root=/dev/shm/git
      Time (mean ± σ):     473.4 ms ±  12.6 ms    [User: 406.7 ms, System: 117.5 ms]
      Range (min … max):   461.0 ms … 502.1 ms    10 runs
     
    Summary
      'GIT_TEST_INSTALLED=/home/avar/g/git/installed/bin ./t0001-init.sh --root=/dev/shm/git' ran
        1.10 ± 0.06 times faster than ' ./t0001-init.sh --root=/dev/shm/git'

And that's on a box where process spawning is really fast, and against a ramdisk.

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.

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

...

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.

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.

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.

Not necessarily. Devil's advocate hat on.

Because you used "ls -ld" in your illustration, we cannot see what happened to contrib/subtree/t/ directory, but presumably that one is gone.

Now, after this, if we "git commit", making contrib/subtree directory truly unnecessary, nothing reminds us that we need to remove it manually with "rmdir" or "git clean -d", no?

With the "before" behaviour, the user still can go back to the top level of the repository by going back to the directory the user was in before typing "cd contrib/subtree", by saying "cd -" and "git status" and everything else would just work, including not having to worry about remembering to clean an empty directory contrib/subtree that is no longer needed.

The goal is roughly that we do not allow rmdir() of a directory at "prefix" in the working tree, or any parent directory of it, for functions that uses RUN_SETUP? By the time we attempt to rmdir(), most likely we'd chdir()'ed up to the top of the working tree ourselves, so we need to remember where the original cwd was when we started, and protecting the original cwd would mean that we protect the directory in which the process that spawned us, like the user's interactive shell, is still sitting, which makes sense.

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/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/
[3] https://lore.kernel.org/git/[email protected]/ [4]
https://lore.kernel.org/git/[email protected]/

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.

  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 <[email protected]>

## 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.

@@ 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.

@@ 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 <[email protected]>
      
       ## 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 <[email protected]>
      
       ## 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 <[email protected]>
      
       ## 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 <[email protected]>
      
     @@ 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 <[email protected]>

## Commit message ##
- stash: do not attempt to remove current working directory
+ stash: do not attempt to remove startup_info->original_cwd

## 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 <[email protected]>

## 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.

## 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 <[email protected]>

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.

---
 t/t2501-cwd-empty.sh | 255 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 255 insertions(+)
 create mode 100755 t/t2501-cwd-empty.sh

new file mode 100755
index 00000000000..5dfb456a691
@@ -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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

To:

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

index 5dfb456a691..f1fb8b4a872 100755
@@ -24,7 +24,7 @@ test_expect_success setup '
 	git commit -m dirORfile
 '
 
-test_expect_failure 'checkout does not clean cwd incidentally' '
+test_expect_success 'checkout cleans cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_path_is_dir foo/bar &&
 

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

@@ -46,14 +46,14 @@ test_expect_failure 'checkout fails if cwd needs to be removed' '
 	(
 		cd dirORfile &&
 
-		test_must_fail git checkout fd_conflict 2>../error &&
-		grep "Refusing to remove the current working directory" ../error
+		git checkout fd_conflict 2>../error &&
+		grep "Switched to branch" ../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'reset --hard does not clean cwd incidentally' '
+test_expect_success 'reset --hard cleans cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_path_is_dir foo/bar &&
 

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

@@ -75,14 +75,14 @@ test_expect_failure 'reset --hard fails if cwd needs to be removed' '
 	(
 		cd dirORfile &&
 
-		test_must_fail git reset --hard fd_conflict 2>../error &&
-		grep "Refusing to remove.*the current working directory" ../error
+		git reset --hard fd_conflict 2>../error &&
+		test_must_be_empty ../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'merge does not remove cwd incidentally' '
+test_expect_success 'merge removes cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -92,24 +92,24 @@ test_expect_failure 'merge does not remove cwd incidentally' '
 	) &&
 
 	test_path_is_missing subdir/file.t &&
-	test_path_is_dir subdir
+	test_path_is_missing subdir
 '
 
-test_expect_failure 'merge fails if cwd needs to be removed' '
+test_expect_success 'merge succeeds if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
 	mkdir dirORfile &&
 	(
 		cd dirORfile &&
-		test_must_fail git merge fd_conflict 2>../error &&
-		grep "Refusing to remove the current working directory" ../error
+		git merge fd_conflict 2>../error &&
+		test_must_be_empty ../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'cherry-pick does not remove cwd incidentally' '
+test_expect_success 'cherry-pick removes cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -119,24 +119,23 @@ test_expect_failure 'cherry-pick does not remove cwd incidentally' '
 	) &&
 
 	test_path_is_missing subdir/file.t &&
-	test_path_is_dir subdir
+	test_path_is_missing subdir
 '
 
-test_expect_failure 'cherry-pick fails if cwd needs to be removed' '
+test_expect_success 'cherry-pick suceeds if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
 	mkdir dirORfile &&
 	(
 		cd dirORfile &&
-		test_must_fail git cherry-pick fd_conflict 2>../error &&
-		grep "Refusing to remove the current working directory" ../error
+		git cherry-pick fd_conflict 2>../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'rebase does not remove cwd incidentally' '
+test_expect_success 'rebase does removes cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -146,24 +145,23 @@ test_expect_failure 'rebase does not remove cwd incidentally' '
 	) &&
 
 	test_path_is_missing subdir/file.t &&
-	test_path_is_dir subdir
+	test_path_is_missing subdir
 '
 
-test_expect_failure 'rebase fails if cwd needs to be removed' '
+test_expect_success 'rebase succeeds if cwd needs to be removed' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 
 	mkdir dirORfile &&
 	(
 		cd dirORfile &&
-		test_must_fail git rebase foo/bar/baz fd_conflict 2>../error &&
-		grep "Refusing to remove the current working directory" ../error
+		git rebase foo/bar/baz fd_conflict 2>../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'revert does not remove cwd incidentally' '
+test_expect_success 'revert removes cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -173,10 +171,10 @@ test_expect_failure 'revert does not remove cwd incidentally' '
 	) &&
 
 	test_path_is_missing subdir/file.t &&
-	test_path_is_dir subdir
+	test_path_is_missing subdir
 '
 
-test_expect_failure 'revert fails if cwd needs to be removed' '
+test_expect_success 'revert succeeds if cwd needs to be removed' '
 	git checkout fd_conflict &&
 	git revert HEAD &&
 	test_when_finished "git clean -fdx" &&

@@ -184,14 +182,13 @@ test_expect_failure 'revert fails if cwd needs to be removed' '
 	mkdir dirORfile &&
 	(
 		cd dirORfile &&
-		test_must_fail git revert HEAD 2>../error &&
-		grep "Refusing to remove the current working directory" ../error
+		git revert HEAD 2>../error
 	) &&
 
-	test_path_is_dir dirORfile
+	test_path_is_file dirORfile
 '
 
-test_expect_failure 'rm does not remove cwd incidentally' '
+test_expect_success 'rm removes cwd incidentally' '
 	test_when_finished "git reset --hard" &&
 	git checkout foo/bar/baz &&
 

@@ -202,10 +199,24 @@ test_expect_failure 'rm does not remove cwd incidentally' '
 
 	test_path_is_missing foo/bar/baz &&
 	test_path_is_missing foo/bar &&
-	test_path_is_dir foo
+	test_path_is_missing foo
 '
 
-test_expect_failure 'apply does not remove cwd incidentally' '
+test_expect_success 'rm -f removes cwd incidentally' '
+	test_when_finished "git reset --hard" &&
+	git checkout foo/bar/baz &&
+
+	(
+		cd foo &&
+		git rm -f bar/baz.t
+	) &&
+
+	test_path_is_missing foo/bar/baz &&
+	test_path_is_missing foo/bar &&
+	test_path_is_missing foo
+'
+
+test_expect_success 'apply removes cwd incidentally' '
 	test_when_finished "git reset --hard" &&
 	git checkout foo/bar/baz &&
 

@@ -215,10 +226,10 @@ test_expect_failure 'apply does not remove cwd incidentally' '
 	) &&
 
 	test_path_is_missing subdir/file.t &&
-	test_path_is_dir subdir
+	test_path_is_missing subdir
 '
 
-test_expect_failure 'clean does not remove cwd incidentally' '
+test_expect_success 'clean does not remove cwd incidentally (cannot match pathspec)' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -227,16 +238,15 @@ test_expect_failure 'clean does not remove cwd incidentally' '
 	>untracked/random &&
 	(
 		cd untracked &&
-		git clean -fd -e warnings :/ >../warnings &&
-		grep "Refusing to remove current working directory" ../warnings
+		test_must_fail git clean -fd .
 	) &&
 
-	test_path_is_missing empty &&
+	test_path_is_dir empty &&
 	test_path_is_missing untracked/random &&
 	test_path_is_dir untracked
 '
 
-test_expect_failure 'stash does not remove cwd incidentally' '
+test_expect_success 'stash removes cwd incidentally' '
 	git checkout foo/bar/baz &&
 	test_when_finished "git clean -fdx" &&
 

@@ -244,12 +254,11 @@ test_expect_failure 'stash does not remove cwd incidentally' '
 	>untracked/random &&
 	(
 		cd untracked &&
-		git stash --include-untracked &&
-		git status
+		git stash --include-untracked
 	) &&
 
 	test_path_is_missing untracked/random &&
-	test_path_is_dir untracked
+	test_path_is_missing untracked
 '
 
 test_done

From: Elijah Newren <[email protected]>

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.

---
 cache.h |  1 +
 git.c   |  2 ++
 setup.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

index eba12487b99..d7903c65b57 100644
@@ -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;
 

index 5ff21be21f3..2c98ab48936 100644
@@ -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:
 	 *

gitgitgadget

Okey, but...

    mkdir x &&
    cd x &&
    rm -rf ../x &&
    git version

Whoops! Yeah, I should have used strbuf_getcwd() there. Thanks for the careful reading.

If I had to do some kind of special casing, or if I did potentially call a path that could die() in the final version, then yes I'd need to test all of these. But by using strbuf_getcwd(), I remove the possibility of die()'ing, so I think testing one of these is good enough.

I didn't think e.g. git-remote-http, git-daemon, etc. mattered. test-tool is supposed to only be used in the testsuite to my knowledge. But fair point, I'll move it to common-main.c. And, as per Junio's suggestion in this thread, I'll make it only affect worktrees.

Nah, I'll just put the original_cwd in a temporary, and then if setup is never called, it won't affect anything else. And setup will only make it affect the worktree (and thus won't affect bare repos, nor affect paths outside the worktree if the user is running from outside the worktree).

There is a chdir_notify(), but of course it isn't called by the codepath that handles the -C argument. I believe the above solution of a temporary handles things just fine, though.

Thanks for the thoughtful and careful review!

Yeah, Ævar caught this mistake too; I should have used strbuf_getcwd(). Will fix.

Thanks.

Yeah, I wondered about this a bit and whether it'd even matter either way if the original_cwd was something outside the working tree. I can obviously come up with scenarios, but can't see protecting or not protecting such directories as mattering; if the user is in a directory where git internals are stored and deleted, well, they're already off-roading.

But, since you bring it up, I might as well scope this to just protecting a directory within the working tree. Perhaps that'd make Ævar more comfortable with the change too. I'll do that.

Since setup_git_directory() is used to figure out the prefix, which is relative path to the directory, in which the command was started, viewed from the top-level of the working tree, it would be a bug if the caller did any chdir(2) before it called the function.

I didn't look at or thought about how chdir(2) done inside this function affects what setup_original_cwd() wants to do, though.

Thanks.

It looks like you accidentally responded to v2 when there's a v3 (something I occasionally do too). v3 changes this to put it in common-main instead of here, as suggested by Ævar, but to answer the question...

In v1, I made that mistake. Then I realized that when users pass the -C option to git, there is a chdir() call immediately upon parsing of the -C option. So I had to move the strbuf_getcwd() call earlier.

Yes, sorry about that. My inbox was delayed in showing me that a v3 existed until I was halfway through reviewing v2. (It then only showed me half of the patches from v3, so something was causing a delay.)

Ok. I wonder if we could setup_original_cwd() earlier than that parsing, but I'm sure you've already explored that option.

Thanks,
-Stolee

From: Elijah Newren <[email protected]>

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.

---
 t/t2501-cwd-empty.sh | 10 +++++-----
 unpack-trees.c       | 17 +++++++++++++----
 unpack-trees.h       |  1 +
 3 files changed, 19 insertions(+), 9 deletions(-)

index 5dfb456a691..212676d71c3 100755
@@ -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" &&

index 89ca95ce90b..6bc16f3a714 100644
@@ -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,12 @@ 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 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;
 }
 

index 71ffb7eeb0c..efb9edfbb27 100644
@@ -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

If someone else wants to submit a patch afterwards to clean up all these error messages, that'd be fine, but I think being internally inconsistent is more jarring than not following the global rule. I'll leave it as is, and let someone else do the wider cleanup.

Seems pretty reasonable, but 9 out of 10 of the messages in that list contain the hard stop. True, that's still inconsistent, but adding the hard stop feels more internally consistent. I think I'd rather have someone else post a subsequent cleanup to remove the hard stops and lowercase the first letter and whatever else needs to be changed with these messages.

From: Elijah Newren <[email protected]>

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_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.

---
 t/t2501-cwd-empty.sh |  2 +-
 unpack-trees.c       | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

index 212676d71c3..526d8ec2ee3 100755
@@ -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" &&
 

index 6bc16f3a714..5852807d2fb 100644
@@ -2261,10 +2261,19 @@ 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 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. */
+		return 0;
+	}
+
 	len = check_leading_path(ce->name, ce_namelen(ce), 0);
 	if (!len)
 		return 0;

gitgitgadget

...

Makes sense; I can add a
git diff --exit-code HEAD &&
to the test.

Thanks for taking a look.

From: Elijah Newren <[email protected]>

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 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.

---
 symlinks.c           |  8 +++++++-
 t/t2501-cwd-empty.sh | 12 ++++++------
 2 files changed, 13 insertions(+), 7 deletions(-)

index 5232d02020c..c667baa949b 100644
@@ -279,7 +279,9 @@ static void do_remove_scheduled_dirs(int new_len)
 {
 	while (removal.len > new_len) {
 		removal.buf[removal.len] = '\0';
-		if (rmdir(removal.buf))
+		if ((startup_info->original_cwd &&
+		     !strcmp(removal.buf, startup_info->original_cwd)) ||
+		    rmdir(removal.buf))
 			break;
 		do {
 			removal.len--;

@@ -293,6 +295,10 @@ void schedule_dir_for_removal(const char *name, int len)
 {
 	int match_len, last_slash, i, previous_slash;
 
+	if (startup_info->original_cwd &&
+	    !strcmp(name, startup_info->original_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);

index 526d8ec2ee3..b92e1a9bb16 100755
@@ -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 <[email protected]>

---
 builtin/clean.c      | 47 +++++++++++++++++++++++++++++++++++---------
 t/t2501-cwd-empty.sh |  2 +-
 2 files changed, 39 insertions(+), 10 deletions(-)

index 98a2860409b..68c8d593c77 100644
@@ -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,

@@ -153,6 +155,8 @@ 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;

@@ -231,16 +235,39 @@ 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);
+		/*
+		 * 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;
+		} 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;
+			}
 		}
 	}