Re: [PATCH v2] Add support for -i/--interactive to git-clean
Jiang Xin worldhello@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/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Add support for -i/--interactive to git-clean
2013/4/29 Matthieu Moy matthieu@grenoble-inp.fr:
Jiang Xin worldhello@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Add support for -i/--interactive to git-clean
Jiang Xin worldhello@gmail.com writes: Reviewed-by: Matthieu Moy matthieu@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/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Add support for -i/--interactive to git-clean
Matthieu Moy matthieu@grenoble-inp.fr writes: Jiang Xin worldhello@gmail.com writes: Reviewed-by: Matthieu Moy matthieu@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] Add support for -i/--interactive to git-clean
On Sat, Apr 27, 2013 at 12:13 PM, Jiang Xin worldhello@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);
