{"thread":{"id":"42064","subject":"[PATCH] mv: allow moving nested submodules","startedAt":"2016-04-18T16:54:49Z","lastAt":"2016-04-18T21:26:06Z","messageCount":4,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"283733","messageId":"1460998489-2155-1-git-send-email-sbeller@google.com","threadId":"42064","inReplyTo":null,"subject":"[PATCH] mv: allow moving nested submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-18T16:54:49Z","receivedAt":"2016-04-18T16:54:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When directories are moved using `git mv` all files in the directory\nhave been just moved, but no further action was taken on them. This\nwas done by assigning the mode = WORKING_DIRECTORY to the files\ninside a moved directory.\n\nsubmodules however need to update their link to the git directory as\nwell as updates to the .gitmodules file. By removing the condition of\n`mode != INDEX` (the remaining modes are BOTH and WORKING_DIRECTORY) for\nthe required submodule actions, we perform these for submodules in a\nmoved directory.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/mv.c  |  7 +++++--\n t/t7001-mv.sh | 16 ++++++++++++++++\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex aeae855..65fff11 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -252,9 +252,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tint pos;\n \t\tif (show_only || verbose)\n \t\t\tprintf(_(\"Renaming %s to %s\\n\"), src, dst);\n-\t\tif (!show_only && mode != INDEX) {\n-\t\t\tif (rename(src, dst) < 0 && !ignore_errors)\n+\t\tif (!show_only) {\n+\t\t\tif (mode != INDEX &&\n+\t\t\t    rename(src, dst) < 0 &&\n+\t\t\t    !ignore_errors)\n \t\t\t\tdie_errno(_(\"renaming '%s' failed\"), src);\n+\n \t\t\tif (submodule_gitfile[i]) {\n \t\t\t\tif (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)\n \t\t\t\t\tconnect_work_tree_and_git_dir(dst, submodule_gitfile[i]);\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 4008fae..4a2570e 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -292,6 +292,9 @@ test_expect_success 'setup submodule' '\n \techo content >file &&\n \tgit add file &&\n \tgit commit -m \"added sub and file\" &&\n+\tmkdir -p deep/directory/hierachy &&\n+\tgit submodule add ./. deep/directory/hierachy/sub &&\n+\tgit commit -m \"added another submodule\" &&\n \tgit branch submodule\n '\n \n@@ -475,4 +478,17 @@ test_expect_success 'mv -k does not accidentally destroy submodules' '\n \tgit checkout .\n '\n \n+test_expect_success 'moving a submodule in nested directories' '\n+\t(\n+\t\tcd deep &&\n+\t\tgit mv directory ../ &&\n+\t\t# git status would fail if the update of linking git dir to\n+\t\t# work dir of the submodule failed.\n+\t\tgit status &&\n+\t\tgit config -f ../.gitmodules submodule.deep/directory/hierachy/sub.path >../actual &&\n+\t\techo \"directory/hierachy/sub\" >../expect\n+\t) &&\n+\ttest_cmp actual expect\n+'\n+\n test_done\n-- \n2.8.0.26.gba39a1b.dirty\n"},{"id":"283748","messageId":"xmqqk2ju4ozy.fsf@gitster.mtv.corp.google.com","threadId":"42064","inReplyTo":"1460998489-2155-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] mv: allow moving nested submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-18T20:54:09Z","receivedAt":"2016-04-18T20:54:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>  \t\tif (show_only || verbose)\n>  \t\t\tprintf(_(\"Renaming %s to %s\\n\"), src, dst);\n> -\t\tif (!show_only && mode != INDEX) {\n> -\t\t\tif (rename(src, dst) < 0 && !ignore_errors)\n> +\t\tif (!show_only) {\n> +\t\t\tif (mode != INDEX &&\n> +\t\t\t    rename(src, dst) < 0 &&\n> +\t\t\t    !ignore_errors)\n>  \t\t\t\tdie_errno(_(\"renaming '%s' failed\"), src);\n> +\n\nIf ignore-errors is set and rename fails, this would fall through\nand try to touch this codepath...\n\n>  \t\t\tif (submodule_gitfile[i]) {\n>  \t\t\t\tif (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)\n>  \t\t\t\t\tconnect_work_tree_and_git_dir(dst, submodule_gitfile[i]);\n\n... but I am not sure if this thing is prepared to cope with such a\ncase?  src should have been moved to dst but if rename() failed we\nwouldn't see what we expect at dst, or would we?\n\n> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\n> index 4008fae..4a2570e 100755\n> --- a/t/t7001-mv.sh\n> +++ b/t/t7001-mv.sh\n> @@ -292,6 +292,9 @@ test_expect_success 'setup submodule' '\n>  \techo content >file &&\n>  \tgit add file &&\n>  \tgit commit -m \"added sub and file\" &&\n> +\tmkdir -p deep/directory/hierachy &&\n> +\tgit submodule add ./. deep/directory/hierachy/sub &&\n> +\tgit commit -m \"added another submodule\" &&\n>  \tgit branch submodule\n>  '\n>  \n> @@ -475,4 +478,17 @@ test_expect_success 'mv -k does not accidentally destroy submodules' '\n>  \tgit checkout .\n>  '\n>  \n> +test_expect_success 'moving a submodule in nested directories' '\n> +\t(\n> +\t\tcd deep &&\n> +\t\tgit mv directory ../ &&\n> +\t\t# git status would fail if the update of linking git dir to\n> +\t\t# work dir of the submodule failed.\n> +\t\tgit status &&\n> +\t\tgit config -f ../.gitmodules submodule.deep/directory/hierachy/sub.path >../actual &&\n> +\t\techo \"directory/hierachy/sub\" >../expect\n> +\t) &&\n> +\ttest_cmp actual expect\n> +'\n> +\n>  test_done\n"},{"id":"283752","messageId":"xmqqfuui4o4e.fsf@gitster.mtv.corp.google.com","threadId":"42064","inReplyTo":"xmqqk2ju4ozy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] mv: allow moving nested submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-18T21:13:05Z","receivedAt":"2016-04-18T21:13:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> If ignore-errors is set and rename fails, this would fall through\n> and try to touch this codepath...\n>\n>>  \t\t\tif (submodule_gitfile[i]) {\n>>  \t\t\t\tif (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)\n>>  \t\t\t\t\tconnect_work_tree_and_git_dir(dst, submodule_gitfile[i]);\n>\n> ... but I am not sure if this thing is prepared to cope with such a\n> case?  src should have been moved to dst but if rename() failed we\n> wouldn't see what we expect at dst, or would we?\n\nIn other words, I was wondering if this part should read more like\nthis:\n\n builtin/mv.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex aeae855..37ed0fc 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -252,9 +252,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\tint pos;\n \t\tif (show_only || verbose)\n \t\t\tprintf(_(\"Renaming %s to %s\\n\"), src, dst);\n-\t\tif (!show_only && mode != INDEX) {\n-\t\t\tif (rename(src, dst) < 0 && !ignore_errors)\n+\t\tif (show_only)\n+\t\t\t;\n+\t\telse {\n+\t\t\tif (mode != INDEX && rename(src, dst) < 0) {\n+\t\t\t\tif (ignore_errors)\n+\t\t\t\t\tcontinue;\n \t\t\t\tdie_errno(_(\"renaming '%s' failed\"), src);\n+\t\t\t}\n \t\t\tif (submodule_gitfile[i]) {\n \t\t\t\tif (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)\n \t\t\t\t\tconnect_work_tree_and_git_dir(dst, submodule_gitfile[i]);\n"},{"id":"283754","messageId":"CAGZ79kbVTUzKmSa9KjEJyDuRCtW5rygQmSYMpssLUKYoO1ooSw@mail.gmail.com","threadId":"42064","inReplyTo":"xmqqfuui4o4e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] mv: allow moving nested submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-18T21:26:06Z","receivedAt":"2016-04-18T21:26:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Apr 18, 2016 at 2:13 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> If ignore-errors is set and rename fails, this would fall through\n>> and try to touch this codepath...\n>>\n>>>                      if (submodule_gitfile[i]) {\n>>>                              if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)\n>>>                                      connect_work_tree_and_git_dir(dst, submodule_gitfile[i]);\n>>\n>> ... but I am not sure if this thing is prepared to cope with such a\n>> case?  src should have been moved to dst but if rename() failed we\n>> wouldn't see what we expect at dst, or would we?\n>\n> In other words, I was wondering if this part should read more like\n> this:\n>\n>  builtin/mv.c | 9 +++++++--\n>  1 file changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> index aeae855..37ed0fc 100644\n> --- a/builtin/mv.c\n> +++ b/builtin/mv.c\n> @@ -252,9 +252,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n>                 int pos;\n>                 if (show_only || verbose)\n>                         printf(_(\"Renaming %s to %s\\n\"), src, dst);\n> -               if (!show_only && mode != INDEX) {\n> -                       if (rename(src, dst) < 0 && !ignore_errors)\n> +               if (show_only)\n> +                       ;\n> +               else {\n> +                       if (mode != INDEX && rename(src, dst) < 0) {\n\nI agree until here.\n\n\n> +                               if (ignore_errors)\n> +                                       continue;\n>                                 die_errno(_(\"renaming '%s' failed\"), src);\n\nThis I thought would be better as:\n\n    if (!ignore_errors)\n        die_errno(...);\n\nand not continue, but continuing is the right thing I would expect.\n\nSpeaking of which, connect_work_tree_and_git_dir as well as\nupdate_path_in_gitmodules need to learn about the ignore_errors\nflag, too.  You would expect them to not die at the faintest problem,\nbut rather honor the promise of \"Skip move or rename actions which\nwould lead to an error condition.\"\n\nThanks for a starting pointer for a new patch!\nStefan\n\n> +                       }\n>                         if (submodule_gitfile[i]) {\n>                                 if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)\n>                                         connect_work_tree_and_git_dir(dst, submodule_gitfile[i]);\n"}]}