Re: [PATCH v7 01/10] Add support for -i/--interactive to git-clean

2013/5/9 Junio C Hamano [off-list ref]:

quoted

@@ -15,9 +15,12 @@
 #include "quote.h"

 static int force = -1; /* unset */
+static int interactive;
+static struct string_list del_list = STRING_LIST_INIT_DUP;
+static const char **the_prefix;

Ehh, why?

Next reroll will save relative paths in del_list, and eliminate "**the_prefix".

quoted

+
+             printf(_("Input ignore patterns>> "));
+             if (strbuf_getline(&confirm, stdin, '\n') != EOF) {
+                     strbuf_trim(&confirm);
+             } else {
+                     putchar('\n');
+                     break;

Why break here?  If we got nothing, wouldn't confirm.len be zero?
If we did get something but the input got flushed without line-end,
sending '\n' to the terminal may be justified, but in that case you
would may have something useful, and asking confirm.len if it is
empty would be the consistent way to check between two cases, no?

Yes, this break is unnecessary, it left from pervious revision.

A few points:

 * Pass prefix as a parameter to this function, just like how
   remove_dirs() gets called, and get rid of the_prefix.

 * The result of quote_* is designed to avoid ambiguities, by
   applying C-style quotes like HT => \t and adding "" pair around
   it as necessary.  I doubt feeding it to is_excluded() makes any
   sense.  You probably meant path_relative(), but I am not sure.

Appreciated, that is what I need. I write a local version of path_relative,
a combination of path_relative (in quote.c) and relative_path (in path.c),
like this:

        static const char *path_relative(const char *in, const char *prefix)

quoted

+     for_each_string_list_item(item, &del_list) {
+             struct stat st;
+
+             if (lstat(item->string, &st))
+                     continue;

Ignoring errors silently?

With the "interactive" stuff, can you get into a situation where you
originally propose to remove D and D/F but the user tells you to
remove D (editing D/F away), or vice versa?

I can not find out such a case, that remove parent directory D,
while left file in it, such as D/F.

I think this patch should be in at least two parts:

 - Introduce the two-phase "collect in del_list, remove in a
   separate loop at the end" restructuring.

 - (optional, if you are feeling ambitious) Change the path that is
   stored in del_list relative to the prefix, so that all functions
   that operate on the string in the del_list do not have to do
   *_relative() thing.  Some functions may instead have to prepend
   prefix but if they are minority compared to the users of
   *_relative(), it may be an overall win from the readability's
   point of view.

 - Add the "interactively allow you to reduce the del_list" bit
   between the two phases.

Will send new reroll soon.


-- 
Jiang Xin