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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: May 1, 2013, 15:12 UTC
Message-ID: <vpq38u6n397.fsf@grenoble-inp.fr>
In-Reply-To: <3ecc9ca1b1363b5bd27ae53cbf5899ce6d44cd48.1367349734.git.worldhello.net@gmail.com>

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

Show 6 quoted lines

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

Show 12 quoted lines

> +			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/