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

2013/5/1 Matthieu Moy [off-list ref]:

Jiang Xin [off-list ref] writes:

quoted

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)

quoted

Reviewed-by: Matthieu Moy <redacted>

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.

quoted

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

quoted

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

quoted

+             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