Re: [PATCH v7 01/10] Add support for -i/--interactive to git-clean

[Jiang Xin]

2013/5/13 Matthieu Moy matthieu@grenoble-inp.fr:
 + /* Confirmation dialog */
 + printf(_(Remove ([y]es/[n]o/[e]dit) ? ));

 To be more consistent with git add -p, this should use [] instead of
 (), and have no space before ?.

Will be replaced with:

 printf(_(Remove [y/n]? ));


 + die(_(clean.requireForce defaults to true and neither 
 -i, -n nor -f given; 
 refusing to clean));

 That makes it a 85 characters message, and we usually break lines before
 80. Adding \n after ; (instead of a space) would be better IMO.


If die with multiple lines, it looks ugly. E.g.

% git clean
fatal: clean.requireForce defaults to true and neither -i, -n nor -f given;
refusing to clean

I think because of this, some die messages are longer than 80 characters.
Such as:

# builtin/apply.c:1496

die(Q_(git diff header lacks filename information when removing 
%d leading pathname component (line %d),
git diff header lacks filename information when removing 
%d leading pathname components (line %d),

-- 
Jiang Xin
http://www.worldhello.net/
--
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 v7 01/10] Add support for -i/--interactive to git-clean

[Matthieu Moy]

Jiang Xin worldhello@gmail.com writes:

 + putchar('\n');
 +
 + /* Display dels in Would remove ... format */
 + for_each_string_list_item(item, del_list) {
 + qname = quote_path_relative(item-string, -1, 
 buf, *the_prefix);
 + printf(_(msg_would_remove), qname);
 + }
 + putchar('\n');


 + putchar('\n');
 +
 + /* Display dels in Would remove ... format */
 + for_each_string_list_item(item, del_list) {
 + qname = quote_path_relative(item-string, -1, buf, 
 *the_prefix);
 + printf(_(msg_would_remove), qname);
 + }
 + putchar('\n');

These two pieces of code are surprisingly similar ... Shouldn't they be
factored into a small helper function?

 + /* Confirmation dialog */
 + printf(_(Remove ([y]es/[n]o/[e]dit) ? ));

To be more consistent with git add -p, this should use [] instead of
(), and have no space before ?.

 + die(_(clean.requireForce defaults to true and neither 
 -i, -n nor -f given; 
 refusing to clean));

That makes it a 85 characters message, and we usually break lines before
80. Adding \n after ; (instead of a space) would be better IMO.

-- 
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 v7 01/10] Add support for -i/--interactive to git-clean

[Jiang Xin]

2013/5/9 Junio C Hamano gits...@pobox.com:
 @@ -15,9 +15,12 @@
  #include quote.h

  static int force = -1; /* unset */
 +static int interactive;
 +static struct string_list del_list = STRING_LIST_INIT_DUP;
 +static const char **the_prefix;

 Ehh, why?

Next reroll will save relative paths in del_list, and eliminate **the_prefix.


 +
 + printf(_(Input ignore patterns ));
 + if (strbuf_getline(confirm, stdin, '\n') != EOF) {
 + strbuf_trim(confirm);
 + } else {
 + putchar('\n');
 + break;

 Why break here?  If we got nothing, wouldn't confirm.len be zero?
 If we did get something but the input got flushed without line-end,
 sending '\n' to the terminal may be justified, but in that case you
 would may have something useful, and asking confirm.len if it is
 empty would be the consistent way to check between two cases, no?

Yes, this break is unnecessary, it left from pervious revision.



 A few points:

  * Pass prefix as a parameter to this function, just like how
remove_dirs() gets called, and get rid of the_prefix.

  * The result of quote_* is designed to avoid ambiguities, by
applying C-style quotes like HT = \t and adding  pair around
it as necessary.  I doubt feeding it to is_excluded() makes any
sense.  You probably meant path_relative(), but I am not sure.

Appreciated, that is what I need. I write a local version of path_relative,
a combination of path_relative (in quote.c) and relative_path (in path.c),
like this:

static const char *path_relative(const char *in, const char *prefix)

 + for_each_string_list_item(item, del_list) {
 + struct stat st;
 +
 + if (lstat(item-string, st))
 + continue;

 Ignoring errors silently?

 With the interactive stuff, can you get into a situation where you
 originally propose to remove D and D/F but the user tells you to
 remove D (editing D/F away), or vice versa?

I can not find out such a case, that remove parent directory D,
while left file in it, such as D/F.

 I think this patch should be in at least two parts:

  - Introduce the two-phase collect in del_list, remove in a
separate loop at the end restructuring.

  - (optional, if you are feeling ambitious) Change the path that is
stored in del_list relative to the prefix, so that all functions
that operate on the string in the del_list do not have to do
*_relative() thing.  Some functions may instead have to prepend
prefix but if they are minority compared to the users of
*_relative(), it may be an overall win from the readability's
point of view.

  - Add the interactively allow you to reduce the del_list bit
between the two phases.


Will send new reroll soon.


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

[PATCH v7 01/10] 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:

 * y/yes:  Start to do cleaning.
 * n/no:   Nothing will be deleted.
 * e/edit: Exclude items from deletion using ignore patterns.

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@gmail.com
Suggested-by: Junio C Hamano gits...@pobox.com
Spelling-checked-by: Eric Sunshine sunsh...@sunshineco.com
Comments-by: Matthieu Moy matthieu@imag.fr
Suggested-by: Eric Sunshine sunsh...@sunshineco.com
---
 Documentation/git-clean.txt |  15 +++-
 builtin/clean.c | 198 +++-
 2 files changed, 192 insertions(+), 21 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..49aab 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -15,9 +15,12 @@
 #include quote.h
 
 static int force = -1; /* unset */
+static int interactive;
+static struct string_list del_list = STRING_LIST_INIT_DUP;
+static const char **the_prefix;
 
 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 +145,139 @@ static int remove_dirs(struct strbuf *path, const char 
*prefix, int force_flag,
return ret;
 }
 
+static void edit_by_patterns_cmd()
+{
+   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 (!del_list.nr) {
+   printf_ln(_(No more files to clean, exiting.));
+   break;
+   }
+
+   if (changed) {
+   putchar('\n');
+
+   /* Display dels in Would remove ... format */
+   for_each_string_list_item(item, del_list) {
+   qname = quote_path_relative(item-string, -1, 
buf, *the_prefix);
+   printf(_(msg_would_remove), qname);
+   }
+   putchar('\n');
+   }
+
+   printf(_(Input ignore patterns ));
+   if (strbuf_getline(confirm, stdin, '\n') != EOF) {
+   strbuf_trim(confirm);
+   } else {
+   putchar('\n');
+   break;
+   }
+
+   /* 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_max(confirm, ' ', 0);
+
+   for (i = 0; ignore_list[i]; i++) {
+   strbuf_trim(ignore_list[i]);
+   if (!ignore_list[i]-len)
+   continue;
+
+   add_exclude(ignore_list[i]-buf, , 0, el, -(i+1));
+   }
+
+   changed = 0;
+   for_each_string_list_item(item, del_list) {
+   int dtype = DT_UNKNOWN;
+  

Re: [PATCH v7 01/10] Add support for -i/--interactive to git-clean

[Junio C Hamano]

Jiang Xin worldhello@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:

  * y/yes:  Start to do cleaning.
  * n/no:   Nothing will be deleted.
  * e/edit: Exclude items from deletion using ignore patterns.

 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@gmail.com
 Suggested-by: Junio C Hamano gits...@pobox.com
 Spelling-checked-by: Eric Sunshine sunsh...@sunshineco.com
 Comments-by: Matthieu Moy matthieu@imag.fr
 Suggested-by: Eric Sunshine sunsh...@sunshineco.com

Listing everybody who has ever said anything in the review thread?
I can understand that you may want to give credit to those who
significantly helped, but please do not overdo it.

In any case, with the help of their inputs, you brought the patch
into its final shape.  Please sign-off at the _end_.

 ---
  Documentation/git-clean.txt |  15 +++-
  builtin/clean.c | 198 
 +++-
  2 files changed, 192 insertions(+), 21 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..49aab 100644
 --- a/builtin/clean.c
 +++ b/builtin/clean.c
 @@ -15,9 +15,12 @@
  #include quote.h
  
  static int force = -1; /* unset */
 +static int interactive;
 +static struct string_list del_list = STRING_LIST_INIT_DUP;
 +static const char **the_prefix;

Ehh, why?

  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 +145,139 @@ static int remove_dirs(struct strbuf *path, const char 
 *prefix, int force_flag,
   return ret;
  }
  
 +static void edit_by_patterns_cmd()

static void edit_by_patterns_cmd(void)

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

An unnecessary and overlong comment.  The message shown already
tells the reader what is going on anyway, no?

 + if (!del_list.nr) {
 + printf_ln(_(No more files to clean, exiting.));
 + break;
 + }
 +
 + if (changed) {
 + putchar('\n');
 +
 + /* Display dels in Would remove ... format */
 + for_each_string_list_item(item, del_list) {
 + qname = quote_path_relative(item-string, -1, 
 buf, *the_prefix);
 + printf(_(msg_would_remove), qname);
 + }
 + putchar('\n');
 + }
 +
 + printf(_(Input ignore patterns ));
 + if (strbuf_getline(confirm, stdin, '\n') != EOF) {
 + strbuf_trim(confirm);
 + } else {
 + putchar('\n');
 + break;

Why break here?  If we got nothing, wouldn't confirm.len be zero?
If we did get something but the input got flushed without line-end,
sending '\n' to the terminal may be justified, but in that case you
would may have