{"thread":{"id":"32542","subject":"[PATCH v4] git-clean: Display more accurate delete messages","startedAt":"2013-01-06T23:16:59Z","lastAt":"2013-01-10T17:45:36Z","messageCount":6,"participants":["Zoltan Klinger","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"206191","messageId":"1357514219-16102-1-git-send-email-zoltan.klinger@gmail.com","threadId":"32542","inReplyTo":null,"subject":"[PATCH v4] git-clean: Display more accurate delete messages","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2013-01-06T23:16:59Z","receivedAt":"2013-01-06T23:16: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 |  158 +++++++++++++++++++++++++++++++++++++++++++++----------\n 1 file changed, 129 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 69c1cda..1714546 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,12 @@ static const char *const builtin_clean_usage[] = {\n \tNULL\n };\n \n+static const char *msg_remove = N_(\"Removing %s\\n\");\n+static const char *msg_would_remove = N_(\"Would remove %s\\n\");\n+static const char *msg_would_ignore_git_dir = N_(\"Would ignore untracked git repository %s\\n\");\n+static const char *msg_warn_ignore_git_dir = N_(\"ignoring untracked git repository %s\");\n+static const char *msg_warn_remove_failed = N_(\"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 +41,116 @@ 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+\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\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n+\t\t\tprintf(_(msg_would_ignore_git_dir), quoted.buf);\n+\t\t} else if (!dry_run) {\n+\t\t\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n+\t\t\twarning(_(msg_warn_ignore_git_dir), quoted.buf);\n+\t\t}\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\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\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\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\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n+\t\t\t\tstring_list_append(&dels, quoted.buf);\n+\t\t\t}\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\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n+\t\t\t\tstring_list_append(&dels, quoted.buf);\n+\t\t\t}\n+\t\t\telse {\n+\t\t\t\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\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+\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\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\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 +161,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 +189,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@@ -149,38 +261,26 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \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\tif (gone && !quiet) {\n+\t\t\t\t\tqname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n+\t\t\t\t\tprintf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);\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}\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\tqname = quote_path_relative(ent->name, -1, &buf, prefix);\n+\t\t\t\twarning(_(msg_warn_remove_failed), qname);\n \t\t\t\terrors++;\n+\t\t\t} else if (!quiet) {\n+\t\t\t\tqname = quote_path_relative(ent->name, -1, &buf, prefix);\n+\t\t\t\tprintf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);\n \t\t\t}\n \t\t}\n \t}\n-- \n1.7.9.5\n"},{"id":"206192","messageId":"20130106234022.GB3823@elie.Belkin","threadId":"32542","inReplyTo":"1357514219-16102-1-git-send-email-zoltan.klinger@gmail.com","subject":"Re: [PATCH v4] git-clean: Display more accurate delete messages","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-06T23:40:22Z","receivedAt":"2013-01-06T23:40:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Zoltan Klinger wrote:\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>\n> The message displayed to the user is slightly misleading. The foo/\n> directory has not been removed because of foo/frotz.git still exists.\n> On the other hand the subdirectories 'bar' and 'emptydir' have been\n> deleted but they're not mentioned anywhere. Also, untracked_some.git\n> has not been removed either.\n[...]\n> Consider 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\nThanks, this looks like a nice improvement.\n\nI wonder whether it's possible to make the output more consistent,\nas in:\n\n\tRemoving tracked_dir/some_untracked_file\n\tRemoving untracked_file\n\tSkipping repository untracked_foo/frotz.git\n\tRemoving untracked_foo/bar\n\tRemoving untracked_foo/emptydir\n\tSkipping repository untracked_some.git\n\nor similar.  What do you think?\n\nThanks,\nJonathan\n"},{"id":"206440","messageId":"CAKJhZwRxkR=W8a3tmnL7P_BgpYzdHRYMEFsCp+QLcpO8Bz4YaQ@mail.gmail.com","threadId":"32542","inReplyTo":"20130106234022.GB3823@elie.Belkin","subject":"Re: [PATCH v4] git-clean: Display more accurate delete messages","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2013-01-10T01:01:00Z","receivedAt":"2013-01-10T01:01:00Z","isPatch":true,"sender":{"key":"zoltan.klinger@gmail.com","avatar":"https://avatars.githubusercontent.com/u/95923?v=4"},"body":"> I wonder whether it's possible to make the output more consistent,\n> as in:\n>\n>     Removing tracked_dir/some_untracked_file\n>     Removing untracked_file\n>     Skipping repository untracked_foo/frotz.git\n>     Removing untracked_foo/bar\n>     Removing untracked_foo/emptydir\n>     Skipping repository untracked_some.git\n>\n> or similar.  What do you think?\n\nAgree, the output looks much neater printed like that. I am going to\nupdate the patch unless someone feels strongly against the proposed\noutput.\n"},{"id":"206441","messageId":"7vk3rl3fw5.fsf@alter.siamese.dyndns.org","threadId":"32542","inReplyTo":"1357514219-16102-1-git-send-email-zoltan.klinger@gmail.com","subject":"Re: [PATCH v4] git-clean: Display more accurate delete messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-10T02:56:10Z","receivedAt":"2013-01-10T02:56:10Z","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> Consider 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>\n> Now it displays only the file and directory names that got actually\n> deleted and shows warnings about ignored untracked git repositories.\n>\n> Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>\n>\n> Signed-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>\n> ---\n\nI think the code before this patch used to say \"Would not remove\"\nand \"Not removing\" in certain cases to report the paths that the\ncommand decided not to remove, but after this patch these two\nmessages no longer appear in the patch.\n\nIs it expected, are we losing information, or...?\n\n>  builtin/clean.c |  158 +++++++++++++++++++++++++++++++++++++++++++++----------\n>  1 file changed, 129 insertions(+), 29 deletions(-)\n>\n> diff --git a/builtin/clean.c b/builtin/clean.c\n> index 69c1cda..1714546 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,12 @@ static const char *const builtin_clean_usage[] = {\n>  \tNULL\n>  };\n>  \n> +static const char *msg_remove = N_(\"Removing %s\\n\");\n> +static const char *msg_would_remove = N_(\"Would remove %s\\n\");\n> +static const char *msg_would_ignore_git_dir = N_(\"Would ignore untracked git repository %s\\n\");\n> +static const char *msg_warn_ignore_git_dir = N_(\"ignoring untracked git repository %s\");\n> +static const char *msg_warn_remove_failed = N_(\"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 +41,116 @@ 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> +\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\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n> +\t\t\tprintf(_(msg_would_ignore_git_dir), quoted.buf);\n> +\t\t} else if (!dry_run) {\n> +\t\t\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n> +\t\t\twarning(_(msg_warn_ignore_git_dir), quoted.buf);\n> +\t\t}\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\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\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\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\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n> +\t\t\t\tstring_list_append(&dels, quoted.buf);\n> +\t\t\t}\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\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\n> +\t\t\t\tstring_list_append(&dels, quoted.buf);\n> +\t\t\t}\n> +\t\t\telse {\n> +\t\t\t\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\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> +\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\tquote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);\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 +161,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 +189,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> @@ -149,38 +261,26 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n>  \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\tif (gone && !quiet) {\n> +\t\t\t\t\tqname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n> +\t\t\t\t\tprintf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);\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}\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\tqname = quote_path_relative(ent->name, -1, &buf, prefix);\n> +\t\t\t\twarning(_(msg_warn_remove_failed), qname);\n>  \t\t\t\terrors++;\n> +\t\t\t} else if (!quiet) {\n> +\t\t\t\tqname = quote_path_relative(ent->name, -1, &buf, prefix);\n> +\t\t\t\tprintf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);\n>  \t\t\t}\n>  \t\t}\n>  \t}\n"},{"id":"206447","messageId":"CAKJhZwQ=RzLgCBUrx_QKY8Xzh-L8QC2UVcQJEpYxxGQz=8LwwQ@mail.gmail.com","threadId":"32542","inReplyTo":"7vk3rl3fw5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4] git-clean: Display more accurate delete messages","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2013-01-10T10:41:56Z","receivedAt":"2013-01-10T10:41:56Z","isPatch":true,"sender":{"key":"zoltan.klinger@gmail.com","avatar":"https://avatars.githubusercontent.com/u/95923?v=4"},"body":"> I think the code before this patch used to say \"Would not remove\"\n> and \"Not removing\" in certain cases to report the paths that the\n> command decided not to remove, but after this patch these two\n> messages no longer appear in the patch.\n>\n> Is it expected, are we losing information, or...?\n>\n\nI do not think we are losing any information.\nSay, we have a repo like this:\n    test.git\n     |-- untracked_file\n     |-- untracked_bar\n     |     |-- bar.txt\n     |-- untracked_foo\n           |-- foo.txt\n\nThe original version prints out:\n  $ git clean -fn\n  Would remove untracked_file\n  Would not remove untracked_bar/\n  Would not remove untracked_foo/\n\nWe never asked for any directories to be removed so IMHO the \"Would\nnot remove ...\" messages are just noise.\n\nThe new version prints out:\n  $ git clean -fn\n  Would remove untracked_file\n"},{"id":"206459","messageId":"7v7gnl2apr.fsf@alter.siamese.dyndns.org","threadId":"32542","inReplyTo":"CAKJhZwQ=RzLgCBUrx_QKY8Xzh-L8QC2UVcQJEpYxxGQz=8LwwQ@mail.gmail.com","subject":"Re: [PATCH v4] git-clean: Display more accurate delete messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-10T17:45:36Z","receivedAt":"2013-01-10T17:45:36Z","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>> I think the code before this patch used to say \"Would not remove\"\n>> and \"Not removing\" in certain cases to report the paths that the\n>> command decided not to remove, but after this patch these two\n>> messages no longer appear in the patch.\n>>\n>> Is it expected, are we losing information, or...?\n>\n> I do not think we are losing any information.\n> Say, we have a repo like this:\n>     test.git\n>      |-- untracked_file\n>      |-- untracked_bar\n>      |     |-- bar.txt\n>      |-- untracked_foo\n>            |-- foo.txt\n>\n> The original version prints out:\n>   $ git clean -fn\n>   Would remove untracked_file\n>   Would not remove untracked_bar/\n>   Would not remove untracked_foo/\n>\n> We never asked for any directories to be removed so IMHO the \"Would\n> not remove ...\" messages are just noise.\n>\n> The new version prints out:\n>   $ git clean -fn\n>   Would remove untracked_file\n\nOh.  I was blinded by the primary reason of your patch being \"be\nmore careful and defer reporting a removal until we know everything\nin a directory did get removed and managed to remove the directory\",\nand did not realize this \"noise removal\".\n\nPerhaps add\n\n\tAlso do not mention that we are not removing directories\n\twhen the user did not ask us to do so with '-d'.\n\nor something to the description?\n\nThanks for a clarification.\n"}]}