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.
>
>> @@ -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.
>
>> +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), "ed, prefix);
>> + if (isdir && quoted.buf[strlen(quoted.buf) -1] != '/')
>> + strbuf_addch("ed, '/');
>> +
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html