Re: [PATCH v4 1/2] rm: better error message on failure for multiple files

2013-06-11 Thread Junio C Hamano
Mathieu Lienard--Mayor 
writes:

> +static void print_error_files(struct string_list *files_list,
> +   const char *main_msg,
> +   const char *hints_msg,
> +   int *errs)
> +{
> + if (files_list->nr) {
> + struct strbuf err_msg = STRBUF_INIT;
> + int i;
> + strbuf_addstr(&err_msg, main_msg);
> + for (i = 0; i < files_list->nr; i++)
> + strbuf_addf(&err_msg,
> + "\n%s",
> + files_list->items[i].string);
> + strbuf_addstr(&err_msg, hints_msg);
> + *errs = error("%s", err_msg.buf);
> + strbuf_release(&err_msg);
> + }
> +}
> +
>  static int check_submodules_use_gitfiles(void)
>  {
>   int i;
>   int errs = 0;
>  
> + struct string_list files = STRING_LIST_INIT_NODUP;
> +
>   for (i = 0; i < list.nr; i++) {

The blank after the initialization lines before the first statement
is very much welcom, but please drop the blank line before this new
initialization, i.e.

int i;
int errs = 0;
struct string_list files = STRING_LIST_INIT_NODUP;

for (i = 0; i < list.nr; i++) {
...

> @@ -61,11 +82,17 @@ static int check_submodules_use_gitfiles(void)
>   continue;
>  
>   if (!submodule_uses_gitfile(name))
> + string_list_append(&files, name);
>   }
> + print_error_files(&files,
> +   Q_("the following submodule (or one of its nested "
> +  "submodules)\n uses a .git directory:",
> +  "the following submodules (or one of its nested "
> +  "submodules)\n use a .git directory:",
> +  files.nr),
> +   _("\n(use 'rm -rf' if you really want to remove "
> + "it including all of its history)"),
> +   &errs);
>  
>   return errs;

No string_list_clear() on files?

>  }
> @@ -82,6 +109,11 @@ static int check_local_mod(unsigned char *head, int 
> index_only)
>   int i, no_head;
>   int errs = 0;
>  
> + struct string_list files_staged = STRING_LIST_INIT_NODUP;
> + struct string_list files_cached = STRING_LIST_INIT_NODUP;
> + struct string_list files_submodule = STRING_LIST_INIT_NODUP;
> + struct string_list files_local = STRING_LIST_INIT_NODUP;
> +
> ...
> + print_error_files(&files_local,
> +   Q_("the following file has local modifications:",
> +  "the following files have local modifications:",
> +  files_local.nr),
> +   _("\n(use --cached to keep the file,"
> + " or -f to force removal)"),
> +   &errs);
> +

No string_list_clear() on files_*?

--
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 v4 1/2] rm: better error message on failure for multiple files

2013-06-11 Thread Mathieu Lienard--Mayor
When 'git rm' fails, it now displays a single message
with the list of files involved, instead of displaying
a list of messages with one file each.

As an example, the old message:
error: 'foo.txt' has changes staged in the index
(use --cached to keep the file, or -f to force removal)
error: 'bar.txt' has changes staged in the index
(use --cached to keep the file, or -f to force removal)

would now be displayed as:
error: the following files have changes staged in the index:
foo.txt
bar.txt
(use --cached to keep the file, or -f to force removal)

Signed-off-by: Mathieu Lienard--Mayor 
Signed-off-by: Jorge Juan Garcia Garcia 

Signed-off-by: Matthieu Moy 
---

Changes since v3:
 - rename function print_error_files()
 - use of strbuf_release to avoid leaking
 - change of a forgotten message, now using print_error_files() aswell
 - removal of useless braces
 - use of Q_() to deal with plurals correctly
 - removal of spaces after redirection <<
 - use of -\EOF to indent tests better

 builtin/rm.c  |   95 +---
 t/t3600-rm.sh |   67 
 2 files changed, 143 insertions(+), 19 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 7b91d52..e284db3 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -36,11 +36,32 @@ static int get_ours_cache_pos(const char *path, int pos)
return -1;
 }
 
+static void print_error_files(struct string_list *files_list,
+ const char *main_msg,
+ const char *hints_msg,
+ int *errs)
+{
+   if (files_list->nr) {
+   struct strbuf err_msg = STRBUF_INIT;
+   int i;
+   strbuf_addstr(&err_msg, main_msg);
+   for (i = 0; i < files_list->nr; i++)
+   strbuf_addf(&err_msg,
+   "\n%s",
+   files_list->items[i].string);
+   strbuf_addstr(&err_msg, hints_msg);
+   *errs = error("%s", err_msg.buf);
+   strbuf_release(&err_msg);
+   }
+}
+
 static int check_submodules_use_gitfiles(void)
 {
int i;
int errs = 0;
 
+   struct string_list files = STRING_LIST_INIT_NODUP;
+
for (i = 0; i < list.nr; i++) {
const char *name = list.entry[i].name;
int pos;
@@ -61,11 +82,17 @@ static int check_submodules_use_gitfiles(void)
continue;
 
if (!submodule_uses_gitfile(name))
-   errs = error(_("submodule '%s' (or one of its nested "
-"submodules) uses a .git directory\n"
-"(use 'rm -rf' if you really want to 
remove "
-"it including all of its history)"), name);
+   string_list_append(&files, name);
}
+   print_error_files(&files,
+ Q_("the following submodule (or one of its nested "
+"submodules)\n uses a .git directory:",
+"the following submodules (or one of its nested "
+"submodules)\n use a .git directory:",
+files.nr),
+ _("\n(use 'rm -rf' if you really want to remove "
+   "it including all of its history)"),
+ &errs);
 
return errs;
 }
@@ -82,6 +109,11 @@ static int check_local_mod(unsigned char *head, int 
index_only)
int i, no_head;
int errs = 0;
 
+   struct string_list files_staged = STRING_LIST_INIT_NODUP;
+   struct string_list files_cached = STRING_LIST_INIT_NODUP;
+   struct string_list files_submodule = STRING_LIST_INIT_NODUP;
+   struct string_list files_local = STRING_LIST_INIT_NODUP;
+
no_head = is_null_sha1(head);
for (i = 0; i < list.nr; i++) {
struct stat st;
@@ -171,29 +203,54 @@ static int check_local_mod(unsigned char *head, int 
index_only)
 */
if (local_changes && staged_changes) {
if (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD))
-   errs = error(_("'%s' has staged content 
different "
-"from both the file and the HEAD\n"
-"(use -f to force removal)"), 
name);
+   string_list_append(&files_staged, name);
}
else if (!index_only) {
if (staged_changes)
-   errs = error(_("'%s' has changes staged in the 
index\n"
-"(use --cached to keep the file, "
-"or -f to forc