Re: [PATCH] clean: confirm before cleaning files and directories

Junio C Hamano [off-list ref] writes:

Matthieu Moy [off-list ref] writes:

quoted

The nice thing with the confirmation dialog is that it shows the list
before asking (and unlike 'rm -i', it asks only once).

I wouldn't object to having "clean -i", which automatically defeats
the requireforce option.

As to a huge single list you have to approve or reject as a whole, I
am on the fence.  When running "rm -i", I often wished to see
something like that, but I am fairly sure that I'll call it unusable
the first time I see a list with a few items I want to keep while
removing all others.

Elaborating on this a bit more, hoping it would help people who want
to design the "--confirm-before-doing" option...

The primary reason I think the user will find "We are going to
remove these.  OK?" irritating is that most of the time, there are
only a few items that the user would want to keep.

	$ rm --confirm-before-doing -r path
	... list of three dozens of items, among which
        ... there may be two items that should be kept
        Remove all? [Y/n]

After seeing this prompt and saying 'n', the user would _not_ thank
the command for reminding about these precious two items, because
the only next step available to the user is to remove the remaining
34 items manually.

"Confirm in bulk before doing" feature can become useful if it had a
"line item veto" option in the confirmation time.  The interaction
then could go like this:

	$ rm --confirm-before-doing -r path
	path/foo    path/frotz/nitfol    path/sotto
        path/bar    path/frotz/xyzzy     path/trail
        ...            ...               ...     
        Remove (list items you want to keep)? path/frotz

and the user could instruct it to remove everything other than those
inside path/fortz.  If the user do not want to remove anything,
there is an option to ^C out of the command.

2013/4/27 Junio C Hamano [off-list ref]:

Junio C Hamano [off-list ref] writes:

quoted

Matthieu Moy [off-list ref] writes:

quoted

The nice thing with the confirmation dialog is that it shows the list
before asking (and unlike 'rm -i', it asks only once).

I wouldn't object to having "clean -i", which automatically defeats
the requireforce option.

As to a huge single list you have to approve or reject as a whole, I
am on the fence.  When running "rm -i", I often wished to see
something like that, but I am fairly sure that I'll call it unusable
the first time I see a list with a few items I want to keep while
removing all others.

Elaborating on this a bit more, hoping it would help people who want
to design the "--confirm-before-doing" option...

The primary reason I think the user will find "We are going to
remove these.  OK?" irritating is that most of the time, there are
only a few items that the user would want to keep.

        $ rm --confirm-before-doing -r path
        ... list of three dozens of items, among which
        ... there may be two items that should be kept
        Remove all? [Y/n]

After seeing this prompt and saying 'n', the user would _not_ thank
the command for reminding about these precious two items, because
the only next step available to the user is to remove the remaining
34 items manually.

"Confirm in bulk before doing" feature can become useful if it had a
"line item veto" option in the confirmation time.  The interaction
then could go like this:

        $ rm --confirm-before-doing -r path
        path/foo    path/frotz/nitfol    path/sotto
        path/bar    path/frotz/xyzzy     path/trail
        ...            ...               ...
        Remove (list items you want to keep)? path/frotz

and the user could instruct it to remove everything other than those
inside path/fortz.  If the user do not want to remove anything,
there is an option to ^C out of the command.

Agree. I will send a reroll latter.

-- 
Jiang Xin

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.

Signed-off-by: Jiang Xin <redacted>
Reviewed-by: Matthieu Moy <redacted>
Suggested-by: Junio C Hamano <redacted>
---
 Documentation/git-clean.txt |  14 +++++-
 builtin/clean.c             | 101 +++++++++++++++++++++++++++++++++++++-------
 2 files changed, 98 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index bdc3a..60a30 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt

@@ -8,7 +8,7 @@ git-clean - Remove untracked files from the working tree
 SYNOPSIS
 --------
 [verse]
-'git clean' [-d] [-f] [-n] [-q] [-e <pattern>] [-x | -X] [--] <path>...
+'git clean' [-d] [-f] [-i] [-n] [-q] [-e <pattern>] [-x | -X] [--] <path>...
 
 DESCRIPTION
 -----------

@@ -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.
 
 -n::
 --dry-run::

diff --git a/builtin/clean.c b/builtin/clean.c
index 04e39..eee04 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c

@@ -15,9 +15,10 @@
 #include "quote.h"
 
 static int force = -1; /* unset */
+static int interactive;
 
 static const char *const builtin_clean_usage[] = {
-	N_("git clean [-d] [-f] [-n] [-q] [-e <pattern>] [-x | -X] [--] <paths>..."),
+	N_("git clean [-d] [-f] [-i] [-n] [-q] [-e <pattern>] [-x | -X] [--] <paths>..."),
 	NULL
 };
 

@@ -154,12 +155,15 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	struct strbuf buf = STRBUF_INIT;
 	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 	struct exclude_list *el;
+	struct string_list dels = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
 	const char *qname;
 	char *seen = NULL;
 	struct option options[] = {
 		OPT__QUIET(&quiet, N_("do not print names of files removed")),
 		OPT__DRY_RUN(&dry_run, N_("dry run")),
 		OPT__FORCE(&force, N_("force")),
+		OPT_BOOL('i', "interactive", &interactive, N_("interactive cleaning")),
 		OPT_BOOLEAN('d', NULL, &remove_directories,
 				N_("remove whole directories")),
 		{ OPTION_CALLBACK, 'e', "exclude", &exclude_list, N_("pattern"),

@@ -186,12 +190,12 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	if (ignored && ignored_only)
 		die(_("-x and -X cannot be used together"));
 
-	if (!dry_run && !force) {
+	if (!dry_run && !force && !interactive) {
 		if (config_set)
-			die(_("clean.requireForce set to true and neither -n nor -f given; "
+			die(_("clean.requireForce set to true and neither -i, -n nor -f given; "
 				  "refusing to clean"));
 		else
-			die(_("clean.requireForce defaults to true and neither -n nor -f given; "
+			die(_("clean.requireForce defaults to true and neither -i, -n nor -f given; "
 				  "refusing to clean"));
 	}
 

@@ -257,26 +261,92 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		}
 
 		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);
 			}
-			strbuf_reset(&directory);
 		} else {
 			if (pathspec && !matches)
 				continue;
-			res = dry_run ? 0 : unlink(ent->name);
+			string_list_append(&dels, ent->name);
+		}
+	}
+
+	if (interactive && dels.nr > 0 && !dry_run && isatty(0) && isatty(1)) {
+		struct strbuf confirm = STRBUF_INIT;
+
+		while (1) {
+			struct strbuf **prefix_list, **prefix_list_head;
+
+			/* dels list may become empty when we run string_list_remove_empty_items latter */
+			if (!dels.nr)
+				break;
+
+			for_each_string_list_item(item, &dels) {
+				qname = quote_path_relative(item->string, -1, &buf, prefix);
+				printf(_(msg_would_remove), qname);
+			}
+
+			printf(_("Remove (press enter to confirm or input items you want to keep)? "));
+			strbuf_getline(&confirm, stdin, '\n');
+			strbuf_trim(&confirm);
+
+			if (!confirm.len)
+				break;
+
+			printf("\n");
+
+			prefix_list_head = strbuf_split_buf(confirm.buf, confirm.len, ' ', 0);
+			for (prefix_list = prefix_list_head; *prefix_list; *prefix_list++)
+			{
+				int prefix_matched = 0;
+
+				strbuf_trim(*prefix_list);
+				if (!(*prefix_list)->len)
+					continue;
+
+				for_each_string_list_item(item, &dels) {
+					if (!strncasecmp(item->string, (*prefix_list)->buf, (*prefix_list)->len)) {
+						*item->string = '\0';
+						prefix_matched++;
+					}
+				}
+				if (!prefix_matched) {
+					warning(_("Cannot find items start with the given prefix: %s"), (*prefix_list)->buf);
+					printf("\n");
+				} else {
+					string_list_remove_empty_items(&dels, 0);
+				}
+			}
+
+			strbuf_reset(&confirm);
+			strbuf_list_free(prefix_list_head);
+		}
+		strbuf_release(&confirm);
+	}
+
+	for_each_string_list_item(item, &dels) {
+		struct stat st;
+
+		if (lstat(item->string, &st))
+			continue;
+
+		if (S_ISDIR(st.st_mode)) {
+			strbuf_addstr(&directory, item->string);
+			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);
+			}
+			strbuf_reset(&directory);
+		} else {
+			res = dry_run ? 0 : unlink(item->string);
 			if (res) {
-				qname = quote_path_relative(ent->name, -1, &buf, prefix);
+				qname = quote_path_relative(item->string, -1, &buf, prefix);
 				warning(_(msg_warn_remove_failed), qname);
 				errors++;
 			} else if (!quiet) {
-				qname = quote_path_relative(ent->name, -1, &buf, prefix);
+				qname = quote_path_relative(item->string, -1, &buf, prefix);
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		}

@@ -285,5 +355,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	strbuf_release(&directory);
 	string_list_clear(&exclude_list, 0);
+	string_list_clear(&dels, 0);
 	return (errors != 0);
 }

-- 
1.8.2.1.921.g1826d07

On Sat, Apr 27, 2013 at 12:13 PM, Jiang Xin [off-list ref] wrote:

s/latter/later/

+                       if (!dels.nr)
+                               break;
+
+                       for_each_string_list_item(item, &dels) {
+                               qname = quote_path_relative(item->string, -1, &buf, prefix);
+                               printf(_(msg_would_remove), qname);
+                       }
+
+                       printf(_("Remove (press enter to confirm or input items you want to keep)? "));
+                       strbuf_getline(&confirm, stdin, '\n');
+                       strbuf_trim(&confirm);
+
+                       if (!confirm.len)
+                               break;
+
+                       printf("\n");
+
+                       prefix_list_head = strbuf_split_buf(confirm.buf, confirm.len, ' ', 0);
+                       for (prefix_list = prefix_list_head; *prefix_list; *prefix_list++)
+                       {
+                               int prefix_matched = 0;
+
+                               strbuf_trim(*prefix_list);
+                               if (!(*prefix_list)->len)
+                                       continue;
+
+                               for_each_string_list_item(item, &dels) {
+                                       if (!strncasecmp(item->string, (*prefix_list)->buf, (*prefix_list)->len)) {
+                                               *item->string = '\0';
+                                               prefix_matched++;
+                                       }
+                               }
+                               if (!prefix_matched) {
+                                       warning(_("Cannot find items start with the given prefix: %s"), (*prefix_list)->buf);

s/start/starting/
[...or...]
s/items start with the given prefix/items prefixed by/

+                                       printf("\n");
+                               } else {
+                                       string_list_remove_empty_items(&dels, 0);
+                               }
+                       }
+
+                       strbuf_reset(&confirm);
+                       strbuf_list_free(prefix_list_head);
+               }
+               strbuf_release(&confirm);
+       }
+
+       for_each_string_list_item(item, &dels) {
+               struct stat st;
+
+               if (lstat(item->string, &st))
+                       continue;
+
+               if (S_ISDIR(st.st_mode)) {
+                       strbuf_addstr(&directory, item->string);
+                       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);
+                       }
+                       strbuf_reset(&directory);
+               } else {
+                       res = dry_run ? 0 : unlink(item->string);
                        if (res) {
-                               qname = quote_path_relative(ent->name, -1, &buf, prefix);
+                               qname = quote_path_relative(item->string, -1, &buf, prefix);
                                warning(_(msg_warn_remove_failed), qname);
                                errors++;
                        } else if (!quiet) {
-                               qname = quote_path_relative(ent->name, -1, &buf, prefix);
+                               qname = quote_path_relative(item->string, -1, &buf, prefix);
                                printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
                        }
                }