(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.klin...@gmail.com>
Reported-by: Soren Brinkmann <soren.brinkm...@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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to