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

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

[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 <Mathieu.Lienard--Mayor@ensimag.imag.fr>
Signed-off-by: Jorge Juan Garcia Garcia <Jorge-Juan.Garcia-Garcia@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---

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 force removal)"), name);
+				string_list_append(&files_cached, name);
 			if (local_changes) {
 				if (S_ISGITLINK(ce->ce_mode) &&
-				    !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);
-				} else
-					errs = error(_("'%s' has local modifications\n"
-						     "(use --cached to keep the file, "
-						     "or -f to force removal)"), name);
+				    !submodule_uses_gitfile(name))
+					string_list_append(&files_submodule, name);
+				else
+					string_list_append(&files_local, name);
 			}
 		}
 	}
+	print_error_files(&files_staged,
+			  Q_("the following file has staged content different "
+			     "from both the\nfile and the HEAD:",
+			     "the following files have staged content different"
+			     " from both the\nfile and the HEAD:",
+			     files_staged.nr),
+			  _("\n(use -f to force removal)"),
+			  &errs);
+	print_error_files(&files_cached,
+			  Q_("the following file has changes "
+			     "staged in the index:",
+			     "the following files have changes "
+			     "staged in the index:", files_cached.nr),
+			  _("\n(use --cached to keep the file,"
+			    " or -f to force removal)"),
+			  &errs);
+	print_error_files(&files_submodule,
+			  Q_("the following submodule (or one of its nested "
+			     "submodule)\nuses a .git directory:",
+			     "the following submodules (or one of its nested "
+			     "submodule)\nuse a .git directory:",
+			     files_submodule.nr),
+			  _("\n(use 'rm -rf' if you really "
+			    "want to remove it including all "
+			    "of its history)"),
+			  &errs);
+	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);
+
 	return errs;
 }
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 0c44e9f..902993b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -687,4 +687,71 @@ test_expect_failure SYMLINKS 'rm across a symlinked leading path (w/ index)' '
 	test_path_is_file e/f
 '
 
+test_expect_success 'setup for testing rm messages' '
+	>bar.txt &&
+	>foo.txt &&
+	git add bar.txt foo.txt
+'
+
+test_expect_success 'rm files with different staged content' '
+	cat >expect <<-\EOF &&
+	error: the following files have staged content different from both the
+	file and the HEAD:
+	    bar.txt
+	    foo.txt
+	(use -f to force removal)
+	EOF
+	echo content1 >foo.txt &&
+	echo content1 >bar.txt &&
+	test_must_fail git rm foo.txt bar.txt 2>actual &&
+	test_i18ncmp expect actual
+'
+
+
+test_expect_success 'rm file with local modification' '
+	cat >expect <<-\EOF &&
+	error: the following file has local modifications:
+	    foo.txt
+	(use --cached to keep the file, or -f to force removal)
+	EOF
+	git commit -m "testing rm 3" &&
+	echo content3 >foo.txt &&
+	test_must_fail git rm foo.txt 2>actual &&
+	test_i18ncmp expect actual
+'
+
+
+test_expect_success 'rm file with changes in the index' '
+	cat >expect <<-\EOF &&
+	error: the following file has changes staged in the index:
+	    foo.txt
+	(use --cached to keep the file, or -f to force removal)
+	EOF
+	git reset --hard &&
+	echo content5 >foo.txt &&
+	git add foo.txt &&
+	test_must_fail git rm foo.txt 2>actual &&
+	test_i18ncmp expect actual
+'
+
+
+test_expect_success 'rm files with two different errors' '
+	cat >expect <<-\EOF &&
+	error: the following file has staged content different from both the
+	file and the HEAD:
+	    foo1.txt
+	(use -f to force removal)
+	error: the following file has changes staged in the index:
+	    bar1.txt
+	(use --cached to keep the file, or -f to force removal)
+	EOF
+	echo content >foo1.txt &&
+	git add foo1.txt &&
+	echo content6 >foo1.txt &&
+	echo content6 >bar1.txt &&
+	git add bar1.txt &&
+	test_must_fail git rm bar1.txt foo1.txt 2>actual &&
+	test_i18ncmp expect actual
+'
+
 test_done
-- 
1.7.8

[PATCH v4 2/2] rm: introduce advice.rmHints to shorten messages

[Mathieu Lienard--Mayor]

Introduce advice.rmHints to choose whether to display advice or not
when git rm fails. Defaults to true, in order to preserve current behavior.

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

would look like, with advice.rmHints=false:
	error: 'foo.txt' has changes staged in the index

Signed-off-by: Mathieu Lienard--Mayor <Mathieu.Lienard--Mayor@ensimag.imag.fr>
Signed-off-by: Jorge Juan Garcia Garcia <Jorge-Juan.Garcia-Garcia@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---

Changes since v3:
 - removal of spaces after redirection <<
 - use of -\EOF to indent tests better

 Documentation/config.txt |    3 +++
 advice.c                 |    2 ++
 advice.h                 |    1 +
 builtin/rm.c             |    3 ++-
 t/t3600-rm.sh            |   29 +++++++++++++++++++++++++++++
 5 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e53fc5..eb04479 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -199,6 +199,9 @@ advice.*::
 	amWorkDir::
 		Advice that shows the location of the patch file when
 		linkgit:git-am[1] fails to apply it.
+	rmHints::
+		In case of failure in the output of linkgit:git-rm[1],
+		show directions on how to proceed from the current state.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index a8deee6..a4c169c 100644
--- a/advice.c
+++ b/advice.c
@@ -14,6 +14,7 @@ int advice_resolve_conflict = 1;
 int advice_implicit_identity = 1;
 int advice_detached_head = 1;
 int advice_set_upstream_failure = 1;
+int advice_rm_hints = 1;
 
 static struct {
 	const char *name;
@@ -33,6 +34,7 @@ static struct {
 	{ "implicitidentity", &advice_implicit_identity },
 	{ "detachedhead", &advice_detached_head },
 	{ "setupstreamfailure", &advice_set_upstream_failure },
+	{ "rmhints", &advice_rm_hints },
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index 94caa32..36104c4 100644
--- a/advice.h
+++ b/advice.h
@@ -17,6 +17,7 @@ extern int advice_resolve_conflict;
 extern int advice_implicit_identity;
 extern int advice_detached_head;
 extern int advice_set_upstream_failure;
+extern int advice_rm_hints;
 
 int git_default_advice_config(const char *var, const char *value);
 void advise(const char *advice, ...);
diff --git a/builtin/rm.c b/builtin/rm.c
index e284db3..70df77c 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -49,7 +49,8 @@ static void print_error_files(struct string_list *files_list,
 			strbuf_addf(&err_msg,
 				    "\n    %s",
 				    files_list->items[i].string);
-		strbuf_addstr(&err_msg, hints_msg);
+		if (advice_rm_hints)
+			strbuf_addstr(&err_msg, hints_msg);
 		*errs = error("%s", err_msg.buf);
 		strbuf_release(&err_msg);
 	}
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 902993b..5c87b55 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -707,6 +707,18 @@ test_expect_success 'rm files with different staged content' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'rm files with different staged content without hints' '
+	cat >expect <<-\EOF &&
+	error: the following files have staged content different from both the
+	file and the HEAD:
+	    bar.txt
+	    foo.txt
+	EOF
+	echo content2 >foo.txt &&
+	echo content2 >bar.txt &&
+	test_must_fail git -c advice.rmhints=false rm foo.txt bar.txt 2>actual &&
+	test_i18ncmp expect actual
+'
 
 test_expect_success 'rm file with local modification' '
 	cat >expect <<-\EOF &&
@@ -720,6 +732,15 @@ test_expect_success 'rm file with local modification' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'rm file with local modification without hints' '
+	cat >expect <<-\EOF &&
+	error: the following file has local modifications:
+	    bar.txt
+	EOF
+	echo content4 >bar.txt &&
+	test_must_fail git -c advice.rmhints=false rm bar.txt 2>actual &&
+	test_i18ncmp expect actual
+'
 
 test_expect_success 'rm file with changes in the index' '
 	cat >expect <<-\EOF &&
@@ -734,6 +755,14 @@ test_expect_success 'rm file with changes in the index' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'rm file with changes in the index without hints' '
+	cat >expect <<-\EOF &&
+	error: the following file has changes staged in the index:
+	    foo.txt
+	EOF
+	test_must_fail git -c advice.rmhints=false rm foo.txt 2>actual &&
+	test_i18ncmp expect actual
+'
 
 test_expect_success 'rm files with two different errors' '
 	cat >expect <<-\EOF &&
-- 
1.7.8

Re: [PATCH v4 2/2] rm: introduce advice.rmHints to shorten messages

[Matthieu Moy]

Mathieu Lienard--Mayor <Mathieu.Lienard--Mayor@ensimag.imag.fr> writes:

> Introduce advice.rmHints to choose whether to display advice or not
> when git rm fails. Defaults to true, in order to preserve current behavior.

I went through the serie, and it seems OK to me.

I like the way this patch 2/2 became an obvious 4-lines patch (+ tests)
after 1/2 is refactored properly :-).

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

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

[Junio C Hamano]

Mathieu Lienard--Mayor <Mathieu.Lienard--Mayor@ensimag.imag.fr>
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_*?