@matheusbsbs / matheus-bagliano / commits / 63bbe8beb7

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>

Elijah Newren committed Dec 9, 2021 at 05:08 UTC 63bbe8beb78ee8af5a7faeee4be747a82d8e2dc7

3 files changed +12 -9

dir.c

@@ -3327,6 +3327,9 @@ int remove_path(const char *name)

3327 slash = dirs + (slash - name);

3328 do {

3329 *slash = '\0';

3330 + if (startup_info->original_cwd &&

3331 + !strcmp(startup_info->original_cwd, dirs))

3332 + break;

3333 } while (rmdir(dirs) == 0 && (slash = strrchr(dirs, '/')));

3334 free(dirs);

3335 }

dir.h

+5 -1

@@ -504,7 +504,11 @@ int get_sparse_checkout_patterns(struct pattern_list *pl);

504 */

505 int remove_dir_recursively(struct strbuf *path, int flag);

506

507 -/* tries to remove the path with empty directories along it, ignores ENOENT */

507 +/*

508 + * Tries to remove the path, along with leading empty directories so long as

509 + * those empty directories are not startup_info->original_cwd. Ignores

510 + * ENOENT.

511 + */

512 int remove_path(const char *path);

513

514 int fspathcmp(const char *a, const char *b);

t/t2501-cwd-empty.sh

+4 -8

@@ -182,12 +182,12 @@ test_expect_success 'revert fails if cwd needs to be removed' '

182 '

183

184 test_expect_success 'rm does not clean cwd incidentally' '

185 - test_incidental_dir_removal failure git rm bar/baz.t

185 + test_incidental_dir_removal success git rm bar/baz.t

186 '

187

188 test_expect_success 'apply does not remove cwd incidentally' '

189 git diff HEAD HEAD~1 >patch &&

190 - test_incidental_dir_removal failure git apply ../patch

190 + test_incidental_dir_removal success git apply ../patch

191 '

192

193 test_incidental_untracked_dir_removal () {

@@ -271,12 +271,8 @@ test_expect_success '`rm -rf dir` even with only tracked files will remove somet

271 ) &&

272

273 test_path_is_missing a/b/c/tracked &&

274 - ## We would prefer if a/b was still present, though empty, since it

275 - ## was the current working directory

276 - #test_path_is_dir a/b

277 - ## But the current behavior is that it not only deletes the directory

278 - ## a/b as requested, but also goes and deletes a

279 - test_path_is_missing a

274 + test_path_is_missing a/b/c &&

275 + test_path_is_dir a/b

276 '

277

278 test_expect_success 'git version continues working from a deleted dir' '