Jiang Xin <worldhello....@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.

You may argue that users are not stupid and know what they do, but the
point of this -i is to protect users against their fingers (typing -f
without thinking) for a potentially very destructive command ...

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.

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

>               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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to