threads / patch / 34063

patch, 2 partsrm: better error message on failure for multiple files

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

## tl;dr

10 messages between Jun 8, 2013 and Jun 10, 2013. Diffs are folded; open one to read it.

replies: 9people: 5as markdown or json

Mathieu Lienard--Mayor· Jun 8, 2013, 08:33 UTC · lore
From: Mathieu Liénard--Mayor <Mathieu.Lienard--Mayor@ensimag.imag.fr>

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>
---
 builtin/rm.c  |   54 ++++++++++++++++++++++++++++++++++++++----------------
 t/t3600-rm.sh |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+), 16 deletions(-)
Show changes to 2 files +83 −16

builtin/rm.c, t/t3600-rm.sh

diff --git a/builtin/rm.c b/builtin/rm.c
index 7b91d52..5b2abd2 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -82,6 +82,11 @@ static int check_local_mod(unsigned char *head, int index_only)
 	int i, no_head;
 	int errs = 0;
 
+	struct strbuf files_staged = STRBUF_INIT;
+	struct strbuf files_cached = STRBUF_INIT;
+	struct strbuf files_submodule = STRBUF_INIT;
+	struct strbuf files_local = STRBUF_INIT;
+
 	no_head = is_null_sha1(head);
 	for (i = 0; i < list.nr; i++) {
 		struct stat st;
@@ -170,30 +175,47 @@ static int check_local_mod(unsigned char *head, int index_only)
 		 * "intent to add" entry.
 		 */
 		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);
+			if (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD)) {
+				strbuf_addstr(&files_staged, "\n	");
+				strbuf_addstr(&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);
+			if (staged_changes) {
+				strbuf_addstr(&files_cached, "\n	");
+				strbuf_addstr(&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);
+					strbuf_addstr(&files_submodule, "\n	");
+					strbuf_addstr(&files_submodule, name);
+				} else {
+					strbuf_addstr(&files_local, "\n	");
+					strbuf_addstr(&files_local, name);
+				}
 			}
 		}
 	}
+
+	if (files_staged.len)
+		errs = error(_("the following files have staged content "
+			       "different from both the\nfileand the HEAD:%s\n"
+			       "(use -f to force removal)"), files_staged.buf);
+	if (files_cached.len)
+		errs = error(_("the following files have changes staged "
+			       "in the index:%s\n(use --cached to keep the file, "
+			       "or -f to force removal)"), files_cached.buf);
+	if (files_submodule.len)
+		errs = error(_("the following submodules (or one of its nested "
+			       "submodule) use a .git directory:%s\n"
+			       "(use 'rm -rf' if you really want to remove "
+			       "it including all of its history)"), files_submodule.buf);
+	if (files_local.len)
+		errs = error(_("the following files have local modifications:"
+			       "%s\n(use --cached to keep the file, or -f to "
+			       "force removal)"), files_local.buf);
+
 	return errs;
 }
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 0c44e9f..e0f3166 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -687,4 +687,49 @@ 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
Mathieu Lienard--Mayor· Jun 8, 2013, 08:33 UTC · re: Mathieu Lienard--Mayor · lore

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

From: Mathieu Liénard--Mayor <Mathieu.Lienard--Mayor@ensimag.imag.fr>

Similarly to advice.*, advice.rmHints has been added to the config variables. By default, it is set to false, in order to keep the messages the same as before. When set to true, advice are no longer included in the error messages.

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=true:
	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>
---
 Documentation/config.txt |    3 +++
 advice.c                 |    2 ++
 advice.h                 |    1 +
 builtin/rm.c             |   38 ++++++++++++++++++++++++++------------
 t/t3600-rm.sh            |   32 ++++++++++++++++++++++++++++++++
 5 files changed, 64 insertions(+), 12 deletions(-)
Show changes to 5 files +64 −12

Documentation/config.txt, advice.c, advice.h, builtin/rm.c, t/t3600-rm.sh

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 5b2abd2..38ceb73 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;
@@ -200,21 +202,33 @@ static int check_local_mod(unsigned char *head, int index_only)
 
 	if (files_staged.len)
 		errs = error(_("the following files have staged content "
-			       "different from both the\nfileand the HEAD:%s\n"
-			       "(use -f to force removal)"), files_staged.buf);
+			       "different from both the\nfile and the HEAD:%s%s"
+			       ), files_staged.buf,
+			       advice_rm_hints
+			       ? "\n(use -f to force removal)"
+			       : "");
 	if (files_cached.len)
 		errs = error(_("the following files have changes staged "
-			       "in the index:%s\n(use --cached to keep the file, "
-			       "or -f to force removal)"), files_cached.buf);
+			       "in the index:%s%s"), files_cached.buf,
+			       advice_rm_hints
+			       ? "\n(use --cached to keep the file, "
+			       "or -f to force removal)"
+			       : "");
 	if (files_submodule.len)
 		errs = error(_("the following submodules (or one of its nested "
-			       "submodule) use a .git directory:%s\n"
-			       "(use 'rm -rf' if you really want to remove "
-			       "it including all of its history)"), files_submodule.buf);
+			       "submodule) use a .git directory:%s%s"),
+			       files_submodule.buf,
+			       advice_rm_hints
+			       ? "\n(use 'rm -rf' if you really want to remove "
+			       "it including all of its history)"
+			       : "");
 	if (files_local.len)
 		errs = error(_("the following files have local modifications:"
-			       "%s\n(use --cached to keep the file, or -f to "
-			       "force removal)"), files_local.buf);
+			       "%s%s"), files_local.buf,
+			       advice_rm_hints
+			       ? "\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 e0f3166..ab10cc6 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -707,6 +707,19 @@ 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 &&
 error: the following files have local modifications:
@@ -719,6 +732,16 @@ 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 &&
 error: the following files have changes staged in the index:
@@ -732,4 +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
Ramkumar Ramachandra· Jun 8, 2013, 14:01 UTC · re: Mathieu Lienard--Mayor · lore

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

Mathieu Lienard--Mayor wrote:
Show 6 quoted lines
> 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=true:
>         error: 'foo.txt' has changes staged in the index

Um, have you switched the true with false? advice.* variables are true by default, and I turn off all of them.

Also, I think you can extend this to also remove add-advice. Why would someone want to turn off advice from rm, but not add? (Unsure about this)

> Similarly to advice.*, advice.rmHints has been added
> to the config variables. By default, it is set to false, in order to
> keep the messages the same as before. When set to true,  advice
> are no longer included in the error messages.
Ugh, why this roundabout-passive-past tone?  Use imperative tone like this:

Introduce advice.rmHints to control the whether to display advice when using 'git rm'. Defaults to true, preserving current behavior.

Mathieu Liénard--Mayor· Jun 10, 2013, 07:52 UTC · re: Ramkumar Ramachandra · lore

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

Le 2013-06-08 16:01, Ramkumar Ramachandra a écrit :
Show 10 quoted lines
> Mathieu Lienard--Mayor wrote:
>> 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=true:
>>         error: 'foo.txt' has changes staged in the index
>
> Um, have you switched the true with false?  advice.* variables are
> true by default, and I turn off all of them.
Whoops, my bad, I obviously meant false.
>
> Also, I think you can extend this to also remove add-advice.  Why
> would someone want to turn off advice from rm, but not add?  (Unsure
> about this)

I'm not so sure i understand. Do you mean rmHints should deactivate addHints aswell, or do you mean that since we're introducing rmHints it would be natural to introduce addHints ?

Show 8 quoted lines
>
>> Similarly to advice.*, advice.rmHints has been added
>> to the config variables. By default, it is set to false, in order to
>> keep the messages the same as before. When set to true,  advice
>> are no longer included in the error messages.
>
> Ugh, why this roundabout-passive-past tone?  Use imperative tone like 
> this:
Sorry about that, we'll work on it.
>
> Introduce advice.rmHints to control the whether to display advice 
> when
> using 'git rm'.  Defaults to true, preserving current behavior.
-- 
Mathieu Liénard--Mayor,
2nd year at Grenoble INP - ENSIMAG
(+33)6 80 56 30 02
Ramkumar Ramachandra· Jun 10, 2013, 07:55 UTC · re: Mathieu Liénard--Mayor · lore

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

Mathieu Liénard--Mayor wrote:
> I'm not so sure i understand. Do you mean rmHints should deactivate addHints
> aswell, or do you mean that since we're introducing rmHints it would be
> natural to introduce addHints ?

More the latter, but I'm tilting towards addRmHints (or something) which affects both add and rm hints.

> Sorry about that, we'll work on it.

Nothing to be sorry about. You're doing good work, and we're helping you make it even better :)

Matthieu Moy· Jun 10, 2013, 08:24 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 7 quoted lines
> Mathieu Liénard--Mayor wrote:
>> I'm not so sure i understand. Do you mean rmHints should deactivate addHints
>> aswell, or do you mean that since we're introducing rmHints it would be
>> natural to introduce addHints ?
>
> More the latter, but I'm tilting towards addRmHints (or something)
> which affects both add and rm hints.

I don't see why add and rm hints should be correlated, or I don't have the same advice as you in mind.

$ git add foo.txt The following paths are ignored by one of your .gitignore files: foo.txt Use -f if you really want to add them. fatal: no files added

$ git rm foo.txt error: 'foo.txt' has changes staged in the index (use --cached to keep the file, or -f to force removal)

Both have completely different meanings: the first is about .gitignore, and the second about not loosing data.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Ramkumar Ramachandra· Jun 10, 2013, 08:26 UTC · re: Matthieu Moy · lore

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

Matthieu Moy wrote:
Show 5 quoted lines
> I don't see why add and rm hints should be correlated, or I don't have
> the same advice as you in mind.
>
> Both have completely different meanings: the first is about .gitignore,
> and the second about not loosing data.

Right, my bad. Please continue with rmHints, and optionally write an addHints while at it.

Ramkumar Ramachandra· Jun 8, 2013, 13:51 UTC · re: Mathieu Lienard--Mayor · lore

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

Mathieu Lienard--Mayor wrote:
Show 10 quoted lines
> @@ -170,30 +175,47 @@ static int check_local_mod(unsigned char *head, int index_only)
>                  * "intent to add" entry.
>                  */
>                 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);
> +                       if (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD)) {
> +                               strbuf_addstr(&files_staged, "\n        ");
Ouch.  Wouldn't a string-list be more appropriate for this kind of thing?
Mathieu Liénard--Mayor· Jun 10, 2013, 07:43 UTC · re: Ramkumar Ramachandra · lore

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

Once again, thanks a lot your feedback, we appreciate it a lot! Le 2013-06-08 15:51, Ramkumar Ramachandra a écrit :

Show 21 quoted lines
> Mathieu Lienard--Mayor wrote:
>> @@ -170,30 +175,47 @@ static int check_local_mod(unsigned char 
>> *head, int index_only)
>>                  * "intent to add" entry.
>>                  */
>>                 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);
>> +                       if (!index_only || !(ce->ce_flags & 
>> CE_INTENT_TO_ADD)) {
>> +                               strbuf_addstr(&files_staged, "\n     
>>   ");
>
> Ouch.  Wouldn't a string-list be more appropriate for this kind of 
> thing?

Matthieu Moy told me string-list would be better aswell, so we're gonna change it.

-- 
Mathieu Liénard--Mayor,
2nd year at Grenoble INP - ENSIMAG
(+33)6 80 56 30 02
Célestin Matte· Jun 10, 2013, 11:28 UTC · re: Mathieu Lienard--Mayor · lore

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

Le 08/06/2013 10:33, Mathieu Lienard--Mayor a écrit :
> +	if (files_staged.len)
> +		errs = error(_("the following files have staged content "
> +			       "different from both the\nfileand the HEAD:%s\n"
> +			       "(use -f to force removal)"), files_staged.buf);
Typo here: "fileand" -> "file and"
-- 
Célestin Matte

← back to recent threads