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

Jiang Xin [off-list ref] 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.

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/