git clean removes directories when not asked to

git clean removes directories when not asked to

[Joachim B Haga]

This is with debian packaged 1.5.4.4.

When invoked from a subdirectory, git clean removes more than it
should. According to the documentation, it should not remove
directories unless "-d" is given. However:


pep ~/src/test 0$ git init
Initialized empty Git repository in .git/
pep ~/src/test|master 0$ mkdir dir
pep ~/src/test|master 0$ mkdir dir/subdir
pep ~/src/test|master 0$ git clean -f
Not removing dir/
pep ~/src/test|master 0$ cd dir
pep ~/src/test/dir|master 0$ git clean -f
Removing subdir/
pep ~/src/test/dir|master 0$ ls subdir
ls: cannot access subdir: No such file or directory


Luckily I just lost some compilation results in this case, but this is
unexpected and dangerous behaviour.


(Additionally, I find the "-f" slightly annoying but that's not an issue here.)


-j

Re: git clean removes directories when not asked to

[Joachim B Haga]

Joachim B Haga <jobh@broadpark.no> writes:

> This is with debian packaged 1.5.4.4.
>
> When invoked from a subdirectory, git clean removes more than it
> should. According to the documentation, it should not remove
> directories unless "-d" is given. However:

I see the same behaviour with 1.5.5, just pulled.

-j.

[PATCH] Re: git clean removes directories when not asked to

[Joachim B Haga]

Joachim B Haga <jobh@broadpark.no> writes:

> Joachim B Haga <jobh@broadpark.no> writes:
>
>> When invoked from a subdirectory, git clean removes more than it
>> should. According to the documentation, it should not remove
>> directories unless "-d" is given. However:

I have tried to fix this, but I don't know the code. The previous logic was
obviously (?) broken, as it had this (paraphrased):

if (remove_directories || matches)
	remove_dir_recursively(...);

which should have been &&. But with only this change, top-level directories
were not removed even if "-d" was given. Looking at the (!ISDIR) branch, I
guessed that it should instead trigger if pathspec is NULL; i.e, generally
treat (!pathspec) as a match. It looks like the behaviour is correct now, but
somebody who knows this code should check my guesses.

-j.




>From 73647e7bb73b6037b9d14535ec027da8ee7d6091 Mon Sep 17 00:00:00 2001
From: Joachim B Haga <jobh@broadpark.no>
Date: Wed, 9 Apr 2008 18:49:34 +0200
Subject: [PATCH] Stop builtin-clean from removing directories unless "-d" is given.

---
 builtin-clean.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/builtin-clean.c b/builtin-clean.c
index fefec30..15201d5 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -130,29 +130,32 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
                        matches = match_pathspec(pathspec, ent->name, ent->len,
                                                 baselen, seen);
                } else {
-                       matches = 0;
+                       matches = 1;
                }
 
                if (S_ISDIR(st.st_mode)) {
                        strbuf_addstr(&directory, ent->name);
                        qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
-                       if (show_only && (remove_directories || matches)) {
-                               printf("Would remove %s\n", qname);
-                       } else if (remove_directories || matches) {
-                               if (!quiet)
-                                       printf("Removing %s\n", qname);
-                               if (remove_dir_recursively(&directory, 0) != 0) {
-                                       warning("failed to remove '%s'", qname);
-                                       errors++;
+                       if (remove_directories && matches) {
+                               if (show_only)
+                                       printf("Would remove %s\n", qname);
+                               else {
+                                       if (!quiet)
+                                               printf("Removing %s\n", qname);
+                                       if (remove_dir_recursively(&directory, 0) != 0) {
+                                               warning("failed to remove '%s'", qname);
+                                               errors++;
+                                       }
                                }
-                       } else if (show_only) {
-                               printf("Would not remove %s\n", qname);
                        } else {
-                               printf("Not removing %s\n", qname);
+                               if (show_only)
+                                       printf("Would not remove %s\n", qname);
+                               else
+                                       printf("Not removing %s\n", qname);
                        }
                        strbuf_reset(&directory);
                } else {
-                       if (pathspec && !matches)
+                       if (!matches)
                                continue;
                        qname = quote_path_relative(ent->name, -1, &buf, prefix);
                        if (show_only) {
-- 
1.5.4.4

[PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Shawn Bohrer]

When git clean is run from a subdirectory it should follow the normal
policy and only remove directories if they are passed in as a pathspec,
or -d is specified.

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---


On Wed, Apr 09, 2008 at 07:04:15PM +0200, Joachim B Haga wrote:
> Joachim B Haga <jobh@broadpark.no> writes:
> I have tried to fix this, but I don't know the code. The previous logic was
> obviously (?) broken, as it had this (paraphrased):
>
> if (remove_directories || matches)
>       remove_dir_recursively(...);

git clean will remove directories if you specify -d (setting the
remove_directories flag), or if you explicitly passed the directory in
as a pathspec.  For example:

mkdir foo
git clean foo

After looking at the code a bit I think the real problem is with
match_pathspec, or at least with how git clean uses match_pathspec.
How does the following patch look?



 builtin-clean.c |   11 +++++------
 dir.c           |    2 +-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/builtin-clean.c b/builtin-clean.c
index fefec30..5c5ec98 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -95,7 +95,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < dir.nr; i++) {
 		struct dir_entry *ent = dir.entries[i];
-		int len, pos, matches;
+		int len, pos;
+		int matches = 0;
 		struct cache_entry *ce;
 		struct stat st;
 
@@ -127,18 +128,16 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 		if (pathspec) {
 			memset(seen, 0, argc > 0 ? argc : 1);
-			matches = match_pathspec(pathspec, ent->name, ent->len,
+			matches = match_pathspec(pathspec, ent->name, len,
 						 baselen, seen);
-		} else {
-			matches = 0;
 		}
 
 		if (S_ISDIR(st.st_mode)) {
 			strbuf_addstr(&directory, ent->name);
 			qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
-			if (show_only && (remove_directories || matches)) {
+			if (show_only && (remove_directories || (matches >= 2))) {
 				printf("Would remove %s\n", qname);
-			} else if (remove_directories || matches) {
+			} else if (remove_directories || (matches >= 2)) {
 				if (!quiet)
 					printf("Removing %s\n", qname);
 				if (remove_dir_recursively(&directory, 0) != 0) {
diff --git a/dir.c b/dir.c
index b5bfbca..63715c9 100644
--- a/dir.c
+++ b/dir.c
@@ -80,7 +80,7 @@ static int match_one(const char *match, const char *name, int namelen)
 	if (strncmp(match, name, matchlen))
 		return !fnmatch(match, name, 0) ? MATCHED_FNMATCH : 0;
 
-	if (!name[matchlen])
+	if (namelen == matchlen)
 		return MATCHED_EXACTLY;
 	if (match[matchlen-1] == '/' || name[matchlen] == '/')
 		return MATCHED_RECURSIVELY;
-- 
1.5.5.106.g42c8b

[PATCH] git clean: Add test to verify directories aren't removed with a prefix

[Shawn Bohrer]

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 t/t7300-clean.sh |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index afccfc9..a50492f 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -75,8 +75,8 @@ test_expect_success 'git-clean src/ src/' '
 
 test_expect_success 'git-clean with prefix' '
 
-	mkdir -p build docs &&
-	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
+	mkdir -p build docs src/test &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so src/test/1.c &&
 	(cd src/ && git-clean) &&
 	test -f Makefile &&
 	test -f README &&
@@ -84,6 +84,7 @@ test_expect_success 'git-clean with prefix' '
 	test -f src/part2.c &&
 	test -f a.out &&
 	test ! -f src/part3.c &&
+	test -f src/test/1.c &&
 	test -f docs/manual.txt &&
 	test -f obj.o &&
 	test -f build/lib.so
-- 
1.5.5.106.g42c8b

Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Joachim Berdal Haga]

Shawn Bohrer wrote:
> When git clean is run from a subdirectory it should follow the normal
> policy and only remove directories if they are passed in as a pathspec,
> or -d is specified.
> 
> Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>

I have tested this version and it fixes my problem/testcase. Thanks,

-j.

Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Junio C Hamano]

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

> diff --git a/builtin-clean.c b/builtin-clean.c
> index fefec30..5c5ec98 100644
> --- a/builtin-clean.c
> +++ b/builtin-clean.c
> @@ -95,7 +95,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  
>  	for (i = 0; i < dir.nr; i++) {
>  		struct dir_entry *ent = dir.entries[i];
> -		int len, pos, matches;
> +		int len, pos;
> +		int matches = 0;
>  		struct cache_entry *ce;
>  		struct stat st;

Initialization of "matches" seems to be an independent clean-up.  Although
it forces the initialization in the codepath that do not need the value of
matches, that is not a big deal --- right?

> @@ -127,18 +128,16 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  
>  		if (pathspec) {
>  			memset(seen, 0, argc > 0 ? argc : 1);
> -			matches = match_pathspec(pathspec, ent->name, ent->len,
> +			matches = match_pathspec(pathspec, ent->name, len,
>  						 baselen, seen);
> -		} else {
> -			matches = 0;
>  		}

And the essential change (fix) is to send len which could be shorter than
ent->len because we have stripped '/' here, plus the one in match_one()
that now allows name[] that is not NUL terminated.

>  		if (S_ISDIR(st.st_mode)) {
>  			strbuf_addstr(&directory, ent->name);
>  			qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
> -			if (show_only && (remove_directories || matches)) {
> +			if (show_only && (remove_directories || (matches >= 2))) {
>  				printf("Would remove %s\n", qname);
> -			} else if (remove_directories || matches) {
> +			} else if (remove_directories || (matches >= 2)) {

These magic numbers are bad.  Please update it to use symbolic constants.

>  				if (!quiet)
>  					printf("Removing %s\n", qname);
>  				if (remove_dir_recursively(&directory, 0) != 0) {
> diff --git a/dir.c b/dir.c
> index b5bfbca..63715c9 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -80,7 +80,7 @@ static int match_one(const char *match, const char *name, int namelen)
>  	if (strncmp(match, name, matchlen))
>  		return !fnmatch(match, name, 0) ? MATCHED_FNMATCH : 0;
>  
> -	if (!name[matchlen])
> +	if (namelen == matchlen)
>  		return MATCHED_EXACTLY;
>  	if (match[matchlen-1] == '/' || name[matchlen] == '/')
>  		return MATCHED_RECURSIVELY;

Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Shawn Bohrer]

On Mon, Apr 14, 2008 at 12:18:13AM -0700, Junio C Hamano wrote:
> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
> > -		int len, pos, matches;
> > +		int len, pos;
> > +		int matches = 0;
> >  		struct cache_entry *ce;
> >  		struct stat st;
> 
> Initialization of "matches" seems to be an independent clean-up.  Although
> it forces the initialization in the codepath that do not need the value of
> matches, that is not a big deal --- right?

Yes this is an independent clean-up.  I can't see any harm in forcing
the initializtion.

> > -			matches = match_pathspec(pathspec, ent->name, ent->len,
> > +			matches = match_pathspec(pathspec, ent->name, len,
> >  						 baselen, seen);
> > -		} else {
> > -			matches = 0;
> >  		}
> 
> And the essential change (fix) is to send len which could be shorter than
> ent->len because we have stripped '/' here, plus the one in match_one()
> that now allows name[] that is not NUL terminated.

Yep, I'll add that to the changelog.

> > -			if (show_only && (remove_directories || matches)) {
> > +			if (show_only && (remove_directories || (matches >= 2))) {
> >  				printf("Would remove %s\n", qname);
> > -			} else if (remove_directories || matches) {
> > +			} else if (remove_directories || (matches >= 2)) {
> 
> These magic numbers are bad.  Please update it to use symbolic constants.

Agreed I'll send an updated patch later tonight.  One additional thought
though.  2 is MATCHED_FNMATCH which worries me a little because I think
this would mean 'git clean -f *' will also remove directories (I haven't
tried though).  Perhaps this should really be 3 MATCHED_EXACTLY just to
be safe.  Does anyone have opinions either way?

--
Shawn

Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Joachim Berdal Haga]

Shawn Bohrer wrote:
> Agreed I'll send an updated patch later tonight.  One additional thought
> though.  2 is MATCHED_FNMATCH which worries me a little because I think
> this would mean 'git clean -f *' will also remove directories (I haven't
> tried though).  Perhaps this should really be 3 MATCHED_EXACTLY just to
> be safe.  Does anyone have opinions either way?

I don't have strong opinions on this since I don't use this form of the
command, but still:

I think that the best option would be to never remove a directory, even if
given explicitly, unless -d is given. Because my gut feeling is that when a
directory name is specified, it is most often meant as "clean inside the
given directory", ie. as a path delimiter. Indeed, if the directory has
tracked files inside of it,
  git clean dir
and
  git clean dir/
have the same effect. If there are no tracked files inside, the current
patch gives the path-delimiting effect on this form
  git clean dir/
but removes the whole directory irrespective of "-d" for this form
  git clean dir
I think that a "honor (lack of) -d even if pathspec matches" would reduce
the consequences of this particular kind of user error (by deleting too
little instead of too much).

-j.

[PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Shawn Bohrer]

When git clean is run from a subdirectory it should follow the normal
policy and only remove directories if they are passed in as a pathspec,
or -d is specified.

The fix is to send len which could be shorter than ent->len because we
have stripped the trailing '/' that read_directory adds. Additionaly
match_one() was modified to allow a name[] that is not NUL terminated.
This allows us to check if the name matched the pathspec exactly
instead of recursively.

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 builtin-clean.c |   13 +++++++------
 dir.c           |    2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin-clean.c b/builtin-clean.c
index fefec30..6778a03 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -95,7 +95,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < dir.nr; i++) {
 		struct dir_entry *ent = dir.entries[i];
-		int len, pos, matches;
+		int len, pos;
+		int matches = 0;
 		struct cache_entry *ce;
 		struct stat st;
 
@@ -127,18 +128,18 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 		if (pathspec) {
 			memset(seen, 0, argc > 0 ? argc : 1);
-			matches = match_pathspec(pathspec, ent->name, ent->len,
+			matches = match_pathspec(pathspec, ent->name, len,
 						 baselen, seen);
-		} else {
-			matches = 0;
 		}
 
 		if (S_ISDIR(st.st_mode)) {
 			strbuf_addstr(&directory, ent->name);
 			qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
-			if (show_only && (remove_directories || matches)) {
+			if (show_only && (remove_directories ||
+			    (matches == MATCHED_EXACTLY))) {
 				printf("Would remove %s\n", qname);
-			} else if (remove_directories || matches) {
+			} else if (remove_directories ||
+				   (matches == MATCHED_EXACTLY)) {
 				if (!quiet)
 					printf("Removing %s\n", qname);
 				if (remove_dir_recursively(&directory, 0) != 0) {
diff --git a/dir.c b/dir.c
index b5bfbca..63715c9 100644
--- a/dir.c
+++ b/dir.c
@@ -80,7 +80,7 @@ static int match_one(const char *match, const char *name, int namelen)
 	if (strncmp(match, name, matchlen))
 		return !fnmatch(match, name, 0) ? MATCHED_FNMATCH : 0;
 
-	if (!name[matchlen])
+	if (namelen == matchlen)
 		return MATCHED_EXACTLY;
 	if (match[matchlen-1] == '/' || name[matchlen] == '/')
 		return MATCHED_RECURSIVELY;
-- 
1.5.5.106.g62ee2.dirty

Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Shawn Bohrer]

On Mon, Apr 14, 2008 at 08:18:13PM +0200, Joachim Berdal Haga wrote:
> I think that the best option would be to never remove a directory, even if
> given explicitly, unless -d is given. Because my gut feeling is that when a
> directory name is specified, it is most often meant as "clean inside the
> given directory", ie. as a path delimiter. Indeed, if the directory has
> tracked files inside of it,
>   git clean dir
> and
>   git clean dir/
> have the same effect. If there are no tracked files inside, the current
> patch gives the path-delimiting effect on this form
>   git clean dir/
> but removes the whole directory irrespective of "-d" for this form
>   git clean dir
> I think that a "honor (lack of) -d even if pathspec matches" would reduce
> the consequences of this particular kind of user error (by deleting too
> little instead of too much).

If there are no tracked files the only difference between the dir/ and
dir case is that the former will leave behind an empty directory.  So
the difference between too much and too little is of little
importance.  However,

git clean dir
Would not remove dir/

is a little strange.

--
Shawn

Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Joachim Berdal Haga]

Shawn Bohrer wrote:
> On Mon, Apr 14, 2008 at 08:18:13PM +0200, Joachim Berdal Haga wrote:
>> I think that the best option would be to never remove a directory, even if
>> given explicitly, unless -d is given. Because my gut feeling is that when a
>> directory name is specified, it is most often meant as "clean inside the
>> given directory", ie. as a path delimiter.
> 
> If there are no tracked files the only difference between the dir/ and
> dir case is that the former will leave behind an empty directory.  So
> the difference between too much and too little is of little importance.

No, check this out; note that only in the very last case dir/subdir/subfile
would be removed.

$ git init; mkdir -p dir/subdir; touch dir/file dir/subdir/subfile
Initialized empty Git repository in .git/
$ touch dir/tracked-file; git add dir/tracked-file
$ ~/src/git/git-clean -n dir/
Would remove dir/file
Would not remove dir/subdir/
$ ~/src/git/git-clean -n dir
Would remove dir/file
Would not remove dir/subdir/
$ git rm -f dir/tracked-file
rm 'dir/tracked-file'
$ ~/src/git/git-clean -n dir/
Would remove dir/file
Would not remove dir/subdir/
$ ~/src/git/git-clean -n dir
Would remove dir/

> However,
> 
> git clean dir
> Would not remove dir/
> 
> is a little strange.

Yes, although it could be made less strange by adding a short explanation,
like "Would not remove dir/ (-d not given)". But I also think that the
difference between "dir" and "dir/" is very (too?) subtle in this case and
therefore should require explicit approval/action from the user.


-j.

Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Shawn Bohrer]

On Tue, Apr 15, 2008 at 08:33:23AM +0200, Joachim Berdal Haga wrote:
> Shawn Bohrer wrote:
> > On Mon, Apr 14, 2008 at 08:18:13PM +0200, Joachim Berdal Haga wrote:
> >> I think that the best option would be to never remove a directory, even if
> >> given explicitly, unless -d is given. Because my gut feeling is that when a
> >> directory name is specified, it is most often meant as "clean inside the
> >> given directory", ie. as a path delimiter.
> > 
> > If there are no tracked files the only difference between the dir/ and
> > dir case is that the former will leave behind an empty directory.  So
> > the difference between too much and too little is of little importance.
> 
> No, check this out; note that only in the very last case dir/subdir/subfile
> would be removed.
> 
> $ git init; mkdir -p dir/subdir; touch dir/file dir/subdir/subfile
> Initialized empty Git repository in .git/
> $ touch dir/tracked-file; git add dir/tracked-file
> $ ~/src/git/git-clean -n dir/
> Would remove dir/file
> Would not remove dir/subdir/
> $ ~/src/git/git-clean -n dir
> Would remove dir/file
> Would not remove dir/subdir/
> $ git rm -f dir/tracked-file
> rm 'dir/tracked-file'
> $ ~/src/git/git-clean -n dir/
> Would remove dir/file
> Would not remove dir/subdir/
> $ ~/src/git/git-clean -n dir
> Would remove dir/

Ah of course, this is the behavior with my patch.  Before it would have
removed everything which is the same bug you initially reported :)

> > However,
> > 
> > git clean dir
> > Would not remove dir/
> > 
> > is a little strange.
> 
> Yes, although it could be made less strange by adding a short explanation,
> like "Would not remove dir/ (-d not given)". But I also think that the
> difference between "dir" and "dir/" is very (too?) subtle in this case and
> therefore should require explicit approval/action from the user.

Yeah, I don't know how I feel about this.  I do think that the behavior
with my current patch is technically correct, but you may be right that
a trailing slash is subtle.  In most cases I use my shell's tab
completion witch adds the trailing slash, and only remove it when
needed.  Additionally, I could argue that by default we require explicit
action to clean files by requiring -n or -f so hopefully users try -n
first (I do).

--
Shawn

Re: [PATCH] git clean: Don't automatically remove directories when run within subdirectory

[Joachim Berdal Haga]

Shawn Bohrer wrote:
> On Tue, Apr 15, 2008 at 08:33:23AM +0200, Joachim Berdal Haga wrote:
>> like "Would not remove dir/ (-d not given)". But I also think that the
>> difference between "dir" and "dir/" is very (too?) subtle in this case and
>> therefore should require explicit approval/action from the user.
> 
> Yeah, I don't know how I feel about this.  I do think that the behavior
> with my current patch is technically correct, but you may be right that
> a trailing slash is subtle.  In most cases I use my shell's tab
> completion witch adds the trailing slash, and only remove it when
> needed.  Additionally, I could argue that by default we require explicit
> action to clean files by requiring -n or -f so hopefully users try -n
> first (I do).

I guess part of the story is that I dislike the -f requirement, because I see it 
as a case of "training users to use -f without thinking" (it's required for 
normal operation). But that's another story, and now that I've raised my points 
I'm quite happy to leave the final decision to you.

Cheers,
-j.