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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Apr 29, 2013, 08:03 UTC
Message-ID: <vpqwqrl931e.fsf@grenoble-inp.fr>
In-Reply-To: <2a68239bfd3e6b9ea1842dd8e468ee3d3217af5b.1367079089.git.worldhello.net@gmail.com>

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.

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.

Show 16 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.

Show 10 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/