{"thread":{"id":"31967","subject":"[PATCH] Teach rm to remove submodules when given with a trailing '/'","startedAt":"2012-10-28T23:28:18Z","lastAt":"2012-11-22T22:32:26Z","messageCount":5,"participants":["Jens Lehmann","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"202075","messageId":"508DBF92.9090200@web.de","threadId":"31967","inReplyTo":null,"subject":"[PATCH] Teach rm to remove submodules when given with a trailing '/'","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-10-28T23:28:18Z","receivedAt":"2012-10-28T23:28:18Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Doing a \"git rm submod/\" on a submodule results in an error:\n\tfatal: pathspec 'submod/' did not match any files\nThis is really inconvenient as e.g. using TAB completion in a shell on a\nsubmodule automatically adds the trailing '/' when it completes the path\nof the submodule directory. The user has then to remove the '/' herself to\nmake a \"git rm\" succeed. Doing a \"git rm -r somedir/\" is working fine, so\nthere is no reason why that shouldn't work for submodules too.\n\nTeach git rm to not error out when a '/' is appended to the path of a\nsubmodule. Achieve this by chopping off trailing slashes from the path\nnames given if they represent directories. Add tests to make sure that\nlogic only applies to directories and not to files.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\n\nThis patch applies on top of the jl/submodule-rm branch merged into\ncurrent next.\n\n\n builtin/rm.c  |  7 +++++++\n t/t3600-rm.sh | 17 +++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 2aea3b5..5381d3f 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -234,6 +234,13 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n\n+\t/* Remove trailing '/' from directories to find submodules in the index */\n+\tfor (i = 0; i < argc; i++) {\n+\t\tsize_t pathlen = strlen(argv[i]);\n+\t\tif (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))\n+\t\t\targv[i] = xmemdupz(argv[i], pathlen - 1);\n+\t}\n+\n \tpathspec = get_pathspec(prefix, argv);\n \trefresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 97254e8..06f6384 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -302,6 +302,23 @@ test_expect_success 'rm removes work tree of unmodified submodules' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'rm removes a submodule with a trailing /' '\n+\tgit reset --hard &&\n+\tgit submodule update &&\n+\tgit rm submod/ &&\n+\ttest ! -d submod &&\n+\tgit status -s -uno --ignore-submodules=none > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rm fails when given a file with a trailing /' '\n+\ttest_must_fail git rm empty/\n+'\n+\n+test_expect_success 'rm succeeds when given a directory with a trailing /' '\n+\tgit rm -r frotz/\n+'\n+\n test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '\n \tgit reset --hard &&\n \tgit submodule update &&\n-- \n1.8.0.42.g3346551\n"},{"id":"202095","messageId":"508E2C05.9010109@viscovery.net","threadId":"31967","inReplyTo":"508DBF92.9090200@web.de","subject":"Re: [PATCH] Teach rm to remove submodules when given with a trailing '/'","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-29T07:11:01Z","receivedAt":"2012-10-29T07:11:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/29/2012 0:28, schrieb Jens Lehmann:\n> +\t/* Remove trailing '/' from directories to find submodules in the index */\n> +\tfor (i = 0; i < argc; i++) {\n> +\t\tsize_t pathlen = strlen(argv[i]);\n> +\t\tif (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))\n> +\t\t\targv[i] = xmemdupz(argv[i], pathlen - 1);\n> +\t}\n> +\n>  \tpathspec = get_pathspec(prefix, argv);\n>  \trefresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);\n\nThat's wrong: Either move the check below get_pathspec() (which normalizes\nbackslashes to forward-slashes on Windows) or use is_dir_sep().\n\nBut isn't it somewhat dangerous to check pathspec for existance in the\nworktree without interpreting them? Think of magic pathspec syntax (that\nwe do not have yet, but which may materialize sometime in the future).\n\n-- Hannes\n"},{"id":"202242","messageId":"50904677.2020308@web.de","threadId":"31967","inReplyTo":"508E2C05.9010109@viscovery.net","subject":"Re: [PATCH] Teach rm to remove submodules when given with a trailing '/'","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-10-30T21:28:23Z","receivedAt":"2012-10-30T21:28:23Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 29.10.2012 08:11, schrieb Johannes Sixt:\n> Am 10/29/2012 0:28, schrieb Jens Lehmann:\n>> +\t/* Remove trailing '/' from directories to find submodules in the index */\n>> +\tfor (i = 0; i < argc; i++) {\n>> +\t\tsize_t pathlen = strlen(argv[i]);\n>> +\t\tif (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))\n>> +\t\t\targv[i] = xmemdupz(argv[i], pathlen - 1);\n>> +\t}\n>> +\n>>  \tpathspec = get_pathspec(prefix, argv);\n>>  \trefresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);\n> \n> That's wrong: Either move the check below get_pathspec() (which normalizes\n> backslashes to forward-slashes on Windows) or use is_dir_sep().\n\nThanks for bringing this up.\n\n> But isn't it somewhat dangerous to check pathspec for existance in the\n> worktree without interpreting them? Think of magic pathspec syntax (that\n> we do not have yet, but which may materialize sometime in the future).\n\nI have to admit I'm not aware of magic pathspec syntax. Do you happen to\nhave any pointers where I could look at code doing similar things right?\n"},{"id":"202291","messageId":"5090C54C.90902@viscovery.net","threadId":"31967","inReplyTo":"50904677.2020308@web.de","subject":"Re: [PATCH] Teach rm to remove submodules when given with a trailing '/'","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-10-31T06:29:32Z","receivedAt":"2012-10-31T06:29:32Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/30/2012 22:28, schrieb Jens Lehmann:\n> Am 29.10.2012 08:11, schrieb Johannes Sixt:\n>> Am 10/29/2012 0:28, schrieb Jens Lehmann:\n>>> +\t/* Remove trailing '/' from directories to find submodules in the index */\n>>> +\tfor (i = 0; i < argc; i++) {\n>>> +\t\tsize_t pathlen = strlen(argv[i]);\n>>> +\t\tif (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))\n>>> +\t\t\targv[i] = xmemdupz(argv[i], pathlen - 1);\n>>> +\t}\n>>> +\n>>>  \tpathspec = get_pathspec(prefix, argv);\n>>>  \trefresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);\n>>\n>> That's wrong: Either move the check below get_pathspec() (which normalizes\n>> backslashes to forward-slashes on Windows) or use is_dir_sep().\n> \n> Thanks for bringing this up.\n> \n>> But isn't it somewhat dangerous to check pathspec for existance in the\n>> worktree without interpreting them? Think of magic pathspec syntax (that\n>> we do not have yet, but which may materialize sometime in the future).\n> \n> I have to admit I'm not aware of magic pathspec syntax. Do you happen to\n> have any pointers where I could look at code doing similar things right?\n\ncmd_mv() in builtin/mv.c looks like a good candidate. It has to check\nwhether the destination (the last argument) is a directory.\n\n-- Hannes\n"},{"id":"203668","messageId":"50AEA7FA.6070801@web.de","threadId":"31967","inReplyTo":"5090C54C.90902@viscovery.net","subject":"[PATCH v2] Teach rm to remove submodules when given with a trailing '/'","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-11-22T22:32:26Z","receivedAt":"2012-11-22T22:32:26Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Doing a \"git rm submod/\" on a submodule results in an error:\n\tfatal: pathspec 'submod/' did not match any files\nThis is really inconvenient as e.g. using TAB completion in a shell on a\nsubmodule automatically adds the trailing '/' when it completes the path\nof the submodule directory. The user has then to remove the '/' herself to\nmake a \"git rm\" succeed. Doing a \"git rm -r somedir/\" is working fine, so\nthere is no reason why that shouldn't work for submodules too.\n\nTeach git rm to not error out when a '/' is appended to the path of a\nsubmodule. Achieve this by chopping off trailing slashes from the path\nnames given if they represent directories. Add tests to make sure that\nlogic only applies to directories and not to files.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\nAm 31.10.2012 07:29, schrieb Johannes Sixt:\n> Am 10/30/2012 22:28, schrieb Jens Lehmann:\n>> Am 29.10.2012 08:11, schrieb Johannes Sixt:\n>>> Am 10/29/2012 0:28, schrieb Jens Lehmann:\n>>>> +\t/* Remove trailing '/' from directories to find submodules in the index */\n>>>> +\tfor (i = 0; i < argc; i++) {\n>>>> +\t\tsize_t pathlen = strlen(argv[i]);\n>>>> +\t\tif (pathlen && is_directory(argv[i]) && (argv[i][pathlen - 1] == '/'))\n>>>> +\t\t\targv[i] = xmemdupz(argv[i], pathlen - 1);\n>>>> +\t}\n>>>> +\n>>>>  \tpathspec = get_pathspec(prefix, argv);\n>>>>  \trefresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);\n>>>\n>>> That's wrong: Either move the check below get_pathspec() (which normalizes\n>>> backslashes to forward-slashes on Windows) or use is_dir_sep().\n>>\n>> Thanks for bringing this up.\n\nAnd sorry for taking so long to follow up on this, but I'm currently\npretty occupied with real life issues and am rather short on git time.\n\n>>> But isn't it somewhat dangerous to check pathspec for existance in the\n>>> worktree without interpreting them? Think of magic pathspec syntax (that\n>>> we do not have yet, but which may materialize sometime in the future).\n>>\n>> I have to admit I'm not aware of magic pathspec syntax. Do you happen to\n>> have any pointers where I could look at code doing similar things right?\n> \n> cmd_mv() in builtin/mv.c looks like a good candidate. It has to check\n> whether the destination (the last argument) is a directory.\n\nThanks, that was a good pointer (and judging from the comment just before\nprefix_pathspec() in setup.c I currently can't do /that/ much about the\nmagic pathspec syntax as it has not materialized yet ;-).\n\nI was thinking about reusing the copy_pathspec() function from mv.c but\ncame to the conclusion not to do so because it a) doesn't care if a '/'\nis following a file (where I believe it should not be dropped but mv\nshould error out, but it currently doesn't) and b) it also has the\nbase_name argument I don't need (and which seems to be rather special to\nthe needs of mv.c). So I coded that loop myself using is_dir_sep()\n(taking care that is_directory() will only be called on paths which have\na trailing directory separator).\n\n\n builtin/rm.c  | 15 +++++++++++++++\n t/t3600-rm.sh | 17 +++++++++++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 2aea3b5..dabfcf6 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -234,6 +234,21 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n\n+\t/*\n+\t * Drop trailing directory separators from directories so we'll find\n+\t * submodules in the index.\n+\t */\n+\tfor (i = 0; i < argc; i++) {\n+\t\tsize_t pathlen = strlen(argv[i]);\n+\t\tif (pathlen && is_dir_sep(argv[i][pathlen - 1]) &&\n+\t\t    is_directory(argv[i])) {\n+\t\t\tdo {\n+\t\t\t\tpathlen--;\n+\t\t\t} while (pathlen && is_dir_sep(argv[i][pathlen - 1]));\n+\t\t\targv[i] = xmemdupz(argv[i], pathlen);\n+\t\t}\n+\t}\n+\n \tpathspec = get_pathspec(prefix, argv);\n \trefresh_index(&the_index, REFRESH_QUIET, pathspec, NULL, NULL);\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 97254e8..06f6384 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -302,6 +302,23 @@ test_expect_success 'rm removes work tree of unmodified submodules' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'rm removes a submodule with a trailing /' '\n+\tgit reset --hard &&\n+\tgit submodule update &&\n+\tgit rm submod/ &&\n+\ttest ! -d submod &&\n+\tgit status -s -uno --ignore-submodules=none > actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'rm fails when given a file with a trailing /' '\n+\ttest_must_fail git rm empty/\n+'\n+\n+test_expect_success 'rm succeeds when given a directory with a trailing /' '\n+\tgit rm -r frotz/\n+'\n+\n test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '\n \tgit reset --hard &&\n \tgit submodule update &&\n-- \n1.8.0.91.gf07e555\n"}]}