[PATCH v5 00/11] Avoid removing the current working directory, even if it becomes empty

From: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Date: Dec 1, 2021, 06:40 UTC
Message-ID: <pull.1140.v5.git.git.1638340854.gitgitgadget@gmail.com>
In-Reply-To: <pull.1140.v4.git.git.1638225434.gitgitgadget@gmail.com>

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

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

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

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

Changes since v4:

 * actually fix bashism

Changes since v3:

 * fixed one codepath from v2 so that the series really is only about the
   working tree
 * used test-tool getcwd instead of pwd -P as suggested by Ævar for some
   less common platforms
 * fixed bashism
 * check for clean index/worktree after verifying that expected-to-abort
   codepaths do abort, to make it clearer that we expect an early abort
 * remove a leftover (and confusing) is_absolute_dir() check in sequencer
   and stash from an earlier round of the series

Changes since v2:

 * the series is now only about the working tree. So if the original cwd is
   outside the worktree (or we're in a bare repo), then the new code is a
   no-op.
 * fixed ugly early die() possibility (uses strbuf_getcwd() instead of
   xgetcwd())
 * modified the initial tests to show both expected and desired behavior.
   subsequent patches fix the tests. One new patch added at the end which
   simplifies the tests to only check for desired behavior.
 * NULLify startup_info->original_cwd when it matches the toplevel worktree;
   that is already protected and we don't need secondary protection for it.
   This simplified some other codepaths so we don't have to check for
   startup_info->original_cwd == "".
 * clarified some commit messages

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. Also, add a bunch more history to the first
   of the two resulting commit messages
 * 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 (11):
  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
  rebase: 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
  t2501: simplify the tests since we can now assume desired behavior

 builtin/clean.c      |  44 +++++--
 builtin/rm.c         |   3 +-
 builtin/stash.c      |   4 +-
 cache.h              |   2 +
 common-main.c        |   4 +
 dir.c                |  15 ++-
 dir.h                |   9 +-
 sequencer.c          |   2 +
 setup.c              |  65 ++++++++++
 symlinks.c           |   8 +-
 t/t2501-cwd-empty.sh | 277 +++++++++++++++++++++++++++++++++++++++++++
 unpack-trees.c       |  30 ++++-
 unpack-trees.h       |   1 +
 13 files changed, 442 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-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1140/newren/cwd_removal-v5
Pull-Request: https://github.com/git/git/pull/1140

Range-diff vs v4:

  1:  a45b3f08802 !  1:  7c72e888d2e t2501: add various tests for removing the current working directory
     @@ t/t2501-cwd-empty.sh (new)
      +
      +		# Although we want pwd & git status to pass, test for existing
      +		# rather than desired behavior.
     -+		if test "$works" == "success"
     ++		if test "$works" = "success"
      +		then
      +			test-tool getcwd &&
      +			git status --porcelain
     @@ t/t2501-cwd-empty.sh (new)
      +
      +	# Although we want dir to be present, test for existing rather
      +	# than desired behavior.
     -+	if test "$works" == "success"
     ++	if test "$works" = "success"
      +	then
      +		test_path_is_dir foo
      +	else
     @@ t/t2501-cwd-empty.sh (new)
      +		# was an untracked file there), and for the index and worktree
      +		# to be left clean with pwd and git status working afterwards.
      +		# But test for existing rather than desired behavior.
     -+		if test "$works" == "success"
     ++		if test "$works" = "success"
      +		then
      +			test_must_fail "$@" 2>../error &&
      +			grep "Refusing to remove.*current working directory" ../error &&
     @@ t/t2501-cwd-empty.sh (new)
      +
      +	# Although we want dirORfile to be present, test for existing rather
      +	# than desired behavior.
     -+	if test "$works" == "success"
     ++	if test "$works" = "success"
      +	then
      +		test_path_is_dir dirORfile
      +	else
     @@ t/t2501-cwd-empty.sh (new)
      +
      +		# Although we want pwd & git status to pass, test for existing
      +		# rather than desired behavior.
     -+		if test "$works" == "success"
     ++		if test "$works" = "success"
      +		then
      +			test-tool getcwd &&
      +			git status --porcelain
     @@ t/t2501-cwd-empty.sh (new)
      +
      +	# Although we want dir to be present, test for existing rather
      +	# than desired behavior.
     -+	if test "$works" == "success"
     ++	if test "$works" = "success"
      +	then
      +		test_path_is_dir untracked
      +	else
     @@ t/t2501-cwd-empty.sh (new)
      +	test_status=
      +	test "$path_status" = dir && test_status=test_must_fail
      +
     -+	# Actually, while path_status == dir && test_status=test_must_fail
     ++	# Actually, while path_status=dir && test_status=test_must_fail
      +	# reflect our desired behavior, current behavior is:
      +	path_status=missing
      +	test_status=
  2:  ca9f632bd11 =  2:  37f333b2024 setup: introduce startup_info->original_cwd
  3:  41a82eff41e =  3:  b611c73bd15 unpack-trees: refuse to remove startup_info->original_cwd
  4:  2e2ea02f97b =  4:  706415a4547 unpack-trees: add special cwd handling
  5:  f444a541da4 =  5:  66ef6b4d943 symlinks: do not include startup_info->original_cwd in dir removal
  6:  1990e36bb41 =  6:  54d059c683e clean: do not attempt to remove startup_info->original_cwd
  7:  1035ee7f9ce =  7:  f8efb7446c3 rebase: do not attempt to remove startup_info->original_cwd
  8:  a2be40a22d1 =  8:  2855ed45112 stash: do not attempt to remove startup_info->original_cwd
  9:  834031be9e0 =  9:  23dfc3e399d dir: avoid incidentally removing the original_cwd in remove_path()
 10:  d5750fcb6d5 ! 10:  fe47c0f0c17 dir: new flag to remove_dir_recurse() to spare the original_cwd
     @@ t/t2501-cwd-empty.sh: test_submodule_removal () {
       	test_status=
       	test "$path_status" = dir && test_status=test_must_fail
       
     --	# Actually, while path_status == dir && test_status=test_must_fail
     +-	# Actually, while path_status=dir && test_status=test_must_fail
      -	# reflect our desired behavior, current behavior is:
      -	path_status=missing
      -	test_status=
 11:  21ff99a767c ! 11:  431dd651a7e t2501: simplify the tests since we can now assume desired behavior
     @@ t/t2501-cwd-empty.sh: test_incidental_dir_removal () {
       
      -		# Although we want pwd & git status to pass, test for existing
      -		# rather than desired behavior.
     --		if test "$works" == "success"
     +-		if test "$works" = "success"
      -		then
      -			test-tool getcwd &&
      -			git status --porcelain
     @@ t/t2501-cwd-empty.sh: test_incidental_dir_removal () {
       
      -	# Although we want dir to be present, test for existing rather
      -	# than desired behavior.
     --	if test "$works" == "success"
     +-	if test "$works" = "success"
      -	then
      -		test_path_is_dir foo
      -	else
     @@ t/t2501-cwd-empty.sh: test_incidental_dir_removal () {
      -		# was an untracked file there), and for the index and worktree
      -		# to be left clean with pwd and git status working afterwards.
      -		# But test for existing rather than desired behavior.
     --		if test "$works" == "success"
     +-		if test "$works" = "success"
      -		then
      -			test_must_fail "$@" 2>../error &&
      -			grep "Refusing to remove.*current working directory" ../error &&
     @@ t/t2501-cwd-empty.sh: test_incidental_dir_removal () {
       
      -	# Although we want dirORfile to be present, test for existing rather
      -	# than desired behavior.
     --	if test "$works" == "success"
     +-	if test "$works" = "success"
      -	then
      -		test_path_is_dir dirORfile
      -	else
     @@ t/t2501-cwd-empty.sh: test_incidental_untracked_dir_removal () {
       
      -		# Although we want pwd & git status to pass, test for existing
      -		# rather than desired behavior.
     --		if test "$works" == "success"
     +-		if test "$works" = "success"
      -		then
      -			test-tool getcwd &&
      -			git status --porcelain
     @@ t/t2501-cwd-empty.sh: test_incidental_untracked_dir_removal () {
       
      -	# Although we want dir to be present, test for existing rather
      -	# than desired behavior.
     --	if test "$works" == "success"
     +-	if test "$works" = "success"
      -	then
      -		test_path_is_dir untracked
      -	else

-- 
gitgitgadget