{"thread":{"id":"34087","subject":"[PATCH 2/2] rm: introduce advice.rmHints to shorten messages","startedAt":"2013-06-10T14:22:07Z","lastAt":"2013-06-10T15:08:22Z","messageCount":4,"participants":["Mathieu Lienard--Mayor","Matthieu Moy","Mathieu Liénard--Mayor"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"220285","messageId":"1370874127-4326-2-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","threadId":"34087","inReplyTo":"1370874127-4326-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-10T14:22:07Z","receivedAt":"2013-06-10T14:22:07Z","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\nChanges since v1:\n -corrected the commit message where \"rmHints=true\" was supposed to be \"false\"\n -better use of English tenses in the commit message\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 76dfc5b..2226037 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@@ -85,7 +87,8 @@ static int print_error_files(struct string_list *files_list,\n \t\tstrbuf_addf(&err_msg,\n \t\t\t \"\\n %s\",\n \t\t\t files_list->items[i].string);\n-\tstrbuf_addstr(&err_msg, hints_msg);\n+\tif (advice_rm_hints)\n+\t\tstrbuf_addstr(&err_msg, hints_msg);\n \terrs = error(\"%s\", err_msg.buf);\n \n \treturn errs;\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 08bc9bb..92f6146 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,5 +755,13 @@ 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_done\n-- \n1.7.8\n"},{"id":"220291","messageId":"vpqtxl6ghf5.fsf@anie.imag.fr","threadId":"34087","inReplyTo":"1370874127-4326-1-git-send-email-Mathieu.Lienard--Mayor@ensimag.imag.fr","subject":"Re: [PATCH v2 1/2] rm: better error message on failure for multiple files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-10T14:38:06Z","receivedAt":"2013-06-10T14:38:06Z","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> 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 list\n\nThere's a \"list\" after my email, probably a typo.\n\n> +/*\n> + * PRECONDITION: files_list is a non-empty string_list\n> + */\n\nAvoid repeating in comments what the code already says. \"file_list is\nnon-empty\" is sufficient, we already know it's a string_list.\n\n> +\tif (files_staged.nr)\n> +\t\terrs = print_error_files(&files_staged,\n> +\t\t\t\t\t _(\"the following files have staged \"\n> +\t\t\t\t\t \"content different from both the\"\n> +\t\t\t\t\t \"\\nfile and the HEAD:\"),\n> +\t\t\t\t\t _(\"\\n(use -f to force removal)\"));\n> +\tif (files_cached.nr)\n> +\t\terrs = print_error_files(&files_cached,\n> +\t\t\t\t\t _(\"the following files have changes \"\n> +\t\t\t\t\t \"staged in the index:\"),\n> +\t\t\t\t\t _(\"\\n(use --cached to keep the file, \"\n> +\t\t\t\t\t \"or -f to force removal)\"));\n\nWhat happens if both conditions are true? It seems the second will\noverride the first. I think it'd be OK because what matters is that errs\nis set by someone, no matter who, and the error message is displayed on\nscreen, not contained in the variable, but this looks weird.\n\nI'd find it more readable with \"errs |= print_error_files(...)\".\n\nAnd actually, you may want to move the if (....nr) inside\nprint_error_files (wich could then be called print_error_files_maybe).\n\nAt least, there should be a test where two conditions are true.\n\n> +\tif (files_submodule.nr)\n> +\t\terrs = print_error_files(&files_submodule,\n> +\t\t\t\t\t _(\"the following submodules (or one \"\n> +\t\t\t\t\t \"of its nested submodule) use a \"\n> +\t\t\t\t\t \".git directory:\"),\n> +\t\t\t\t\t _(\"\\n(use 'rm -rf' if you really \"\n> +\t\t\t\t\t \"want to remove i including all \"\n\ni -> it\n?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"220297","messageId":"580989b4b95a7302a42c7f25024c3375@ensibm.imag.fr","threadId":"34087","inReplyTo":"vpqtxl6ghf5.fsf@anie.imag.fr","subject":"Re: [PATCH v2 1/2] rm: better error message on failure for multiple files","fromName":"Mathieu Liénard--Mayor","fromEmail":"mathieu.lienard--mayor@ensimag.fr","sentAt":"2013-06-10T14:57:00Z","receivedAt":"2013-06-10T14:57:00Z","isPatch":true,"sender":{"key":"mathieu.lienard--mayor@ensimag.fr","avatar":null},"body":"Le 2013-06-10 16:38, Matthieu Moy a écrit :\n> Mathieu Lienard--Mayor \n> writes:\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>> \n>> Signed-off-by: Jorge Juan Garcia Garcia \n>> \n>> Signed-off-by: Matthieu Moy list\n>\n> There's a \"list\" after my email, probably a typo.\nyes, that's a leftover from a rebase-i\n>\n>> +/*\n>> + * PRECONDITION: files_list is a non-empty string_list\n>> + */\n>\n> Avoid repeating in comments what the code already says. \"file_list is\n> non-empty\" is sufficient, we already know it's a string_list.\nOkay\n>\n>> +\tif (files_staged.nr)\n>> +\t\terrs = print_error_files(&files_staged,\n>> +\t\t\t\t\t _(\"the following files have staged \"\n>> +\t\t\t\t\t \"content different from both the\"\n>> +\t\t\t\t\t \"\\nfile and the HEAD:\"),\n>> +\t\t\t\t\t _(\"\\n(use -f to force removal)\"));\n>> +\tif (files_cached.nr)\n>> +\t\terrs = print_error_files(&files_cached,\n>> +\t\t\t\t\t _(\"the following files have changes \"\n>> +\t\t\t\t\t \"staged in the index:\"),\n>> +\t\t\t\t\t _(\"\\n(use --cached to keep the file, \"\n>> +\t\t\t\t\t \"or -f to force removal)\"));\n>\n> What happens if both conditions are true? It seems the second will\n> override the first. I think it'd be OK because what matters is that \n> errs\n> is set by someone, no matter who, and the error message is displayed \n> on\n> screen, not contained in the variable, but this looks weird.\n>\n> I'd find it more readable with \"errs |= print_error_files(...)\".\nWell the current code is only using errs=error(...), using the same \nvariable errs over and over, no matter how many times it loops.\nThat's why i implemented it similarly.\n>\n> And actually, you may want to move the if (....nr) inside\n> print_error_files (wich could then be called \n> print_error_files_maybe).\n>\n> At least, there should be a test where two conditions are true.\nI'll do that, to be sure about the behaviour.\n>\n>> +\tif (files_submodule.nr)\n>> +\t\terrs = print_error_files(&files_submodule,\n>> +\t\t\t\t\t _(\"the following submodules (or one \"\n>> +\t\t\t\t\t \"of its nested submodule) use a \"\n>> +\t\t\t\t\t \".git directory:\"),\n>> +\t\t\t\t\t _(\"\\n(use 'rm -rf' if you really \"\n>> +\t\t\t\t\t \"want to remove i including all \"\n>\n> i -> it\n> ?\n\n-- \nMathieu Liénard--Mayor,\n2nd year at Grenoble INP - ENSIMAG\n(+33)6 80 56 30 02\n"},{"id":"220298","messageId":"vpqobbef1g9.fsf@anie.imag.fr","threadId":"34087","inReplyTo":"580989b4b95a7302a42c7f25024c3375@ensibm.imag.fr","subject":"Re: [PATCH v2 1/2] rm: better error message on failure for multiple files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-06-10T15:08:22Z","receivedAt":"2013-06-10T15:08:22Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Mathieu Liénard--Mayor writes:\n\n> Well the current code is only using errs=error(...), using the same\n> variable errs over and over, no matter how many times it loops.\n> That's why i implemented it similarly.\n\nOK, consistency is a good argument then.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}