2013/5/9 Junio C Hamano <[email protected]>:
>> @@ -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".
>> +
>> + 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)
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
