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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Jiang Xin <worldhello.net@gmail.com> writes:

> Show what would be done and the user must confirm before actually
> cleaning. In the confirmation dialog, the user has three choices:
>
>  * Yes: Start to do cleaning.
>  * No:  Nothing will be deleted.
>  * Edit (default): Enter edit mode.

I like this much more than the previous one. I played with it a bit, and
found it much more pleasant than "rm -i": by default, only one querry,
but still an option to select which files to clean.

I'm wondering whether "Enter" in the edit mode should return to the
yes/no/Edit querry instead of applying the clean. It would make it clear
for the user that it's still possible to cancel completely (the
Control-C hint is not visible in the UI otherwise).

> Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>

Please, no. I already mentionned it in my previous patch, but I did not
review the patch. See SubmittingPatches:

  3. "Reviewed-by:", unlike the other tags, can only be offered by the
     reviewer and means that she is completely satisfied that the patch
     is ready for application.  It is usually offered only after a
     detailed review.

Commenting != reviewing.

> +		/* dels list may become empty when we run string_list_remove_empty_items later */
> +		if (!dels->nr)
> +			break;

This happens when the user removed everything from the list in the edit
mode. This could print something before breaking (and then exiting
silently). Maybe "No more files to clean, exiting." or so.

> +			printf(_("Remove (yes/no/Edit) ? "));
> +			strbuf_getline(&confirm, stdin, '\n');
> +			strbuf_trim(&confirm);
> +			if (confirm.len) {
> +				if (!strncasecmp(confirm.buf, "yes", confirm.len)) {
> +					break;
> +				} else if (!strncasecmp(confirm.buf, "no", confirm.len)) {
> +					string_list_clear(dels, 0);
> +					break;
> +				}
> +			}
> +			edit_mode = 1;

It's weird that anything but "yes" and "no" enter the edit mode without
complaining. It's safe, but surprising. If I type "foo", I'd rather get
an error and be asked again.

> +		if (!matches) {
> +			strbuf_addf(&message, _("WARNING: Cannot find items prefixed by: %s"), confirm.buf);

"prefixed" seems a remainder of the previous version of the patch. You
probably mean "matched by: %s".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/