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

[Junio C Hamano <gitster@pobox.com>]

Zoltan Klinger <zoltan.klinger@gmail.com> 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???

> @@ -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.

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.