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

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

[Zoltan Klinger]

(1) Only print out the names of the files and directories that got
    actually deleted.
(2) Show warning message for ignored untracked git repositories

Consider the following repo layout:

  test.git/
    |-- tracked_file
    |-- tracked_dir/
    |     |-- some_tracked_file
    |     |-- some_untracked_file
    |-- untracked_file
    |-- untracked_foo/
    |     |-- bar/
    |     |     |-- bar.txt
    |     |-- emptydir/
    |     |-- frotz.git/
    |           |-- frotz.tx
    |-- untracked_some.git/
          |-- some.txt

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 tracked_dir/some_untracked_file
  Removing untracked_file
  Removing untracked_foo/
  Removing untracked_some.git/

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 subdirectories 'bar' and 'emptydir' have been
deleted but they're not mentioned anywhere. Also, untracked_some.git
has not been removed either.

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:
        (a) will be or have been removed.
        (b) could not be removed due to file system permissions, etc.
        (c) will be or have been ignored because they are untracked
            git repositories that are not removed by default unless
            the --force --force option is used.
  (3) After finishing the delete process print out:
        (a) the names of all deleted topmost directories and nothing
            about their (recursive) contents if all content was removed
            successfully
        (b) the names of all files that have been deleted but their parent
            directory still exists
        (c) warning for all untracked git repositories that have been
            ignored
        (d) warning about files and directories that failed to delete.

Consider the output of the improved version:

  $ git clean -fd
  Removing tracked_dir/some_untracked_file
  Removing untracked_file
  Removing untracked_foo/bar/
  Removing untracked_foo/emptydir/
  warning: ignoring untracked git repository untracked_foo/frotz.git/
  warning: ignoring untracked git repository untracked_some.git/

Now it displays only the file and directory names that got actually
deleted and shows warnings about ignored untracked git repositories.

Signed-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>
Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---

 Have updated patch with feedback received from Junio and Soren Brinkmann

 builtin/clean.c |   78 +++++++++++++++++++++++++++++++++++--------------------
 dir.c           |   65 +++++++++++++++++++++++++++++++++++++++-------
 dir.h           |    4 +++
 3 files changed, 109 insertions(+), 38 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 69c1cda..4824bac 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -34,22 +34,42 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static void print_filtered(const char *msg, struct string_list *lst)
+{
+	int i;
+	char *name;
+	char *dir = 0;
+
+	sort_string_list(lst);
+
+	for (i = 0; i < lst->nr; i++) {
+		name = lst->items[i].string;
+		if (dir == 0 || strncmp(name, dir, strlen(dir)) != 0)
+			printf("%s %s\n", msg, name);
+		if (name[strlen(name) - 1] == '/')
+			dir = name;
+	}
+}
+
 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 skips = 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 +97,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 +170,45 @@ 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, &skips, &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_filtered("Would remove", &dels);
+			print_filtered("Would ignore untracked git repository", &skips);
+		} else {
+			print_filtered("Removing", &dels);
+			print_filtered("warning: ignoring untracked git repository", &skips);
+		}
+	}
+
+	errors = errs.nr;
+	if (errors)
+		print_filtered("warning: 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..fd38d5d 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,30 @@ 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 *skips,
+		struct string_list *errs, char *name, const char * prefix, int failed, int isdir)
+{
+	struct strbuf quoted = STRBUF_INIT;
+
+	quote_path_relative(name, strlen(name), &quoted, prefix);
+	if (isdir && quoted.buf[strlen(quoted.buf) -1] != '/')
+		strbuf_addch(&quoted, '/');
+
+	if (skips)
+		string_list_append(skips, quoted.buf);
+	else if (!failed && dels)
+		string_list_append(dels, quoted.buf);
+	else if (errs)
+		string_list_append(errs, quoted.buf);
+}
+
+static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up,
+	int dry_run, struct string_list *dels, struct string_list *skips,
+	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];
@@ -1306,6 +1327,7 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
 	if ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
 	    !resolve_gitlink_ref(path->buf, "HEAD", submodule_head)) {
 		/* Do not descend and nuke a nested git work tree. */
+		append_dir_name(NULL, skips, NULL, path->buf, prefix, 0, 1);
 		if (kept_up)
 			*kept_up = 1;
 		return 0;
@@ -1315,8 +1337,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, NULL, errs, path->buf, prefix, res, 1);
+			return res;
+		}
 		else
 			return -1;
 	}
@@ -1334,10 +1361,17 @@ 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,
+						skips, 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, NULL, errs, path->buf, prefix, res, 0);
+			if (!res)
+				continue; /* happy, too */
+		}
 
 		/* path too long, stat fails, or non-directory still exists */
 		ret = -1;
@@ -1346,8 +1380,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, NULL, errs, path->buf, prefix, res, 1);
+	}
 	else if (kept_up)
 		/*
 		 * report the uplevel that it is not an error that we
@@ -1359,7 +1397,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, NULL);
+}
+
+int remove_dir_recursively_with_dryrun(struct strbuf *path, int flag,
+		int dry_run, struct string_list *dels, struct string_list *skips,
+		struct string_list *errs, const char *prefix)
+{
+	return remove_dir_recurse(path, flag, NULL, dry_run, dels, skips, errs, prefix);
 }
 
 void setup_standard_excludes(struct dir_struct *dir)
diff --git a/dir.h b/dir.h
index f5c89e3..780885a 100644
--- a/dir.h
+++ b/dir.h
@@ -131,6 +131,10 @@ 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 *skips, 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 v2] git-clean: Display more accurate delete messages

[Junio C Hamano]

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

> +static void print_filtered(const char *msg, struct string_list *lst)
> +{
> +	int i;
> +	char *name;
> +	char *dir = 0;
> +
> +	sort_string_list(lst);
> +
> +	for (i = 0; i < lst->nr; i++) {
> +		name = lst->items[i].string;
> +		if (dir == 0 || strncmp(name, dir, strlen(dir)) != 0)
> +			printf("%s %s\n", msg, name);
> +		if (name[strlen(name) - 1] == '/')
> +			dir = name;
> +	}
> +}

Here, prefixcmp() may be easier to read than strncmp().  We tend to
prefer writing comparison with zero like this:

	if (!dir || prefixcmp(name, dir))
		...

but I think we can go either way.

My reading of the above is that "lst" after sorting is expected to
have something like:

	a/
        a/b/
	a/b/to-be-removed
        a/to-be-removed

and we first show "a/", remember that prefix in "dir", not show
"a/b/" because it matches prefix, but still update the prefix to
"a/b/", not show "a/b/to-be-removed", and because "a/to-be-removed"
does not match the latest prefix, it is now shown.  Am I confused???

> @@ -150,43 +170,45 @@ 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, &skips, &errs, prefix);
>  			}

Moving the above logic to a single helper function makes sense, but
can we name it a bit more concisely?  Also this helper feels very
specific to "clean"---does it need to go to dir.[ch], I have to
wonder.

Other than the above two points, the resulting builtin/clean.c looks
much more nicely structured than before.

I am not very much pleased by the change to dir.[ch] in this patch,
though.

> +static void append_dir_name(struct string_list *dels, struct string_list *skips,
> +		struct string_list *errs, char *name, const char * prefix, int failed, int isdir)
> +{
> +	struct strbuf quoted = STRBUF_INIT;
> +
> +	quote_path_relative(name, strlen(name), &quoted, prefix);
> +	if (isdir && quoted.buf[strlen(quoted.buf) -1] != '/')
> +		strbuf_addch(&quoted, '/');
> +
> +	if (skips)
> +		string_list_append(skips, quoted.buf);
> +	else if (!failed && dels)
> +		string_list_append(dels, quoted.buf);
> +	else if (errs)
> +		string_list_append(errs, quoted.buf);
> +}

The three lists dels/skips/errs are mostly mutually exclusive (the
caller knows which one to throw the element in) except that failed
controls which one between dels or errs is used.

That's an ugly interface, I have to say.  I think the quote-path
part should become a separate helper function to be used by the
callers of this function, and the callers should stuff the path to
the list they want to put the element in.  That will eliminate the
need for this ugliness.

Also, didn't you make remove_dir_recursively() excessively leaky by
doing this?  The string in quoted is still created, even though the
caller passes NULL to all the lists.

Thanks.

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

[Zoltan Klinger]

Thanks for the feedback.

> My reading of the above is that "lst" after sorting is expected to
> have something like:
>
>         a/
>         a/b/
>         a/b/to-be-removed
>         a/to-be-removed
>
> and we first show "a/", remember that prefix in "dir", not show
> "a/b/" because it matches prefix, but still update the prefix to
> "a/b/", not show "a/b/to-be-removed", and because "a/to-be-removed"
> does not match the latest prefix, it is now shown.  Am I confused???

No, it's a bug. The correct output should be just "a/". Thanks for
pointing it out, I'm going to fix that.

>
>> @@ -150,43 +170,45 @@ 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, &skips, &errs, prefix);
>>                       }
>
> Moving the above logic to a single helper function makes sense, but
> can we name it a bit more concisely?  Also this helper feels very
> specific to "clean"---does it need to go to dir.[ch], I have to
> wonder.

Would you have a better name in mind for the
remove_dir_recursively_with_dryrun() function? I'm kinda stuck.

My thinking was that since the private function remove_dir_recurse()
in dir.c already handles the recursive removing of files and
directories and checks for nested git directories, it would be better
to modify that function rather than implement something similar but
with dels, skips and errs lists in clean.c.

> I am not very much pleased by the change to dir.[ch] in this patch,
> though.
>
>> +static void append_dir_name(struct string_list *dels, struct string_list *skips,
>> +             struct string_list *errs, char *name, const char * prefix, int failed, int isdir)
>> +{
>> +     struct strbuf quoted = STRBUF_INIT;
>> +
>> +     quote_path_relative(name, strlen(name), &quoted, prefix);
>> +     if (isdir && quoted.buf[strlen(quoted.buf) -1] != '/')
>> +             strbuf_addch(&quoted, '/');
>> +
>> +     if (skips)
>> +             string_list_append(skips, quoted.buf);
>> +     else if (!failed && dels)
>> +             string_list_append(dels, quoted.buf);
>> +     else if (errs)
>> +             string_list_append(errs, quoted.buf);
>> +}
>
> The three lists dels/skips/errs are mostly mutually exclusive (the
> caller knows which one to throw the element in) except that failed
> controls which one between dels or errs is used.
>
> That's an ugly interface, I have to say.  I think the quote-path
> part should become a separate helper function to be used by the
> callers of this function, and the callers should stuff the path to
> the list they want to put the element in.  That will eliminate the
> need for this ugliness.

Will get rid of append_dir_name() and reimplement things the way you
suggested above.

Cheers,
Zoltan

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

[Junio C Hamano]

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

> Thanks for the feedback.
>
>> My reading of the above is that "lst" after sorting is expected to
>> have something like:
>>
>>         a/
>>         a/b/
>>         a/b/to-be-removed
>>         a/to-be-removed
>>
>> and we first show "a/", remember that prefix in "dir", not show
>> "a/b/" because it matches prefix, but still update the prefix to
>> "a/b/", not show "a/b/to-be-removed", and because "a/to-be-removed"
>> does not match the latest prefix, it is now shown.  Am I confused???
>
> No, it's a bug. The correct output should be just "a/". Thanks for
> pointing it out, I'm going to fix that.

I am not sure if the approach taken by the patch is an effective
design to achieve what you are trying to do.

Imagine the code is told to "clean" (or "clean a") and is currentlly
looking at "a/b" directory.  If it cannot remove some paths under
that directory, you know that you cannot abbreviate the result to
"removed a/b" and have to report a/b/<paths you managed to remove>
at that point.  On the other hand, if you removed everything in that
directory, you know you have only two possible outcomes regarding
that directory in the final output:

 (1) You would say "removed a/b" if you failed to remove paths that
     are neighbours to that directory (e.g. "a/to-be-removed" may
     not go away for some reason), because you will also list
     "removed a/<other path>" next to it, and report that you
     couldn't remove "a/to-be-removed".  You will not report
     anything about "a/b/to-be-removed" in such a case; or

 (2) You would not even say "removed a/b" if you will successfully
     remove all other paths under "a/".

So in either case, if you managed to remove everything in "a/b", I
do not see any reason to keep the list of successfully removed paths
annd report them upwards.  They will never be used by the caller
that is looking at "a/", or its caller that is looking at the root
level, will they?

On the other hand, if you failed to remove some paths under "a/b",
before recursion leaves that directory, you know which paths to be
reported as successful or failure, which means you can start
producing output without waiting until the traversal touches the
entire tree. That can be a huge latency win, which matters a lot in
a large project.