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

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

[Zoltan Klinger]

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

Consider the following repo layout:

  test.git/
    |-- tracked_dir/
    |     |-- some_tracked_file
    |     |-- some_untracked_file
    |-- tracked_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) Create a recursive delete function 'remove_dirs' in builtin/clean.c
      to run in both dry_run and delete modes with the delete logic as
      follows:
        (a) Check if the current directory to be deleted is an untracked
            git repository. If it is and --force --force option is not set
            do not touch this directory, print ignore message, set dir_gone
            flag to false for the caller and return.
        (b) Otherwise for each item in current directory:
              (i)   If current directory cannot be accessed, print warning,
                    set dir_gone flag to false and return.
              (ii)  If the item is a subdirectory recurse into it,
                    check for the returned value of the dir_gone flag.
                    If the subdirectory is gone, add the name of the deleted
                    directory to a list of successfully removed items 'dels'.
                    Else set the dir_gone flag as the current directory
                    cannot be removed because we have at least one subdirectory
                    hanging around.
              (iii) If it is a file try to remove it. If success add the
                    file name to the 'dels' list, else print error and set
                    dir_gone flag to false.
        (c) After we finished deleting all items in the current directory and
            the dir_gone flag is still true, remove the directory itself.
            If failed set the dir_gone flag to false.

        (d) If the current directory cannot be deleted because the dir_gone flag
            has been set to false, print out all the successfully deleted items
            for this directory from the 'dels' list.
        (e) We're done with the current directory, return.

  (2) Modify the cmd_clean() function to:
        (a) call the recursive delete function 'remove_dirs()' for each
            topmost directory it wants to remove
        (b) check for the returned value of dir_gone flag. If it's true
            print the name of the directory as being removed.

Consider the output of the improved version:

  $ git clean -fd
  Removing tracked_dir/some_untracked_file
  Removing untracked_file
  warning: ignoring untracked git repository untracked_foo/frotz.git
  Removing untracked_foo/bar
  Removing untracked_foo/emptydir
  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.

Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

Signed-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>
---
 builtin/clean.c |  149 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 120 insertions(+), 29 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 69c1cda..37e403a 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -10,6 +10,7 @@
 #include "cache.h"
 #include "dir.h"
 #include "parse-options.h"
+#include "refs.h"
 #include "string-list.h"
 #include "quote.h"
 
@@ -20,6 +21,13 @@ static const char *const builtin_clean_usage[] = {
 	NULL
 };
 
+static const char* MSG_REMOVE = "Removing %s\n";
+static const char* MSG_WOULD_REMOVE = "Would remove %s\n";
+static const char* MSG_WOULD_NOT_REMOVE = "Would not remove %s\n";
+static const char* MSG_WOULD_IGNORE_GIT_DIR = "Would ignore untracked git repository %s\n";
+static const char* MSG_WARN_GIT_DIR_IGNORE = "ignoring untracked git repository %s";
+static const char* MSG_WARN_REMOVE_FAILED = "failed to remove %s";
+
 static int git_clean_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "clean.requireforce"))
@@ -34,11 +42,109 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
+		int dry_run, int quiet, int *dir_gone)
+{
+	DIR *dir;
+	struct strbuf quoted = STRBUF_INIT;
+	struct dirent *e;
+	int res = 0, ret = 0, gone = 1, original_len = path->len, len, i;
+	unsigned char submodule_head[20];
+	struct string_list dels = STRING_LIST_INIT_DUP;
+
+	*dir_gone = 1;
+
+	quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
+	if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
+	    !resolve_gitlink_ref(path->buf, "HEAD", submodule_head)) {
+		if (dry_run && !quiet)
+			printf(_(MSG_WOULD_IGNORE_GIT_DIR), quoted.buf);
+		else if (!dry_run)
+			warning(_(MSG_WARN_GIT_DIR_IGNORE), quoted.buf);
+
+		*dir_gone = 0;
+		return 0;
+	}
+
+	dir = opendir(path->buf);
+	if (!dir) {
+		/* an empty dir could be removed even if it is unreadble */
+		res = dry_run ? 0 : rmdir(path->buf);
+		if (res) {
+			warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
+			*dir_gone = 0;
+		}
+		return res;
+	}
+
+	if (path->buf[original_len - 1] != '/')
+		strbuf_addch(path, '/');
+
+	len = path->len;
+	while ((e = readdir(dir)) != NULL) {
+		struct stat st;
+		if (is_dot_or_dotdot(e->d_name))
+			continue;
+
+		strbuf_setlen(path, len);
+		strbuf_addstr(path, e->d_name);
+		quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
+		if (lstat(path->buf, &st))
+			; /* fall thru */
+		else if (S_ISDIR(st.st_mode)) {
+			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
+				ret = 1;
+			if (gone)
+				string_list_append(&dels, quoted.buf);
+			else
+				*dir_gone = 0;
+			continue;
+		} else {
+			res = dry_run ? 0 : unlink(path->buf);
+			if (!res)
+				string_list_append(&dels, quoted.buf);
+			else {
+				warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
+				*dir_gone = 0;
+				ret = 1;
+			}
+			continue;
+		}
+
+		/* path too long, stat fails, or non-directory still exists */
+		*dir_gone = 0;
+		ret = 1;
+		break;
+	}
+	closedir(dir);
+
+	strbuf_setlen(path, original_len);
+	quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
+
+	if (*dir_gone) {
+		res = dry_run ? 0 : rmdir(path->buf);
+		if (!res)
+			*dir_gone = 1;
+		else {
+			warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
+			*dir_gone = 0;
+			ret = 1;
+		}
+	}
+
+	if (!*dir_gone && !quiet) {
+		for (i = 0; i < dels.nr; i++)
+			printf(dry_run ?  _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), dels.items[i].string);
+	}
+	string_list_clear(&dels, 0);
+	return ret;
+}
+
 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 ignored_only = 0, config_set = 0, errors = 0;
+	int i, res;
+	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
+	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
 	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
 	struct strbuf directory = STRBUF_INIT;
 	struct dir_struct dir;
@@ -49,7 +155,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	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 +183,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,38 +256,23 @@ 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);
+			if (remove_directories || (matches == MATCHED_EXACTLY)) {
+				if (remove_dirs(&directory, prefix, rm_flags, dry_run, quiet, &gone))
 					errors++;
-				}
-			} else if (show_only) {
-				printf(_("Would not remove %s\n"), qname);
-			} else {
-				printf(_("Not removing %s\n"), qname);
+				if (gone && !quiet)
+					printf(dry_run ? _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), qname);
 			}
 			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);
+			res = dry_run ? 0 : unlink(ent->name);
+			if (res) {
+				warning(_(MSG_WARN_REMOVE_FAILED), qname);
 				errors++;
-			}
+			} else if (!quiet)
+				printf(dry_run ? _(MSG_WOULD_REMOVE) :_(MSG_REMOVE), qname);
 		}
 	}
 	free(seen);
-- 
1.7.9.5

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

[Junio C Hamano]

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

> +static const char* MSG_REMOVE = "Removing %s\n";
> +static const char* MSG_WOULD_REMOVE = "Would remove %s\n";
> +static const char* MSG_WOULD_NOT_REMOVE = "Would not remove %s\n";
> +static const char* MSG_WOULD_IGNORE_GIT_DIR = "Would ignore untracked git repository %s\n";
> +static const char* MSG_WARN_GIT_DIR_IGNORE = "ignoring untracked git repository %s";
> +static const char* MSG_WARN_REMOVE_FAILED = "failed to remove %s";

"foo* bar" should be "foo *bar".  Also I personally find these
upcased message constants somewhat hard to read in the sites of use
in the code below.

Also gettext machinery needs to be told that these strings may be
asked to be replaced with their translations using _() elsewhere in
the code.  I.e.

	static const char *msg_remove = N_("Removing %s\n");

Aren't WOULD_IGNORE_GIT_DIR and WARN_GIT_DIR_IGNORE named somewhat
inconsistently?  Perhaps the latter is WARN_IGNORED_GIT_DIR or
something?

> @@ -34,11 +42,109 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>  
> +static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
> +		int dry_run, int quiet, int *dir_gone)
> +{
> +	DIR *dir;
> +	struct strbuf quoted = STRBUF_INIT;
> +	struct dirent *e;
> +	int res = 0, ret = 0, gone = 1, original_len = path->len, len, i;
> +	unsigned char submodule_head[20];
> +	struct string_list dels = STRING_LIST_INIT_DUP;
> +
> +	*dir_gone = 1;
> +
> +	quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);

Shouldn't this be inside the next if() statement body?  I also think
you could even omit this call when (!dry_run && quiet).  The same
comment applies to all uses of quote_path_relative() in this patch,
including the ones that were kept from the original in clean.c,
which made sense because they were used in all if/else bodies that
followed them but this patch makes it no longer true.

> +	if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
> +	    !resolve_gitlink_ref(path->buf, "HEAD", submodule_head)) {
> +		if (dry_run && !quiet)
> +			printf(_(MSG_WOULD_IGNORE_GIT_DIR), quoted.buf);
> +		else if (!dry_run)
> +			warning(_(MSG_WARN_GIT_DIR_IGNORE), quoted.buf);
> +
> +		*dir_gone = 0;
> +		return 0;
> +	}
> +
> +	dir = opendir(path->buf);
> +	if (!dir) {
> +		/* an empty dir could be removed even if it is unreadble */
> +		res = dry_run ? 0 : rmdir(path->buf);
> +		if (res) {
> +			warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
> +			*dir_gone = 0;
> +		}
> +		return res;
> +	}
> +
> +	if (path->buf[original_len - 1] != '/')
> +		strbuf_addch(path, '/');
> +
> +	len = path->len;
> +	while ((e = readdir(dir)) != NULL) {
> +		struct stat st;
> +		if (is_dot_or_dotdot(e->d_name))
> +			continue;
> +
> +		strbuf_setlen(path, len);
> +		strbuf_addstr(path, e->d_name);
> +		quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
> +		if (lstat(path->buf, &st))
> +			; /* fall thru */
> +		else if (S_ISDIR(st.st_mode)) {
> +			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
> +				ret = 1;
> +			if (gone)
> +				string_list_append(&dels, quoted.buf);
> +			else
> +				*dir_gone = 0;
> +			continue;
> +		} else {
> +			res = dry_run ? 0 : unlink(path->buf);
> +			if (!res)
> +				string_list_append(&dels, quoted.buf);
> +			else {
> +				warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
> +				*dir_gone = 0;
> +				ret = 1;
> +			}
> +			continue;
> +		}
> +
> +		/* path too long, stat fails, or non-directory still exists */
> +		*dir_gone = 0;
> +		ret = 1;
> +		break;
> +	}
> +	closedir(dir);
> +
> +	strbuf_setlen(path, original_len);
> +	quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
> +	if (*dir_gone) {
> +		res = dry_run ? 0 : rmdir(path->buf);
> +		if (!res)
> +			*dir_gone = 1;
> +		else {
> +			warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
> +			*dir_gone = 0;
> +			ret = 1;
> +		}
> +	}
> +
> +	if (!*dir_gone && !quiet) {
> +		for (i = 0; i < dels.nr; i++)
> +			printf(dry_run ?  _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), dels.items[i].string);
> +	}
> +	string_list_clear(&dels, 0);
> +	return ret;
> +}

>  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 ignored_only = 0, config_set = 0, errors = 0;
> +	int i, res;
> +	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
> +	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
>  	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
>  	struct strbuf directory = STRBUF_INIT;
>  	struct dir_struct dir;
> @@ -49,7 +155,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
>  	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 +183,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,38 +256,23 @@ 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 (remove_directories || (matches == MATCHED_EXACTLY)) {
> +				if (remove_dirs(&directory, prefix, rm_flags, dry_run, quiet, &gone))
>  					errors++;
> +				if (gone && !quiet)
> +					printf(dry_run ? _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), qname);
>  			}
>  			strbuf_reset(&directory);
>  		} else {
>  			if (pathspec && !matches)
>  				continue;
>  			qname = quote_path_relative(ent->name, -1, &buf, prefix);
> +			res = dry_run ? 0 : unlink(ent->name);
> +			if (res) {
> +				warning(_(MSG_WARN_REMOVE_FAILED), qname);
>  				errors++;
> -			}
> +			} else if (!quiet)
> +				printf(dry_run ? _(MSG_WOULD_REMOVE) :_(MSG_REMOVE), qname);

spaces required around that ':' (ctx:WxV)
#313: FILE: builtin/clean.c:275:
+				printf(dry_run ? _(MSG_WOULD_REMOVE) :_(MSG_REMOVE), qname);

>  		}
>  	}
>  	free(seen);

The updated code structure is much nicer than the previous round,
but I am somewhat puzzled how return value of remove_dirs() and
&gone relate to each other.  Surely when gone is set to zero,
remove_dirs() is reporting that the directory it was asked to remove
recursively did not go away, so it must report failure, no?  Having
the &gone flag looks redundant and checking for gone in some places
while checking for the return value for others feels like an
invitation for future bugs.

Also the remove_dirs() function seems to replace the use of
remove_dir_recurse() from dir.c by copying large part of it, with
error message sprinkled.  Does remove_dir_recurse() still get used
by other codepaths?  If so, do the remaining callsites benefit from
using this updated version?

Thanks; will replace what has been sitting on the 'pu' branch with
this copy.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Zoltan Klinger <zoltan.klinger@gmail.com> writes:
>
>> +static const char* MSG_REMOVE = "Removing %s\n";
>> +static const char* MSG_WOULD_REMOVE = "Would remove %s\n";
>> +static const char* MSG_WOULD_NOT_REMOVE = "Would not remove %s\n";

I also noticed that this message is not used, which mwans that the
program used to say "Would not remove" for some paths but the
updated one will never do so.

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

[Zoltan Klinger]

> The updated code structure is much nicer than the previous round,
> but I am somewhat puzzled how return value of remove_dirs() and
> &gone relate to each other.  Surely when gone is set to zero,
> remove_dirs() is reporting that the directory it was asked to remove
> recursively did not go away, so it must report failure, no?  Having
> the &gone flag looks redundant and checking for gone in some places
> while checking for the return value for others feels like an
> invitation for future bugs.

The return value of remove_dirs() has an overall effect on the exit
code of git-clean, and &gone indicates whether the directory we asked
remove_dirs() to delete was actually removed. If all goes well  in
remove_dirs() the return code is 0 and gone flag is 1. If file or
subdirectory delete fails return code is 1 and the gone flag is set to
0. The special case is when remove_dirs() is asked to remove an
untracked git repo that should be ignored. In this case remove_dirs()
is not going to remove the directory so the gone flag is set to zero
but it is not an error so the return value will be set to zero too.

> Also the remove_dirs() function seems to replace the use of
> remove_dir_recurse() from dir.c by copying large part of it, with
> error message sprinkled.  Does remove_dir_recurse() still get used
> by other codepaths?  If so, do the remaining callsites benefit from
> using this updated version?

In dir.c the remove_dir_recurse() is a private function that is called
by the public remove_dir_recursively() wrapper function. The
remove_dir_recursively() function is called from the following places:

    builtin/clone.c:387:
    builtin/clone.c:392:
    builtin/rm.c:349:
    notes-merge.c:771:
    refs.c:1527:
    sequencer.c:27:
    transport.c:247:
    transport.c:393:

The messages that remove_dirs() prints out are very specific to
git-clean and they are not really relevant in the above places where
remove_dir_recursively() is called from. Also, the remove logic for
files is slightly different in remove_dirs() when it comes to handling
a failed file delete. While remove_dirs() continues removing other
files in the same directory upon failure, remove_dir_recurse() will
stop at the first error. So perhaps having the remove_dirs() in
builtin/clean.c is OK.