{"thread":{"id":"34114","subject":"[PATCH v5 1/2] rm: better error message on failure for multiple files","startedAt":"2013-06-12T08:06:43Z","lastAt":"2013-06-12T22:13:41Z","messageCount":3,"participants":["Mathieu Lienard--Mayor","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":2},"messages":[{"id":"220587","messageId":"1371024404-22468-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","threadId":"34114","inReplyTo":null,"subject":"[PATCH v5 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-12T08:06:43Z","receivedAt":"2013-06-12T08:06:43Z","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 v4:\n -removal of useless blanks after variable declarations\n -use of string_list_clear\n\n builtin/rm.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++-----------\n t/t3600-rm.sh | 67 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 147 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 7b91d52..18df253 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -36,10 +36,31 @@ static int get_ours_cache_pos(const char *path, int pos)\n \treturn -1;\n }\n \n+static void print_error_files(struct string_list *files_list,\n+\t\t\t const char *main_msg,\n+\t\t\t const char *hints_msg,\n+\t\t\t int *errs)\n+{\n+\tif (files_list->nr) {\n+\t\tint i;\n+\t\tstruct strbuf err_msg = STRBUF_INIT;\n+\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\tstrbuf_release(&err_msg);\n+\t}\n+}\n+\n static int check_submodules_use_gitfiles(void)\n {\n \tint i;\n \tint errs = 0;\n+\tstruct string_list files = STRING_LIST_INIT_NODUP;\n \n \tfor (i = 0; i < list.nr; i++) {\n \t\tconst char *name = list.entry[i].name;\n@@ -61,11 +82,18 @@ static int check_submodules_use_gitfiles(void)\n \t\t\tcontinue;\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\tstring_list_append(&files, name);\n \t}\n+\tprint_error_files(&files,\n+\t\t\t Q_(\"the following submodule (or one of its nested \"\n+\t\t\t \"submodules)\\n uses a .git directory:\",\n+\t\t\t \"the following submodules (or one of its nested \"\n+\t\t\t \"submodules)\\n use a .git directory:\",\n+\t\t\t files.nr),\n+\t\t\t _(\"\\n(use 'rm -rf' if you really want to remove \"\n+\t\t\t \"it including all of its history)\"),\n+\t\t\t &errs);\n+\tstring_list_clear(&files, 0);\n \n \treturn errs;\n }\n@@ -81,6 +109,10 @@ static int check_local_mod(unsigned char *head, int index_only)\n \t */\n \tint i, no_head;\n \tint errs = 0;\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@@ -171,29 +203,58 @@ 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 !submodule_uses_gitfile(name))\n+\t\t\t\t\tstring_list_append(&files_submodule, name);\n+\t\t\t\telse\n+\t\t\t\t\tstring_list_append(&files_local, name);\n \t\t\t}\n \t\t}\n \t}\n+\tprint_error_files(&files_staged,\n+\t\t\t Q_(\"the following file has staged content different \"\n+\t\t\t \"from both the\\nfile and the HEAD:\",\n+\t\t\t \"the following files have staged content different\"\n+\t\t\t \" from both the\\nfile and the HEAD:\",\n+\t\t\t files_staged.nr),\n+\t\t\t _(\"\\n(use -f to force removal)\"),\n+\t\t\t &errs);\n+\tstring_list_clear(&files_staged, 0);\n+\tprint_error_files(&files_cached,\n+\t\t\t Q_(\"the following file has changes \"\n+\t\t\t \"staged in the index:\",\n+\t\t\t \"the following files have changes \"\n+\t\t\t \"staged in the index:\", files_cached.nr),\n+\t\t\t _(\"\\n(use --cached to keep the file,\"\n+\t\t\t \" or -f to force removal)\"),\n+\t\t\t &errs);\n+\tstring_list_clear(&files_cached, 0);\n+\tprint_error_files(&files_submodule,\n+\t\t\t Q_(\"the following submodule (or one of its nested \"\n+\t\t\t \"submodule)\\nuses a .git directory:\",\n+\t\t\t \"the following submodules (or one of its nested \"\n+\t\t\t \"submodule)\\nuse a .git directory:\",\n+\t\t\t files_submodule.nr),\n+\t\t\t _(\"\\n(use 'rm -rf' if you really \"\n+\t\t\t \"want to remove it including all \"\n+\t\t\t \"of its history)\"),\n+\t\t\t &errs);\n+\tstring_list_clear(&files_submodule, 0);\n+\tprint_error_files(&files_local,\n+\t\t\t Q_(\"the following file has local modifications:\",\n+\t\t\t \"the following files have local modifications:\",\n+\t\t\t files_local.nr),\n+\t\t\t _(\"\\n(use --cached to keep the file,\"\n+\t\t\t \" or -f to force removal)\"),\n+\t\t\t &errs);\n+\tstring_list_clear(&files_local, 0);\n+\n \treturn errs;\n }\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 0c44e9f..902993b 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+\terror: the following files have staged content different from both the\n+\tfile and the HEAD:\n+\t bar.txt\n+\t foo.txt\n+\t(use -f to force removal)\n+\tEOF\n+\techo content1 >foo.txt &&\n+\techo content1 >bar.txt &&\n+\ttest_must_fail git rm foo.txt bar.txt 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+\n+test_expect_success 'rm file with local modification' '\n+\tcat >expect <<-\\EOF &&\n+\terror: the following file has local modifications:\n+\t foo.txt\n+\t(use --cached to keep the file, or -f to force removal)\n+\tEOF\n+\tgit commit -m \"testing rm 3\" &&\n+\techo content3 >foo.txt &&\n+\ttest_must_fail git rm foo.txt 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+\n+test_expect_success 'rm file with changes in the index' '\n+\tcat >expect <<-\\EOF &&\n+\terror: the following file has changes staged in the index:\n+\t foo.txt\n+\t(use --cached to keep the file, or -f to force removal)\n+\tEOF\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_i18ncmp expect actual\n+'\n+\n+\n+test_expect_success 'rm files with two different errors' '\n+\tcat >expect <<-\\EOF &&\n+\terror: the following file has staged content different from both the\n+\tfile and the HEAD:\n+\t foo1.txt\n+\t(use -f to force removal)\n+\terror: the following file has changes staged in the index:\n+\t bar1.txt\n+\t(use --cached to keep the file, or -f to force removal)\n+\tEOF\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_i18ncmp expect actual\n+'\n+\n test_done\n-- \n1.7.8\n"},{"id":"220588","messageId":"1371024404-22468-2-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","threadId":"34114","inReplyTo":"1371024404-22468-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","subject":"[PATCH 2/2] rm: introduce advice.rmHints to shorten messages","fromName":"Mathieu Lienard--Mayor","fromEmail":"mathieu.lienard--mayor@ensimag.imag.fr","sentAt":"2013-06-12T08:06:44Z","receivedAt":"2013-06-12T08:06:44Z","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\nNo changes since v4\n\n Documentation/config.txt | 3 +++\n advice.c | 2 ++\n advice.h | 1 +\n builtin/rm.c | 3 ++-\n t/t3600-rm.sh | 29 +++++++++++++++++++++++++++++\n 5 files changed, 37 insertions(+), 1 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 18df253..11ad53a 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -50,7 +50,8 @@ static void print_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\tstrbuf_release(&err_msg);\n \t}\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 902993b..5c87b55 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -707,6 +707,18 @@ test_expect_success 'rm files with different staged content' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'rm files with different staged content without hints' '\n+\tcat >expect <<-\\EOF &&\n+\terror: the following files have staged content different from both the\n+\tfile and the HEAD:\n+\t bar.txt\n+\t foo.txt\n+\tEOF\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_i18ncmp expect actual\n+'\n \n test_expect_success 'rm file with local modification' '\n \tcat >expect <<-\\EOF &&\n@@ -720,6 +732,15 @@ test_expect_success 'rm file with local modification' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'rm file with local modification without hints' '\n+\tcat >expect <<-\\EOF &&\n+\terror: the following file has local modifications:\n+\t bar.txt\n+\tEOF\n+\techo content4 >bar.txt &&\n+\ttest_must_fail git -c advice.rmhints=false rm bar.txt 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n \n test_expect_success 'rm file with changes in the index' '\n \tcat >expect <<-\\EOF &&\n@@ -734,6 +755,14 @@ test_expect_success 'rm file with changes in the index' '\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'rm file with changes in the index without hints' '\n+\tcat >expect <<-\\EOF &&\n+\terror: the following file has changes staged in the index:\n+\t foo.txt\n+\tEOF\n+\ttest_must_fail git -c advice.rmhints=false rm foo.txt 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n \n test_expect_success 'rm files with two different errors' '\n \tcat >expect <<-\\EOF &&\n-- \n1.7.8\n"},{"id":"220680","messageId":"7v4nd30yga.fsf@alter.siamese.dyndns.org","threadId":"34114","inReplyTo":"1371024404-22468-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","subject":"Re: [PATCH v5 1/2] rm: better error message on failure for multiple files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-12T22:13:41Z","receivedAt":"2013-06-12T22:13:41Z","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 v4:\n> -removal of useless blanks after variable declarations\n> -use of string_list_clear\n\nThanks. Will queue.\n"}]}