[PATCH] git-clean: Display more accurate delete messages

[PATCH] git-clean: Display more accurate delete messages

[Zoltan Klinger]

Only print out the names of the files and directories that got actually
deleted.

Consider the following repo layout:
  |-- test.git/
        |-- foo/
             |-- bar/
                  |-- bar.txt
             |-- frotz.git/
                  |-- frotz.txt
        |-- tracked_file1
        |-- untracked_file1

Suppose the user issues 'git clean -fd' from the test.git directory.

When -d option is used and untracked directory 'foo' contains a
subdirectory 'frotz.git' that is managed by a different git repository
therefore it will not be removed.

  $ git clean -fd
  Removing foo/
  Removing untracked_file1

The message displayed to the user is slightly misleading. The foo/
directory has not been removed because of foo/frotz.git still exists.
On the other hand the subdirectory bar has been deleted but it's not
mentioned anywhere.

This behaviour is the result of the way the deletion of untracked
directories are reported. In the current implementation they are
deleted recursively but only the name of the top most directory is
printed out. The calling function does not know about any
subdirectories that could not be removed during the recursion.

Improve the way the deleted directories are reported back to
the user:
  (1) Modify the recursive delete function to run in both dry_run and
      delete modes.
  (2) During the recursion collect the name of the files and
      directories that will be or have been removed. Also collect the
      names of files and directories that could not be removed.
  (3) After finishing the deletes print out the names of all deleted
      files and any files or directories that failed to delete.

Consider the output of the improved version:

  $ git clean -fd
  Removed foo/bar/bar.txt
  Removed foo/bar
  Removed untracked_file1

Now it displays only the file and directory names that got actually
deleted.

Signed-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>
---
Hi there,

My first patch. Hope you find it useful.

Looking forward to your feedback.

Cheers,
Zoltan

 builtin/clean.c |   64 +++++++++++++++++++++++++++++++------------------------
 dir.c           |   58 ++++++++++++++++++++++++++++++++++++++++---------
 dir.h           |    3 +++
 3 files changed, 87 insertions(+), 38 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 69c1cda..9b056b9 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -34,22 +34,31 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static void print_result(const char *msg, struct string_list *lst)
+{
+  int i;
+  for (i = 0; i < lst->nr; i++)
+		printf("%s %s\n", msg, lst->items[i].string);
+}
+
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
 	int i;
-	int show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;
+	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
 	int ignored_only = 0, config_set = 0, errors = 0;
 	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
 	struct strbuf directory = STRBUF_INIT;
 	struct dir_struct dir;
 	static const char **pathspec;
 	struct strbuf buf = STRBUF_INIT;
+	struct string_list dels = STRING_LIST_INIT_DUP;
+	struct string_list errs = STRING_LIST_INIT_DUP;
 	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 	const char *qname;
 	char *seen = NULL;
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print names of files removed")),
-		OPT__DRY_RUN(&show_only, N_("dry run")),
+		OPT__DRY_RUN(&dry_run, N_("dry run")),
 		OPT__FORCE(&force, N_("force")),
 		OPT_BOOLEAN('d', NULL, &remove_directories,
 				N_("remove whole directories")),
@@ -77,7 +86,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	if (ignored && ignored_only)
 		die(_("-x and -X cannot be used together"));
 
-	if (!show_only && !force) {
+	if (!dry_run && !force) {
 		if (config_set)
 			die(_("clean.requireForce set to true and neither -n nor -f given; "
 				  "refusing to clean"));
@@ -150,43 +159,42 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		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 == MATCHED_EXACTLY))) {
-				printf(_("Would remove %s\n"), qname);
-			} else if (remove_directories ||
-				   (matches == MATCHED_EXACTLY)) {
-				if (!quiet)
-					printf(_("Removing %s\n"), qname);
-				if (remove_dir_recursively(&directory,
-							   rm_flags) != 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 (remove_directories || (matches == MATCHED_EXACTLY)) {
+				remove_dir_recursively_with_dryrun(&directory, rm_flags, dry_run,
+						&dels, &errs, prefix);
 			}
 			strbuf_reset(&directory);
 		} else {
 			if (pathspec && !matches)
 				continue;
 			qname = quote_path_relative(ent->name, -1, &buf, prefix);
-			if (show_only) {
-				printf(_("Would remove %s\n"), qname);
-				continue;
-			} else if (!quiet) {
-				printf(_("Removing %s\n"), qname);
-			}
-			if (unlink(ent->name) != 0) {
-				warning(_("failed to remove %s"), qname);
-				errors++;
+			if (dry_run)
+				string_list_append(&dels, qname);
+			else {
+				if (unlink(ent->name) != 0)
+					string_list_append(&errs, qname);
+				else
+					string_list_append(&dels, qname);
 			}
 		}
 	}
+
+	if (!quiet) {
+		if (dry_run)
+			print_result("Would remove", &dels);
+		else
+			print_result("Removed", &dels);
+	}
+
+	errors = errs.nr;
+	if (errors)
+		print_result("Failed to remove", &errs);
+
 	free(seen);
 
 	strbuf_release(&directory);
+	string_list_clear(&dels, 0);
+	string_list_clear(&errs, 0);
 	string_list_clear(&exclude_list, 0);
 	return (errors != 0);
 }
diff --git a/dir.c b/dir.c
index 5a83aa7..f580c51 100644
--- a/dir.c
+++ b/dir.c
@@ -7,7 +7,9 @@
  */
 #include "cache.h"
 #include "dir.h"
+#include "quote.h"
 #include "refs.h"
+#include "string-list.h"
 
 struct path_simplify {
 	int len;
@@ -1294,11 +1296,25 @@ int is_empty_dir(const char *path)
 	return ret;
 }
 
-static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
+static void append_dir_name(struct string_list *dels, struct string_list *errs,
+    char *name, const char * prefix, int failed)
+{
+	struct strbuf buf = STRBUF_INIT;
+	const char *qname;
+	qname = quote_path_relative(name, strlen(name), &buf, prefix);
+	if (!failed && dels)
+		string_list_append(dels, qname);
+	else if (errs)
+		string_list_append(errs, qname);
+}
+
+static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up,
+	int dry_run, struct string_list *dels, struct string_list *errs,
+  const char *prefix)
 {
 	DIR *dir;
 	struct dirent *e;
-	int ret = 0, original_len = path->len, len, kept_down = 0;
+	int ret = 0, original_len = path->len, len, kept_down = 0, res = 0;
 	int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);
 	int keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);
 	unsigned char submodule_head[20];
@@ -1315,8 +1331,13 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 	dir = opendir(path->buf);
 	if (!dir) {
 		/* an empty dir could be removed even if it is unreadble */
-		if (!keep_toplevel)
-			return rmdir(path->buf);
+		if (!keep_toplevel) {
+			res = 0;
+			if (!dry_run)
+				res = rmdir(path->buf);
+			append_dir_name(dels, errs, path->buf, prefix, res);
+			return res;
+		}
 		else
 			return -1;
 	}
@@ -1334,10 +1355,16 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 		if (lstat(path->buf, &st))
 			; /* fall thru */
 		else if (S_ISDIR(st.st_mode)) {
-			if (!remove_dir_recurse(path, flag, &kept_down))
+			if (!remove_dir_recurse(path, flag, &kept_down, dry_run, dels, errs, prefix))
 				continue; /* happy */
-		} else if (!only_empty && !unlink(path->buf))
-			continue; /* happy, too */
+		} else if (!only_empty) {
+			res = 0;
+			if (!dry_run)
+				res = unlink(path->buf);
+			append_dir_name(dels, errs, path->buf, prefix, res);
+			if (!res)
+				continue; /* happy, too */
+		}
 
 		/* path too long, stat fails, or non-directory still exists */
 		ret = -1;
@@ -1346,8 +1373,12 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 	closedir(dir);
 
 	strbuf_setlen(path, original_len);
-	if (!ret && !keep_toplevel && !kept_down)
-		ret = rmdir(path->buf);
+	if (!ret && !keep_toplevel && !kept_down) {
+		ret = 0;
+		if (!dry_run)
+			ret = rmdir(path->buf);
+		append_dir_name(dels, errs, path->buf, prefix, res);
+	}
 	else if (kept_up)
 		/*
 		 * report the uplevel that it is not an error that we
@@ -1359,7 +1390,14 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 
 int remove_dir_recursively(struct strbuf *path, int flag)
 {
-	return remove_dir_recurse(path, flag, NULL);
+	return remove_dir_recurse(path, flag, NULL, 0, NULL, NULL, NULL);
+}
+
+int remove_dir_recursively_with_dryrun(struct strbuf *path, int flag,
+		int dry_run, struct string_list *dels, struct string_list *errs,
+		const char *prefix)
+{
+	return remove_dir_recurse(path, flag, NULL, dry_run, dels, errs, prefix);
 }
 
 void setup_standard_excludes(struct dir_struct *dir)
diff --git a/dir.h b/dir.h
index f5c89e3..828bd49 100644
--- a/dir.h
+++ b/dir.h
@@ -131,6 +131,9 @@ extern void setup_standard_excludes(struct dir_struct *dir);
 #define REMOVE_DIR_KEEP_NESTED_GIT 02
 #define REMOVE_DIR_KEEP_TOPLEVEL 04
 extern int remove_dir_recursively(struct strbuf *path, int flag);
+extern int remove_dir_recursively_with_dryrun(struct strbuf *path,
+			int flag, int dryrun, struct string_list *dels,
+			struct string_list *errs, const char *prefix);
 
 /* tries to remove the path with empty directories along it, ignores ENOENT */
 extern int remove_path(const char *path);
-- 
1.7.9.5

Re: [PATCH] git-clean: Display more accurate delete messages

[Junio C Hamano]

Zoltan Klinger <zoltan.klinger@gmail.com> writes:

> Only print out the names of the files and directories that got actually
> deleted.
>
> Consider the following repo layout:
>   |-- test.git/
>         |-- foo/
>              |-- bar/
>                   |-- bar.txt
>              |-- frotz.git/
>                   |-- frotz.txt
>         |-- tracked_file1
>         |-- untracked_file1
> ...
> Consider the output of the improved version:
>
>   $ git clean -fd
>   Removed foo/bar/bar.txt
>   Removed foo/bar
>   Removed untracked_file1

Hrm, following your discussion (ellided above), I would have
expected that you would show

    Removing directory foo/bar
    Removing untracked_file1

Re: [PATCH] git-clean: Display more accurate delete messages

[Soren Brinkmann]

Hi,

On Thu, Dec 06, 2012 at 09:37:31AM -0800, Junio C Hamano wrote:
> Zoltan Klinger <zoltan.klinger@gmail.com> writes:
> 
> > Only print out the names of the files and directories that got actually
> > deleted.
> >
> > Consider the following repo layout:
> >   |-- test.git/
> >         |-- foo/
> >              |-- bar/
> >                   |-- bar.txt
> >              |-- frotz.git/
> >                   |-- frotz.txt
> >         |-- tracked_file1
> >         |-- untracked_file1
> > ...
> > Consider the output of the improved version:
> >
> >   $ git clean -fd
> >   Removed foo/bar/bar.txt
> >   Removed foo/bar
> >   Removed untracked_file1
> 
> Hrm, following your discussion (ellided above), I would have
> expected that you would show
> 
>     Removing directory foo/bar
>     Removing untracked_file1

Also it would be nice to have warnings about undeleted directories since this git
clean behavior (or the work around to pass -f twice) is not documented.
Without a warning you would probably miss that something was _not_ deleted.

So something like:
	Removing foo
	Removing bar
	...

	Warning: Not all untracked objects have been deleted:
	<list objects here> (optional)
	Use git clean --force --force to delete all objects.


But clearly going into a good direction. Thanks.

	Soren

Re: [PATCH] git-clean: Display more accurate delete messages

[Zoltan Klinger]

>> Hrm, following your discussion (ellided above), I would have
>> expected that you would show
>>
>>     Removing directory foo/bar
>>     Removing untracked_file1
>
> Also it would be nice to have warnings about undeleted directories since this git
> clean behavior (or the work around to pass -f twice) is not documented.
> Without a warning you would probably miss that something was _not_ deleted.

Thanks for the feedback. I think you're right. Showing 'foo/bar/bar.txt' in
the list when 'foo/bar/' directory has been successfully deleted is just noise.

Would like to get some more feedback on the proposed output in case of
 (1) an untracked subdirectory with multiple files where at least one of them
     cannot be removed.
 (2) reporting ignored untracked git subdirectories

Suppose we have a repo like the one below:
  test.git/
    |-- tracked_file
    |-- untracked_file
    |-- untracked_foo/
    |     |-- bar/
    |     |     |-- bar.txt
    |     |-- emptydir/
    |     |-- frotz.git/
    |     |     |-- frotx.txt
    |     |-- quux/
    |           |-- failedquux.txt
    |           |-- quux.txt
    |-- untracked_unreadable_dir/
    |     |-- afile
    |-- untracked_some.git/
          |-- some.txt

$ git clean -fd
Removing untracked_file
Removing untracked_foo/bar
Removing untracked_foo/emptydir
Removing untracked_foo/quux/quux.txt
warning: failed to remove untracked_foo/quux/failedquux.txt
warning: failed to remove remove untracked_unreadable_dir/
warning: ignoring untracked git repository untracked_foo/frotz.git/
warning: ignoring untracked git repository untracked_some.git/
Use git clean --force --force to delete all untracked git repositories

$ # use forced remove
$ git clean --force --force -d
Removing untracked_foo/frotz.git
Removing untracked_foo/quux/quux.txt
Removing untracked_some.git/
warning: failed to remove untracked_foo/quux/failedquux.txt
warning: failed to remove untracked_unreadable_dir/

Can you see any issues with the proposed output, wording above? If
everyone is happy,
I'm going to prepare patch V2 for it.

Thanks,
Zoltan

Re: [PATCH] git-clean: Display more accurate delete messages

[Junio C Hamano]

Zoltan Klinger <zoltan.klinger@gmail.com> writes:

> Would like to get some more feedback on the proposed output in case of
>  (1) an untracked subdirectory with multiple files where at least one of them
>      cannot be removed.
>  (2) reporting ignored untracked git subdirectories
>
> Suppose we have a repo like the one below:
>   test.git/
>     |-- tracked_file
>     |-- untracked_file
>     |-- untracked_foo/
>     |     |-- bar/
>     |     |     |-- bar.txt
>     |     |-- emptydir/
>     |     |-- frotz.git/
>     |     |     |-- frotx.txt
>     |     |-- quux/
>     |           |-- failedquux.txt
>     |           |-- quux.txt
>     |-- untracked_unreadable_dir/
>     |     |-- afile
>     |-- untracked_some.git/
>           |-- some.txt
>
> $ git clean -fd
> Removing untracked_file
> Removing untracked_foo/bar
> Removing untracked_foo/emptydir
> Removing untracked_foo/quux/quux.txt
> warning: failed to remove untracked_foo/quux/failedquux.txt
> warning: failed to remove remove untracked_unreadable_dir/

"remove remove" is a typo, I presume.

> warning: ignoring untracked git repository untracked_foo/frotz.git/
> warning: ignoring untracked git repository untracked_some.git/

If you mean "we report the topmost directory and nothing about
(recursive) contents in it if everything is removed successfully"
(in other words, if we had subdirectories and files inside
untracked_foo/bar/ and we successfully removed all of them, the
above output does not change), it seems quite reasonable.

> Use git clean --force --force to delete all untracked git repositories

But I am not sure if this is ever sane.  Especially the one that
removes an embedded repository is suspicious.  "git clean" should
not ever touch it with or without --superforce or any other command.

I do not think trying to remove something that cannot be removed due
to filesystem permissions is sensible, either. We simply should treat
such a case a grave error and have the user sort things out, instead
of blindly attempt to "chmod" them ourselves (which may still fail).

Thanks.

Re: [PATCH] git-clean: Display more accurate delete messages

[Soren Brinkmann]

Hi Zoltan,

On Sun, Dec 09, 2012 at 10:18:19PM +1100, Zoltan Klinger wrote:
> >> Hrm, following your discussion (ellided above), I would have
> >> expected that you would show
> >>
> >>     Removing directory foo/bar
> >>     Removing untracked_file1
> >
> > Also it would be nice to have warnings about undeleted directories since this git
> > clean behavior (or the work around to pass -f twice) is not documented.
> > Without a warning you would probably miss that something was _not_ deleted.
> 
> Thanks for the feedback. I think you're right. Showing 'foo/bar/bar.txt' in
> the list when 'foo/bar/' directory has been successfully deleted is just noise.
> 
> Would like to get some more feedback on the proposed output in case of
>  (1) an untracked subdirectory with multiple files where at least one of them
>      cannot be removed.
>  (2) reporting ignored untracked git subdirectories
> 
> Suppose we have a repo like the one below:
>   test.git/
>     |-- tracked_file
>     |-- untracked_file
>     |-- untracked_foo/
>     |     |-- bar/
>     |     |     |-- bar.txt
>     |     |-- emptydir/
>     |     |-- frotz.git/
>     |     |     |-- frotx.txt
>     |     |-- quux/
>     |           |-- failedquux.txt
>     |           |-- quux.txt
>     |-- untracked_unreadable_dir/
>     |     |-- afile
>     |-- untracked_some.git/
>           |-- some.txt
> 
> $ git clean -fd
> Removing untracked_file
> Removing untracked_foo/bar
> Removing untracked_foo/emptydir
> Removing untracked_foo/quux/quux.txt
> warning: failed to remove untracked_foo/quux/failedquux.txt
> warning: failed to remove remove untracked_unreadable_dir/
> warning: ignoring untracked git repository untracked_foo/frotz.git/
> warning: ignoring untracked git repository untracked_some.git/
> Use git clean --force --force to delete all untracked git repositories
> 
> $ # use forced remove
> $ git clean --force --force -d
> Removing untracked_foo/frotz.git
> Removing untracked_foo/quux/quux.txt
> Removing untracked_some.git/
> warning: failed to remove untracked_foo/quux/failedquux.txt
> warning: failed to remove untracked_unreadable_dir/
> 
> Can you see any issues with the proposed output, wording above? If
> everyone is happy,
> I'm going to prepare patch V2 for it.
Looks good to me.

Thanks,
Soren

Re: [PATCH] git-clean: Display more accurate delete messages

[Soren Brinkmann]

Hi, 

On Sun, Dec 09, 2012 at 11:04:59PM -0800, Junio C Hamano wrote:
> Zoltan Klinger <zoltan.klinger@gmail.com> writes:
> 
> > Would like to get some more feedback on the proposed output in case of
> >  (1) an untracked subdirectory with multiple files where at least one of them
> >      cannot be removed.
> >  (2) reporting ignored untracked git subdirectories
> >
> > Suppose we have a repo like the one below:
> >   test.git/
> >     |-- tracked_file
> >     |-- untracked_file
> >     |-- untracked_foo/
> >     |     |-- bar/
> >     |     |     |-- bar.txt
> >     |     |-- emptydir/
> >     |     |-- frotz.git/
> >     |     |     |-- frotx.txt
> >     |     |-- quux/
> >     |           |-- failedquux.txt
> >     |           |-- quux.txt
> >     |-- untracked_unreadable_dir/
> >     |     |-- afile
> >     |-- untracked_some.git/
> >           |-- some.txt
> >
> > $ git clean -fd
> > Removing untracked_file
> > Removing untracked_foo/bar
> > Removing untracked_foo/emptydir
> > Removing untracked_foo/quux/quux.txt
> > warning: failed to remove untracked_foo/quux/failedquux.txt
> > warning: failed to remove remove untracked_unreadable_dir/
> 
> "remove remove" is a typo, I presume.
> 
> > warning: ignoring untracked git repository untracked_foo/frotz.git/
> > warning: ignoring untracked git repository untracked_some.git/
> 
> If you mean "we report the topmost directory and nothing about
> (recursive) contents in it if everything is removed successfully"
> (in other words, if we had subdirectories and files inside
> untracked_foo/bar/ and we successfully removed all of them, the
> above output does not change), it seems quite reasonable.
> 
> > Use git clean --force --force to delete all untracked git repositories
> 
> But I am not sure if this is ever sane.  Especially the one that
> removes an embedded repository is suspicious.  "git clean" should
> not ever touch it with or without --superforce or any other command.
As I mentioned in my email where I reported this incorrect git clean output, I
have a use case where I want git clean to remove embedded repositories.
Whether it is a sane one is probably a different discussion.

	Soren

Re: [PATCH] git-clean: Display more accurate delete messages

[Junio C Hamano]

Soren Brinkmann <soren.brinkmann@xilinx.com> writes:

>> > Use git clean --force --force to delete all untracked git repositories
>> 
>> But I am not sure if this is ever sane.  Especially the one that
>> removes an embedded repository is suspicious.  "git clean" should
>> not ever touch it with or without --superforce or any other command.
> As I mentioned in my email where I reported this incorrect git clean output, I
> have a use case where I want git clean to remove embedded repositories.
> Whether it is a sane one is probably a different discussion.

Why is it a different discussion?  If something is not sane, the
tool shouldn't encourage users to do such an insane thing.

Re: [PATCH] git-clean: Display more accurate delete messages

[Soren Brinkmann]

On Mon, Dec 10, 2012 at 10:03:28AM -0800, Junio C Hamano wrote:
> Soren Brinkmann <soren.brinkmann@xilinx.com> writes:
> 
> >> > Use git clean --force --force to delete all untracked git repositories
> >> 
> >> But I am not sure if this is ever sane.  Especially the one that
> >> removes an embedded repository is suspicious.  "git clean" should
> >> not ever touch it with or without --superforce or any other command.
> > As I mentioned in my email where I reported this incorrect git clean output, I
> > have a use case where I want git clean to remove embedded repositories.
> > Whether it is a sane one is probably a different discussion.
> 
> Why is it a different discussion?  If something is not sane, the
> tool shouldn't encourage users to do such an insane thing.
> 
Well, ok. So I have a repository which essentially consists of a bunch of
scripts which then pull sources via git to build root filesystems, busybox,
kernel etc.
So I have the master repository I'm actually interested in. And then all the
other projects which are pulled in to build stuff from.
looking somehow like this:
	top.git
	 |-src
	 |  |-proj1.git
	 |  |-proj2.git
	 |  |-projn.git
	 |-build
	     |-proj1
	     |-proj2
	     ...

Since the scripts are not perfect I usually used 'git clean -xdf' to wipe
everything and build from scratch. And I had to experience that the git clean
behavior somehow changed recently and the 'projn.git' directories were no longer
removed anymore, despite git indicating otherwise in its output.

So, I think having 'git clean -ff' removing embedded git repos is okay. But either
way, the output of git clean should match what it is doing. And at least tell me
if it didn't remove certain dirs or files.

	Soren

Re: [PATCH] git-clean: Display more accurate delete messages

[Junio C Hamano]

Soren Brinkmann <soren.brinkmann@xilinx.com> writes:

> But either
> way, the output of git clean should match what it is doing. And at least tell me
> if it didn't remove certain dirs or files.

Oh, no question about that part.  I was reacting to --force --force
in general, and an unrelated git repository inside a working tree is
just a subset of the issue.

Re: [PATCH] git-clean: Display more accurate delete messages

[Zoltan Klinger]

>> Use git clean --force --force to delete all untracked git repositories
>
> But I am not sure if this is ever sane.  Especially the one that
> removes an embedded repository is suspicious.  "git clean" should
> not ever touch it with or without --superforce or any other command.

My original intention with this patch was to provide more accurate
delete messages for the git-clean command when it's used with the
current set of command line options. I didn't know that --force
--force was so controversial.

The --force --force option has been around since v1.6.4.2. Commit
a0f4afbe introduced it. If the consensus is that it is not a sane
option to have let's remove it by all means. But I think it should be
done in a separate patch '[PATCH] git-clean: Never delete any embedded
git repository' or such.

> I do not think trying to remove something that cannot be removed due
> to filesystem permissions is sensible, either. We simply should treat
> such a case a grave error and have the user sort things out, instead
> of blindly attempt to "chmod" them ourselves (which may still fail).

But this is not how git-clean works with or without the --force
--force flag. The recursive delete does the right thing: it tries to
delete a file or directory, if that fails for whatever reason it will
report the error and move on. That's it. No "chmod" or any other
hackery at all. The --force --force flag only means "if during
recursion you encounter an embedded git directory that is not tracked
you are allowed to recurse into it and keep on deleting files and
sub-directories as per usual".

Cheers,
Zoltan