{"thread":{"id":"32508","subject":"[PATCH v3] git-clean: Display more accurate delete messages","startedAt":"2013-01-02T01:45:59Z","lastAt":"2013-01-03T23:21:45Z","messageCount":4,"participants":["Zoltan Klinger","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"205825","messageId":"1357091159-22080-1-git-send-email-zoltan.klinger@gmail.com","threadId":"32508","inReplyTo":null,"subject":"[PATCH v3] git-clean: Display more accurate delete messages","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2013-01-02T01:45:59Z","receivedAt":"2013-01-02T01:45:59Z","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_dir/\n    |     |-- some_tracked_file\n    |     |-- some_untracked_file\n    |-- tracked_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) Create a recursive delete function 'remove_dirs' in builtin/clean.c\n      to run in both dry_run and delete modes with the delete logic as\n      follows:\n        (a) Check if the current directory to be deleted is an untracked\n            git repository. If it is and --force --force option is not set\n            do not touch this directory, print ignore message, set dir_gone\n            flag to false for the caller and return.\n        (b) Otherwise for each item in current directory:\n              (i)   If current directory cannot be accessed, print warning,\n                    set dir_gone flag to false and return.\n              (ii)  If the item is a subdirectory recurse into it,\n                    check for the returned value of the dir_gone flag.\n                    If the subdirectory is gone, add the name of the deleted\n                    directory to a list of successfully removed items 'dels'.\n                    Else set the dir_gone flag as the current directory\n                    cannot be removed because we have at least one subdirectory\n                    hanging around.\n              (iii) If it is a file try to remove it. If success add the\n                    file name to the 'dels' list, else print error and set\n                    dir_gone flag to false.\n        (c) After we finished deleting all items in the current directory and\n            the dir_gone flag is still true, remove the directory itself.\n            If failed set the dir_gone flag to false.\n\n        (d) If the current directory cannot be deleted because the dir_gone flag\n            has been set to false, print out all the successfully deleted items\n            for this directory from the 'dels' list.\n        (e) We're done with the current directory, return.\n\n  (2) Modify the cmd_clean() function to:\n        (a) call the recursive delete function 'remove_dirs()' for each\n            topmost directory it wants to remove\n        (b) check for the returned value of dir_gone flag. If it's true\n            print the name of the directory as being removed.\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  warning: ignoring untracked git repository untracked_foo/frotz.git\n  Removing untracked_foo/bar\n  Removing untracked_foo/emptydir\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\nReported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>\n\nSigned-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>\n---\n builtin/clean.c |  149 ++++++++++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 120 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 69c1cda..37e403a 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -10,6 +10,7 @@\n #include \"cache.h\"\n #include \"dir.h\"\n #include \"parse-options.h\"\n+#include \"refs.h\"\n #include \"string-list.h\"\n #include \"quote.h\"\n \n@@ -20,6 +21,13 @@ static const char *const builtin_clean_usage[] = {\n \tNULL\n };\n \n+static const char* MSG_REMOVE = \"Removing %s\\n\";\n+static const char* MSG_WOULD_REMOVE = \"Would remove %s\\n\";\n+static const char* MSG_WOULD_NOT_REMOVE = \"Would not remove %s\\n\";\n+static const char* MSG_WOULD_IGNORE_GIT_DIR = \"Would ignore untracked git repository %s\\n\";\n+static const char* MSG_WARN_GIT_DIR_IGNORE = \"ignoring untracked git repository %s\";\n+static const char* MSG_WARN_REMOVE_FAILED = \"failed to remove %s\";\n+\n static int git_clean_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"clean.requireforce\"))\n@@ -34,11 +42,109 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n+\t\tint dry_run, int quiet, int *dir_gone)\n+{\n+\tDIR *dir;\n+\tstruct strbuf quoted = STRBUF_INIT;\n+\tstruct dirent *e;\n+\tint res = 0, ret = 0, gone = 1, original_len = path->len, len, i;\n+\tunsigned char submodule_head[20];\n+\tstruct string_list dels = STRING_LIST_INIT_DUP;\n+\n+\t*dir_gone = 1;\n+\n+\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n+\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n+\t    !resolve_gitlink_ref(path->buf, \"HEAD\", submodule_head)) {\n+\t\tif (dry_run && !quiet)\n+\t\t\tprintf(_(MSG_WOULD_IGNORE_GIT_DIR), quoted.buf);\n+\t\telse if (!dry_run)\n+\t\t\twarning(_(MSG_WARN_GIT_DIR_IGNORE), quoted.buf);\n+\n+\t\t*dir_gone = 0;\n+\t\treturn 0;\n+\t}\n+\n+\tdir = opendir(path->buf);\n+\tif (!dir) {\n+\t\t/* an empty dir could be removed even if it is unreadble */\n+\t\tres = dry_run ? 0 : rmdir(path->buf);\n+\t\tif (res) {\n+\t\t\twarning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);\n+\t\t\t*dir_gone = 0;\n+\t\t}\n+\t\treturn res;\n+\t}\n+\n+\tif (path->buf[original_len - 1] != '/')\n+\t\tstrbuf_addch(path, '/');\n+\n+\tlen = path->len;\n+\twhile ((e = readdir(dir)) != NULL) {\n+\t\tstruct stat st;\n+\t\tif (is_dot_or_dotdot(e->d_name))\n+\t\t\tcontinue;\n+\n+\t\tstrbuf_setlen(path, len);\n+\t\tstrbuf_addstr(path, e->d_name);\n+\t\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\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_dirs(path, prefix, force_flag, dry_run, quiet, &gone))\n+\t\t\t\tret = 1;\n+\t\t\tif (gone)\n+\t\t\t\tstring_list_append(&dels, quoted.buf);\n+\t\t\telse\n+\t\t\t\t*dir_gone = 0;\n+\t\t\tcontinue;\n+\t\t} else {\n+\t\t\tres = dry_run ? 0 : unlink(path->buf);\n+\t\t\tif (!res)\n+\t\t\t\tstring_list_append(&dels, quoted.buf);\n+\t\t\telse {\n+\t\t\t\twarning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);\n+\t\t\t\t*dir_gone = 0;\n+\t\t\t\tret = 1;\n+\t\t\t}\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/* path too long, stat fails, or non-directory still exists */\n+\t\t*dir_gone = 0;\n+\t\tret = 1;\n+\t\tbreak;\n+\t}\n+\tclosedir(dir);\n+\n+\tstrbuf_setlen(path, original_len);\n+\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n+\n+\tif (*dir_gone) {\n+\t\tres = dry_run ? 0 : rmdir(path->buf);\n+\t\tif (!res)\n+\t\t\t*dir_gone = 1;\n+\t\telse {\n+\t\t\twarning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);\n+\t\t\t*dir_gone = 0;\n+\t\t\tret = 1;\n+\t\t}\n+\t}\n+\n+\tif (!*dir_gone && !quiet) {\n+\t\tfor (i = 0; i < dels.nr; i++)\n+\t\t\tprintf(dry_run ?  _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), dels.items[i].string);\n+\t}\n+\tstring_list_clear(&dels, 0);\n+\treturn ret;\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 ignored_only = 0, config_set = 0, errors = 0;\n+\tint i, res;\n+\tint dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;\n+\tint ignored_only = 0, config_set = 0, errors = 0, gone = 1;\n \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n \tstruct strbuf directory = STRBUF_INIT;\n \tstruct dir_struct dir;\n@@ -49,7 +155,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\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 +183,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,38 +256,23 @@ 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\tif (remove_directories || (matches == MATCHED_EXACTLY)) {\n+\t\t\t\tif (remove_dirs(&directory, prefix, rm_flags, dry_run, quiet, &gone))\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\t\tif (gone && !quiet)\n+\t\t\t\t\tprintf(dry_run ? _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), qname);\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\tres = dry_run ? 0 : unlink(ent->name);\n+\t\t\tif (res) {\n+\t\t\t\twarning(_(MSG_WARN_REMOVE_FAILED), qname);\n \t\t\t\terrors++;\n-\t\t\t}\n+\t\t\t} else if (!quiet)\n+\t\t\t\tprintf(dry_run ? _(MSG_WOULD_REMOVE) :_(MSG_REMOVE), qname);\n \t\t}\n \t}\n \tfree(seen);\n-- \n1.7.9.5\n"},{"id":"205877","messageId":"7vfw2j2vlp.fsf@alter.siamese.dyndns.org","threadId":"32508","inReplyTo":"1357091159-22080-1-git-send-email-zoltan.klinger@gmail.com","subject":"Re: [PATCH v3] git-clean: Display more accurate delete messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-02T20:11:46Z","receivedAt":"2013-01-02T20:11:46Z","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 const char* MSG_REMOVE = \"Removing %s\\n\";\n> +static const char* MSG_WOULD_REMOVE = \"Would remove %s\\n\";\n> +static const char* MSG_WOULD_NOT_REMOVE = \"Would not remove %s\\n\";\n> +static const char* MSG_WOULD_IGNORE_GIT_DIR = \"Would ignore untracked git repository %s\\n\";\n> +static const char* MSG_WARN_GIT_DIR_IGNORE = \"ignoring untracked git repository %s\";\n> +static const char* MSG_WARN_REMOVE_FAILED = \"failed to remove %s\";\n\n\"foo* bar\" should be \"foo *bar\".  Also I personally find these\nupcased message constants somewhat hard to read in the sites of use\nin the code below.\n\nAlso gettext machinery needs to be told that these strings may be\nasked to be replaced with their translations using _() elsewhere in\nthe code.  I.e.\n\n\tstatic const char *msg_remove = N_(\"Removing %s\\n\");\n\nAren't WOULD_IGNORE_GIT_DIR and WARN_GIT_DIR_IGNORE named somewhat\ninconsistently?  Perhaps the latter is WARN_IGNORED_GIT_DIR or\nsomething?\n\n> @@ -34,11 +42,109 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)\n>  \treturn 0;\n>  }\n>  \n> +static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,\n> +\t\tint dry_run, int quiet, int *dir_gone)\n> +{\n> +\tDIR *dir;\n> +\tstruct strbuf quoted = STRBUF_INIT;\n> +\tstruct dirent *e;\n> +\tint res = 0, ret = 0, gone = 1, original_len = path->len, len, i;\n> +\tunsigned char submodule_head[20];\n> +\tstruct string_list dels = STRING_LIST_INIT_DUP;\n> +\n> +\t*dir_gone = 1;\n> +\n> +\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n\nShouldn't this be inside the next if() statement body?  I also think\nyou could even omit this call when (!dry_run && quiet).  The same\ncomment applies to all uses of quote_path_relative() in this patch,\nincluding the ones that were kept from the original in clean.c,\nwhich made sense because they were used in all if/else bodies that\nfollowed them but this patch makes it no longer true.\n\n> +\tif ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&\n> +\t    !resolve_gitlink_ref(path->buf, \"HEAD\", submodule_head)) {\n> +\t\tif (dry_run && !quiet)\n> +\t\t\tprintf(_(MSG_WOULD_IGNORE_GIT_DIR), quoted.buf);\n> +\t\telse if (!dry_run)\n> +\t\t\twarning(_(MSG_WARN_GIT_DIR_IGNORE), quoted.buf);\n> +\n> +\t\t*dir_gone = 0;\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tdir = opendir(path->buf);\n> +\tif (!dir) {\n> +\t\t/* an empty dir could be removed even if it is unreadble */\n> +\t\tres = dry_run ? 0 : rmdir(path->buf);\n> +\t\tif (res) {\n> +\t\t\twarning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);\n> +\t\t\t*dir_gone = 0;\n> +\t\t}\n> +\t\treturn res;\n> +\t}\n> +\n> +\tif (path->buf[original_len - 1] != '/')\n> +\t\tstrbuf_addch(path, '/');\n> +\n> +\tlen = path->len;\n> +\twhile ((e = readdir(dir)) != NULL) {\n> +\t\tstruct stat st;\n> +\t\tif (is_dot_or_dotdot(e->d_name))\n> +\t\t\tcontinue;\n> +\n> +\t\tstrbuf_setlen(path, len);\n> +\t\tstrbuf_addstr(path, e->d_name);\n> +\t\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\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_dirs(path, prefix, force_flag, dry_run, quiet, &gone))\n> +\t\t\t\tret = 1;\n> +\t\t\tif (gone)\n> +\t\t\t\tstring_list_append(&dels, quoted.buf);\n> +\t\t\telse\n> +\t\t\t\t*dir_gone = 0;\n> +\t\t\tcontinue;\n> +\t\t} else {\n> +\t\t\tres = dry_run ? 0 : unlink(path->buf);\n> +\t\t\tif (!res)\n> +\t\t\t\tstring_list_append(&dels, quoted.buf);\n> +\t\t\telse {\n> +\t\t\t\twarning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);\n> +\t\t\t\t*dir_gone = 0;\n> +\t\t\t\tret = 1;\n> +\t\t\t}\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/* path too long, stat fails, or non-directory still exists */\n> +\t\t*dir_gone = 0;\n> +\t\tret = 1;\n> +\t\tbreak;\n> +\t}\n> +\tclosedir(dir);\n> +\n> +\tstrbuf_setlen(path, original_len);\n> +\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n> +\tif (*dir_gone) {\n> +\t\tres = dry_run ? 0 : rmdir(path->buf);\n> +\t\tif (!res)\n> +\t\t\t*dir_gone = 1;\n> +\t\telse {\n> +\t\t\twarning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);\n> +\t\t\t*dir_gone = 0;\n> +\t\t\tret = 1;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!*dir_gone && !quiet) {\n> +\t\tfor (i = 0; i < dels.nr; i++)\n> +\t\t\tprintf(dry_run ?  _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), dels.items[i].string);\n> +\t}\n> +\tstring_list_clear(&dels, 0);\n> +\treturn ret;\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 ignored_only = 0, config_set = 0, errors = 0;\n> +\tint i, res;\n> +\tint dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;\n> +\tint ignored_only = 0, config_set = 0, errors = 0, gone = 1;\n>  \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n>  \tstruct strbuf directory = STRBUF_INIT;\n>  \tstruct dir_struct dir;\n> @@ -49,7 +155,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\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 +183,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,38 +256,23 @@ 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 (remove_directories || (matches == MATCHED_EXACTLY)) {\n> +\t\t\t\tif (remove_dirs(&directory, prefix, rm_flags, dry_run, quiet, &gone))\n>  \t\t\t\t\terrors++;\n> +\t\t\t\tif (gone && !quiet)\n> +\t\t\t\t\tprintf(dry_run ? _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), qname);\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\tres = dry_run ? 0 : unlink(ent->name);\n> +\t\t\tif (res) {\n> +\t\t\t\twarning(_(MSG_WARN_REMOVE_FAILED), qname);\n>  \t\t\t\terrors++;\n> -\t\t\t}\n> +\t\t\t} else if (!quiet)\n> +\t\t\t\tprintf(dry_run ? _(MSG_WOULD_REMOVE) :_(MSG_REMOVE), qname);\n\nspaces required around that ':' (ctx:WxV)\n#313: FILE: builtin/clean.c:275:\n+\t\t\t\tprintf(dry_run ? _(MSG_WOULD_REMOVE) :_(MSG_REMOVE), qname);\n\n>  \t\t}\n>  \t}\n>  \tfree(seen);\n\nThe updated code structure is much nicer than the previous round,\nbut I am somewhat puzzled how return value of remove_dirs() and\n&gone relate to each other.  Surely when gone is set to zero,\nremove_dirs() is reporting that the directory it was asked to remove\nrecursively did not go away, so it must report failure, no?  Having\nthe &gone flag looks redundant and checking for gone in some places\nwhile checking for the return value for others feels like an\ninvitation for future bugs.\n\nAlso the remove_dirs() function seems to replace the use of\nremove_dir_recurse() from dir.c by copying large part of it, with\nerror message sprinkled.  Does remove_dir_recurse() still get used\nby other codepaths?  If so, do the remaining callsites benefit from\nusing this updated version?\n\nThanks; will replace what has been sitting on the 'pu' branch with\nthis copy.\n"},{"id":"205883","messageId":"7v1ue32sv6.fsf@alter.siamese.dyndns.org","threadId":"32508","inReplyTo":"7vfw2j2vlp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] git-clean: Display more accurate delete messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-02T21:10:53Z","receivedAt":"2013-01-02T21:10:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Zoltan Klinger <zoltan.klinger@gmail.com> writes:\n>\n>> +static const char* MSG_REMOVE = \"Removing %s\\n\";\n>> +static const char* MSG_WOULD_REMOVE = \"Would remove %s\\n\";\n>> +static const char* MSG_WOULD_NOT_REMOVE = \"Would not remove %s\\n\";\n\nI also noticed that this message is not used, which mwans that the\nprogram used to say \"Would not remove\" for some paths but the\nupdated one will never do so.\n"},{"id":"205960","messageId":"CAKJhZwS6VUwWoX1QmNL19asNt1B3dPsDeg5-JTzq8FMd1WYkSw@mail.gmail.com","threadId":"32508","inReplyTo":"7vfw2j2vlp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] git-clean: Display more accurate delete messages","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2013-01-03T23:21:45Z","receivedAt":"2013-01-03T23:21:45Z","isPatch":true,"sender":{"key":"zoltan.klinger@gmail.com","avatar":"https://avatars.githubusercontent.com/u/95923?v=4"},"body":"> The updated code structure is much nicer than the previous round,\n> but I am somewhat puzzled how return value of remove_dirs() and\n> &gone relate to each other.  Surely when gone is set to zero,\n> remove_dirs() is reporting that the directory it was asked to remove\n> recursively did not go away, so it must report failure, no?  Having\n> the &gone flag looks redundant and checking for gone in some places\n> while checking for the return value for others feels like an\n> invitation for future bugs.\n\nThe return value of remove_dirs() has an overall effect on the exit\ncode of git-clean, and &gone indicates whether the directory we asked\nremove_dirs() to delete was actually removed. If all goes well  in\nremove_dirs() the return code is 0 and gone flag is 1. If file or\nsubdirectory delete fails return code is 1 and the gone flag is set to\n0. The special case is when remove_dirs() is asked to remove an\nuntracked git repo that should be ignored. In this case remove_dirs()\nis not going to remove the directory so the gone flag is set to zero\nbut it is not an error so the return value will be set to zero too.\n\n> Also the remove_dirs() function seems to replace the use of\n> remove_dir_recurse() from dir.c by copying large part of it, with\n> error message sprinkled.  Does remove_dir_recurse() still get used\n> by other codepaths?  If so, do the remaining callsites benefit from\n> using this updated version?\n\nIn dir.c the remove_dir_recurse() is a private function that is called\nby the public remove_dir_recursively() wrapper function. The\nremove_dir_recursively() function is called from the following places:\n\n    builtin/clone.c:387:\n    builtin/clone.c:392:\n    builtin/rm.c:349:\n    notes-merge.c:771:\n    refs.c:1527:\n    sequencer.c:27:\n    transport.c:247:\n    transport.c:393:\n\nThe messages that remove_dirs() prints out are very specific to\ngit-clean and they are not really relevant in the above places where\nremove_dir_recursively() is called from. Also, the remove logic for\nfiles is slightly different in remove_dirs() when it comes to handling\na failed file delete. While remove_dirs() continues removing other\nfiles in the same directory upon failure, remove_dir_recurse() will\nstop at the first error. So perhaps having the remove_dirs() in\nbuiltin/clean.c is OK.\n"}]}