[BUG] "git clean" does not pay attention to its parameters

[BUG] "git clean" does not pay attention to its parameters

[Nanako Shiraishi]

In a repository with LaTeX documents, I tried to see what *.aux files are left behind after formatting, by running "git clean -n" with the latest git (1.5.3.7-1005-gdada0c1):

  % git clean -n '*.aux'

This however showed more than just '*.aux' files.  With the released version 1.5.3.6, the output is correctly limited to the files that match the pattern.

----------------------------------------------------------------------
Get a free email account with anti spam protection.
http://www.bluebottle.com/tag/2

Re:* [BUG] "git clean" does not pay attention to its parameters

[Junio C Hamano]

Nanako Shiraishi <nanako3@bluebottle.com> writes:

> In a repository with LaTeX documents, I tried to see what *.aux files are left behind after formatting, by running "git clean -n" with the latest git (1.5.3.7-1005-gdada0c1):
>
>   % git clean -n '*.aux'
>
> This however showed more than just '*.aux' files.  With the released version 1.5.3.6, the output is correctly limited to the files that match the pattern.

Yuck.  People actually use git-clean?

But thanks for reporting.

Comparing the corresponding part from builtin-ls-files.c and what
builtin-clean.c does, it does look broken.

Does this patch help?  I am not sure why the directory side of the code
is written that way, but I have a suspicion that "was a directory
explicitly given as one of the pathspec" check is also bogus, although I
did not touch that part.

-- >8 --
[PATCH] git-clean: Honor pathspec.

git-clean "*.rej" should attempt to look at only paths that match
pattern "*.rej", but rewrite to C broke it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-clean.c |   31 ++++++++++++++++++++-----------
 1 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/builtin-clean.c b/builtin-clean.c
index 56ae4eb..7dd901e 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -27,13 +27,14 @@ static int git_clean_config(const char *var, const char *value)
 
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
-	int j;
+	int i;
 	int show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;
 	int ignored_only = 0, baselen = 0, config_set = 0;
 	struct strbuf directory;
 	struct dir_struct dir;
 	const char *path, *base;
 	static const char **pathspec;
+	char *seen = NULL;
 	struct option options[] = {
 		OPT__QUIET(&quiet),
 		OPT__DRY_RUN(&show_only),
@@ -85,12 +86,17 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	read_directory(&dir, path, base, baselen, pathspec);
 	strbuf_init(&directory, 0);
 
-	for (j = 0; j < dir.nr; ++j) {
-		struct dir_entry *ent = dir.entries[j];
-		int len, pos, specs;
+	if (pathspec) {
+		for (i = 0; pathspec[i]; i++)
+			; /* nothing */
+		seen = xmalloc();
+	}
+
+	for (i = 0; i < dir.nr; i++) {
+		struct dir_entry *ent = dir.entries[i];
+		int len, pos;
 		struct cache_entry *ce;
 		struct stat st;
-		char *seen;
 
 		/*
 		 * Remove the '/' at the end that directory
@@ -114,15 +120,13 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			int matched_path = 0;
 			strbuf_addstr(&directory, ent->name);
 			if (pathspec) {
-				for (specs =0; pathspec[specs]; ++specs)
-					/* nothing */;
-				seen = xcalloc(specs, 1);
-				/* Check if directory was explictly passed as
-				 * pathspec.  If so we want to remove it */
+				/*
+				 * Check if directory was explictly passed as
+				 * pathspec. If so we want to remove it.
+				 */
 				if (match_pathspec(pathspec, ent->name, ent->len,
 						   baselen, seen))
 					matched_path = 1;
-				free(seen);
 			}
 			if (show_only && (remove_directories || matched_path)) {
 				printf("Would remove %s\n", directory.buf);
@@ -138,6 +142,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			}
 			strbuf_reset(&directory);
 		} else {
+			if (pathspec &&
+			    !match_pathspec(pathspec, ent->name, ent->len,
+					    baselen, seen))
+				continue; /* excluded */
 			if (show_only) {
 				printf("Would remove %s\n", ent->name);
 				continue;
@@ -147,6 +155,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 			unlink(ent->name);
 		}
 	}
+	free(seen);
 
 	strbuf_release(&directory);
 	return 0;
-- 
1.5.3.7-2115-geb804

Re:* [BUG] "git clean" does not pay attention to its parameters

[Johannes Schindelin]

Hi,

On Tue, 4 Dec 2007, Junio C Hamano wrote:

> @@ -85,12 +86,17 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	read_directory(&dir, path, base, baselen, pathspec);
>  	strbuf_init(&directory, 0);
>  
> -	for (j = 0; j < dir.nr; ++j) {
> -		struct dir_entry *ent = dir.entries[j];
> -		int len, pos, specs;
> +	if (pathspec) {
> +		for (i = 0; pathspec[i]; i++)
> +			; /* nothing */
> +		seen = xmalloc();

Did you mean xmalloc(i)?

Ciao,
Dscho

Re: * [BUG] "git clean" does not pay attention to its parameters

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Tue, 4 Dec 2007, Junio C Hamano wrote:
>
>> @@ -85,12 +86,17 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>>  	read_directory(&dir, path, base, baselen, pathspec);
>>  	strbuf_init(&directory, 0);
>>  
>> -	for (j = 0; j < dir.nr; ++j) {
>> -		struct dir_entry *ent = dir.entries[j];
>> -		int len, pos, specs;
>> +	if (pathspec) {
>> +		for (i = 0; pathspec[i]; i++)
>> +			; /* nothing */
>> +		seen = xmalloc();
>
> Did you mean xmalloc(i)?

Yes.

Re: * [BUG] "git clean" does not pay attention to its parameters

[Sam Ravnborg]

On Tue, Dec 04, 2007 at 11:55:41PM -0800, Junio C Hamano wrote:
> Nanako Shiraishi <nanako3@bluebottle.com> writes:
> 
> > In a repository with LaTeX documents, I tried to see what *.aux files are left behind after formatting, by running "git clean -n" with the latest git (1.5.3.7-1005-gdada0c1):
> >
> >   % git clean -n '*.aux'
> >
> > This however showed more than just '*.aux' files.  With the released version 1.5.3.6, the output is correctly limited to the files that match the pattern.
> 
> Yuck.  People actually use git-clean?

git clean -d -x is my friend.
It it a great way to delete all my temporary files named 'kurt', 'fisk', 'viggo', 'x*' etc.

	Sam

Re: * [BUG] "git clean" does not pay attention to its parameters

[Shawn Bohrer]

On Tue, Dec 04, 2007 at 11:55:41PM -0800, Junio C Hamano wrote:
> Does this patch help?  I am not sure why the directory side of the code
> is written that way, but I have a suspicion that "was a directory
> explicitly given as one of the pathspec" check is also bogus, although I
> did not touch that part.

Before the rewrite in C git clean would refuse to remove a directory if
you said:

   git clean dir

without using the -d parameter.  Per your suggestion this check causes
git clean to remove the directory anyway since you explicitly asked it
to.

Re: * [BUG] "git clean" does not pay attention to its parameters

[Jeff King]

On Tue, Dec 04, 2007 at 11:55:41PM -0800, Junio C Hamano wrote:

> Yuck.  People actually use git-clean?

I use it all the time (though never with arguments, or I probably would
have noticed this bug). I think it is coupled with the "use git status
to see what is going on" workflow that, IIRC, you don't use.

Also, nit: don't you mean "git clean"? :)

-Peff

Re: * [BUG] "git clean" does not pay attention to its parameters

[Junio C Hamano]

Shawn Bohrer <shawn.bohrer@gmail.com> writes:

> Before the rewrite in C git clean would refuse to remove a directory if
> you said:
>
>    git clean dir
>
> without using the -d parameter.  Per your suggestion this check causes
> git clean to remove the directory anyway since you explicitly asked it
> to.

Thanks for reminding me of that issue.

Re: * [BUG] "git clean" does not pay attention to its parameters

[Jeff King]

On Tue, Dec 04, 2007 at 11:55:41PM -0800, Junio C Hamano wrote:

> [PATCH] git-clean: Honor pathspec.
> 
> git-clean "*.rej" should attempt to look at only paths that match
> pattern "*.rej", but rewrite to C broke it.

And here is a test that fails without your patch (probably the commit
message should say "fixed in XX" once the commit id is known, or it
should be squashed in with your patch).

-- >8 --
t7300: add test for clean with wildcard pathspec

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7300-clean.sh |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index f013c17..dfd1188 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -126,6 +126,20 @@ test_expect_success 'git-clean symbolic link' '
 
 '
 
+test_expect_success 'git-clean with wildcard' '
+
+	touch a.clean b.clean other.c &&
+	git-clean "*.clean" &&
+	test -f Makefile &&
+	test -f README &&
+	test -f src/part1.c &&
+	test -f src/part2.c &&
+	test ! -f a.clean &&
+	test ! -f b.clean &&
+	test -f other.c
+
+'
+
 test_expect_success 'git-clean -n' '
 
 	mkdir -p build docs &&
-- 
1.5.3.7.2099.gd6d7-dirty