Jiang Xin <worldhello.net@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html