clean: avoid removing untracked files in a nested git repository

Users expect files in a nested git repository to be left alone unless sufficiently forced (with two -f's). Unfortunately, in certain circumstances, git would delete both tracked (and possibly dirty) files and untracked files within a nested repository. To explain how this happens, let's contrast a couple cases. First, take the following example setup (which assumes we are already within a git repo): git init nested cd nested >tracked git add tracked git commit -m init >untracked cd .. In this setup, everything works as expected; running 'git clean -fd' will result in fill_directory() returning the following paths: nested/ nested/tracked nested/untracked and then correct_untracked_entries() would notice this can be compressed to nested/ and then since "nested/" is a directory, we would call remove_dirs("nested/", ...), which would check is_nonbare_repository_dir() and then decide to skip it. However, if someone also creates an ignored file: >nested/ignored then running 'git clean -fd' would result in fill_directory() returning the same paths: nested/ nested/tracked nested/untracked but correct_untracked_entries() will notice that we had ignored entries under nested/ and thus simplify this list to nested/tracked nested/untracked Since these are not directories, we do not call remove_dirs() which was the only place that had the is_nonbare_repository_dir() safety check -- resulting in us deleting both the untracked file and the tracked (and possibly dirty) file. One possible fix for this issue would be walking the parent directories of each path and checking if they represent nonbare repositories, but that would be wasteful. Even if we added caching of some sort, it's still a waste because we should have been able to check that "nested/" represented a nonbare repository before even descending into it in the first place. Add a DIR_SKIP_NESTED_GIT flag to dir_struct.flags and use it to prevent fill_directory() and friends from descending into nested git repos. With this change, we also modify two regression tests added in commit 91479b9c72f1 ("t7300: add tests to document behavior of clean and nested git", 2015-06-15). That commit, nor its series, nor the six previous iterations of that series on the mailing list discussed why those tests coded the expectation they did. In fact, it appears their purpose was simply to test _existing_ behavior to make sure that the performance changes didn't change the behavior. However, these two tests directly contradicted the manpage's claims that two -f's were required to delete files/directories under a nested git repository. While one could argue that the user gave an explicit path which matched files/directories that were within a nested repository, there's a slippery slope that becomes very difficult for users to understand once you go down that route (e.g. what if they specified "git clean -f -d '*.c'"?) It would also be hard to explain what the exact behavior was; avoid such problems by making it really simple. Also, clean up some grammar errors describing this functionality in the git-clean manpage. Finally, there are still a couple bugs with -ffd not cleaning out enough (e.g. missing the nested .git) and with -ffdX possibly cleaning out the wrong files (paying attention to outer .gitignore instead of inner). This patch does not address these cases at all (and does not change the behavior relative to those flags), it only fixes the handling when given a single -f. See https://public-inbox.org/git/20190905212043.GC32087@szeder.dev/ for more discussion of the -ffd[X?] bugs. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Sep 17, 2019 at 09:35 UTC 09487f2cbad36d5da42e204c9e0d201e8607acb8
5 files changed +22 -9
Documentation/git-clean.txt
+3 -3
@@ -37,9 +37,9 @@ OPTIONS
37 --force::
38 If the Git configuration variable clean.requireForce is not set
39 to false, 'git clean' will refuse to delete files or directories
40 - unless given -f or -i. Git will refuse to delete directories
41 - with .git sub directory or file unless a second -f
42 - is given.
40 + unless given -f or -i. Git will refuse to modify untracked
41 + nested git repositories (directories with a .git subdirectory)
42 + unless a second -f is given.
43
44 -i::
45 --interactive::
builtin/clean.c
+2
@@ -946,6 +946,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
946
947 if (force > 1)
948 rm_flags = 0;
949 + else
950 + dir.flags |= DIR_SKIP_NESTED_GIT;
951
952 dir.flags |= DIR_SHOW_OTHER_DIRECTORIES;
953
dir.c
+10
@@ -1451,6 +1451,16 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
1451 return path_none;
1452
1453 case index_nonexistent:
1454 + if (dir->flags & DIR_SKIP_NESTED_GIT) {
1455 + int nested_repo;
1456 + struct strbuf sb = STRBUF_INIT;
1457 + strbuf_addstr(&sb, dirname);
1458 + nested_repo = is_nonbare_repository_dir(&sb);
1459 + strbuf_release(&sb);
1460 + if (nested_repo)
1461 + return path_none;
1462 + }
1463 +
1464 if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
1465 break;
1466 if (exclude &&
dir.h
+2 -1
@@ -156,7 +156,8 @@ struct dir_struct {
156 DIR_SHOW_IGNORED_TOO = 1<<5,
157 DIR_COLLECT_KILLED_ONLY = 1<<6,
158 DIR_KEEP_UNTRACKED_CONTENTS = 1<<7,
159 - DIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8
159 + DIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8,
160 + DIR_SKIP_NESTED_GIT = 1<<9
161 } flags;
162 struct dir_entry **entries;
163 struct dir_entry **ignored;
t/t7300-clean.sh
+5 -5
@@ -549,7 +549,7 @@ test_expect_failure 'nested (non-empty) bare repositories should be cleaned even
549 test_path_is_missing strange_bare
550 '
551
552 -test_expect_success 'giving path in nested git work tree will remove it' '
552 +test_expect_success 'giving path in nested git work tree will NOT remove it' '
553 rm -fr repo &&
554 mkdir repo &&
555 (
@@ -561,7 +561,7 @@ test_expect_success 'giving path in nested git work tree will remove it' '
561 git clean -f -d repo/bar/baz &&
562 test_path_is_file repo/.git/HEAD &&
563 test_path_is_dir repo/bar/ &&
564 - test_path_is_missing repo/bar/baz
564 + test_path_is_file repo/bar/baz/hello.world
565 '
566
567 test_expect_success 'giving path to nested .git will not remove it' '
@@ -579,7 +579,7 @@ test_expect_success 'giving path to nested .git will not remove it' '
579 test_path_is_dir untracked/
580 '
581
582 -test_expect_success 'giving path to nested .git/ will remove contents' '
582 +test_expect_success 'giving path to nested .git/ will NOT remove contents' '
583 rm -fr repo untracked &&
584 mkdir repo untracked &&
585 (
@@ -589,7 +589,7 @@ test_expect_success 'giving path to nested .git/ will remove contents' '
589 ) &&
590 git clean -f -d repo/.git/ &&
591 test_path_is_dir repo/.git &&
592 - test_dir_is_empty repo/.git &&
592 + test_path_is_file repo/.git/HEAD &&
593 test_path_is_dir untracked/
594 '
595
@@ -671,7 +671,7 @@ test_expect_success 'git clean -d skips untracked dirs containing ignored files'
671 test_path_is_missing foo/b/bb
672 '
673
674 -test_expect_failure 'git clean -d skips nested repo containing ignored files' '
674 +test_expect_success 'git clean -d skips nested repo containing ignored files' '
675 test_when_finished "rm -rf nested-repo-with-ignored-file" &&
676
677 git init nested-repo-with-ignored-file &&