diff mbox series

[RFC,v2,10/12] clean: avoid removing untracked files in a nested git repository

Message ID 20190905154735.29784-11-newren@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fix some git clean issues | expand

Commit Message

Elijah Newren Sept. 5, 2019, 3:47 p.m. UTC 
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 is one somewhat related bug which this patch does not
fix, coming from the opposite angle.  If the user runs
   git clean -ffd
to force deletion of untracked nested repositories, and within an
untracked nested repo the user has ignored files (according to the inner
OR outer repositories' .gitignore), then not only will those ignored
files be left alone but the .git/ subdirectory of the nested repo will
be left alone too.  I am not completely sure if this should be
considered a bug (though it seems like it since the lack of the
untracked file would result in the .git/ subdirectory being deleted),
but in any event it is very minor compared to accidentally deleting user
data and I did not dive into it.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-clean.txt |  6 +++---
 builtin/clean.c             |  2 ++
 dir.c                       | 10 ++++++++++
 dir.h                       |  3 ++-
 t/t7300-clean.sh            | 10 +++++-----
 5 files changed, 22 insertions(+), 9 deletions(-)

Comments

SZEDER Gábor Sept. 5, 2019, 9:20 p.m. UTC | #1 
On Thu, Sep 05, 2019 at 08:47:33AM -0700, Elijah Newren wrote:
> 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.

> Finally, there is one somewhat related bug which this patch does not
> fix, coming from the opposite angle.  If the user runs
>    git clean -ffd
> to force deletion of untracked nested repositories, and within an
> untracked nested repo the user has ignored files (according to the inner
> OR outer repositories' .gitignore), then not only will those ignored
> files be left alone but the .git/ subdirectory of the nested repo will
> be left alone too.  I am not completely sure if this should be
> considered a bug (though it seems like it since the lack of the
> untracked file would result in the .git/ subdirectory being deleted),
> but in any event it is very minor compared to accidentally deleting user
> data and I did not dive into it.

We briefly mentioned this "ignored file in a nested repo fools 'git
clean -d'" issue in an unrelated thread as well, where Philip
suggested that the gitignore of the outer repository should not have
any effect on the nested repository.  I'm inclined to agree.

  https://public-inbox.org/git/e221aaf8-7d0b-6feb-3f58-1e9e4382939b@iee.email/

Now, 'git clean -X' is supposed to "Remove only files ignored by
Git.".  I'm not entirely sure what 'git clean -ffdX' is supposed to do
(or whether it makes any sense in the first place), but it does delete
files in the nested repository that are ignored only in the outer
repository, both tracked (and possibly dirty) and untracked, even with
this patch series.  Without this series '-fdX' is just as bad, but
with this patch (i.e. by not descending into nested repositories)
'-fdX' becomes sensible and leaves the nested repository alone.

> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
> index 3ab749b921..ba31d8d166 100644
> --- a/Documentation/git-clean.txt
> +++ b/Documentation/git-clean.txt
> @@ -37,9 +37,9 @@ OPTIONS
>  --force::
>  	If the Git configuration variable clean.requireForce is not set
>  	to false, 'git clean' will refuse to delete files or directories
> -	unless given -f or -i. Git will refuse to delete directories
> -	with .git sub directory or file unless a second -f
> -	is given.
> +	unless given -f or -i.  Git will refuse to modify untracked
> +	nested git repositories (directories with a .git subdirectory)
> +	unless a second -f is given.

I like this wording.
diff mbox series

Patch

diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 3ab749b921..ba31d8d166 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -37,9 +37,9 @@  OPTIONS
 --force::
 	If the Git configuration variable clean.requireForce is not set
 	to false, 'git clean' will refuse to delete files or directories
-	unless given -f or -i. Git will refuse to delete directories
-	with .git sub directory or file unless a second -f
-	is given.
+	unless given -f or -i.  Git will refuse to modify untracked
+	nested git repositories (directories with a .git subdirectory)
+	unless a second -f is given.
 
 -i::
 --interactive::
diff --git a/builtin/clean.c b/builtin/clean.c
index 68d70e41c0..3a7a63ae71 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -946,6 +946,8 @@  int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	if (force > 1)
 		rm_flags = 0;
+	else
+		dir.flags |= DIR_SKIP_NESTED_GIT;
 
 	dir.flags |= DIR_SHOW_OTHER_DIRECTORIES;
 
diff --git a/dir.c b/dir.c
index 3b2fe1701c..7ff79170fc 100644
--- a/dir.c
+++ b/dir.c
@@ -1451,6 +1451,16 @@  static enum path_treatment treat_directory(struct dir_struct *dir,
 		return path_none;
 
 	case index_nonexistent:
+		if (dir->flags & DIR_SKIP_NESTED_GIT) {
+			int nested_repo;
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_addstr(&sb, dirname);
+			nested_repo = is_nonbare_repository_dir(&sb);
+			strbuf_release(&sb);
+			if (nested_repo)
+				return path_none;
+		}
+
 		if (dir->flags & DIR_SHOW_OTHER_DIRECTORIES)
 			break;
 		if (exclude &&
diff --git a/dir.h b/dir.h
index 46c238ab49..739aea7c96 100644
--- a/dir.h
+++ b/dir.h
@@ -156,7 +156,8 @@  struct dir_struct {
 		DIR_SHOW_IGNORED_TOO = 1<<5,
 		DIR_COLLECT_KILLED_ONLY = 1<<6,
 		DIR_KEEP_UNTRACKED_CONTENTS = 1<<7,
-		DIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8
+		DIR_SHOW_IGNORED_TOO_MODE_MATCHING = 1<<8,
+		DIR_SKIP_NESTED_GIT = 1<<9
 	} flags;
 	struct dir_entry **entries;
 	struct dir_entry **ignored;
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 530dfdab34..6e6d24c1c3 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -549,7 +549,7 @@  test_expect_failure 'nested (non-empty) bare repositories should be cleaned even
 	test_path_is_missing strange_bare
 '
 
-test_expect_success 'giving path in nested git work tree will remove it' '
+test_expect_success 'giving path in nested git work tree will NOT remove it' '
 	rm -fr repo &&
 	mkdir repo &&
 	(
@@ -561,7 +561,7 @@  test_expect_success 'giving path in nested git work tree will remove it' '
 	git clean -f -d repo/bar/baz &&
 	test_path_is_file repo/.git/HEAD &&
 	test_path_is_dir repo/bar/ &&
-	test_path_is_missing repo/bar/baz
+	test_path_is_file repo/bar/baz/hello.world
 '
 
 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' '
 	test_path_is_dir untracked/
 '
 
-test_expect_success 'giving path to nested .git/ will remove contents' '
+test_expect_success 'giving path to nested .git/ will NOT remove contents' '
 	rm -fr repo untracked &&
 	mkdir repo untracked &&
 	(
@@ -589,7 +589,7 @@  test_expect_success 'giving path to nested .git/ will remove contents' '
 	) &&
 	git clean -f -d repo/.git/ &&
 	test_path_is_dir repo/.git &&
-	test_dir_is_empty repo/.git &&
+	test_path_is_file repo/.git/HEAD &&
 	test_path_is_dir untracked/
 '
 
@@ -671,7 +671,7 @@  test_expect_success 'git clean -d skips untracked dirs containing ignored files'
 	test_path_is_missing foo/b/bb
 '
 
-test_expect_failure 'git clean -d skips nested repo containing ignored files' '
+test_expect_success 'git clean -d skips nested repo containing ignored files' '
 	test_when_finished "rm -rf nested-repo-with-ignored-file" &&
 
 	git init nested-repo-with-ignored-file &&