{"thread":{"id":"34094","subject":"[PATCH v3 2/2] rm: introduce advice.rmHints to shorten messages","startedAt":"2013-06-10T15:59:40Z","lastAt":"2013-06-10T20:55:48Z","messageCount":7,"participants":["Mathieu Lienard--Mayor","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"220308","messageId":"1370879981-18937-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","threadId":"34094","inReplyTo":null,"subject":"[PATCH v3 1/2] rm: better error message on failure for multiple files","fromName":"Mathieu Lienard--Mayor","fromEmail":"mathieu.lienard--mayor@ensimag.imag.fr","sentAt":"2013-06-10T15:59:40Z","receivedAt":"2013-06-10T15:59:40Z","isPatch":true,"sender":{"key":"mathieu.lienard--mayor@ensimag.imag.fr","avatar":null},"body":"When 'git rm' fails, it now displays a single message\nwith the list of files involved, instead of displaying\na list of messages with one file each.\n\nAs an example, the old message:\n\terror: 'foo.txt' has changes staged in the index\n\t(use --cached to keep the file, or -f to force removal)\n\terror: 'bar.txt' has changes staged in the index\n\t(use --cached to keep the file, or -f to force removal)\n\nwould now be displayed as:\n\terror: the following files have changes staged in the index:\n\t foo.txt\n\t bar.txt\n\t(use --cached to keep the file, or -f to force removal)\n\nSigned-off-by: Mathieu Lienard--Mayor \nSigned-off-by: Jorge Juan Garcia Garcia \nSigned-off-by: Matthieu Moy \n---\n\nChanges since v2:\n -couple typo in commit message and in code\n -rename and redefinition of the intermediate function\n -move the 4 \"if(....nr)\" inside the function\n\n builtin/rm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-----------\n t/t3600-rm.sh | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 124 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 7b91d52..07306eb 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -70,6 +70,24 @@ static int check_submodules_use_gitfiles(void)\n \treturn errs;\n }\n \n+static void print_eventual_error_files(struct string_list *files_list,\n+\t\t\t\t const char *main_msg,\n+\t\t\t\t const char *hints_msg,\n+\t\t\t\t int *errs)\n+{\n+\tif (files_list->nr) {\n+\t\tstruct strbuf err_msg = STRBUF_INIT;\n+\t\tint i;\n+\t\tstrbuf_addstr(&err_msg, main_msg);\n+\t\tfor (i = 0; i < files_list->nr; i++)\n+\t\t\tstrbuf_addf(&err_msg,\n+\t\t\t\t \"\\n %s\",\n+\t\t\t\t files_list->items[i].string);\n+\t\tstrbuf_addstr(&err_msg, hints_msg);\n+\t\t*errs = error(\"%s\", err_msg.buf);\n+\t}\n+}\n+\n static int check_local_mod(unsigned char *head, int index_only)\n {\n \t/*\n@@ -82,6 +100,11 @@ static int check_local_mod(unsigned char *head, int index_only)\n \tint i, no_head;\n \tint errs = 0;\n \n+\tstruct string_list files_staged = STRING_LIST_INIT_NODUP;\n+\tstruct string_list files_cached = STRING_LIST_INIT_NODUP;\n+\tstruct string_list files_submodule = STRING_LIST_INIT_NODUP;\n+\tstruct string_list files_local = STRING_LIST_INIT_NODUP;\n+\n \tno_head = is_null_sha1(head);\n \tfor (i = 0; i < list.nr; i++) {\n \t\tstruct stat st;\n@@ -171,29 +194,49 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t\t */\n \t\tif (local_changes && staged_changes) {\n \t\t\tif (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD))\n-\t\t\t\terrs = error(_(\"'%s' has staged content different \"\n-\t\t\t\t\t \"from both the file and the HEAD\\n\"\n-\t\t\t\t\t \"(use -f to force removal)\"), name);\n+\t\t\t\tstring_list_append(&files_staged, name);\n \t\t}\n \t\telse if (!index_only) {\n \t\t\tif (staged_changes)\n-\t\t\t\terrs = error(_(\"'%s' has changes staged in the index\\n\"\n-\t\t\t\t\t \"(use --cached to keep the file, \"\n-\t\t\t\t\t \"or -f to force removal)\"), name);\n+\t\t\t\tstring_list_append(&files_cached, name);\n \t\t\tif (local_changes) {\n \t\t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n \t\t\t\t !submodule_uses_gitfile(name)) {\n-\t\t\t\t\terrs = error(_(\"submodule '%s' (or one of its nested \"\n-\t\t\t\t\t\t \"submodules) uses a .git directory\\n\"\n-\t\t\t\t\t\t \"(use 'rm -rf' if you really want to remove \"\n-\t\t\t\t\t\t \"it including all of its history)\"), name);\n-\t\t\t\t} else\n-\t\t\t\t\terrs = error(_(\"'%s' has local modifications\\n\"\n-\t\t\t\t\t\t \"(use --cached to keep the file, \"\n-\t\t\t\t\t\t \"or -f to force removal)\"), name);\n+\t\t\t\t\tstring_list_append(&files_submodule,\n+\t\t\t\t\t\t\t name);\n+\t\t\t\t} else {\n+\t\t\t\t\tstring_list_append(&files_local, name);\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n \t}\n+\tprint_eventual_error_files(&files_staged,\n+\t\t\t\t _(\"the following files have staged \"\n+\t\t\t\t \"content different from both the\"\n+\t\t\t\t \"\\nfile and the HEAD:\"),\n+\t\t\t\t _(\"\\n(use -f to force removal)\"),\n+\t\t\t\t &errs);\n+\tprint_eventual_error_files(&files_cached,\n+\t\t\t\t _(\"the following files have changes \"\n+\t\t\t\t \"staged in the index:\"),\n+\t\t\t\t _(\"\\n(use --cached to keep the file,\"\n+\t\t\t\t \" or -f to force removal)\"),\n+\t\t\t\t &errs);\n+\tprint_eventual_error_files(&files_submodule,\n+\t\t\t\t _(\"the following submodules (or one \"\n+\t\t\t\t \"of its nested submodule) use a \"\n+\t\t\t\t \".git directory:\"),\n+\t\t\t\t _(\"\\n(use 'rm -rf' if you really \"\n+\t\t\t\t \"want to remove it including all \"\n+\t\t\t\t \"of its history)\"),\n+\t\t\t\t &errs);\n+\tprint_eventual_error_files(&files_local,\n+\t\t\t\t _(\"the following files have \"\n+\t\t\t\t \"local modifications:\"),\n+\t\t\t\t _(\"\\n(use --cached to keep the file,\"\n+\t\t\t\t \" or -f to force removal)\"),\n+\t\t\t\t &errs);\n+\n \treturn errs;\n }\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 0c44e9f..10dd380 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -687,4 +687,71 @@ test_expect_failure SYMLINKS 'rm across a symlinked leading path (w/ index)' '\n \ttest_path_is_file e/f\n '\n \n+test_expect_success 'setup for testing rm messages' '\n+\t>bar.txt &&\n+\t>foo.txt &&\n+\tgit add bar.txt foo.txt\n+'\n+\n+test_expect_success 'rm files with different staged content' '\n+\tcat >expect << EOF &&\n+error: the following files have staged content different from both the\n+file and the HEAD:\n+ bar.txt\n+ foo.txt\n+(use -f to force removal)\n+EOF\n+\techo content1 >foo.txt &&\n+\techo content1 >bar.txt &&\n+\ttest_must_fail git rm foo.txt bar.txt 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n+test_expect_success 'rm file with local modification' '\n+\tcat >expect << EOF &&\n+error: the following files have local modifications:\n+ foo.txt\n+(use --cached to keep the file, or -f to force removal)\n+EOF\n+\tgit commit -m \"testing rm 3\" &&\n+\techo content3 >foo.txt &&\n+\ttest_must_fail git rm foo.txt 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n+test_expect_success 'rm file with changes in the index' '\n+ cat >expect << EOF &&\n+error: the following files have changes staged in the index:\n+ foo.txt\n+(use --cached to keep the file, or -f to force removal)\n+EOF\n+\tgit reset --hard &&\n+\techo content5 >foo.txt &&\n+\tgit add foo.txt &&\n+\ttest_must_fail git rm foo.txt 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n+test_expect_success 'rm files with two different errors' '\n+\tcat >expect << EOF &&\n+error: the following files have staged content different from both the\n+file and the HEAD:\n+ foo1.txt\n+(use -f to force removal)\n+error: the following files have changes staged in the index:\n+ bar1.txt\n+(use --cached to keep the file, or -f to force removal)\n+EOF\n+\techo content >foo1.txt &&\n+\tgit add foo1.txt &&\n+\techo content6 >foo1.txt &&\n+\techo content6 >bar1.txt &&\n+\tgit add bar1.txt &&\n+\ttest_must_fail git rm bar1.txt foo1.txt 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.8\n"},{"id":"220306","messageId":"1370879981-18937-2-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","threadId":"34094","inReplyTo":"1370879981-18937-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","subject":"[PATCH v3 2/2] rm: introduce advice.rmHints to shorten messages","fromName":"Mathieu Lienard--Mayor","fromEmail":"mathieu.lienard--mayor@ensimag.imag.fr","sentAt":"2013-06-10T15:59:41Z","receivedAt":"2013-06-10T15:59:41Z","isPatch":true,"sender":{"key":"mathieu.lienard--mayor@ensimag.imag.fr","avatar":null},"body":"Introduce advice.rmHints to choose whether to display advice or not\nwhen git rm fails. Defaults to true, in order to preserve current behavior.\n\nAs an example, the message:\n\terror: 'foo.txt' has changes staged in the index\n\t(use --cached to keep the file, or -f to force removal)\n\nwould look like, with advice.rmHints=false:\n\terror: 'foo.txt' has changes staged in the index\n\nSigned-off-by: Mathieu Lienard--Mayor \nSigned-off-by: Jorge Juan Garcia Garcia \nSigned-off-by: Matthieu Moy \n---\n Documentation/config.txt | 3 +++\n advice.c | 2 ++\n advice.h | 1 +\n builtin/rm.c | 11 +++++++----\n t/t3600-rm.sh | 29 +++++++++++++++++++++++++++++\n 5 files changed, 42 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 6e53fc5..eb04479 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -199,6 +199,9 @@ advice.*::\n \tamWorkDir::\n \t\tAdvice that shows the location of the patch file when\n \t\tlinkgit:git-am[1] fails to apply it.\n+\trmHints::\n+\t\tIn case of failure in the output of linkgit:git-rm[1],\n+\t\tshow directions on how to proceed from the current state.\n --\n \n core.fileMode::\ndiff --git a/advice.c b/advice.c\nindex a8deee6..a4c169c 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -14,6 +14,7 @@ int advice_resolve_conflict = 1;\n int advice_implicit_identity = 1;\n int advice_detached_head = 1;\n int advice_set_upstream_failure = 1;\n+int advice_rm_hints = 1;\n \n static struct {\n \tconst char *name;\n@@ -33,6 +34,7 @@ static struct {\n \t{ \"implicitidentity\", &advice_implicit_identity },\n \t{ \"detachedhead\", &advice_detached_head },\n \t{ \"setupstreamfailure\", &advice_set_upstream_failure },\n+\t{ \"rmhints\", &advice_rm_hints },\n \n \t/* make this an alias for backward compatibility */\n \t{ \"pushnonfastforward\", &advice_push_update_rejected }\ndiff --git a/advice.h b/advice.h\nindex 94caa32..36104c4 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -17,6 +17,7 @@ extern int advice_resolve_conflict;\n extern int advice_implicit_identity;\n extern int advice_detached_head;\n extern int advice_set_upstream_failure;\n+extern int advice_rm_hints;\n \n int git_default_advice_config(const char *var, const char *value);\n void advise(const char *advice, ...);\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 07306eb..c991fe6 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -62,9 +62,11 @@ static int check_submodules_use_gitfiles(void)\n \n \t\tif (!submodule_uses_gitfile(name))\n \t\t\terrs = error(_(\"submodule '%s' (or one of its nested \"\n-\t\t\t\t \"submodules) uses a .git directory\\n\"\n-\t\t\t\t \"(use 'rm -rf' if you really want to remove \"\n-\t\t\t\t \"it including all of its history)\"), name);\n+\t\t\t\t \"submodules) uses a .git directory%s\"), name,\n+\t\t\t\t advice_rm_hints\n+\t\t\t\t ? \"\\n(use 'rm -rf' if you really want to remove \"\n+\t\t\t\t \"it including all of its history)\"\n+\t\t\t\t : \"\");\n \t}\n \n \treturn errs;\n@@ -83,7 +85,8 @@ static void print_eventual_error_files(struct string_list *files_list,\n \t\t\tstrbuf_addf(&err_msg,\n \t\t\t\t \"\\n %s\",\n \t\t\t\t files_list->items[i].string);\n-\t\tstrbuf_addstr(&err_msg, hints_msg);\n+\t\tif (advice_rm_hints)\n+\t\t\tstrbuf_addstr(&err_msg, hints_msg);\n \t\t*errs = error(\"%s\", err_msg.buf);\n \t}\n }\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 10dd380..74f048c 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -707,6 +707,18 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rm files with different staged content without hints' '\n+\tcat >expect << EOF &&\n+error: the following files have staged content different from both the\n+file and the HEAD:\n+ bar.txt\n+ foo.txt\n+EOF\n+\techo content2 >foo.txt &&\n+\techo content2 >bar.txt &&\n+\ttest_must_fail git -c advice.rmhints=false rm foo.txt bar.txt 2>actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'rm file with local modification' '\n \tcat >expect << EOF &&\n@@ -720,6 +732,15 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rm file with local modification without hints' '\n+\tcat >expect << EOF &&\n+error: the following files have local modifications:\n+ bar.txt\n+EOF\n+\techo content4 >bar.txt &&\n+\ttest_must_fail git -c advice.rmhints=false rm bar.txt 2>actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'rm file with changes in the index' '\n cat >expect << EOF &&\n@@ -734,6 +755,14 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rm file with changes in the index without hints' '\n+\tcat >expect << EOF &&\n+error: the following files have changes staged in the index:\n+ foo.txt\n+EOF\n+\ttest_must_fail git -c advice.rmhints=false rm foo.txt 2>actual &&\n+\ttest_cmp expect actual\n+'\n \n test_expect_success 'rm files with two different errors' '\n \tcat >expect << EOF &&\n-- \n1.7.8\n"},{"id":"220311","messageId":"vpqppvudk6w.fsf@anie.imag.fr","threadId":"34094","inReplyTo":"1370879981-18937-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","subject":"Re: [PATCH v3 1/2] rm: better error message on failure for multiple files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-10T16:06:31Z","receivedAt":"2013-06-10T16:06:31Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mathieu Lienard--Mayor writes:\n\n> +static void print_eventual_error_files(struct string_list *files_list,\n\nToo french ;-).\n\nEventual (en) = final, utlime (fr).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"220324","messageId":"7v8v2hkhqc.fsf@alter.siamese.dyndns.org","threadId":"34094","inReplyTo":"1370879981-18937-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","subject":"Re: [PATCH v3 1/2] rm: better error message on failure for multiple files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-10T17:17:47Z","receivedAt":"2013-06-10T17:17:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathieu Lienard--Mayor \nwrites:\n\n> When 'git rm' fails, it now displays a single message\n> with the list of files involved, instead of displaying\n> a list of messages with one file each.\n>\n> As an example, the old message:\n> \terror: 'foo.txt' has changes staged in the index\n> \t(use --cached to keep the file, or -f to force removal)\n> \terror: 'bar.txt' has changes staged in the index\n> \t(use --cached to keep the file, or -f to force removal)\n>\n> would now be displayed as:\n> \terror: the following files have changes staged in the index:\n> \t foo.txt\n> \t bar.txt\n> \t(use --cached to keep the file, or -f to force removal)\n>\n> Signed-off-by: Mathieu Lienard--Mayor \n> Signed-off-by: Jorge Juan Garcia Garcia \n> Signed-off-by: Matthieu Moy \n> ---\n>\n> Changes since v2:\n> -couple typo in commit message and in code\n> -rename and redefinition of the intermediate function\n> -move the 4 \"if(....nr)\" inside the function\n>\n> builtin/rm.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-----------\n> t/t3600-rm.sh | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 124 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 7b91d52..07306eb 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -70,6 +70,24 @@ static int check_submodules_use_gitfiles(void)\n> \treturn errs;\n> }\n> \n> +static void print_eventual_error_files(struct string_list *files_list,\n> +\t\t\t\t const char *main_msg,\n> +\t\t\t\t const char *hints_msg,\n> +\t\t\t\t int *errs)\n\nHrm, I do not see the point of \"eventual\" there, by the way. Are\nthere other kinds of error files?\n\n> +{\n> +\tif (files_list->nr) {\n> +\t\tstruct strbuf err_msg = STRBUF_INIT;\n> +\t\tint i;\n> +\t\tstrbuf_addstr(&err_msg, main_msg);\n> +\t\tfor (i = 0; i < files_list->nr; i++)\n> +\t\t\tstrbuf_addf(&err_msg,\n> +\t\t\t\t \"\\n %s\",\n\nIs there an implication of having always 4 spaces here to l10n/i18n\nhere? I am wondering if it should be _(\"\\n %s\").\n\n> +\t\t\t\t files_list->items[i].string);\n> +\t\tstrbuf_addstr(&err_msg, hints_msg);\n> +\t\t*errs = error(\"%s\", err_msg.buf);\n\nThere needs a strbuf_release(&err_msg) somewhere before leaving this\nscope to avoid leaking its buffer, no?\n\n> +\t}\n> +}\n> +\n> static int check_local_mod(unsigned char *head, int index_only)\n> {\n> \t/*\n> @@ -82,6 +100,11 @@ static int check_local_mod(unsigned char *head, int index_only)\n> \tint i, no_head;\n> \tint errs = 0;\n> \n> +\tstruct string_list files_staged = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list files_cached = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list files_submodule = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list files_local = STRING_LIST_INIT_NODUP;\n> +\n> \tno_head = is_null_sha1(head);\n> \tfor (i = 0; i < list.nr; i++) {\n> \t\tstruct stat st;\n> @@ -171,29 +194,49 @@ static int check_local_mod(unsigned char *head, int index_only)\n> \t\t */\n> \t\tif (local_changes && staged_changes) {\n> \t\t\tif (!index_only || !(ce->ce_flags & CE_INTENT_TO_ADD))\n> +\t\t\t\tstring_list_append(&files_staged, name);\n> \t\t}\n> \t\telse if (!index_only) {\n> \t\t\tif (staged_changes)\n> -\t\t\t\terrs = error(_(\"'%s' has changes staged in the index\\n\"\n> -\t\t\t\t\t \"(use --cached to keep the file, \"\n> -\t\t\t\t\t \"or -f to force removal)\"), name);\n> +\t\t\t\tstring_list_append(&files_cached, name);\n> \t\t\tif (local_changes) {\n> \t\t\t\tif (S_ISGITLINK(ce->ce_mode) &&\n> \t\t\t\t !submodule_uses_gitfile(name)) {\n> +\t\t\t\t\tstring_list_append(&files_submodule,\n> +\t\t\t\t\t\t\t name);\n> +\t\t\t\t} else {\n> +\t\t\t\t\tstring_list_append(&files_local, name);\n> +\t\t\t\t}\n\nThe innermost if/else no longer needs braces. Also even though it\nmay push it slightly over 80-column, I think the files_submodule\nside of string_list_append() is easier to read if it were on a\nsingle line.\n\n> +\tprint_eventual_error_files(&files_staged,\n> +\t\t\t\t _(\"the following files have staged \"\n> +\t\t\t\t \"content different from both the\"\n> +\t\t\t\t \"\\nfile and the HEAD:\"),\n> +\t\t\t\t _(\"\\n(use -f to force removal)\"),\n> +\t\t\t\t &errs);\n\nHmph. I wonder if we want to properly i18n plurals, depending on\nthe number of files, e.g.\n\n print_error_files(&files_staged,\n Q_(\"the following file has staged \"\n \"content different from both the\\n\"\n \"file and the HEAD:\",\n \"the following files have staged \"\n \"content different from both the\\n\"\n \"file and the HEAD:\", files_staged.nr),\n _(\"\\n(use -f to force removal)\"), &errs);\n\nThis was not a problem back when we showed one error message per one\npath, but with this patch, it starts to matter.\n\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index 0c44e9f..10dd380 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -687,4 +687,71 @@ test_expect_failure SYMLINKS 'rm across a symlinked leading path (w/ index)' '\n> \ttest_path_is_file e/f\n> '\n> \n> +test_expect_success 'setup for testing rm messages' '\n> +\t>bar.txt &&\n> +\t>foo.txt &&\n> +\tgit add bar.txt foo.txt\n> +'\n> +\n> +test_expect_success 'rm files with different staged content' '\n> +\tcat >expect << EOF &&\n> +error: the following files have staged content different from both the\n> +file and the HEAD:\n> + bar.txt\n> + foo.txt\n> +(use -f to force removal)\n> +EOF\n> +\techo content1 >foo.txt &&\n\nIt is easier to read if it is done this way:\n\n test_expect_success 'rm files with different staged content' '\n cat >expect <<\\-EOF &&\n error: the following files have staged content different from both the\n file and the HEAD:\n bar.txt\n foo.txt\n (use -f to force removal)\n EOF\n echo content1 >foo.txt &&\n\nTwo and half points to note:\n\n (0) no whitespace between redirection << and its source\n (the end-of-here-text marker in this case).\n\n (1) by quoting the end-of-here-text marker with a backslash '\\',\n you tell the readers that there is no variable substitution in\n it, which reduces the mental burden on them.\n\n (2) by using a dash '-' before the end-of-here-text marker, you can\n align the body of here text with a leading tab (HT).\n\n> +\techo content1 >bar.txt &&\n> +\ttest_must_fail git rm foo.txt bar.txt 2>actual &&\n> +\ttest_cmp expect actual\n\nThis should be test_i18ncmp as the error messages are marked for\nl10n.\n\nOther than that, looks nicely done.\n\n> +'\n> +\n> +\n> +test_expect_success 'rm file with local modification' '\n> +\tcat >expect << EOF &&\n> +error: the following files have local modifications:\n> + foo.txt\n> +(use --cached to keep the file, or -f to force removal)\n> +EOF\n> +\tgit commit -m \"testing rm 3\" &&\n> +\techo content3 >foo.txt &&\n> +\ttest_must_fail git rm foo.txt 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +\n> +test_expect_success 'rm file with changes in the index' '\n> + cat >expect << EOF &&\n> +error: the following files have changes staged in the index:\n> + foo.txt\n> +(use --cached to keep the file, or -f to force removal)\n> +EOF\n> +\tgit reset --hard &&\n> +\techo content5 >foo.txt &&\n> +\tgit add foo.txt &&\n> +\ttest_must_fail git rm foo.txt 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +\n> +test_expect_success 'rm files with two different errors' '\n> +\tcat >expect << EOF &&\n> +error: the following files have staged content different from both the\n> +file and the HEAD:\n> + foo1.txt\n> +(use -f to force removal)\n> +error: the following files have changes staged in the index:\n> + bar1.txt\n> +(use --cached to keep the file, or -f to force removal)\n> +EOF\n> +\techo content >foo1.txt &&\n> +\tgit add foo1.txt &&\n> +\techo content6 >foo1.txt &&\n> +\techo content6 >bar1.txt &&\n> +\tgit add bar1.txt &&\n> +\ttest_must_fail git rm bar1.txt foo1.txt 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"220328","messageId":"7v4nd5khdl.fsf@alter.siamese.dyndns.org","threadId":"34094","inReplyTo":"1370879981-18937-2-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","subject":"Re: [PATCH v3 2/2] rm: introduce advice.rmHints to shorten messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-10T17:25:26Z","receivedAt":"2013-06-10T17:25:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathieu Lienard--Mayor \nwrites:\n\n> Introduce advice.rmHints to choose whether to display advice or not\n> when git rm fails. Defaults to true, in order to preserve current behavior.\n>\n> As an example, the message:\n> \terror: 'foo.txt' has changes staged in the index\n> \t(use --cached to keep the file, or -f to force removal)\n>\n> would look like, with advice.rmHints=false:\n> \terror: 'foo.txt' has changes staged in the index\n>\n> Signed-off-by: Mathieu Lienard--Mayor \n> Signed-off-by: Jorge Juan Garcia Garcia \n> Signed-off-by: Matthieu Moy \n> ---\n> Documentation/config.txt | 3 +++\n> advice.c | 2 ++\n> advice.h | 1 +\n> builtin/rm.c | 11 +++++++----\n> t/t3600-rm.sh | 29 +++++++++++++++++++++++++++++\n> 5 files changed, 42 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 6e53fc5..eb04479 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -199,6 +199,9 @@ advice.*::\n> \tamWorkDir::\n> \t\tAdvice that shows the location of the patch file when\n> \t\tlinkgit:git-am[1] fails to apply it.\n> +\trmHints::\n> +\t\tIn case of failure in the output of linkgit:git-rm[1],\n> +\t\tshow directions on how to proceed from the current state.\n> --\n> \n> core.fileMode::\n> diff --git a/advice.c b/advice.c\n> index a8deee6..a4c169c 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -14,6 +14,7 @@ int advice_resolve_conflict = 1;\n> int advice_implicit_identity = 1;\n> int advice_detached_head = 1;\n> int advice_set_upstream_failure = 1;\n> +int advice_rm_hints = 1;\n> \n> static struct {\n> \tconst char *name;\n> @@ -33,6 +34,7 @@ static struct {\n> \t{ \"implicitidentity\", &advice_implicit_identity },\n> \t{ \"detachedhead\", &advice_detached_head },\n> \t{ \"setupstreamfailure\", &advice_set_upstream_failure },\n> +\t{ \"rmhints\", &advice_rm_hints },\n> \n> \t/* make this an alias for backward compatibility */\n> \t{ \"pushnonfastforward\", &advice_push_update_rejected }\n> diff --git a/advice.h b/advice.h\n> index 94caa32..36104c4 100644\n> --- a/advice.h\n> +++ b/advice.h\n> @@ -17,6 +17,7 @@ extern int advice_resolve_conflict;\n> extern int advice_implicit_identity;\n> extern int advice_detached_head;\n> extern int advice_set_upstream_failure;\n> +extern int advice_rm_hints;\n\nThe handling of a new advice variable (i.e. definition, declaration\nand reading from configuration) looks correct in this patch. Good\njob.\n\n> int git_default_advice_config(const char *var, const char *value);\n> void advise(const char *advice, ...);\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 07306eb..c991fe6 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -62,9 +62,11 @@ static int check_submodules_use_gitfiles(void)\n> \n> \t\tif (!submodule_uses_gitfile(name))\n> \t\t\terrs = error(_(\"submodule '%s' (or one of its nested \"\n> +\t\t\t\t \"submodules) uses a .git directory%s\"), name,\n> +\t\t\t\t advice_rm_hints\n> +\t\t\t\t ? \"\\n(use 'rm -rf' if you really want to remove \"\n> +\t\t\t\t \"it including all of its history)\"\n> +\t\t\t\t : \"\");\n\nThe advice part is not subject to i18n?\n\n> \t}\n> \n> \treturn errs;\n\nInteresting.\n\nIs there a reason why this kind of errors are not collected together\ninto one \"error message and then list of paths\", like all the other\nkinds of errors are done with print_eventual_error_files()?\n\n> @@ -83,7 +85,8 @@ static void print_eventual_error_files(struct string_list *files_list,\n> \t\t\tstrbuf_addf(&err_msg,\n> \t\t\t\t \"\\n %s\",\n> \t\t\t\t files_list->items[i].string);\n> -\t\tstrbuf_addstr(&err_msg, hints_msg);\n> +\t\tif (advice_rm_hints)\n> +\t\t\tstrbuf_addstr(&err_msg, hints_msg);\n> \t\t*errs = error(\"%s\", err_msg.buf);\n> \t}\n> }\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index 10dd380..74f048c 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -707,6 +707,18 @@ EOF\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'rm files with different staged content without hints' '\n> +\tcat >expect << EOF &&\n> +error: the following files have staged content different from both the\n> +file and the HEAD:\n> + bar.txt\n> + foo.txt\n> +EOF\n> +\techo content2 >foo.txt &&\n> +\techo content2 >bar.txt &&\n> +\ttest_must_fail git -c advice.rmhints=false rm foo.txt bar.txt 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n\nSame comments as the ones for 1/2 applies to the tests in this patch.\n"},{"id":"220333","messageId":"vpqtxl5dfrf.fsf@anie.imag.fr","threadId":"34094","inReplyTo":"7v8v2hkhqc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/2] rm: better error message on failure for multiple files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-10T17:42:12Z","receivedAt":"2013-06-10T17:42:12Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n>> +{\n>> +\tif (files_list->nr) {\n>> +\t\tstruct strbuf err_msg = STRBUF_INIT;\n>> +\t\tint i;\n>> +\t\tstrbuf_addstr(&err_msg, main_msg);\n>> +\t\tfor (i = 0; i < files_list->nr; i++)\n>> +\t\t\tstrbuf_addf(&err_msg,\n>> +\t\t\t\t \"\\n %s\",\n>\n> Is there an implication of having always 4 spaces here to l10n/i18n\n> here? I am wondering if it should be _(\"\\n %s\").\n\nI'd say this is just formatting and should be the same in every\nlanguages, but I'm far from an expert in the domain. Maybe some\nright-to-left languages would need this.\n\n> test_expect_success 'rm files with different staged content' '\n> cat >expect <<\\-EOF &&\n\n(that should be -\\EOF, not \\-EOF I think)\n\n> (2) by using a dash '-' before the end-of-here-text marker, you can\n> align the body of here text with a leading tab (HT).\n\nThis works because the list of files is aligned with spaces, but is\nseems a bit fragile to me to use this -EOF on a text which uses\nindentation. Anyway, I'm fine with both.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"220377","messageId":"7v4nd5fzxn.fsf@alter.siamese.dyndns.org","threadId":"34094","inReplyTo":"vpqtxl5dfrf.fsf@anie.imag.fr","subject":"Re: [PATCH v3 1/2] rm: better error message on failure for multiple files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-10T20:55:48Z","receivedAt":"2013-06-10T20:55:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Junio C Hamano writes:\n>\n>>> +{\n>>> +\tif (files_list->nr) {\n>>> +\t\tstruct strbuf err_msg = STRBUF_INIT;\n>>> +\t\tint i;\n>>> +\t\tstrbuf_addstr(&err_msg, main_msg);\n>>> +\t\tfor (i = 0; i < files_list->nr; i++)\n>>> +\t\t\tstrbuf_addf(&err_msg,\n>>> +\t\t\t\t \"\\n %s\",\n>>\n>> Is there an implication of having always 4 spaces here to l10n/i18n\n>> here? I am wondering if it should be _(\"\\n %s\").\n>\n> I'd say this is just formatting and should be the same in every\n> languages, but I'm far from an expert in the domain.\n\nAfter looking at the patch again I do not think 4-SP matters. I was\nprimarily worried if this was to align with some column of the first\nline of output, e.g.\n\n\terror: lorem ipsum dolor sit amet, consectetur adipisicing\n elit, sed do eiusmod tempor incididunt ut labore et\n dolore magna aliqua.\n\nbut that is not what this 4-SP indent is about, so it is OK.\n\n>> test_expect_success 'rm files with different staged content' '\n>> cat >expect <<\\-EOF &&\n>\n> (that should be -\\EOF, not \\-EOF I think)\n\nSorry, my bad. You are of course right.\n\n>> (2) by using a dash '-' before the end-of-here-text marker, you can\n>> align the body of here text with a leading tab (HT).\n>\n> This works because the list of files is aligned with spaces, but is\n> seems a bit fragile to me to use this -EOF on a text which uses\n> indentation. Anyway, I'm fine with both.\n\nTrue.\n"}]}