Re: [PATCH v3] Add support for -i/--interactive to git-clean

> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> Show what would be done and the user must confirm before actually
>> cleaning. In the confirmation dialog, the user has three choices:
>>
>>  * Yes: Start to do cleaning.
>>  * No:  Nothing will be deleted.
>>  * Edit (default): Enter edit mode.
>
> I like this much more than the previous one. I played with it a bit, and
> found it much more pleasant than "rm -i": by default, only one querry,
> but still an option to select which files to clean.

>> Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>
> Please, no. I already mentionned it in my previous patch, but I did not
> review the patch. See SubmittingPatches:
>
>   3. "Reviewed-by:", unlike the other tags, can only be offered by the
>      reviewer and means that she is completely satisfied that the patch
>      is ready for application.  It is usually offered only after a
>      detailed review.
>
> Commenting != reviewing.

>
>> +             /* dels list may become empty when we run string_list_remove_empty_items later */
>> +             if (!dels->nr)
>> +                     break;
>
> This happens when the user removed everything from the list in the edit
> mode. This could print something before breaking (and then exiting
> silently). Maybe "No more files to clean, exiting." or so.

>> +                     printf(_("Remove (yes/no/Edit) ? "));
>> +                     strbuf_getline(&confirm, stdin, '\n');
>> +                     strbuf_trim(&confirm);
>> +                     if (confirm.len) {
>> +                             if (!strncasecmp(confirm.buf, "yes", confirm.len)) {
>> +                                     break;
>> +                             } else if (!strncasecmp(confirm.buf, "no", confirm.len)) {
>> +                                     string_list_clear(dels, 0);
>> +                                     break;
>> +                             }
>> +                     }
>> +                     edit_mode = 1;
>
> It's weird that anything but "yes" and "no" enter the edit mode without
> complaining. It's safe, but surprising. If I type "foo", I'd rather get
> an error and be asked again.
>
>> +             if (!matches) {
>> +                     strbuf_addf(&message, _("WARNING: Cannot find items prefixed by: %s"), confirm.buf);
>
> "prefixed" seems a remainder of the previous version of the patch. You
> probably mean "matched by: %s".