Accidentially deleted directory, bug in git clean -d?

I accidentially deleted a directory using git clean. I would think this is a bug, but I'm not sure. Was using 1.8.1, but upgraded to 1.9.0 just to see if it was still reproducable, and it was.

Here's a minimal way to reproduce:

$ git init $ mkdir foo foobar $ git clean -df foobar Removing foo/ Removing foobar/ $ ls $

I expected only "foobar" to be deleted, but "foo" was also deleted.

The same thing happens in the opposite case:

$ git init $ mkdir foo foobar $ git clean -df foo Removing foo/ Removing foobar/ $ ls $

However, it only happens when there is a common prefix in the names:

$ git init $ mkdir foo bar $ git clean -df foo Removing foo/ $ ls bar $

In this case, "bar" was not deleted.

git-clean uses read_directory to fill in a `struct dir` with potential hits. However, read_directory does not actually check against our pathspec. It uses a simplified version that may turn up false positives. As a result, we need to check that any hits match our pathspec. We do so reliably for non-directories. For directories, if "-d" is not given we check that the pathspec matched exactly (i.e., we are even stricter, and require an explicit "git clean foo" to clean "foo/"). But if "-d" is given, rather than relaxing the exact match to allow a recursive match, we do not check the pathspec at all.

This regression was introduced in 113f10f (Make git-clean a builtin, 2007-11-11).

Signed-off-by: Jeff King <peff@peff.net>
---
On Mon, Mar 10, 2014 at 11:31:37AM +0100, Robin Pedersen wrote:

Definitely a bug, and it dates back quite a while. Thanks for a very clear bug report.

-- >8 --
 builtin/clean.c  | 5 +++--
 t/t7300-clean.sh | 8 ++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

On Mon, Mar 10, 2014 at 01:20:02PM -0400, Jeff King wrote:

Whoops, accidentally included a scissors line here that will break people using "git am --scissors" to pick up the patch. Here it is correctly formatted.

-- >8 --
Subject: clean: respect pathspecs with "-d"

git-clean uses read_directory to fill in a `struct dir` with potential hits. However, read_directory does not actually check against our pathspec. It uses a simplified version that may turn up false positives. As a result, we need to check that any hits match our pathspec. We do so reliably for non-directories. For directories, if "-d" is not given we check that the pathspec matched exactly (i.e., we are even stricter, and require an explicit "git clean foo" to clean "foo/"). But if "-d" is given, rather than relaxing the exact match to allow a recursive match, we do not check the pathspec at all.

This regression was introduced in 113f10f (Make git-clean a builtin, 2007-11-11).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clean.c  | 5 +++--
 t/t7300-clean.sh | 8 ++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

On Mon, Mar 10, 2014 at 01:22:15PM -0400, Jeff King wrote:

I think we should also test removing foo, which was also in the original report, to make sure we don't match prefixes, e.g.:

test_expect_success 'git clean -d respects pathspecs' '
	mkdir foo &&
	mkdir foobar &&
	git clean -df foo &&
	test_path_is_missing foo &&
	test_path_is_dir foobar
'

Regards Simon

On Mon, Mar 10, 2014 at 09:02:35PM +0100, Simon Ruderich wrote:

Yeah, it probably makes sense to test both ways (though the root cause and fix are the same). Those mkdirs need to be "mkdir -p", though.

Here's an updated patch with your suggestion:

-- >8 --
Subject: clean: respect pathspecs with "-d"

git-clean uses read_directory to fill in a `struct dir` with potential hits. However, read_directory does not actually check against our pathspec. It uses a simplified version that may turn up false positives. As a result, we need to check that any hits match our pathspec. We do so reliably for non-directories. For directories, if "-d" is not given we check that the pathspec matched exactly (i.e., we are even stricter, and require an explicit "git clean foo" to clean "foo/"). But if "-d" is given, rather than relaxing the exact match to allow a recursive match, we do not check the pathspec at all.

This regression was introduced in 113f10f (Make git-clean a builtin, 2007-11-11).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clean.c  |  5 +++--
 t/t7300-clean.sh | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

On Mon, Mar 10, 2014 at 01:20:02PM -0400, Jeff King wrote:

The code has been cleaned up quite a bit from that original version, and it was pretty easy to see the discrepancy between the two code paths. However, if the code were structured like the cleanup patch below, I think it would have been even easier.

This comes on top of my other patch. So the bug is already fixed, but I think the end result is more readable.

-- >8 -- When we get a list of paths from read_directory, we further prune it to create the final list of items to remove. The code paths for directories and non-directories repeat the same "add to list" code.

This patch restructures the code so that we don't repeat ourselves. Also, by following a "if (condition) continue" pattern like the pathspec check above, it makes it more obvious that the conditional is about excluding directories under certain circumstances.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clean.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)