[PATCH] clean: confirm before cleaning files and directories

[PATCH] clean: confirm before cleaning files and directories

[Jiang Xin]

When running `git clean`, it will be convenient and safe to show a
confirm dialog and only delete files and directories when confirmed.
The confirm dialog will popup when:

 * run `git clean` in interactive sessions,
 * not a dry run,
 * and not quiet.

There may be existing scripts that call `git clean -f` while leave the
standard input and the standard output of the `git clean` connected to
whatever environment the scripts were started, and such invocation might
trigger the confirm dialog. In this case, add a `-q` option, such as
`git clean -q -f`, then the confirm dialog won't popup.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 builtin/clean.c | 66 +++++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 14 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 04e39..e31a1 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -169,6 +169,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 				N_("remove only ignored files")),
 		OPT_END()
 	};
+	struct string_list dels = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
+	struct strbuf confirm = STRBUF_INIT;
+	int confirmed = 0;
 
 	git_config(git_clean_config, NULL);
 	if (force < 0)
@@ -257,33 +261,67 @@ 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)) {
+				string_list_append(&dels, ent->name);
+			}
+		} else {
+			if (pathspec && !matches)
+				continue;
+			string_list_append(&dels, ent->name);
+		}
+	}
+
+	if (!isatty(0) || !isatty(1) || quiet || dry_run) {
+		confirmed = 1;
+	}
+
+	if (!confirmed && dels.nr > 0) {
+		for_each_string_list_item(item, &dels) {
+			qname = quote_path_relative(item->string, -1, &buf, prefix);
+			printf(_(msg_would_remove), qname);
+		}
+		printf(_("Are you sure [y/N]? "));
+		strbuf_getline(&confirm, stdin, '\n');
+		strbuf_trim(&confirm);
+		if (!strcasecmp(confirm.buf, "y")) {
+			confirmed = 1;
+		}
+	}
+
+	if (confirmed) {
+		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 {
-			if (pathspec && !matches)
-				continue;
-			res = dry_run ? 0 : unlink(ent->name);
-			if (res) {
-				qname = quote_path_relative(ent->name, -1, &buf, prefix);
-				warning(_(msg_warn_remove_failed), qname);
-				errors++;
-			} else if (!quiet) {
-				qname = quote_path_relative(ent->name, -1, &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(item->string, -1, &buf, prefix);
+					warning(_(msg_warn_remove_failed), qname);
+					errors++;
+				} else if (!quiet) {
+					qname = quote_path_relative(item->string, -1, &buf, prefix);
+					printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
+				}
 			}
 		}
 	}
 	free(seen);
 
 	strbuf_release(&directory);
+	strbuf_release(&confirm);
 	string_list_clear(&exclude_list, 0);
+	string_list_clear(&dels, 0);
 	return (errors != 0);
 }
-- 
1.8.2.1.628.gcd33b41

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

[Matthieu Moy]

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

>  * run `git clean` in interactive sessions,
>  * not a dry run,
>  * and not quiet.

Err, does this mean I'll have:

$ git clean
fatal: clean.requireForce defaults to true and neither -n nor -f given; refusing to clean
$ git clean --force
Are you sure [y/n]?

An optional confirmation dialog seems interesting, but activating it by
default even with --force seems really weird.

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

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

[Jiang Xin]

2013/4/26 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> >  * run `git clean` in interactive sessions,
> >  * not a dry run,
> >  * and not quiet.
>
> Err, does this mean I'll have:
>
> $ git clean
> fatal: clean.requireForce defaults to true and neither -n nor -f given; refusing to clean
> $ git clean --force

( you omit something, because nothing to clean won't trigger this
confirm dialog)

> Are you sure [y/n]?
>
> An optional confirmation dialog seems interesting, but activating it by
> default even with --force seems really weird.

I don't know how many programmers had been bitten by runing `git clean -fdx`,
but I bet there were some. I think safety should be put to the 1st place.
It is because "clean.requireForce" defaults to true, all people trend to run
'git clean' with the '--force/-f' option.

Maybe we can do like this:

1. Set the default value of 'clean.requireForce' to false.
2. Show a error message and do nothing, if there is not 'clean.requireForce'
    setting, but the user called with a '--force' flag.
    ( like a transition for the change of push.default in git 2.0)

any opinions?

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




--
蒋鑫

北京群英汇信息技术有限公司
邮件: worldhello.net@gmail.com
网址: http://www.ossxp.com/
博客: http://www.worldhello.net/
微博: http://weibo.com/gotgit/
电话: 18601196889

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

[Matthieu Moy]

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

> Maybe we can do like this:
>
> 1. Set the default value of 'clean.requireForce' to false.
> 2. Show a error message and do nothing, if there is not 'clean.requireForce'
>     setting, but the user called with a '--force' flag.
>     ( like a transition for the change of push.default in git 2.0)

Perhaps introducing a new value for 'clean.requireForce':

$ git config --global clean.requireForce ask
$ git clean
will remove ...
are you sure [y/N]?

The error message when clean.requireForce is unset and --force is not
given could point the user to clean.requireForce=ask.

Then, maybe, later, this could become the default. But I tend to like
the non-interactive nature of most Git commands, so I'm a bit reluctant
here. My way of doing the confirmation dialog is

$ git clean -n
would remove ...
$ git clean -f

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

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

[Jiang Xin]

2013/4/26 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:
> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> Maybe we can do like this:
>>
>> 1. Set the default value of 'clean.requireForce' to false.
>> 2. Show a error message and do nothing, if there is not 'clean.requireForce'
>>     setting, but the user called with a '--force' flag.
>>     ( like a transition for the change of push.default in git 2.0)
>
> Perhaps introducing a new value for 'clean.requireForce':
>
> $ git config --global clean.requireForce ask
> $ git clean
> .will remove ...
> are you sure [y/N]?
>
> The error message when clean.requireForce is unset and --force is not
> given could point the user to clean.requireForce=ask.

Add new value for clean.requireForce would break old git clent.

   $ git clean
   fatal: bad config value for 'clean.requireforce' in .git/config

>
> Then, maybe, later, this could become the default. But I tend to like
> the non-interactive nature of most Git commands, so I'm a bit reluctant
> here. My way of doing the confirmation dialog is
>
> $ git clean -n
> would remove ...
> $ git clean -f
>

I try to put all cases in one table, but still looks weird.

    | clean.requireForce  |     git clean     | git clean --force |
    +---------------------+-------------------|-------------------|
    | TRUE                | error             | delete...         |
    +---------------------+-------------------|-------------------|
    | FALSE               | delete...         | delete...         |
    +---------------------+-------------------|-------------------|
    | unset               | confirm           | warn + confirm    |
    +---------------------+-------------------|-------------------|

Does anyone really set and use "clean.requireForce" setting?
And if there is a confirm dialog, do we need 'clean.requireForce' any more?
Or we can add a `--no-ask` option, in order to override the confirm dialog.


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



--
蒋鑫

北京群英汇信息技术有限公司
邮件: worldhello.net@gmail.com
网址: http://www.ossxp.com/
博客: http://www.worldhello.net/
微博: http://weibo.com/gotgit/
电话: 18601196889

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

[Thomas Rast]

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

> I don't know how many programmers had been bitten by runing `git clean -fdx`,
> but I bet there were some. I think safety should be put to the 1st place.
> It is because "clean.requireForce" defaults to true, all people trend to run
> 'git clean' with the '--force/-f' option.

But if that's the real problem, wouldn't it be better to introduce
something like clean.requireForce=interactive (or 'ask', or whatever you
make up) which will be like clean.requireForce=true except that when on
a TTY, we ask the user?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

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

[Junio C Hamano]

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

> I don't know how many programmers had been bitten by runing `git clean -fdx`,
> but I bet there were some. I think safety should be put to the 1st place.

"git clean" without -n/f errors out, hinting the availablilty of -n
while mentioning -f; that is the safety, isn't it?  Once the user
decides to give -f, the user _wants_ to remove cruft, and it is a
hinderance to require any further confirmation.

Your problem description sounds like that you want to drive without
wearing seatbelts but in case you get in a collision, you suggest to
make the seat automatically stick to the back of anybody who sits on
it with superglue to make it safer.

That approach might give it an alternative safety, but it would make
it extremely annoying if you apply it also to people who do wear
belts, no?

This extra confirmation should never be the default.

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> "git clean" without -n/f errors out, hinting the availablilty of -n
> while mentioning -f; that is the safety, isn't it?  Once the user
> decides to give -f, the user _wants_ to remove cruft, and it is a
> hinderance to require any further confirmation.

This is only half true: because "git clean" does nothing without -f,
people's fingers get trained to type "git clean -f" without really
thinking about it. Just like people alias rm='rm -i' and then type
"rm -fr *" mechanically.

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

But as I said in another thread, I fully agree that adding the
confirmation by default even with -f is nonsense.

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> 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.

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

[Jiang Xin]

2013/4/27 Junio C Hamano <gitster@pobox.com>:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>
>>> 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

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

[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 <worldhello.net@gmail.com>
Reviewed-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Suggested-by: Junio C Hamano <gitster@pobox.com>
---
 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

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

[Matthieu Moy]

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

> Reviewed-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>

Err, no: I commented on the intention of the first patch, but did not
_review_ it, and didn't read this version yet.

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> Reviewed-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
>
> Err, no: I commented on the intention of the first patch, but did not
> _review_ it, and didn't read this version yet.

Yeah I suspected as such.

And thanks for saying it loud in public, so that other contributors
can learn from this.

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

[Eric Sunshine]

On Sat, Apr 27, 2013 at 12:13 PM, Jiang Xin <worldhello.net@gmail.com> wrote:
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -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 */

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);
>                         }
>                 }

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

[Matthieu Moy]

Jiang Xin <worldhello.net@gmail.com> 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/

[PATCH] clean: Introduce -z for machine readable output

[Michael J Gruber]

-z makes clean output only the names of paths which are or would be
deleted, and separates them with \0.

Use as "xargs -0 -a <(git clean -nz [-d]) rm -ri", e.g., as a quick
"git clean -i".

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
Here's an alternative approach to that problem with a complete different attach
vector but a similar purpose. I've been using it for a while, but it's kind of
unpolished. My "git-clean-i" looks like:

--->%---
#!/bin/bash
xargs -0 -a <(git clean -n -z "$@") rm -ri
--->%---

 Documentation/git-clean.txt |  9 ++++++++-
 builtin/clean.c             | 27 +++++++++++++++++++--------
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index bdc3ab8..849e775 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] [-n] [-q] [-z] [-e <pattern>] [-x | -X] [--] <path>...
 
 DESCRIPTION
 -----------
@@ -63,6 +63,13 @@ OPTIONS
 	Remove only files ignored by Git.  This may be useful to rebuild
 	everything from scratch, but keep manually created files.
 
+-z::
+	Use machine readable output for (to be) removed paths: Output the paths
+	which are or would be removed only (without extra wording) and
+	separate them with \0.
++
+This does not imply `-n` but can be combined with it.
+
 SEE ALSO
 --------
 linkgit:gitignore[5]
diff --git a/builtin/clean.c b/builtin/clean.c
index 04e396b..0ebba24 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -41,8 +41,18 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+void write_name(const char *fmt, const char *name, int nul_terminated)
+{
+	if (nul_terminated) {
+		fputs(name, stdout);
+		fputc(0, stdout);
+	} else {
+		printf(fmt, name);
+	}
+}
+
 static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
-		int dry_run, int quiet, int *dir_gone)
+		int dry_run, int quiet, int *dir_gone, int nul_terminated)
 {
 	DIR *dir;
 	struct strbuf quoted = STRBUF_INIT;
@@ -57,8 +67,8 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 			!resolve_gitlink_ref(path->buf, "HEAD", submodule_head)) {
 		if (!quiet) {
 			quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
-			printf(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),
-					quoted.buf);
+			write_name(dry_run ?  _(msg_would_skip_git_dir) : _(msg_skip_git_dir),
+					quoted.buf, nul_terminated);
 		}
 
 		*dir_gone = 0;
@@ -91,7 +101,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 		if (lstat(path->buf, &st))
 			; /* fall thru */
 		else if (S_ISDIR(st.st_mode)) {
-			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
+			if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone, nul_terminated))
 				ret = 1;
 			if (gone) {
 				quote_path_relative(path->buf, strlen(path->buf), &quoted, prefix);
@@ -136,7 +146,7 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 
 	if (!*dir_gone && !quiet) {
 		for (i = 0; i < dels.nr; i++)
-			printf(dry_run ?  _(msg_would_remove) : _(msg_remove), dels.items[i].string);
+			write_name(dry_run ?  _(msg_would_remove) : _(msg_remove), dels.items[i].string, nul_terminated);
 	}
 	string_list_clear(&dels, 0);
 	return ret;
@@ -146,7 +156,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 {
 	int i, res;
 	int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
-	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
+	int ignored_only = 0, config_set = 0, errors = 0, gone = 1, nul_terminated = 0;
 	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
 	struct strbuf directory = STRBUF_INIT;
 	struct dir_struct dir;
@@ -167,6 +177,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN('x', NULL, &ignored, N_("remove ignored files, too")),
 		OPT_BOOLEAN('X', NULL, &ignored_only,
 				N_("remove only ignored files")),
+		OPT_BOOLEAN('z', NULL, &nul_terminated, "(actually or to be) removed paths are separated with NUL character"),
 		OPT_END()
 	};
 
@@ -259,7 +270,7 @@ 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))
+				if (remove_dirs(&directory, prefix, rm_flags, dry_run, quiet, &gone, nul_terminated))
 					errors++;
 				if (gone && !quiet) {
 					qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
@@ -277,7 +288,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 				errors++;
 			} else if (!quiet) {
 				qname = quote_path_relative(ent->name, -1, &buf, prefix);
-				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
+				write_name(dry_run ? _(msg_would_remove) : _(msg_remove), qname, nul_terminated);
 			}
 		}
 	}
-- 
1.8.3.rc0.297.g8c90ec5

Re: [PATCH] clean: Introduce -z for machine readable output

[Matthieu Moy]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> -z makes clean output only the names of paths which are or would be
> deleted, and separates them with \0.

My first reaction was: Is it not a job for "git ls-files"?

Actually, you already almost have it:

git clean -d  => git ls-files --exclude-standard --directory -o
git clean -dx => git ls-files -z --exclude-standard --directory -io

OTOH, the "git clean" without -d are not exposed directly AFAICT, and
the set of options of "git clean" may make more sense.

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

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

[Jiang Xin]

2013/4/29 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:
> Jiang Xin <worldhello.net@gmail.com> 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.

Because of this (typing "no" or "n" not stop the whole cleaning),
I write a loop, only when user press Enter (input nothing), begin
to clean files/directories, and there would be a warning if nothing
excluded by the prefix/pattern.

That means when user input "n" or "no", the interactive git-clean
will not delete entries starting with "n" or "no" (instead of delete
nothing), but it still give the user a hint what happened. It will
show the filtered cleaning list or a warning message that there
are no entries starting with "n" or "no" to be filtered out (may be
the warning messages should be cached and displayed in
the end, not at the beginning).

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

It's not confirmation, but a last chance to save your cleaning files.
The use can input a space-seperated prefixes list, but I think most
people would like to input a space-seperated patterns (like .gitignore).
I try to implement like this, but still need sometime to refector the code.

+                               el = add_exclude_list(&dir, EXC_CMDL,
"exclude by interactive git clean");
+                               add_exclude((*prefix_list)->buf, "",
0, el, -(++i));

git-clean is a simple task, I doubt write a rich interface has no much value,
and make things complicated. But render prompt, error outputs in different
color like interactive git-add is interesting.


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

Oops, I code lots of ruby recently, and vim is not smart to handle txt file.
I will correct them.

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



-- 
Jiang Xin

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

[Jiang Xin]

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.

When the user chooses the edit mode, the user can input space-
separated patterns (the same syntax as gitignore), and each clean
candidate that matches with one of the patterns will be excluded
from cleaning. When the user feels it's OK, presses ENTER to start
cleaning.

When in the edit mode, if the user wants to cancel the whole
cleaning, simply inputs ctrl-c to abort the cleaning.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
Suggested-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-clean.txt |  16 ++++-
 builtin/clean.c             | 141 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 140 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index bdc3a..6bc99 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,19 @@ 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 the user must confirm before actually
+	cleaning. In the confirmation dialog, the user can choose to abort
+	the cleaning, or enter into an edit mode. In the edit mode, the
+	user can input space-separated patterns (the same syntax as
+	gitignore), and each clean candidate that matches with one of the
+	patterns will be excluded from cleaning. When the user feels it's
+	OK, presses ENTER to start cleaning. If the user wants to cancel
+	the whole cleaning in the edit mode, simply inputs ctrl-c to abort.
 
 -n::
 --dry-run::
diff --git a/builtin/clean.c b/builtin/clean.c
index 04e39..b26c0 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
 };
 
@@ -142,6 +143,96 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	return ret;
 }
 
+void interactive_clean(struct string_list *dels, const char *prefix)
+{
+	struct dir_struct dir;
+	struct strbuf confirm = STRBUF_INIT;
+	struct strbuf message = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf **ignore_list;
+	struct string_list_item *item;
+	struct exclude_list *el;
+	const char *qname;
+	int edit_mode = 0, i;
+
+	while (1) {
+		int matches = 0;
+
+		/* dels list may become empty when we run string_list_remove_empty_items 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);
+		}
+
+		if (message.len) {
+			printf("\n%s\n", message.buf);
+			strbuf_reset(&message);
+		}
+
+		if (!edit_mode) {
+			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;
+		}
+
+		printf(_("Input ignore patterns to keep items, or press ENTER to confirm: "));
+		strbuf_getline(&confirm, stdin, '\n');
+		strbuf_trim(&confirm);
+		printf("\n");
+
+		if (!confirm.len)
+			break;
+
+		memset(&dir, 0, sizeof(dir));
+		el = add_exclude_list(&dir, EXC_CMDL, "manual exclude");
+		ignore_list = strbuf_split_buf(confirm.buf, confirm.len, ' ', 0);
+
+		for (i = 0; ignore_list[i]; i++)
+		{
+
+			strbuf_trim(*ignore_list);
+			if (!(*ignore_list)->len)
+				continue;
+
+			add_exclude(ignore_list[i]->buf, "", 0, el, -(i+1));
+		}
+
+		for_each_string_list_item(item, dels) {
+			int dtype = DT_UNKNOWN;
+			if (is_excluded(&dir, item->string, &dtype)) {
+				*item->string = '\0';
+				matches++;
+			}
+		}
+
+		if (!matches) {
+			strbuf_addf(&message, _("WARNING: Cannot find items prefixed by: %s"), confirm.buf);
+			strbuf_addch(&message, '\n');
+		} else {
+			string_list_remove_empty_items(dels, 0);
+		}
+
+		strbuf_reset(&confirm);
+		strbuf_list_free(ignore_list);
+		clear_directory(&dir);
+	}
+
+	strbuf_release(&confirm);
+	strbuf_release(&message);
+}
+
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
 	int i, res;
@@ -154,12 +245,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 +280,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 +351,42 @@ 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))
+		interactive_clean(&dels, prefix);
+
+	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 +395,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.3.rc0.306.gee4c747

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

[Matthieu Moy]

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/

[PATCH v4 1/3] Add support for -i/--interactive to git-clean

[Jiang Xin]

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 for the first time): Enter edit mode.

When the user chooses the edit mode, the user can input space-
separated patterns (the same syntax as gitignore), and each clean
candidate that matches with one of the patterns will be excluded
from cleaning. When the user feels it's OK, presses ENTER and back
to the confirmation dialog.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
Suggested-by: Junio C Hamano <gitster@pobox.com>
Spelling-check-by: Eric Sunshine <sunshine@sunshineco.com>
Comments-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 Documentation/git-clean.txt |  15 +++-
 builtin/clean.c             | 183 ++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 181 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index bdc3a..f5572 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,18 @@ 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 the user must confirm before actually
+	cleaning. In the confirmation dialog, the user can choose to abort
+	the cleaning, or enter into an edit mode. In the edit mode, the
+	user can input space-separated patterns (the same syntax as
+	gitignore), and each clean candidate that matches with one of the
+	patterns will be excluded from cleaning. When the user feels it's
+	OK, presses ENTER and back to the confirmation dialog.
 
 -n::
 --dry-run::
diff --git a/builtin/clean.c b/builtin/clean.c
index 04e39..407744e5 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
 };
 
@@ -142,6 +143,138 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	return ret;
 }
 
+void interactive_clean_edit(struct string_list *dels, const char *prefix)
+{
+	struct dir_struct dir;
+	struct strbuf confirm = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf **ignore_list;
+	struct string_list_item *item;
+	struct exclude_list *el;
+	const char *qname;
+	int changed = -1, i;
+
+	while (1) {
+		/* dels list may become empty when we run string_list_remove_empty_items later */
+		if (!dels->nr) {
+			printf_ln(_("No more files to clean, exiting."));
+			break;
+		}
+
+		if (changed) {
+			printf(_(
+				 "NOTE: Will remove the following items. You can input space-seperated\n"
+				 "NOTE: patterns (just like .gitignore) to exclude items from deletion,\n"
+				 "NOTE: or press ENTER to continue."
+				));
+			putchar('\n');
+			putchar('\n');
+
+			/* Display dels in "Would remove ..." format */
+			for_each_string_list_item(item, dels) {
+				qname = quote_path_relative(item->string, -1, &buf, prefix);
+				printf(_(msg_would_remove), qname);
+			}
+			putchar('\n');
+		}
+
+		printf(_("Input ignore patterns> "));
+		strbuf_getline(&confirm, stdin, '\n');
+		strbuf_trim(&confirm);
+		putchar('\n');
+
+		/* Quit edit mode */
+		if (!confirm.len)
+			break;
+
+		memset(&dir, 0, sizeof(dir));
+		el = add_exclude_list(&dir, EXC_CMDL, "manual exclude");
+		ignore_list = strbuf_split_buf(confirm.buf, confirm.len, ' ', 0);
+
+		for (i = 0; ignore_list[i]; i++) {
+			strbuf_trim(*ignore_list);
+			if (!(*ignore_list)->len)
+				continue;
+
+			add_exclude(ignore_list[i]->buf, "", 0, el, -(i+1));
+		}
+
+		changed = 0;
+		for_each_string_list_item(item, dels) {
+			int dtype = DT_UNKNOWN;
+			const char *qname;
+
+			qname = quote_path_relative(item->string, -1, &buf, prefix);
+
+			if (is_excluded(&dir, qname, &dtype)) {
+				*item->string = '\0';
+				changed++;
+			}
+		}
+
+		if (changed) {
+			string_list_remove_empty_items(dels, 0);
+		} else {
+			printf(_("WARNING: Cannot find items matched by: %s"), confirm.buf);
+			putchar('\n');
+		}
+
+		strbuf_list_free(ignore_list);
+		clear_directory(&dir);
+	}
+
+	strbuf_release(&buf);
+	strbuf_release(&confirm);
+}
+
+void interactive_clean(struct string_list *dels, const char *prefix)
+{
+	struct strbuf confirm = STRBUF_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	struct string_list_item *item;
+	const char *qname;
+	int count = 0;
+
+	/* dels list may become empty after return back from edit mode */
+	while (dels->nr) {
+		/* Display dels in "Would remove ..." format */
+		for_each_string_list_item(item, dels) {
+			qname = quote_path_relative(item->string, -1, &buf, prefix);
+			printf(_(msg_would_remove), qname);
+		}
+		putchar('\n');
+
+		/* Confirmation dialog */
+		printf(count > 0 ? _("Remove (Yes/no/edit) ? ") : _("Remove (yes/no/Edit) ? "));
+		strbuf_getline(&confirm, stdin, '\n');
+		strbuf_trim(&confirm);
+		putchar('\n');
+
+		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;
+			} else if (!strncasecmp(confirm.buf, "edit", confirm.len)) {
+				interactive_clean_edit(dels, prefix);
+			} else {
+				continue;
+			}
+		} else if (count > 0) {
+			/* If back from edit_mode, confirmation dialog defaults to "yes" */
+			break;
+		} else {
+			/* For the first time, confirmation dialog defaults to "edit" */
+			interactive_clean_edit(dels, prefix);
+		}
+		count++;
+	}
+
+	strbuf_release(&buf);
+	strbuf_release(&confirm);
+}
+
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
 	int i, res;
@@ -154,12 +287,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 +322,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 +393,42 @@ 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))
+		interactive_clean(&dels, prefix);
+
+	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 +437,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.3.rc0.364.gbb5463f

[PATCH v4 2/3] Show items of interactive git-clean in columns

[Jiang Xin]

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 builtin/clean.c | 63 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 407744e5..ac48e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -13,9 +13,11 @@
 #include "refs.h"
 #include "string-list.h"
 #include "quote.h"
+#include "column.h"
 
 static int force = -1; /* unset */
 static int interactive;
+static unsigned int colopts;
 
 static const char *const builtin_clean_usage[] = {
 	N_("git clean [-d] [-f] [-i] [-n] [-q] [-e <pattern>] [-x | -X] [--] <paths>..."),
@@ -30,8 +32,12 @@ static const char *msg_warn_remove_failed = N_("failed to remove %s");
 
 static int git_clean_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "clean.requireforce"))
+	if (!prefixcmp(var, "column."))
+		return git_column_config(var, value, "clean", &colopts);
+	if (!strcmp(var, "clean.requireforce")) {
 		force = !git_config_bool(var, value);
+		return 0;
+	}
 	return git_default_config(var, value, cb);
 }
 
@@ -143,6 +149,34 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
 	return ret;
 }
 
+void pretty_print_dels(struct string_list *dels, const char *prefix)
+{
+	struct string_list list = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
+	struct strbuf buf = STRBUF_INIT;
+	const char *qname;
+	struct column_options copts;
+
+	for_each_string_list_item(item, dels) {
+		qname = quote_path_relative(item->string, -1, &buf, prefix);
+		string_list_append(&list, qname);
+	}
+
+	/*
+	 * always enable column display, we only consult column.*
+	 * about layout strategy and stuff
+	 */
+	colopts = (colopts & ~COL_ENABLE_MASK) | COL_ENABLED;
+	memset(&copts, 0, sizeof(copts));
+	copts.indent = "  ";
+	copts.padding = 2;
+	print_columns(&list, colopts, &copts);
+
+	putchar('\n');
+	strbuf_release(&buf);
+	string_list_clear(&list, 0);
+}
+
 void interactive_clean_edit(struct string_list *dels, const char *prefix)
 {
 	struct dir_struct dir;
@@ -151,7 +185,6 @@ void interactive_clean_edit(struct string_list *dels, const char *prefix)
 	struct strbuf **ignore_list;
 	struct string_list_item *item;
 	struct exclude_list *el;
-	const char *qname;
 	int changed = -1, i;
 
 	while (1) {
@@ -170,12 +203,8 @@ void interactive_clean_edit(struct string_list *dels, const char *prefix)
 			putchar('\n');
 			putchar('\n');
 
-			/* Display dels in "Would remove ..." format */
-			for_each_string_list_item(item, dels) {
-				qname = quote_path_relative(item->string, -1, &buf, prefix);
-				printf(_(msg_would_remove), qname);
-			}
-			putchar('\n');
+			/* Display dels in columns */
+			pretty_print_dels(dels, prefix);
 		}
 
 		printf(_("Input ignore patterns> "));
@@ -230,20 +259,21 @@ void interactive_clean_edit(struct string_list *dels, const char *prefix)
 void interactive_clean(struct string_list *dels, const char *prefix)
 {
 	struct strbuf confirm = STRBUF_INIT;
-	struct strbuf buf = STRBUF_INIT;
-	struct string_list_item *item;
-	const char *qname;
 	int count = 0;
 
 	/* dels list may become empty after return back from edit mode */
 	while (dels->nr) {
-		/* Display dels in "Would remove ..." format */
-		for_each_string_list_item(item, dels) {
-			qname = quote_path_relative(item->string, -1, &buf, prefix);
-			printf(_(msg_would_remove), qname);
-		}
+		printf_ln(_(
+			    "WARNING: The following items will be removed permanently. Press \"y\"\n"
+			    "WARNING: to start cleaning, and press \"n\" to abort the cleaning.\n"
+			    "WARNING: You can also enter the \"edit\" mode, and select items\n"
+			    "WARNING: to be excluded from the cleaning."
+			   ));
 		putchar('\n');
 
+		/* Display dels in columns */
+		pretty_print_dels(dels, prefix);
+
 		/* Confirmation dialog */
 		printf(count > 0 ? _("Remove (Yes/no/edit) ? ") : _("Remove (yes/no/Edit) ? "));
 		strbuf_getline(&confirm, stdin, '\n');
@@ -271,7 +301,6 @@ void interactive_clean(struct string_list *dels, const char *prefix)
 		count++;
 	}
 
-	strbuf_release(&buf);
 	strbuf_release(&confirm);
 }
 
-- 
1.8.3.rc0.364.gbb5463f

Re: [PATCH v4 2/3] Show items of interactive git-clean in columns

[Matthieu Moy]

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

> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>

This lacks a proper commit message (why is this a good thing?), and
documentation (you introduce column.clean) but the code sounds good
(that's a very quick look from me, not a "review" sorry).

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

Re: [PATCH v4 2/3] Show items of interactive git-clean in columns

[Jiang Xin]

2013/5/2 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:
> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
>
> This lacks a proper commit message (why is this a good thing?), and

Rewrite the log as following:

Show items of interactive git-clean in columns

When there are lots of items to be cleaned, it is hard to see them all
in one screen. Show them in columns instead of in one column will solve
this problem.

Since no longer show items to be cleaned using the "Would remove ..."
format (only plain filenames), we add instructions/warnings as header
before them.

> documentation (you introduce column.clean) but the code sounds good
> (that's a very quick look from me, not a "review" sorry).

Will honors column.ui config variable only, not introduce new config variable.
So no more documentations needed ;-)

Fix-up looks like this:

 static int git_clean_config(const char *var, const char *value, void *cb)
 {
+       /* honors the column.ui config variable only */
        if (!prefixcmp(var, "column."))
-               return git_column_config(var, value, "clean", &colopts);
+               return git_column_config(var, value, NULL, &colopts);
        if (!strcmp(var, "color.interactive")) {
                clean_use_color = git_config_colorbool(var, value);
                return 0;

-- 
Jiang Xin

Re: [PATCH v4 2/3] Show items of interactive git-clean in columns

[Matthieu Moy]

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

> Rewrite the log as following:

That's probably more than needed ;-). Thanks,

> Show items of interactive git-clean in columns
>
> When there are lots of items to be cleaned, it is hard to see them all
> in one screen. Show them in columns instead of in one column will solve
> this problem.
>
> Since no longer show items to be cleaned using the "Would remove ..."

"Since _we_ no longer ..."?

> Will honors column.ui config variable only, not introduce new config variable.
> So no more documentations needed ;-)

I don't think it's a good idea. Usually, the *.ui variables are
shortcuts for "set all the individual variables", and having things
customizeable by column.ui and not by anything else breaks this.

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

[PATCH v4 3/3] Add colors to interactive git-clean

[Jiang Xin]

Show help, error messages, and prompt in colors for interactive
git-clean. Re-use config variables from git-add--interactive:

 * color.interactive: When set to always, always use colors for
   interactive prompts and displays. When false (or never),
   never. When set to true or auto, use colors only when the
   output is to the terminal.

 * color.interactive.<slot>: Use customized color for interactive
   git-clean output (like git add --interactive). <slot> may be
   prompt, header, help or error.

Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
---
 builtin/clean.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index ac48e..c85b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -14,6 +14,7 @@
 #include "string-list.h"
 #include "quote.h"
 #include "column.h"
+#include "color.h"
 
 static int force = -1; /* unset */
 static int interactive;
@@ -30,15 +31,71 @@ static const char *msg_skip_git_dir = N_("Skipping repository %s\n");
 static const char *msg_would_skip_git_dir = N_("Would skip repository %s\n");
 static const char *msg_warn_remove_failed = N_("failed to remove %s");
 
+static int clean_use_color = -1;
+static char clean_colors[][COLOR_MAXLEN] = {
+	GIT_COLOR_RESET,
+	GIT_COLOR_NORMAL,	/* PLAIN */
+	GIT_COLOR_BOLD_BLUE,	/* PROMPT */
+	GIT_COLOR_BOLD_RED,	/* HELP */
+	GIT_COLOR_BOLD_RED,	/* ERROR */
+};
+enum color_clean {
+	CLEAN_COLOR_RESET = 0,
+	CLEAN_COLOR_PLAIN = 1,
+	CLEAN_COLOR_PROMPT = 2,
+	CLEAN_COLOR_HELP = 3,
+	CLEAN_COLOR_ERROR = 4
+};
+
+static int parse_clean_color_slot(const char *var, int ofs)
+{
+	if (!strcasecmp(var+ofs, "reset"))
+		return CLEAN_COLOR_RESET;
+	if (!strcasecmp(var+ofs, "plain"))
+		return CLEAN_COLOR_PLAIN;
+	if (!strcasecmp(var+ofs, "prompt"))
+		return CLEAN_COLOR_PROMPT;
+	if (!strcasecmp(var+ofs, "help"))
+		return CLEAN_COLOR_HELP;
+	if (!strcasecmp(var+ofs, "error"))
+		return CLEAN_COLOR_ERROR;
+	return -1;
+}
+
 static int git_clean_config(const char *var, const char *value, void *cb)
 {
 	if (!prefixcmp(var, "column."))
 		return git_column_config(var, value, "clean", &colopts);
+	if (!strcmp(var, "color.interactive")) {
+		clean_use_color = git_config_colorbool(var, value);
+		return 0;
+	}
+	if (!prefixcmp(var, "color.interactive.")) {
+		int slot = parse_clean_color_slot(var, 18);
+		if (slot < 0)
+			return 0;
+		if (!value)
+			return config_error_nonbool(var);
+		color_parse(value, var, clean_colors[slot]);
+		return 0;
+	}
 	if (!strcmp(var, "clean.requireforce")) {
 		force = !git_config_bool(var, value);
 		return 0;
 	}
-	return git_default_config(var, value, cb);
+	return git_color_default_config(var, value, cb);
+}
+
+static const char *clean_get_color(enum color_clean ix)
+{
+	if (want_color(clean_use_color))
+		return clean_colors[ix];
+	return "";
+}
+
+static void clean_print_color(enum color_clean ix)
+{
+	printf("%s", clean_get_color(ix));
 }
 
 static int exclude_cb(const struct option *opt, const char *arg, int unset)
@@ -190,16 +247,20 @@ void interactive_clean_edit(struct string_list *dels, const char *prefix)
 	while (1) {
 		/* dels list may become empty when we run string_list_remove_empty_items later */
 		if (!dels->nr) {
+			clean_print_color(CLEAN_COLOR_ERROR);
 			printf_ln(_("No more files to clean, exiting."));
+			clean_print_color(CLEAN_COLOR_RESET);
 			break;
 		}
 
 		if (changed) {
+			clean_print_color(CLEAN_COLOR_HELP);
 			printf(_(
 				 "NOTE: Will remove the following items. You can input space-seperated\n"
 				 "NOTE: patterns (just like .gitignore) to exclude items from deletion,\n"
 				 "NOTE: or press ENTER to continue."
 				));
+			clean_print_color(CLEAN_COLOR_RESET);
 			putchar('\n');
 			putchar('\n');
 
@@ -207,7 +268,9 @@ void interactive_clean_edit(struct string_list *dels, const char *prefix)
 			pretty_print_dels(dels, prefix);
 		}
 
+		clean_print_color(CLEAN_COLOR_PROMPT);
 		printf(_("Input ignore patterns> "));
+		clean_print_color(CLEAN_COLOR_RESET);
 		strbuf_getline(&confirm, stdin, '\n');
 		strbuf_trim(&confirm);
 		putchar('\n');
@@ -244,7 +307,9 @@ void interactive_clean_edit(struct string_list *dels, const char *prefix)
 		if (changed) {
 			string_list_remove_empty_items(dels, 0);
 		} else {
+			clean_print_color(CLEAN_COLOR_ERROR);
 			printf(_("WARNING: Cannot find items matched by: %s"), confirm.buf);
+			clean_print_color(CLEAN_COLOR_RESET);
 			putchar('\n');
 		}
 
@@ -263,6 +328,7 @@ void interactive_clean(struct string_list *dels, const char *prefix)
 
 	/* dels list may become empty after return back from edit mode */
 	while (dels->nr) {
+		clean_print_color(CLEAN_COLOR_HELP);
 		printf_ln(_(
 			    "WARNING: The following items will be removed permanently. Press \"y\"\n"
 			    "WARNING: to start cleaning, and press \"n\" to abort the cleaning.\n"
@@ -270,12 +336,15 @@ void interactive_clean(struct string_list *dels, const char *prefix)
 			    "WARNING: to be excluded from the cleaning."
 			   ));
 		putchar('\n');
+		clean_print_color(CLEAN_COLOR_RESET);
 
 		/* Display dels in columns */
 		pretty_print_dels(dels, prefix);
 
 		/* Confirmation dialog */
+		clean_print_color(CLEAN_COLOR_PROMPT);
 		printf(count > 0 ? _("Remove (Yes/no/edit) ? ") : _("Remove (yes/no/Edit) ? "));
+		clean_print_color(CLEAN_COLOR_RESET);
 		strbuf_getline(&confirm, stdin, '\n');
 		strbuf_trim(&confirm);
 		putchar('\n');
-- 
1.8.3.rc0.364.gbb5463f

Re: [PATCH v4 3/3] Add colors to interactive git-clean

[Matthieu Moy]

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

> Show help, error messages, and prompt in colors for interactive
> git-clean.

I find the red WARNING a bit agressive. Also, the NOTE: is the same
color as the WARNING, hence visually similar. I first thought it was
repeating the same message. I think it would make more sense to use
default color, black, for the NOTE, by analogy with "git add -p" which
uses it for patch hunk headers (somehow, this is the question you're
replying to after the blue prompt below).

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

Re: [PATCH v4 3/3] Add colors to interactive git-clean

[Jiang Xin]

2013/5/2 Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>:
> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> Show help, error messages, and prompt in colors for interactive
>> git-clean.
>
> I find the red WARNING a bit agressive. Also, the NOTE: is the same
> color as the WARNING, hence visually similar. I first thought it was
> repeating the same message. I think it would make more sense to use
> default color, black,

Will use new style -- CLEAN_COLOR_HEADER (GIT_COLOR_BOLD)
as header for interactive git-clean (i.e.  WARNING)

> for the NOTE, by analogy with "git add -p" which
> uses it for patch hunk headers (somehow, this is the question you're
> replying to after the blue prompt below).

I agree. NOTE should only be displayed once, and will be fixed
in patch 3/1 of next reroll.

--
Jiang Xin

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

[Jiang Xin]

2013/5/1 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 add columns and colors display for interactive git-clean, and wish you
like them too.

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

In patch v4 1/3, I make it a three stages cleaning, and it is also
easy to do real cleaning -- just press three ENTERs.

1. Remove (yes/no/Edit) ? -- Press the 1st ENTER (default to EDIT mode)
2. Input ignore patterns>  -- Press the 2nd ENTER (back to confirm)
3. Remove (Yes/no/edit) ? -- Press the 3rd ENTER (start to do cleaning)

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

I will add you with 'Comments-by:' tag temporarily, and invent a
'Spelling-check-by:' tag (now there are 100+ kinds of tags in Git) to
say thank you to Eric. After this series of patches reach a consensus,
I may change the tag from 'Comments-by: to 'Reviewed-by:' with
your permissions.

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

added in patch v4 1/3..

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

Updated in patch v4 1/3.

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


-- 
蒋鑫

北京群英汇信息技术有限公司
邮件: worldhello.net@gmail.com
网址: http://www.ossxp.com/
博客: http://www.worldhello.net/
微博: http://weibo.com/gotgit/
电话: 18601196889