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

From: Jiang Xin <worldhello.net@gmail.com>
Date: May 9, 2013, 16:35 UTC
Message-ID: <CANYiYbEvYXiRE0OeR5vqBAaOFFpJTPBetQbawZD5EcMCj8veFw@mail.gmail.com>
In-Reply-To: <7vr4hhflym.fsf@alter.siamese.dyndns.org>

2013/5/9 Junio C Hamano <gitster@pobox.com>:

Show 9 quoted lines

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

Show 13 quoted lines

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

Show 10 quoted lines

>
> 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)

Show 11 quoted lines

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

Show 16 quoted lines

> 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