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

From: Jiang Xin <worldhello.net@gmail.com>
Date: Apr 29, 2013, 16:15 UTC
Message-ID: <CANYiYbHKWv6R2vtwG=bTNhj8q0iC4EBt8usC3posBCtYBTXOvA@mail.gmail.com>
In-Reply-To: <vpqwqrl931e.fsf@grenoble-inp.fr>

2013/4/29 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:

Show 12 quoted lines

> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> Show what would be done and a confirmation dialog before actually
>> cleaning. In the confirmation dialog, the user can input a space
>> separated prefix list, and each clean candidate that matches with
>> one of prefix, will be excluded from cleaning.
>
> That seems a really weird interface. In particular, that means people
> typing "no" or "n" mechanically will have everything not starting with
> "n" or "no" deleted. We've learnt from the "git send-email" interface
> that people (including me ;-) ) do type "yes" mechanically to some
> questions.

Because of this (typing "no" or "n" not stop the whole cleaning), I write a loop, only when user press Enter (input nothing), begin to clean files/directories, and there would be a warning if nothing excluded by the prefix/pattern.

That means when user input "n" or "no", the interactive git-clean will not delete entries starting with "n" or "no" (instead of delete nothing), but it still give the user a hint what happened. It will show the filtered cleaning list or a warning message that there are no entries starting with "n" or "no" to be filtered out (may be the warning messages should be cached and displayed in the end, not at the beginning).

>
> My feeling is that you're doing a bad compromise between a confirmation
> dialog (y/n) and a really interactive command (like "git add -i") with a
> rich interface.

It's not confirmation, but a last chance to save your cleaning files. The use can input a space-seperated prefixes list, but I think most people would like to input a space-seperated patterns (like .gitignore). I try to implement like this, but still need sometime to refector the code.

+                               el = add_exclude_list(&dir, EXC_CMDL,
"exclude by interactive git clean");
+                               add_exclude((*prefix_list)->buf, "",
0, el, -(++i));

git-clean is a simple task, I doubt write a rich interface has no much value, and make things complicated. But render prompt, error outputs in different color like interactive git-add is interesting.

Show 20 quoted lines

>> @@ -34,7 +34,17 @@ OPTIONS
>>  -f::
>>  --force::
>>       If the Git configuration variable clean.requireForce is not set
>> -     to false, 'git clean' will refuse to run unless given -f or -n.
>> +     to false, 'git clean' will refuse to run unless given -f, -n or
>> +     -i.
>> +
>> +-i::
>> +--interactive::
>> +  Show what would be done and a confirmation dialog before actually
>> +  cleaning. In the confirmation dialog, the user can input a space
>> +  separated prefix list, and each clean candidate that matches with
>> +  one of prefix, will be excluded from cleaning. When the user feels
>> +  it's OK, press ENTER to start cleaning. If the user wants to cancel
>> +  the whole cleaning, simply input ctrl-c in the confirmation dialog.
>
> Broken indentation. It seems you've set your tab-width to 2, in which
> case you can't see it, but you've indented with 2 spaces where the rest
> of the file is indented with tabs.

Oops, I code lots of ruby recently, and vim is not smart to handle txt file. I will correct them.

Show 19 quoted lines

>>               if (S_ISDIR(st.st_mode)) {
>> -                     strbuf_addstr(&directory, ent->name);
>>                       if (remove_directories || (matches == MATCHED_EXACTLY)) {
>> -                             if (remove_dirs(&directory, prefix, rm_flags, dry_run, quiet, &gone))
>> -                                     errors++;
>> -                             if (gone && !quiet) {
>> -                                     qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
>> -                                     printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
>> -                             }
>> +                             string_list_append(&dels, ent->name);
>
> The patch would be much easier to read if split into a first refactoring
> patch that would introduce this "dels" list, and a second patch that
> would introduce -i. Reading this, I wondered why the code was removed,
> while it was actually just moved.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

-- 
Jiang Xin