From 63bbe8beb78ee8af5a7faeee4be747a82d8e2dc7 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 9 Dec 2021 05:08:33 +0000 Subject: dir: avoid incidentally removing the original_cwd in remove_path() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Acked-by: Ævar Arnfjörð Bjarmason Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- dir.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'dir.h') diff --git a/dir.h b/dir.h index 83f46c0fb4..d6a5d03bec 100644 --- 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); -- cgit v1.3 From 580a5d7f75fc7b6c4c369ef429742d9d417acddd Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 9 Dec 2021 05:08:34 +0000 Subject: dir: new flag to remove_dir_recurse() to spare the original_cwd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remove_dir_recurse(), and its non-static wrapper called remove_dir_recursively(), both take flags for modifying its behavior. As with the previous commits, we would generally like to protect the original_cwd, but we want to forced user commands (e.g. 'git rm -rf ...') or other special cases to remove it. Add a flag for this purpose. After reading through every caller of remove_dir_recursively() in the current codebase, there was only one that should be adjusted and that one only in a very unusual circumstance. Add a pair of new testcases to highlight that very specific case involving submodules && --git-dir && --work-tree. Acked-by: Derrick Stolee Acked-by: Ævar Arnfjörð Bjarmason Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/rm.c | 3 ++- dir.c | 12 +++++++++--- dir.h | 3 +++ t/t2501-cwd-empty.sh | 5 ----- 4 files changed, 14 insertions(+), 9 deletions(-) (limited to 'dir.h') diff --git a/builtin/rm.c b/builtin/rm.c index 3d0967cdc1..b4132e5d8e 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -399,12 +399,13 @@ int cmd_rm(int argc, const char **argv, const char *prefix) if (!index_only) { int removed = 0, gitmodules_modified = 0; struct strbuf buf = STRBUF_INIT; + int flag = force ? REMOVE_DIR_PURGE_ORIGINAL_CWD : 0; for (i = 0; i < list.nr; i++) { const char *path = list.entry[i].name; if (list.entry[i].is_submodule) { strbuf_reset(&buf); strbuf_addstr(&buf, path); - if (remove_dir_recursively(&buf, 0)) + if (remove_dir_recursively(&buf, flag)) die(_("could not remove '%s'"), path); removed = 1; diff --git a/dir.c b/dir.c index 97d6b71c87..52064345a6 100644 --- a/dir.c +++ b/dir.c @@ -3204,6 +3204,7 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up) int ret = 0, original_len = path->len, len, kept_down = 0; int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY); int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL); + int purge_original_cwd = (flag & REMOVE_DIR_PURGE_ORIGINAL_CWD); struct object_id submodule_head; if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) && @@ -3259,9 +3260,14 @@ 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 (!purge_original_cwd && + startup_info->original_cwd && + !strcmp(startup_info->original_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. diff --git a/dir.h b/dir.h index d6a5d03bec..8e02dfb505 100644 --- a/dir.h +++ b/dir.h @@ -495,6 +495,9 @@ int get_sparse_checkout_patterns(struct pattern_list *pl); /* Remove the contents of path, but leave path itself. */ #define REMOVE_DIR_KEEP_TOPLEVEL 04 +/* Remove the_original_cwd too */ +#define REMOVE_DIR_PURGE_ORIGINAL_CWD 0x08 + /* * Remove path and its contents, recursively. flags is a combination * of the above REMOVE_DIR_* constants. Return 0 on success. diff --git a/t/t2501-cwd-empty.sh b/t/t2501-cwd-empty.sh index ce2efb9d30..bc92230f2f 100755 --- a/t/t2501-cwd-empty.sh +++ b/t/t2501-cwd-empty.sh @@ -291,11 +291,6 @@ test_submodule_removal () { test_status= test "$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= - test_when_finished "git reset --hard HEAD~1" && test_when_finished "rm -rf .git/modules/my_submodule" && -- cgit v1.3