git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2]  rm: better error message on failure for multiple files
@ 2013-06-10 16:05 Mathieu Lienard--Mayor
  2013-06-10 16:05 ` [PATCH v3 2/2] rm: introduce advice.rmHints to shorten messages Mathieu Lienard--Mayor
  0 siblings, 1 reply; 7+ messages in thread
From: Mathieu Lienard--Mayor @ 2013-06-10 16:05 UTC (permalink / raw)
  To: git
  Cc: gitster, Mathieu Lienard--Mayor, Jorge Juan Garcia Garcia, Matthieu Moy

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 v2:
 -couple typo in commit message and in code
 -rename and redefinition of the intermediate function
 -move if(....nr) inside the function

 builtin/rm.c  |   71 +++++++++++++++++++++++++++++++++++++++++++++-----------
 t/t3600-rm.sh |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+), 14 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 7b91d52..07306eb 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -70,6 +70,24 @@ static int check_submodules_use_gitfiles(void)
 	return errs;
 }
 
+static void print_eventual_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);
+	}
+}
+
 static int check_local_mod(unsigned char *head, int index_only)
 {
 	/*
@@ -82,6 +100,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 +194,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);
+				}
 			}
 		}
 	}
+	print_eventual_error_files(&files_staged,
+				   _("the following files have staged "
+				     "content different from both the"
+				     "\nfile and the HEAD:"),
+				   _("\n(use -f to force removal)"),
+				   &errs);
+	print_eventual_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)"),
+				   &errs);
+	print_eventual_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 it including all "
+				     "of its history)"),
+				   &errs);
+	print_eventual_error_files(&files_local,
+				   _("the following files have "
+				     "local modifications:"),
+				   _("\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..10dd380 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_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_expect_success 'rm files with two different errors' '
+	cat >expect << EOF &&
+error: the following files have staged content different from both the
+file and the HEAD:
+    foo1.txt
+(use -f to force removal)
+error: the following files have 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_cmp expect actual
+'
+
 test_done
-- 
1.7.8

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v3 2/2] rm: introduce advice.rmHints to shorten messages
  2013-06-10 16:05 [PATCH v3 1/2] rm: better error message on failure for multiple files Mathieu Lienard--Mayor
@ 2013-06-10 16:05 ` Mathieu Lienard--Mayor
  0 siblings, 0 replies; 7+ messages in thread
From: Mathieu Lienard--Mayor @ 2013-06-10 16:05 UTC (permalink / raw)
  To: git
  Cc: gitster, Mathieu Lienard--Mayor, Jorge Juan Garcia Garcia, Matthieu Moy

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             |   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 07306eb..c991fe6 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;
@@ -83,7 +85,8 @@ static void print_eventual_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);
 	}
 }
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 10dd380..74f048c 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,6 +755,14 @@ 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_expect_success 'rm files with two different errors' '
 	cat >expect << EOF &&
-- 
1.7.8

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2]  rm: better error message on failure for multiple files
  2013-06-10 17:42   ` Matthieu Moy
@ 2013-06-10 20:55     ` Junio C Hamano
  0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2013-06-10 20:55 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: Mathieu Lienard--Mayor, git, Jorge Juan Garcia Garcia

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> +{
>>> +	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",
>>
>> Is there an implication of having always 4 spaces here to l10n/i18n
>> here?  I am wondering if it should be _("\n    %s").
>
> I'd say this is just formatting and should be the same in every
> languages, but I'm far from an expert in the domain.

After looking at the patch again I do not think 4-SP matters.  I was
primarily worried if this was to align with some column of the first
line of output, e.g.

	error: lorem ipsum dolor sit amet, consectetur adipisicing
               elit, sed do eiusmod tempor incididunt ut labore et
               dolore magna aliqua.

but that is not what this 4-SP indent is about, so it is OK.

>>         test_expect_success 'rm files with different staged content' '
>>                 cat >expect <<\-EOF &&
>
> (that should be -\EOF, not \-EOF I think)

Sorry, my bad.  You are of course right.

>>  (2) by using a dash '-' before the end-of-here-text marker, you can
>>      align the body of here text with a leading tab (HT).
>
> This works because the list of files is aligned with spaces, but is
> seems a bit fragile to me to use this -EOF on a text which uses
> indentation. Anyway, I'm fine with both.

True.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2]  rm: better error message on failure for multiple files
  2013-06-10 17:17 ` Junio C Hamano
@ 2013-06-10 17:42   ` Matthieu Moy
  2013-06-10 20:55     ` Junio C Hamano
  0 siblings, 1 reply; 7+ messages in thread
From: Matthieu Moy @ 2013-06-10 17:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Mathieu Lienard--Mayor, git, Jorge Juan Garcia Garcia

Junio C Hamano <gitster@pobox.com> writes:

>> +{
>> +	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",
>
> Is there an implication of having always 4 spaces here to l10n/i18n
> here?  I am wondering if it should be _("\n    %s").

I'd say this is just formatting and should be the same in every
languages, but I'm far from an expert in the domain. Maybe some
right-to-left languages would need this.

>         test_expect_success 'rm files with different staged content' '
>                 cat >expect <<\-EOF &&

(that should be -\EOF, not \-EOF I think)

>  (2) by using a dash '-' before the end-of-here-text marker, you can
>      align the body of here text with a leading tab (HT).

This works because the list of files is aligned with spaces, but is
seems a bit fragile to me to use this -EOF on a text which uses
indentation. Anyway, I'm fine with both.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2]  rm: better error message on failure for multiple files
  2013-06-10 15:59 [PATCH v3 1/2] rm: better error message on failure for multiple files Mathieu Lienard--Mayor
  2013-06-10 16:06 ` Matthieu Moy
@ 2013-06-10 17:17 ` Junio C Hamano
  2013-06-10 17:42   ` Matthieu Moy
  1 sibling, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2013-06-10 17:17 UTC (permalink / raw)
  To: Mathieu Lienard--Mayor; +Cc: git, Jorge Juan Garcia Garcia, 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>
> ---
>
> Changes since v2:
>  -couple typo in commit message and in code
>  -rename and redefinition of the intermediate function
>  -move the 4 "if(....nr)" inside the function
>
>  builtin/rm.c  |   71 +++++++++++++++++++++++++++++++++++++++++++++-----------
>  t/t3600-rm.sh |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 7b91d52..07306eb 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -70,6 +70,24 @@ static int check_submodules_use_gitfiles(void)
>  	return errs;
>  }
>  
> +static void print_eventual_error_files(struct string_list *files_list,
> +				       const char *main_msg,
> +				       const char *hints_msg,
> +				       int *errs)

Hrm, I do not see the point of "eventual" there, by the way.  Are
there other kinds of error files?

> +{
> +	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",

Is there an implication of having always 4 spaces here to l10n/i18n
here?  I am wondering if it should be _("\n    %s").

> +				    files_list->items[i].string);
> +		strbuf_addstr(&err_msg, hints_msg);
> +		*errs = error("%s", err_msg.buf);

There needs a strbuf_release(&err_msg) somewhere before leaving this
scope to avoid leaking its buffer, no?

> +	}
> +}
> +
>  static int check_local_mod(unsigned char *head, int index_only)
>  {
>  	/*
> @@ -82,6 +100,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 +194,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))
> +				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)) {
> +					string_list_append(&files_submodule,
> +							   name);
> +				} else {
> +					string_list_append(&files_local, name);
> +				}

The innermost if/else no longer needs braces.  Also even though it
may push it slightly over 80-column, I think the files_submodule
side of string_list_append() is easier to read if it were on a
single line.

> +	print_eventual_error_files(&files_staged,
> +				   _("the following files have staged "
> +				     "content different from both the"
> +				     "\nfile and the HEAD:"),
> +				   _("\n(use -f to force removal)"),
> +				   &errs);

Hmph.  I wonder if we want to properly i18n plurals, depending on
the number of files, e.g.

        print_error_files(&files_staged,
                          Q_("the following file has staged "
                             "content different from both the\n"
                             "file and the HEAD:",
                             "the following files have staged "
                             "content different from both the\n"
                             "file and the HEAD:", files_staged.nr),
                           _("\n(use -f to force removal)"), &errs);

This was not a problem back when we showed one error message per one
path, but with this patch, it starts to matter.

> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 0c44e9f..10dd380 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 &&

It is easier to read if it is done this way:

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

Two and half points to note:

 (0) no whitespace between redirection << and its source
     (the end-of-here-text marker in this case).

 (1) by quoting the end-of-here-text marker with a backslash '\',
     you tell the readers that there is no variable substitution in
     it, which reduces the mental burden on them.

 (2) by using a dash '-' before the end-of-here-text marker, you can
     align the body of here text with a leading tab (HT).

> +	echo content1 >bar.txt &&
> +	test_must_fail git rm foo.txt bar.txt 2>actual &&
> +	test_cmp expect actual

This should be test_i18ncmp as the error messages are marked for
l10n.

Other than that, looks nicely done.

> +'
> +
> +
> +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_expect_success 'rm files with two different errors' '
> +	cat >expect << EOF &&
> +error: the following files have staged content different from both the
> +file and the HEAD:
> +    foo1.txt
> +(use -f to force removal)
> +error: the following files have 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_cmp expect actual
> +'
> +
>  test_done

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2]  rm: better error message on failure for multiple files
  2013-06-10 15:59 [PATCH v3 1/2] rm: better error message on failure for multiple files Mathieu Lienard--Mayor
@ 2013-06-10 16:06 ` Matthieu Moy
  2013-06-10 17:17 ` Junio C Hamano
  1 sibling, 0 replies; 7+ messages in thread
From: Matthieu Moy @ 2013-06-10 16:06 UTC (permalink / raw)
  To: Mathieu Lienard--Mayor; +Cc: git, gitster, Jorge Juan Garcia Garcia

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

> +static void print_eventual_error_files(struct string_list *files_list,

Too french ;-).

Eventual (en) = final, utlime (fr).

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 1/2]  rm: better error message on failure for multiple files
@ 2013-06-10 15:59 Mathieu Lienard--Mayor
  2013-06-10 16:06 ` Matthieu Moy
  2013-06-10 17:17 ` Junio C Hamano
  0 siblings, 2 replies; 7+ messages in thread
From: Mathieu Lienard--Mayor @ 2013-06-10 15:59 UTC (permalink / raw)
  To: git
  Cc: gitster, Mathieu Lienard--Mayor, Jorge Juan Garcia Garcia, Matthieu Moy

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 v2:
 -couple typo in commit message and in code
 -rename and redefinition of the intermediate function
 -move the 4 "if(....nr)" inside the function

 builtin/rm.c  |   71 +++++++++++++++++++++++++++++++++++++++++++++-----------
 t/t3600-rm.sh |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 124 insertions(+), 14 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 7b91d52..07306eb 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -70,6 +70,24 @@ static int check_submodules_use_gitfiles(void)
 	return errs;
 }
 
+static void print_eventual_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);
+	}
+}
+
 static int check_local_mod(unsigned char *head, int index_only)
 {
 	/*
@@ -82,6 +100,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 +194,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);
+				}
 			}
 		}
 	}
+	print_eventual_error_files(&files_staged,
+				   _("the following files have staged "
+				     "content different from both the"
+				     "\nfile and the HEAD:"),
+				   _("\n(use -f to force removal)"),
+				   &errs);
+	print_eventual_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)"),
+				   &errs);
+	print_eventual_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 it including all "
+				     "of its history)"),
+				   &errs);
+	print_eventual_error_files(&files_local,
+				   _("the following files have "
+				     "local modifications:"),
+				   _("\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..10dd380 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_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_expect_success 'rm files with two different errors' '
+	cat >expect << EOF &&
+error: the following files have staged content different from both the
+file and the HEAD:
+    foo1.txt
+(use -f to force removal)
+error: the following files have 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_cmp expect actual
+'
+
 test_done
-- 
1.7.8

^ permalink raw reply related	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2013-06-10 20:55 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-10 16:05 [PATCH v3 1/2] rm: better error message on failure for multiple files Mathieu Lienard--Mayor
2013-06-10 16:05 ` [PATCH v3 2/2] rm: introduce advice.rmHints to shorten messages Mathieu Lienard--Mayor
  -- strict thread matches above, loose matches on Subject: below --
2013-06-10 15:59 [PATCH v3 1/2] rm: better error message on failure for multiple files Mathieu Lienard--Mayor
2013-06-10 16:06 ` Matthieu Moy
2013-06-10 17:17 ` Junio C Hamano
2013-06-10 17:42   ` Matthieu Moy
2013-06-10 20:55     ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).