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

[PATCH 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>
---
 builtin/rm.c |   93 +++++++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 79 insertions(+), 14 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 7b91d52..1bff656 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 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 +176,89 @@ 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) {
+		struct strbuf msg_staged = STRBUF_INIT;
+		int j;
+		strbuf_addstr(
+			&msg_staged,
+			"the following files have staged content different "
+			"from both the\nfile and the HEAD:");
+		for (j = 0; j < files_staged.nr; j++) {
+			strbuf_addf(&msg_staged,
+				    "\n	%s",
+				    files_staged.items[j].string);
+		}
+		strbuf_addstr(&msg_staged,
+			      "\n(use -f to force removal)");
+		errs = error(_("%s"), msg_staged.buf);
+	}
+	if (files_cached.nr) {
+		struct strbuf msg_cached = STRBUF_INIT;
+		int j;
+		strbuf_addstr(
+			&msg_cached,
+			"the following files have changes staged "
+			"in the index:");
+		for (j = 0; j < files_cached.nr; j++) {
+			strbuf_addf(&msg_cached,
+				    "\n	%s",
+				    files_cached.items[j].string);
+		}
+		strbuf_addstr(&msg_cached,
+			      "\n(use --cached to keep the file, "
+			      "or -f to force removal)");
+		errs = error(_("%s"), msg_cached.buf);
+	}
+	if (files_submodule.nr) {
+		struct strbuf msg_sub = STRBUF_INIT;
+		int j;
+		strbuf_addstr(
+			&msg_sub,
+			"the following submodules (or one of its nested "
+			"submodule) use a .git directory:");
+		for (j = 0; j < files_submodule.nr; j++) {
+			strbuf_addf(&msg_sub,
+				    "\n	%s",
+				    files_submodule.items[j].string);
+		}
+		strbuf_addstr(&msg_sub,
+			      "\n(use 'rm -rf' if you really want "
+			      "to remove i including all "
+			      "of its history)");
+		errs = error(_("%s"), msg_sub.buf);
+	}
+	if (files_local.nr) {
+		struct strbuf msg_local = STRBUF_INIT;
+		int j;
+		strbuf_addstr(&msg_local,
+			      "the following files have local modifications:");
+		for (j = 0; j < files_local.nr; j++) {
+			strbuf_addf(&msg_local,
+				    "\n	%s",
+				    files_local.items[j].string);
+		}
+		strbuf_addstr(&msg_local,
+			      "\n(use --cached to keep the file, "
+			      "or -f to force removal)");
+		errs = error(_("%s"), msg_local.buf);
+	}
+
 	return errs;
 }
 
-- 
1.7.8

[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>
---
 Documentation/config.txt |    3 ++
 advice.c                 |    2 +
 advice.h                 |    1 +
 builtin/rm.c             |   36 ++++++++++++---------
 t/t3600-rm.sh            |   77 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 104 insertions(+), 15 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 1bff656..c9081cd 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;
@@ -204,8 +206,9 @@ static int check_local_mod(unsigned char *head, int index_only)
 				    "\n	%s",
 				    files_staged.items[j].string);
 		}
-		strbuf_addstr(&msg_staged,
-			      "\n(use -f to force removal)");
+		if (advice_rm_hints)
+			strbuf_addstr(&msg_staged,
+				   "\n(use -f to force removal)");
 		errs = error(_("%s"), msg_staged.buf);
 	}
 	if (files_cached.nr) {
@@ -220,9 +223,10 @@ static int check_local_mod(unsigned char *head, int index_only)
 				    "\n	%s",
 				    files_cached.items[j].string);
 		}
-		strbuf_addstr(&msg_cached,
-			      "\n(use --cached to keep the file, "
-			      "or -f to force removal)");
+		if (advice_rm_hints)
+			strbuf_addstr(&msg_cached,
+				      "\n(use --cached to keep the file, "
+				      "or -f to force removal)");
 		errs = error(_("%s"), msg_cached.buf);
 	}
 	if (files_submodule.nr) {
@@ -237,10 +241,11 @@ static int check_local_mod(unsigned char *head, int index_only)
 				    "\n	%s",
 				    files_submodule.items[j].string);
 		}
-		strbuf_addstr(&msg_sub,
-			      "\n(use 'rm -rf' if you really want "
-			      "to remove i including all "
-			      "of its history)");
+		if (advice_rm_hints)
+			strbuf_addstr(&msg_sub,
+				      "\n(use 'rm -rf' if you really want "
+				      "to remove i including all "
+				      "of its history)");
 		errs = error(_("%s"), msg_sub.buf);
 	}
 	if (files_local.nr) {
@@ -253,9 +258,10 @@ static int check_local_mod(unsigned char *head, int index_only)
 				    "\n	%s",
 				    files_local.items[j].string);
 		}
-		strbuf_addstr(&msg_local,
-			      "\n(use --cached to keep the file, "
-			      "or -f to force removal)");
+		if (advice_rm_hints)
+			strbuf_addstr(&msg_local,
+				      "\n(use --cached to keep the file, "
+				      "or -f to force removal)");
 		errs = error(_("%s"), msg_local.buf);
 	}
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 0c44e9f..ab10cc6 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -687,4 +687,81 @@ 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 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:
+	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 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:
+	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_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

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

[Mathieu Liénard--Mayor]

Please ignore this, manipulation error while in the git send-email 
command line.

Le 2013-06-10 14:51, Mathieu Lienard--Mayor a écrit :
> 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>
> ---
>  Documentation/config.txt |    3 ++
>  advice.c                 |    2 +
>  advice.h                 |    1 +
>  builtin/rm.c             |   36 ++++++++++++---------
>  t/t3600-rm.sh            |   77 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 104 insertions(+), 15 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 1bff656..c9081cd 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;
> @@ -204,8 +206,9 @@ static int check_local_mod(unsigned char *head,
> int index_only)
>  				    "\n	%s",
>  				    files_staged.items[j].string);
>  		}
> -		strbuf_addstr(&msg_staged,
> -			      "\n(use -f to force removal)");
> +		if (advice_rm_hints)
> +			strbuf_addstr(&msg_staged,
> +				   "\n(use -f to force removal)");
>  		errs = error(_("%s"), msg_staged.buf);
>  	}
>  	if (files_cached.nr) {
> @@ -220,9 +223,10 @@ static int check_local_mod(unsigned char *head,
> int index_only)
>  				    "\n	%s",
>  				    files_cached.items[j].string);
>  		}
> -		strbuf_addstr(&msg_cached,
> -			      "\n(use --cached to keep the file, "
> -			      "or -f to force removal)");
> +		if (advice_rm_hints)
> +			strbuf_addstr(&msg_cached,
> +				      "\n(use --cached to keep the file, "
> +				      "or -f to force removal)");
>  		errs = error(_("%s"), msg_cached.buf);
>  	}
>  	if (files_submodule.nr) {
> @@ -237,10 +241,11 @@ static int check_local_mod(unsigned char *head,
> int index_only)
>  				    "\n	%s",
>  				    files_submodule.items[j].string);
>  		}
> -		strbuf_addstr(&msg_sub,
> -			      "\n(use 'rm -rf' if you really want "
> -			      "to remove i including all "
> -			      "of its history)");
> +		if (advice_rm_hints)
> +			strbuf_addstr(&msg_sub,
> +				      "\n(use 'rm -rf' if you really want "
> +				      "to remove i including all "
> +				      "of its history)");
>  		errs = error(_("%s"), msg_sub.buf);
>  	}
>  	if (files_local.nr) {
> @@ -253,9 +258,10 @@ static int check_local_mod(unsigned char *head,
> int index_only)
>  				    "\n	%s",
>  				    files_local.items[j].string);
>  		}
> -		strbuf_addstr(&msg_local,
> -			      "\n(use --cached to keep the file, "
> -			      "or -f to force removal)");
> +		if (advice_rm_hints)
> +			strbuf_addstr(&msg_local,
> +				      "\n(use --cached to keep the file, "
> +				      "or -f to force removal)");
>  		errs = error(_("%s"), msg_local.buf);
>  	}
>
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 0c44e9f..ab10cc6 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -687,4 +687,81 @@ 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 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:
> +	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 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:
> +	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_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

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

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

[Junio C Hamano]

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

> Please ignore this, manipulation error while in the git send-email
> command line.

Here is what my mailbox looks like (the penultimate one with 252
lines is what I am responding to).  

  R. [ 146: Mathieu Lienard--Mayor ] [PATCH 1/2] rm: better error messa
  R. [ 231: Mathieu Lienard--Mayor ] [PATCH 2/2] rm: introduce advice.r
  R. [ 157: Mathieu Lienard--Mayor ] [PATCH 2/2] rm: introduce advice.r
  R. [ 198: Mathieu Lienard--Mayor ] [PATCH v2 1/2] rm: better error me
  R. [ 157: Mathieu Lienard--Mayor ] [PATCH v2 2/2] rm: introduce advic
   . [ 153: Mathieu Lienard--Mayor ] [PATCH v3 2/2] rm: introduce advic
   . [ 214: Mathieu Lienard--Mayor ] [PATCH v3 1/2]  rm: better error m
   . [ 214: Mathieu Lienard--Mayor ] [PATCH v3 1/2]  rm: better error m
   . [ 153: Mathieu Lienard--Mayor ] [PATCH v3 2/2] rm: introduce advic
  R  [  33: Mathieu Liénard--Mayor ] Re: [PATCH 1/2] rm: better error m
  O  [  38: Mathieu Liénard--Mayor ] Re: [PATCH 2/2] rm: introduce advi
  R. [ 156: Mathieu Liénard--Mayor ] Re: [PATCH 1/2] rm: better error m
  R. [ 252: Mathieu Liénard--Mayor ] Re: [PATCH 2/2] rm: introduce advi
   . [  84: Mathieu Liénard--Mayor ] Re: [PATCH v2 1/2] rm: better erro

I am guessing that [v3 1/2] and [v3 2/2] are the final ones but it
that is not the case please holler.

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

[Mathieu Liénard--Mayor]

Le 2013-06-10 18:57, Junio C Hamano a écrit :
> Mathieu Liénard--Mayor  <mathieu.lienard--mayor@ensimag.fr> writes:
>
>> Please ignore this, manipulation error while in the git send-email
>> command line.
>
> Here is what my mailbox looks like (the penultimate one with 252
> lines is what I am responding to).
>
>   R. [ 146: Mathieu Lienard--Mayor ] [PATCH 1/2] rm: better error 
> messa
>   R. [ 231: Mathieu Lienard--Mayor ] [PATCH 2/2] rm: introduce 
> advice.r
>   R. [ 157: Mathieu Lienard--Mayor ] [PATCH 2/2] rm: introduce 
> advice.r
>   R. [ 198: Mathieu Lienard--Mayor ] [PATCH v2 1/2] rm: better error 
> me
>   R. [ 157: Mathieu Lienard--Mayor ] [PATCH v2 2/2] rm: introduce 
> advic
>    . [ 153: Mathieu Lienard--Mayor ] [PATCH v3 2/2] rm: introduce 
> advic
>    . [ 214: Mathieu Lienard--Mayor ] [PATCH v3 1/2]  rm: better error 
> m
>    . [ 214: Mathieu Lienard--Mayor ] [PATCH v3 1/2]  rm: better error 
> m
>    . [ 153: Mathieu Lienard--Mayor ] [PATCH v3 2/2] rm: introduce 
> advic
>   R  [  33: Mathieu Liénard--Mayor ] Re: [PATCH 1/2] rm: better error 
> m
>   O  [  38: Mathieu Liénard--Mayor ] Re: [PATCH 2/2] rm: introduce 
> advi
>   R. [ 156: Mathieu Liénard--Mayor ] Re: [PATCH 1/2] rm: better error 
> m
>   R. [ 252: Mathieu Liénard--Mayor ] Re: [PATCH 2/2] rm: introduce 
> advi
>    . [  84: Mathieu Liénard--Mayor ] Re: [PATCH v2 1/2] rm: better 
> erro
>
> I am guessing that [v3 1/2] and [v3 2/2] are the final ones but it
> that is not the case please holler.
Yes, [v3 1/2] and [v3 2/2] are the final ones.
i'm sorry, i really don't know how i managed to create such a mess, i'm 
still not familiar with the send-email tool =/
-- 
Mathieu Liénard--Mayor,
2nd year at Grenoble INP - ENSIMAG
(+33)6 80 56 30 02

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

[Mathieu Liénard--Mayor]

Please ignore this, manipulation error while in the git send-email 
command line.

Le 2013-06-10 14:51, Mathieu Lienard--Mayor a écrit :
> 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 |   93 
> +++++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 79 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 7b91d52..1bff656 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 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 +176,89 @@ 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) {
> +		struct strbuf msg_staged = STRBUF_INIT;
> +		int j;
> +		strbuf_addstr(
> +			&msg_staged,
> +			"the following files have staged content different "
> +			"from both the\nfile and the HEAD:");
> +		for (j = 0; j < files_staged.nr; j++) {
> +			strbuf_addf(&msg_staged,
> +				    "\n	%s",
> +				    files_staged.items[j].string);
> +		}
> +		strbuf_addstr(&msg_staged,
> +			      "\n(use -f to force removal)");
> +		errs = error(_("%s"), msg_staged.buf);
> +	}
> +	if (files_cached.nr) {
> +		struct strbuf msg_cached = STRBUF_INIT;
> +		int j;
> +		strbuf_addstr(
> +			&msg_cached,
> +			"the following files have changes staged "
> +			"in the index:");
> +		for (j = 0; j < files_cached.nr; j++) {
> +			strbuf_addf(&msg_cached,
> +				    "\n	%s",
> +				    files_cached.items[j].string);
> +		}
> +		strbuf_addstr(&msg_cached,
> +			      "\n(use --cached to keep the file, "
> +			      "or -f to force removal)");
> +		errs = error(_("%s"), msg_cached.buf);
> +	}
> +	if (files_submodule.nr) {
> +		struct strbuf msg_sub = STRBUF_INIT;
> +		int j;
> +		strbuf_addstr(
> +			&msg_sub,
> +			"the following submodules (or one of its nested "
> +			"submodule) use a .git directory:");
> +		for (j = 0; j < files_submodule.nr; j++) {
> +			strbuf_addf(&msg_sub,
> +				    "\n	%s",
> +				    files_submodule.items[j].string);
> +		}
> +		strbuf_addstr(&msg_sub,
> +			      "\n(use 'rm -rf' if you really want "
> +			      "to remove i including all "
> +			      "of its history)");
> +		errs = error(_("%s"), msg_sub.buf);
> +	}
> +	if (files_local.nr) {
> +		struct strbuf msg_local = STRBUF_INIT;
> +		int j;
> +		strbuf_addstr(&msg_local,
> +			      "the following files have local modifications:");
> +		for (j = 0; j < files_local.nr; j++) {
> +			strbuf_addf(&msg_local,
> +				    "\n	%s",
> +				    files_local.items[j].string);
> +		}
> +		strbuf_addstr(&msg_local,
> +			      "\n(use --cached to keep the file, "
> +			      "or -f to force removal)");
> +		errs = error(_("%s"), msg_local.buf);
> +	}
> +
>  	return errs;
>  }

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

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

[Mathieu Lienard--Mayor]

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

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

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

[Ramkumar Ramachandra]

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?

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

[Mathieu Liénard--Mayor]

Once again, thanks a lot your feedback, we appreciate it a lot!
Le 2013-06-08 15:51, Ramkumar Ramachandra a écrit :
> 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

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

[Célestin Matte]

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