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

[PATCH 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 v1:
 -corrected the commit message where "rmHints=true" was supposed to be "false"
 -better use of English tenses in the commit message

 Documentation/config.txt |    3 +++
 advice.c                 |    2 ++
 advice.h                 |    1 +
 builtin/rm.c             |   11 +++++++----
 t/t3600-rm.sh            |   29 +++++++++++++++++++++++++++++
 5 files changed, 42 insertions(+), 4 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 76dfc5b..2226037 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -62,9 +62,11 @@ static int check_submodules_use_gitfiles(void)
 
 		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);
+				       "submodules) uses a .git directory%s"), name,
+				       advice_rm_hints
+				       ? "\n(use 'rm -rf' if you really want to remove "
+				       "it including all of its history)"
+				       : "");
 	}
 
 	return errs;
@@ -85,7 +87,8 @@ static int 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);
 
 	return errs;
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 08bc9bb..92f6146 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -707,6 +707,18 @@ EOF
 	test_cmp 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_cmp expect actual
+'
 
 test_expect_success 'rm file with local modification' '
 	cat >expect << EOF &&
@@ -720,6 +732,15 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_success 'rm file with local modification without hints' '
+	cat >expect << EOF &&
+error: the following files have local modifications:
+    bar.txt
+EOF
+	echo content4 >bar.txt &&
+	test_must_fail git -c advice.rmhints=false rm bar.txt 2>actual &&
+	test_cmp expect actual
+'
 
 test_expect_success 'rm file with changes in the index' '
     cat >expect << EOF &&
@@ -734,5 +755,13 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_success 'rm file with changes in the index without hints' '
+	cat >expect << EOF &&
+error: the following files have changes staged in the index:
+    foo.txt
+EOF
+	test_must_fail git -c advice.rmhints=false rm foo.txt 2>actual &&
+	test_cmp expect actual
+'
 
 test_done
-- 
1.7.8

[PATCH v2 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 v1:
 -introduction of print_error_files() to avoid repetitions
 -implementation with string_list instead of strbuf 
 -typo "fileand" switched to "file and"

 builtin/rm.c  |   74 ++++++++++++++++++++++++++++++++++++++++++++++----------
 t/t3600-rm.sh |   48 +++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+), 14 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 7b91d52..76dfc5b 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -70,6 +70,27 @@ static int check_submodules_use_gitfiles(void)
 	return errs;
 }
 
+/*
+ * PRECONDITION: files_list is a non-empty string_list
+ */
+static int print_error_files(struct string_list *files_list,
+			     const char *main_msg,
+			     const char *hints_msg)
+{
+	int errs = 0;
+	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);
+
+	return errs;
+}
+
 static int check_local_mod(unsigned char *head, int index_only)
 {
 	/*
@@ -82,6 +103,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 +197,49 @@ 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);
+					string_list_append(&files_submodule,
+							   name);
+				} else {
+					string_list_append(&files_local, name);
+				}
 			}
 		}
 	}
+	if (files_staged.nr)
+		errs = print_error_files(&files_staged,
+					 _("the following files have staged "
+					   "content different from both the"
+					   "\nfile and the HEAD:"),
+					 _("\n(use -f to force removal)"));
+	if (files_cached.nr)
+		errs = print_error_files(&files_cached,
+					 _("the following files have changes "
+					   "staged in the index:"),
+					 _("\n(use --cached to keep the file, "
+					   "or -f to force removal)"));
+	if (files_submodule.nr)
+		errs = print_error_files(&files_submodule,
+					 _("the following submodules (or one "
+					   "of its nested submodule) use a "
+					   ".git directory:"),
+					 _("\n(use 'rm -rf' if you really "
+					   "want to remove i including all "
+					   "of its history)"));
+	if (files_local.nr)
+		errs = print_error_files(&files_local,
+					 _("the following files have "
+					   "local modifications:"),
+					 _("\n(use --cached to keep the file, "
+					   "or -f to force removal)"));
+
 	return errs;
 }
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 0c44e9f..08bc9bb 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -687,4 +687,52 @@ 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_cmp expect actual
+'
+
+
+test_expect_success 'rm file with local modification' '
+	cat >expect << EOF &&
+error: the following files have 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_cmp expect actual
+'
+
+
+test_expect_success 'rm file with changes in the index' '
+    cat >expect << EOF &&
+error: the following files have 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_cmp expect actual
+'
+
+
 test_done
-- 
1.7.8

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

[Matthieu Moy]

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

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

There's a "list" after my email, probably a typo.

> +/*
> + * PRECONDITION: files_list is a non-empty string_list
> + */

Avoid repeating in comments what the code already says. "file_list is
non-empty" is sufficient, we already know it's a string_list.

> +	if (files_staged.nr)
> +		errs = print_error_files(&files_staged,
> +					 _("the following files have staged "
> +					   "content different from both the"
> +					   "\nfile and the HEAD:"),
> +					 _("\n(use -f to force removal)"));
> +	if (files_cached.nr)
> +		errs = print_error_files(&files_cached,
> +					 _("the following files have changes "
> +					   "staged in the index:"),
> +					 _("\n(use --cached to keep the file, "
> +					   "or -f to force removal)"));

What happens if both conditions are true? It seems the second will
override the first. I think it'd be OK because what matters is that errs
is set by someone, no matter who, and the error message is displayed on
screen, not contained in the variable, but this looks weird.

I'd find it more readable with "errs |= print_error_files(...)".

And actually, you may want to move the if (....nr) inside
print_error_files (wich could then be called print_error_files_maybe).

At least, there should be a test where two conditions are true.

> +	if (files_submodule.nr)
> +		errs = print_error_files(&files_submodule,
> +					 _("the following submodules (or one "
> +					   "of its nested submodule) use a "
> +					   ".git directory:"),
> +					 _("\n(use 'rm -rf' if you really "
> +					   "want to remove i including all "

i -> it
?

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

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

[Mathieu Liénard--Mayor]

Le 2013-06-10 16:38, Matthieu Moy a écrit :
> Mathieu Lienard--Mayor <Mathieu.Lienard--Mayor@ensimag.imag.fr> 
> writes:
>
>> 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>list
>
> There's a "list" after my email, probably a typo.
yes, that's a leftover from a rebase-i
>
>> +/*
>> + * PRECONDITION: files_list is a non-empty string_list
>> + */
>
> Avoid repeating in comments what the code already says. "file_list is
> non-empty" is sufficient, we already know it's a string_list.
Okay
>
>> +	if (files_staged.nr)
>> +		errs = print_error_files(&files_staged,
>> +					 _("the following files have staged "
>> +					   "content different from both the"
>> +					   "\nfile and the HEAD:"),
>> +					 _("\n(use -f to force removal)"));
>> +	if (files_cached.nr)
>> +		errs = print_error_files(&files_cached,
>> +					 _("the following files have changes "
>> +					   "staged in the index:"),
>> +					 _("\n(use --cached to keep the file, "
>> +					   "or -f to force removal)"));
>
> What happens if both conditions are true? It seems the second will
> override the first. I think it'd be OK because what matters is that 
> errs
> is set by someone, no matter who, and the error message is displayed 
> on
> screen, not contained in the variable, but this looks weird.
>
> I'd find it more readable with "errs |= print_error_files(...)".
Well the current code is only using errs=error(...), using the same 
variable errs over and over, no matter how many times it loops.
That's why i implemented it similarly.
>
> And actually, you may want to move the if (....nr) inside
> print_error_files (wich could then be called 
> print_error_files_maybe).
>
> At least, there should be a test where two conditions are true.
I'll do that, to be sure about the behaviour.
>
>> +	if (files_submodule.nr)
>> +		errs = print_error_files(&files_submodule,
>> +					 _("the following submodules (or one "
>> +					   "of its nested submodule) use a "
>> +					   ".git directory:"),
>> +					 _("\n(use 'rm -rf' if you really "
>> +					   "want to remove i including all "
>
> i -> it
> ?

-- 
Mathieu Liénard--Mayor,
2nd year at Grenoble INP - ENSIMAG
(+33)6 80 56 30 02

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

[Matthieu Moy]

Mathieu Liénard--Mayor <mathieu.lienard--mayor@ensimag.fr> writes:

> Well the current code is only using errs=error(...), using the same
> variable errs over and over, no matter how many times it loops.
> That's why i implemented it similarly.

OK, consistency is a good argument then.

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