{"thread":{"id":"32374","subject":"[PATCH v2] git-clean: Display more accurate delete messages","startedAt":"2012-12-17T11:29:25Z","lastAt":"2012-12-19T02:37:14Z","messageCount":4,"participants":["Zoltan Klinger","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"205046","messageId":"1355743765-17549-1-git-send-email-zoltan.klinger@gmail.com","threadId":"32374","inReplyTo":null,"subject":"[PATCH v2] git-clean: Display more accurate delete messages","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2012-12-17T11:29:25Z","receivedAt":"2012-12-17T11:29:25Z","isPatch":true,"sender":{"key":"zoltan.klinger@gmail.com","avatar":"https://avatars.githubusercontent.com/u/95923?v=4"},"body":"(1) Only print out the names of the files and directories that got\n    actually deleted.\n(2) Show warning message for ignored untracked git repositories\n\nConsider the following repo layout:\n\n  test.git/\n    |-- tracked_file\n    |-- tracked_dir/\n    |     |-- some_tracked_file\n    |     |-- some_untracked_file\n    |-- untracked_file\n    |-- untracked_foo/\n    |     |-- bar/\n    |     |     |-- bar.txt\n    |     |-- emptydir/\n    |     |-- frotz.git/\n    |           |-- frotz.tx\n    |-- untracked_some.git/\n          |-- some.txt\n\nSuppose the user issues 'git clean -fd' from the test.git directory.\n\nWhen -d option is used and untracked directory 'foo' contains a\nsubdirectory 'frotz.git' that is managed by a different git repository\ntherefore it will not be removed.\n\n  $ git clean -fd\n  Removing tracked_dir/some_untracked_file\n  Removing untracked_file\n  Removing untracked_foo/\n  Removing untracked_some.git/\n\nThe message displayed to the user is slightly misleading. The foo/\ndirectory has not been removed because of foo/frotz.git still exists.\nOn the other hand the subdirectories 'bar' and 'emptydir' have been\ndeleted but they're not mentioned anywhere. Also, untracked_some.git\nhas not been removed either.\n\nThis behaviour is the result of the way the deletion of untracked\ndirectories are reported. In the current implementation they are\ndeleted recursively but only the name of the top most directory is\nprinted out. The calling function does not know about any\nsubdirectories that could not be removed during the recursion.\n\nImprove the way the deleted directories are reported back to\nthe user:\n  (1) Modify the recursive delete function to run in both dry_run and\n      delete modes.\n  (2) During the recursion collect the name of the files and\n      directories that:\n        (a) will be or have been removed.\n        (b) could not be removed due to file system permissions, etc.\n        (c) will be or have been ignored because they are untracked\n            git repositories that are not removed by default unless\n            the --force --force option is used.\n  (3) After finishing the delete process print out:\n        (a) the names of all deleted topmost directories and nothing\n            about their (recursive) contents if all content was removed\n            successfully\n        (b) the names of all files that have been deleted but their parent\n            directory still exists\n        (c) warning for all untracked git repositories that have been\n            ignored\n        (d) warning about files and directories that failed to delete.\n\nConsider the output of the improved version:\n\n  $ git clean -fd\n  Removing tracked_dir/some_untracked_file\n  Removing untracked_file\n  Removing untracked_foo/bar/\n  Removing untracked_foo/emptydir/\n  warning: ignoring untracked git repository untracked_foo/frotz.git/\n  warning: ignoring untracked git repository untracked_some.git/\n\nNow it displays only the file and directory names that got actually\ndeleted and shows warnings about ignored untracked git repositories.\n\nSigned-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>\nReported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>\n---\n\n Have updated patch with feedback received from Junio and Soren Brinkmann\n\n builtin/clean.c |   78 +++++++++++++++++++++++++++++++++++--------------------\n dir.c           |   65 +++++++++++++++++++++++++++++++++++++++-------\n dir.h           |    4 +++\n 3 files changed, 109 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 69c1cda..4824bac 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -34,22 +34,42 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static void print_filtered(const char *msg, struct string_list *lst)\n+{\n+\tint i;\n+\tchar *name;\n+\tchar *dir = 0;\n+\n+\tsort_string_list(lst);\n+\n+\tfor (i = 0; i < lst->nr; i++) {\n+\t\tname = lst->items[i].string;\n+\t\tif (dir == 0 || strncmp(name, dir, strlen(dir)) != 0)\n+\t\t\tprintf(\"%s %s\\n\", msg, name);\n+\t\tif (name[strlen(name) - 1] == '/')\n+\t\t\tdir = name;\n+\t}\n+}\n+\n int cmd_clean(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n-\tint show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;\n+\tint dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;\n \tint ignored_only = 0, config_set = 0, errors = 0;\n \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n \tstruct strbuf directory = STRBUF_INIT;\n \tstruct dir_struct dir;\n \tstatic const char **pathspec;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct string_list dels = STRING_LIST_INIT_DUP;\n+\tstruct string_list skips = STRING_LIST_INIT_DUP;\n+\tstruct string_list errs = STRING_LIST_INIT_DUP;\n \tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n \tconst char *qname;\n \tchar *seen = NULL;\n \tstruct option options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"do not print names of files removed\")),\n-\t\tOPT__DRY_RUN(&show_only, N_(\"dry run\")),\n+\t\tOPT__DRY_RUN(&dry_run, N_(\"dry run\")),\n \t\tOPT__FORCE(&force, N_(\"force\")),\n \t\tOPT_BOOLEAN('d', NULL, &remove_directories,\n \t\t\t\tN_(\"remove whole directories\")),\n@@ -77,7 +97,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tif (ignored && ignored_only)\n \t\tdie(_(\"-x and -X cannot be used together\"));\n \n-\tif (!show_only && !force) {\n+\tif (!dry_run && !force) {\n \t\tif (config_set)\n \t\t\tdie(_(\"clean.requireForce set to true and neither -n nor -f given; \"\n \t\t\t\t  \"refusing to clean\"));\n@@ -150,43 +170,45 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tstrbuf_addstr(&directory, ent->name);\n \t\t\tqname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n-\t\t\tif (show_only && (remove_directories ||\n-\t\t\t    (matches == MATCHED_EXACTLY))) {\n-\t\t\t\tprintf(_(\"Would remove %s\\n\"), qname);\n-\t\t\t} else if (remove_directories ||\n-\t\t\t\t   (matches == MATCHED_EXACTLY)) {\n-\t\t\t\tif (!quiet)\n-\t\t\t\t\tprintf(_(\"Removing %s\\n\"), qname);\n-\t\t\t\tif (remove_dir_recursively(&directory,\n-\t\t\t\t\t\t\t   rm_flags) != 0) {\n-\t\t\t\t\twarning(_(\"failed to remove %s\"), qname);\n-\t\t\t\t\terrors++;\n-\t\t\t\t}\n-\t\t\t} else if (show_only) {\n-\t\t\t\tprintf(_(\"Would not remove %s\\n\"), qname);\n-\t\t\t} else {\n-\t\t\t\tprintf(_(\"Not removing %s\\n\"), qname);\n+\t\t\tif (remove_directories || (matches == MATCHED_EXACTLY)) {\n+\t\t\t\tremove_dir_recursively_with_dryrun(&directory, rm_flags, dry_run,\n+\t\t\t\t\t\t&dels, &skips, &errs, prefix);\n \t\t\t}\n \t\t\tstrbuf_reset(&directory);\n \t\t} else {\n \t\t\tif (pathspec && !matches)\n \t\t\t\tcontinue;\n \t\t\tqname = quote_path_relative(ent->name, -1, &buf, prefix);\n-\t\t\tif (show_only) {\n-\t\t\t\tprintf(_(\"Would remove %s\\n\"), qname);\n-\t\t\t\tcontinue;\n-\t\t\t} else if (!quiet) {\n-\t\t\t\tprintf(_(\"Removing %s\\n\"), qname);\n-\t\t\t}\n-\t\t\tif (unlink(ent->name) != 0) {\n-\t\t\t\twarning(_(\"failed to remove %s\"), qname);\n-\t\t\t\terrors++;\n+\t\t\tif (dry_run)\n+\t\t\t\tstring_list_append(&dels, qname);\n+\t\t\telse {\n+\t\t\t\tif (unlink(ent->name) != 0)\n+\t\t\t\t\tstring_list_append(&errs, qname);\n+\t\t\t\telse\n+\t\t\t\t\tstring_list_append(&dels, qname);\n \t\t\t}\n \t\t}\n \t}\n+\n+\tif (!quiet) {\n+\t\tif (dry_run) {\n+\t\t\tprint_filtered(\"Would remove\", &dels);\n+\t\t\tprint_filtered(\"Would ignore untracked git repository\", &skips);\n+\t\t} else {\n+\t\t\tprint_filtered(\"Removing\", &dels);\n+\t\t\tprint_filtered(\"warning: ignoring untracked git repository\", &skips);\n+\t\t}\n+\t}\n+\n+\terrors = errs.nr;\n+\tif (errors)\n+\t\tprint_filtered(\"warning: failed to remove\", &errs);\n+\n \tfree(seen);\n \n \tstrbuf_release(&directory);\n+\tstring_list_clear(&dels, 0);\n+\tstring_list_clear(&errs, 0);\n \tstring_list_clear(&exclude_list, 0);\n \treturn (errors != 0);\n }\ndiff --git a/dir.c b/dir.c\nindex 5a83aa7..fd38d5d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -7,7 +7,9 @@\n  */\n #include \"cache.h\"\n #include \"dir.h\"\n+#include \"quote.h\"\n #include \"refs.h\"\n+#include \"string-list.h\"\n \n struct path_simplify {\n \tint len;\n@@ -1294,11 +1296,30 @@ int is_empty_dir(const char *path)\n \treturn ret;\n }\n \n-static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n+static void append_dir_name(struct string_list *dels, struct string_list *skips,\n+\t\tstruct string_list *errs, char *name, const char * prefix, int failed, int isdir)\n+{\n+\tstruct strbuf quoted = STRBUF_INIT;\n+\n+\tquote_path_relative(name, strlen(name), &quoted, prefix);\n+\tif (isdir && quoted.buf[strlen(quoted.buf) -1] != '/')\n+\t\tstrbuf_addch(&quoted, '/');\n+\n+\tif (skips)\n+\t\tstring_list_append(skips, quoted.buf);\n+\telse if (!failed && dels)\n+\t\tstring_list_append(dels, quoted.buf);\n+\telse if (errs)\n+\t\tstring_list_append(errs, quoted.buf);\n+}\n+\n+static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up,\n+\tint dry_run, struct string_list *dels, struct string_list *skips,\n+\tstruct string_list *errs, const char *prefix)\n {\n \tDIR *dir;\n \tstruct dirent *e;\n-\tint ret = 0, original_len = path->len, len, kept_down = 0;\n+\tint ret = 0, original_len = path->len, len, kept_down = 0, res = 0;\n \tint only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n \tint keep_toplevel = (flag & REMOVE_DIR_KEEP_TOPLEVEL);\n \tunsigned char submodule_head[20];\n@@ -1306,6 +1327,7 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \tif ((flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n \t    !resolve_gitlink_ref(path->buf, \"HEAD\", submodule_head)) {\n \t\t/* Do not descend and nuke a nested git work tree. */\n+\t\tappend_dir_name(NULL, skips, NULL, path->buf, prefix, 0, 1);\n \t\tif (kept_up)\n \t\t\t*kept_up = 1;\n \t\treturn 0;\n@@ -1315,8 +1337,13 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \tdir = opendir(path->buf);\n \tif (!dir) {\n \t\t/* an empty dir could be removed even if it is unreadble */\n-\t\tif (!keep_toplevel)\n-\t\t\treturn rmdir(path->buf);\n+\t\tif (!keep_toplevel) {\n+\t\t\tres = 0;\n+\t\t\tif (!dry_run)\n+\t\t\t\tres = rmdir(path->buf);\n+\t\t\tappend_dir_name(dels, NULL, errs, path->buf, prefix, res, 1);\n+\t\t\treturn res;\n+\t\t}\n \t\telse\n \t\t\treturn -1;\n \t}\n@@ -1334,10 +1361,17 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \t\tif (lstat(path->buf, &st))\n \t\t\t; /* fall thru */\n \t\telse if (S_ISDIR(st.st_mode)) {\n-\t\t\tif (!remove_dir_recurse(path, flag, &kept_down))\n+\t\t\tif (!remove_dir_recurse(path, flag, &kept_down, dry_run, dels,\n+\t\t\t\t\t\tskips, errs, prefix))\n \t\t\t\tcontinue; /* happy */\n-\t\t} else if (!only_empty && !unlink(path->buf))\n-\t\t\tcontinue; /* happy, too */\n+\t\t} else if (!only_empty) {\n+\t\t\tres = 0;\n+\t\t\tif (!dry_run)\n+\t\t\t\tres = unlink(path->buf);\n+\t\t\tappend_dir_name(dels, NULL, errs, path->buf, prefix, res, 0);\n+\t\t\tif (!res)\n+\t\t\t\tcontinue; /* happy, too */\n+\t\t}\n \n \t\t/* path too long, stat fails, or non-directory still exists */\n \t\tret = -1;\n@@ -1346,8 +1380,12 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \tclosedir(dir);\n \n \tstrbuf_setlen(path, original_len);\n-\tif (!ret && !keep_toplevel && !kept_down)\n-\t\tret = rmdir(path->buf);\n+\tif (!ret && !keep_toplevel && !kept_down) {\n+\t\tret = 0;\n+\t\tif (!dry_run)\n+\t\t\tret = rmdir(path->buf);\n+\t\tappend_dir_name(dels, NULL, errs, path->buf, prefix, res, 1);\n+\t}\n \telse if (kept_up)\n \t\t/*\n \t\t * report the uplevel that it is not an error that we\n@@ -1359,7 +1397,14 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \n int remove_dir_recursively(struct strbuf *path, int flag)\n {\n-\treturn remove_dir_recurse(path, flag, NULL);\n+\treturn remove_dir_recurse(path, flag, NULL, 0, NULL, NULL, NULL, NULL);\n+}\n+\n+int remove_dir_recursively_with_dryrun(struct strbuf *path, int flag,\n+\t\tint dry_run, struct string_list *dels, struct string_list *skips,\n+\t\tstruct string_list *errs, const char *prefix)\n+{\n+\treturn remove_dir_recurse(path, flag, NULL, dry_run, dels, skips, errs, prefix);\n }\n \n void setup_standard_excludes(struct dir_struct *dir)\ndiff --git a/dir.h b/dir.h\nindex f5c89e3..780885a 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -131,6 +131,10 @@ extern void setup_standard_excludes(struct dir_struct *dir);\n #define REMOVE_DIR_KEEP_NESTED_GIT 02\n #define REMOVE_DIR_KEEP_TOPLEVEL 04\n extern int remove_dir_recursively(struct strbuf *path, int flag);\n+extern int remove_dir_recursively_with_dryrun(struct strbuf *path,\n+\t\t\tint flag, int dryrun, struct string_list *dels,\n+\t\t\tstruct string_list *skips, struct string_list *errs,\n+\t\t\tconst char *prefix);\n \n /* tries to remove the path with empty directories along it, ignores ENOENT */\n extern int remove_path(const char *path);\n-- \n1.7.9.5\n"},{"id":"205078","messageId":"7vsj74jr2k.fsf@alter.siamese.dyndns.org","threadId":"32374","inReplyTo":"1355743765-17549-1-git-send-email-zoltan.klinger@gmail.com","subject":"Re: [PATCH v2] git-clean: Display more accurate delete messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-17T21:40:03Z","receivedAt":"2012-12-17T21:40:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zoltan Klinger <zoltan.klinger@gmail.com> writes:\n\n> +static void print_filtered(const char *msg, struct string_list *lst)\n> +{\n> +\tint i;\n> +\tchar *name;\n> +\tchar *dir = 0;\n> +\n> +\tsort_string_list(lst);\n> +\n> +\tfor (i = 0; i < lst->nr; i++) {\n> +\t\tname = lst->items[i].string;\n> +\t\tif (dir == 0 || strncmp(name, dir, strlen(dir)) != 0)\n> +\t\t\tprintf(\"%s %s\\n\", msg, name);\n> +\t\tif (name[strlen(name) - 1] == '/')\n> +\t\t\tdir = name;\n> +\t}\n> +}\n\nHere, prefixcmp() may be easier to read than strncmp().  We tend to\nprefer writing comparison with zero like this:\n\n\tif (!dir || prefixcmp(name, dir))\n\t\t...\n\nbut I think we can go either way.\n\nMy reading of the above is that \"lst\" after sorting is expected to\nhave something like:\n\n\ta/\n        a/b/\n\ta/b/to-be-removed\n        a/to-be-removed\n\nand we first show \"a/\", remember that prefix in \"dir\", not show\n\"a/b/\" because it matches prefix, but still update the prefix to\n\"a/b/\", not show \"a/b/to-be-removed\", and because \"a/to-be-removed\"\ndoes not match the latest prefix, it is now shown.  Am I confused???\n\n> @@ -150,43 +170,45 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n>  \t\tif (S_ISDIR(st.st_mode)) {\n>  \t\t\tstrbuf_addstr(&directory, ent->name);\n>  \t\t\tqname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n> -\t\t\tif (show_only && (remove_directories ||\n> -\t\t\t    (matches == MATCHED_EXACTLY))) {\n> -\t\t\t\tprintf(_(\"Would remove %s\\n\"), qname);\n> -\t\t\t} else if (remove_directories ||\n> -\t\t\t\t   (matches == MATCHED_EXACTLY)) {\n> -\t\t\t\tif (!quiet)\n> -\t\t\t\t\tprintf(_(\"Removing %s\\n\"), qname);\n> -\t\t\t\tif (remove_dir_recursively(&directory,\n> -\t\t\t\t\t\t\t   rm_flags) != 0) {\n> -\t\t\t\t\twarning(_(\"failed to remove %s\"), qname);\n> -\t\t\t\t\terrors++;\n> -\t\t\t\t}\n> -\t\t\t} else if (show_only) {\n> -\t\t\t\tprintf(_(\"Would not remove %s\\n\"), qname);\n> -\t\t\t} else {\n> -\t\t\t\tprintf(_(\"Not removing %s\\n\"), qname);\n> +\t\t\tif (remove_directories || (matches == MATCHED_EXACTLY)) {\n> +\t\t\t\tremove_dir_recursively_with_dryrun(&directory, rm_flags, dry_run,\n> +\t\t\t\t\t\t&dels, &skips, &errs, prefix);\n>  \t\t\t}\n\nMoving the above logic to a single helper function makes sense, but\ncan we name it a bit more concisely?  Also this helper feels very\nspecific to \"clean\"---does it need to go to dir.[ch], I have to\nwonder.\n\nOther than the above two points, the resulting builtin/clean.c looks\nmuch more nicely structured than before.\n\nI am not very much pleased by the change to dir.[ch] in this patch,\nthough.\n\n> +static void append_dir_name(struct string_list *dels, struct string_list *skips,\n> +\t\tstruct string_list *errs, char *name, const char * prefix, int failed, int isdir)\n> +{\n> +\tstruct strbuf quoted = STRBUF_INIT;\n> +\n> +\tquote_path_relative(name, strlen(name), &quoted, prefix);\n> +\tif (isdir && quoted.buf[strlen(quoted.buf) -1] != '/')\n> +\t\tstrbuf_addch(&quoted, '/');\n> +\n> +\tif (skips)\n> +\t\tstring_list_append(skips, quoted.buf);\n> +\telse if (!failed && dels)\n> +\t\tstring_list_append(dels, quoted.buf);\n> +\telse if (errs)\n> +\t\tstring_list_append(errs, quoted.buf);\n> +}\n\nThe three lists dels/skips/errs are mostly mutually exclusive (the\ncaller knows which one to throw the element in) except that failed\ncontrols which one between dels or errs is used.\n\nThat's an ugly interface, I have to say.  I think the quote-path\npart should become a separate helper function to be used by the\ncallers of this function, and the callers should stuff the path to\nthe list they want to put the element in.  That will eliminate the\nneed for this ugliness.\n\nAlso, didn't you make remove_dir_recursively() excessively leaky by\ndoing this?  The string in quoted is still created, even though the\ncaller passes NULL to all the lists.\n\nThanks.\n"},{"id":"205168","messageId":"CAKJhZwRPzrsnbnW_HgRTo86T6jqmm_osznDqpYo7pKO=cUaVDA@mail.gmail.com","threadId":"32374","inReplyTo":"7vsj74jr2k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] git-clean: Display more accurate delete messages","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2012-12-19T00:59:59Z","receivedAt":"2012-12-19T00:59:59Z","isPatch":true,"sender":{"key":"zoltan.klinger@gmail.com","avatar":"https://avatars.githubusercontent.com/u/95923?v=4"},"body":"Thanks for the feedback.\n\n> My reading of the above is that \"lst\" after sorting is expected to\n> have something like:\n>\n>         a/\n>         a/b/\n>         a/b/to-be-removed\n>         a/to-be-removed\n>\n> and we first show \"a/\", remember that prefix in \"dir\", not show\n> \"a/b/\" because it matches prefix, but still update the prefix to\n> \"a/b/\", not show \"a/b/to-be-removed\", and because \"a/to-be-removed\"\n> does not match the latest prefix, it is now shown.  Am I confused???\n\nNo, it's a bug. The correct output should be just \"a/\". Thanks for\npointing it out, I'm going to fix that.\n\n>\n>> @@ -150,43 +170,45 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n>>               if (S_ISDIR(st.st_mode)) {\n>>                       strbuf_addstr(&directory, ent->name);\n>>                       qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n>> -                     if (show_only && (remove_directories ||\n>> -                         (matches == MATCHED_EXACTLY))) {\n>> -                             printf(_(\"Would remove %s\\n\"), qname);\n>> -                     } else if (remove_directories ||\n>> -                                (matches == MATCHED_EXACTLY)) {\n>> -                             if (!quiet)\n>> -                                     printf(_(\"Removing %s\\n\"), qname);\n>> -                             if (remove_dir_recursively(&directory,\n>> -                                                        rm_flags) != 0) {\n>> -                                     warning(_(\"failed to remove %s\"), qname);\n>> -                                     errors++;\n>> -                             }\n>> -                     } else if (show_only) {\n>> -                             printf(_(\"Would not remove %s\\n\"), qname);\n>> -                     } else {\n>> -                             printf(_(\"Not removing %s\\n\"), qname);\n>> +                     if (remove_directories || (matches == MATCHED_EXACTLY)) {\n>> +                             remove_dir_recursively_with_dryrun(&directory, rm_flags, dry_run,\n>> +                                             &dels, &skips, &errs, prefix);\n>>                       }\n>\n> Moving the above logic to a single helper function makes sense, but\n> can we name it a bit more concisely?  Also this helper feels very\n> specific to \"clean\"---does it need to go to dir.[ch], I have to\n> wonder.\n\nWould you have a better name in mind for the\nremove_dir_recursively_with_dryrun() function? I'm kinda stuck.\n\nMy thinking was that since the private function remove_dir_recurse()\nin dir.c already handles the recursive removing of files and\ndirectories and checks for nested git directories, it would be better\nto modify that function rather than implement something similar but\nwith dels, skips and errs lists in clean.c.\n\n> I am not very much pleased by the change to dir.[ch] in this patch,\n> though.\n>\n>> +static void append_dir_name(struct string_list *dels, struct string_list *skips,\n>> +             struct string_list *errs, char *name, const char * prefix, int failed, int isdir)\n>> +{\n>> +     struct strbuf quoted = STRBUF_INIT;\n>> +\n>> +     quote_path_relative(name, strlen(name), &quoted, prefix);\n>> +     if (isdir && quoted.buf[strlen(quoted.buf) -1] != '/')\n>> +             strbuf_addch(&quoted, '/');\n>> +\n>> +     if (skips)\n>> +             string_list_append(skips, quoted.buf);\n>> +     else if (!failed && dels)\n>> +             string_list_append(dels, quoted.buf);\n>> +     else if (errs)\n>> +             string_list_append(errs, quoted.buf);\n>> +}\n>\n> The three lists dels/skips/errs are mostly mutually exclusive (the\n> caller knows which one to throw the element in) except that failed\n> controls which one between dels or errs is used.\n>\n> That's an ugly interface, I have to say.  I think the quote-path\n> part should become a separate helper function to be used by the\n> callers of this function, and the callers should stuff the path to\n> the list they want to put the element in.  That will eliminate the\n> need for this ugliness.\n\nWill get rid of append_dir_name() and reimplement things the way you\nsuggested above.\n\nCheers,\nZoltan\n"},{"id":"205170","messageId":"7vy5gudaxx.fsf@alter.siamese.dyndns.org","threadId":"32374","inReplyTo":"CAKJhZwRPzrsnbnW_HgRTo86T6jqmm_osznDqpYo7pKO=cUaVDA@mail.gmail.com","subject":"Re: [PATCH v2] git-clean: Display more accurate delete messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-19T02:37:14Z","receivedAt":"2012-12-19T02:37:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zoltan Klinger <zoltan.klinger@gmail.com> writes:\n\n> Thanks for the feedback.\n>\n>> My reading of the above is that \"lst\" after sorting is expected to\n>> have something like:\n>>\n>>         a/\n>>         a/b/\n>>         a/b/to-be-removed\n>>         a/to-be-removed\n>>\n>> and we first show \"a/\", remember that prefix in \"dir\", not show\n>> \"a/b/\" because it matches prefix, but still update the prefix to\n>> \"a/b/\", not show \"a/b/to-be-removed\", and because \"a/to-be-removed\"\n>> does not match the latest prefix, it is now shown.  Am I confused???\n>\n> No, it's a bug. The correct output should be just \"a/\". Thanks for\n> pointing it out, I'm going to fix that.\n\nI am not sure if the approach taken by the patch is an effective\ndesign to achieve what you are trying to do.\n\nImagine the code is told to \"clean\" (or \"clean a\") and is currentlly\nlooking at \"a/b\" directory.  If it cannot remove some paths under\nthat directory, you know that you cannot abbreviate the result to\n\"removed a/b\" and have to report a/b/<paths you managed to remove>\nat that point.  On the other hand, if you removed everything in that\ndirectory, you know you have only two possible outcomes regarding\nthat directory in the final output:\n\n (1) You would say \"removed a/b\" if you failed to remove paths that\n     are neighbours to that directory (e.g. \"a/to-be-removed\" may\n     not go away for some reason), because you will also list\n     \"removed a/<other path>\" next to it, and report that you\n     couldn't remove \"a/to-be-removed\".  You will not report\n     anything about \"a/b/to-be-removed\" in such a case; or\n\n (2) You would not even say \"removed a/b\" if you will successfully\n     remove all other paths under \"a/\".\n\nSo in either case, if you managed to remove everything in \"a/b\", I\ndo not see any reason to keep the list of successfully removed paths\nannd report them upwards.  They will never be used by the caller\nthat is looking at \"a/\", or its caller that is looking at the root\nlevel, will they?\n\nOn the other hand, if you failed to remove some paths under \"a/b\",\nbefore recursion leaves that directory, you know which paths to be\nreported as successful or failure, which means you can start\nproducing output without waiting until the traversal touches the\nentire tree. That can be a huge latency win, which matters a lot in\na large project.\n"}]}