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

Zoltan Klinger [off-list ref] writes:

+static void print_filtered(const char *msg, struct string_list *lst)
+{
+	int i;
+	char *name;
+	char *dir = 0;
+
+	sort_string_list(lst);
+
+	for (i = 0; i < lst->nr; i++) {
+		name = lst->items[i].string;
+		if (dir == 0 || strncmp(name, dir, strlen(dir)) != 0)
+			printf("%s %s\n", msg, name);
+		if (name[strlen(name) - 1] == '/')
+			dir = name;
+	}
+}

Here, prefixcmp() may be easier to read than strncmp().  We tend to
prefer writing comparison with zero like this:

	if (!dir || prefixcmp(name, dir))
		...

but I think we can go either way.

My reading of the above is that "lst" after sorting is expected to
have something like:

	a/
        a/b/
	a/b/to-be-removed
        a/to-be-removed

and we first show "a/", remember that prefix in "dir", not show
"a/b/" because it matches prefix, but still update the prefix to
"a/b/", not show "a/b/to-be-removed", and because "a/to-be-removed"
does not match the latest prefix, it is now shown.  Am I confused???

Moving the above logic to a single helper function makes sense, but
can we name it a bit more concisely?  Also this helper feels very
specific to "clean"---does it need to go to dir.[ch], I have to
wonder.

Other than the above two points, the resulting builtin/clean.c looks
much more nicely structured than before.

I am not very much pleased by the change to dir.[ch] in this patch,
though.

+static void append_dir_name(struct string_list *dels, struct string_list *skips,
+		struct string_list *errs, char *name, const char * prefix, int failed, int isdir)
+{
+	struct strbuf quoted = STRBUF_INIT;
+
+	quote_path_relative(name, strlen(name), &quoted, prefix);
+	if (isdir && quoted.buf[strlen(quoted.buf) -1] != '/')
+		strbuf_addch(&quoted, '/');
+
+	if (skips)
+		string_list_append(skips, quoted.buf);
+	else if (!failed && dels)
+		string_list_append(dels, quoted.buf);
+	else if (errs)
+		string_list_append(errs, quoted.buf);
+}

The three lists dels/skips/errs are mostly mutually exclusive (the
caller knows which one to throw the element in) except that failed
controls which one between dels or errs is used.

That's an ugly interface, I have to say.  I think the quote-path
part should become a separate helper function to be used by the
callers of this function, and the callers should stuff the path to
the list they want to put the element in.  That will eliminate the
need for this ugliness.

Also, didn't you make remove_dir_recursively() excessively leaky by
doing this?  The string in quoted is still created, even though the
caller passes NULL to all the lists.

Thanks.

Thanks for the feedback.

My reading of the above is that "lst" after sorting is expected to
have something like:

        a/
        a/b/
        a/b/to-be-removed
        a/to-be-removed

and we first show "a/", remember that prefix in "dir", not show
"a/b/" because it matches prefix, but still update the prefix to
"a/b/", not show "a/b/to-be-removed", and because "a/to-be-removed"
does not match the latest prefix, it is now shown.  Am I confused???

No, it's a bug. The correct output should be just "a/". Thanks for
pointing it out, I'm going to fix that.

quoted

@@ -150,43 +170,45 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
              if (S_ISDIR(st.st_mode)) {
                      strbuf_addstr(&directory, ent->name);
                      qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
-                     if (show_only && (remove_directories ||
-                         (matches == MATCHED_EXACTLY))) {
-                             printf(_("Would remove %s\n"), qname);
-                     } else if (remove_directories ||
-                                (matches == MATCHED_EXACTLY)) {
-                             if (!quiet)
-                                     printf(_("Removing %s\n"), qname);
-                             if (remove_dir_recursively(&directory,
-                                                        rm_flags) != 0) {
-                                     warning(_("failed to remove %s"), qname);
-                                     errors++;
-                             }
-                     } else if (show_only) {
-                             printf(_("Would not remove %s\n"), qname);
-                     } else {
-                             printf(_("Not removing %s\n"), qname);
+                     if (remove_directories || (matches == MATCHED_EXACTLY)) {
+                             remove_dir_recursively_with_dryrun(&directory, rm_flags, dry_run,
+                                             &dels, &skips, &errs, prefix);
                      }

Moving the above logic to a single helper function makes sense, but
can we name it a bit more concisely?  Also this helper feels very
specific to "clean"---does it need to go to dir.[ch], I have to
wonder.

Would you have a better name in mind for the
remove_dir_recursively_with_dryrun() function? I'm kinda stuck.

My thinking was that since the private function remove_dir_recurse()
in dir.c already handles the recursive removing of files and
directories and checks for nested git directories, it would be better
to modify that function rather than implement something similar but
with dels, skips and errs lists in clean.c.

I am not very much pleased by the change to dir.[ch] in this patch,
though.

quoted

+static void append_dir_name(struct string_list *dels, struct string_list *skips,
+             struct string_list *errs, char *name, const char * prefix, int failed, int isdir)
+{
+     struct strbuf quoted = STRBUF_INIT;
+
+     quote_path_relative(name, strlen(name), &quoted, prefix);
+     if (isdir && quoted.buf[strlen(quoted.buf) -1] != '/')
+             strbuf_addch(&quoted, '/');
+
+     if (skips)
+             string_list_append(skips, quoted.buf);
+     else if (!failed && dels)
+             string_list_append(dels, quoted.buf);
+     else if (errs)
+             string_list_append(errs, quoted.buf);
+}

The three lists dels/skips/errs are mostly mutually exclusive (the
caller knows which one to throw the element in) except that failed
controls which one between dels or errs is used.

That's an ugly interface, I have to say.  I think the quote-path
part should become a separate helper function to be used by the
callers of this function, and the callers should stuff the path to
the list they want to put the element in.  That will eliminate the
need for this ugliness.

Will get rid of append_dir_name() and reimplement things the way you
suggested above.

Cheers,
Zoltan