dir: avoid incidentally removing the original_cwd in remove_path()

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:

    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.

Acked-by: Derrick Stolee <stolee@gmail.com>
Acked-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/dir.c b/dir.c
index 94489298f4c7614874bf59c325048e27b7289639..97d6b71c87255d5bcf37e4b0553f3e8ea8b6edda 100644 (file)
--- a/dir.c
+++ b/dir.c
@@ -3327,6 +3327,9 @@ int remove_path(const char *name)
                slash = dirs + (slash - name);
                do {
                        *slash = '\0';
+                       if (startup_info->original_cwd &&
+                           !strcmp(startup_info->original_cwd, dirs))
+                               break;
                } while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));
                free(dirs);
        }

diff --git a/dir.h b/dir.h
index 83f46c0fb4c4415c79d3a9fcdddbcbf372b35415..d6a5d03bec2e4dd21cf4788e9682b0e03fa4ef84 100644 (file)
--- a/dir.h
+++ b/dir.h
@@ -504,7 +504,11 @@ 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);

diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh
index be9ef903bd4de6e1b0feddd8bbb0d4e71dfbe0c8..ce2efb9d30a8b0bb4e29ea13cd53974debbdbc77 100755 (executable)
--- a/t/t2501-cwd-empty.sh
+++ b/t/t2501-cwd-empty.sh
@@ -182,12 +182,12 @@ test_expect_success 'revert fails if cwd needs to be removed' '
 '
 
 test_expect_success 'rm does not clean cwd incidentally' '
-       test_incidental_dir_removal failure git rm bar/baz.t
+       test_incidental_dir_removal success git rm bar/baz.t
 '
 
 test_expect_success 'apply does not remove cwd incidentally' '
        git diff HEAD HEAD~1 >patch &&
-       test_incidental_dir_removal failure git apply ../patch
+       test_incidental_dir_removal success git apply ../patch
 '
 
 test_incidental_untracked_dir_removal () {
@@ -271,12 +271,8 @@ test_expect_success '`rm -rf dir` even with only tracked files will remove somet
        ) &&
 
        test_path_is_missing a/b/c/tracked &&
-       ## We would prefer if a/b was still present, though empty, since it
-       ## was the current working directory
-       #test_path_is_dir a/b
-       ## But the current behavior is that it not only deletes the directory
-       ## a/b as requested, but also goes and deletes a
-       test_path_is_missing a
+       test_path_is_missing a/b/c &&
+       test_path_is_dir a/b
 '
 
 test_expect_success 'git version continues working from a deleted dir' '