{"thread":{"id":"34104","subject":"[PATCH v4 1/2] rm: better error message on failure for multiple files","startedAt":"2013-06-11T14:56:00Z","lastAt":"2013-06-11T18:07:41Z","messageCount":4,"participants":["Mathieu Lienard--Mayor","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"220445","messageId":"1370962561-12519-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","threadId":"34104","inReplyTo":null,"subject":"[PATCH v4 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-11T14:56:00Z","receivedAt":"2013-06-11T14:56:00Z","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 <Mathieu.Lienard--Mayor@ensimag.imag.fr>\nSigned-off-by: Jorge Juan Garcia Garcia <Jorge-Juan.Garcia-Garcia@ensimag.imag.fr>\nSigned-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>\n---\n\nChanges since v3:\n - rename function print_error_files()\n - use of strbuf_release to avoid leaking\n - change of a forgotten message, now using print_error_files() aswell\n - removal of useless braces\n - use of Q_() to deal with plurals correctly\n - removal of spaces after redirection <<\n - use of -\\EOF to indent tests better\n\n builtin/rm.c  |   95 +++++++++++++++++++++++++++++++++++++++++++++-----------\n t/t3600-rm.sh |   67 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 143 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 7b91d52..e284db3 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -36,11 +36,32 @@ 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\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\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 \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 \t\tint pos;\n@@ -61,11 +82,17 @@ 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 \n \treturn errs;\n }\n@@ -82,6 +109,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 +203,54 @@ 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+\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+\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+\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+\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":"220446","messageId":"1370962561-12519-2-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","threadId":"34104","inReplyTo":"1370962561-12519-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","subject":"[PATCH v4 2/2] rm: introduce advice.rmHints to shorten messages","fromName":"Mathieu Lienard--Mayor","fromEmail":"mathieu.lienard--mayor@ensimag.imag.fr","sentAt":"2013-06-11T14:56:01Z","receivedAt":"2013-06-11T14:56:01Z","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 <Mathieu.Lienard--Mayor@ensimag.imag.fr>\nSigned-off-by: Jorge Juan Garcia Garcia <Jorge-Juan.Garcia-Garcia@ensimag.imag.fr>\nSigned-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>\n---\n\nChanges since v3:\n - removal of spaces after redirection <<\n - use of -\\EOF to indent tests better\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 e284db3..70df77c 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -49,7 +49,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":"220457","messageId":"vpq38so38sq.fsf@anie.imag.fr","threadId":"34104","inReplyTo":"1370962561-12519-2-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","subject":"Re: [PATCH v4 2/2] rm: introduce advice.rmHints to shorten messages","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-11T16:35:01Z","receivedAt":"2013-06-11T16:35:01Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mathieu Lienard--Mayor <Mathieu.Lienard--Mayor@ensimag.imag.fr> writes:\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\nI went through the serie, and it seems OK to me.\n\nI like the way this patch 2/2 became an obvious 4-lines patch (+ tests)\nafter 1/2 is refactored properly :-).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"220476","messageId":"7vli6gbjwy.fsf@alter.siamese.dyndns.org","threadId":"34104","inReplyTo":"1370962561-12519-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","subject":"Re: [PATCH v4 1/2] rm: better error message on failure for multiple files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-11T18:07:41Z","receivedAt":"2013-06-11T18:07:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mathieu Lienard--Mayor <Mathieu.Lienard--Mayor@ensimag.imag.fr>\nwrites:\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\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\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>  \n> +\tstruct string_list files = STRING_LIST_INIT_NODUP;\n> +\n>  \tfor (i = 0; i < list.nr; i++) {\n\nThe blank after the initialization lines before the first statement\nis very much welcom, but please drop the blank line before this new\ninitialization, i.e.\n\n\tint i;\n        int errs = 0;\n        struct string_list files = STRING_LIST_INIT_NODUP;\n\n\tfor (i = 0; i < list.nr; i++) {\n\t\t...\n\n> @@ -61,11 +82,17 @@ static int check_submodules_use_gitfiles(void)\n>  \t\t\tcontinue;\n>  \n>  \t\tif (!submodule_uses_gitfile(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>  \n>  \treturn errs;\n\nNo string_list_clear() on files?\n\n>  }\n> @@ -82,6 +109,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> ...\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> +\n\nNo string_list_clear() on files_*?\n"}]}