# [PATCH] mv: allow moving nested submodules 4 messages from 2016-04-18 to 2016-04-18. Participants: Stefan Beller, Junio C Hamano. Thread: https://gitlist.dev/t/42064 ## Stefan Beller, 2016-04-18 16:54 Subject: [PATCH] mv: allow moving nested submodules Message-ID: <1460998489-2155-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1460998489-2155-1-git-send-email-sbeller%40google.com ``` When directories are moved using `git mv` all files in the directory have been just moved, but no further action was taken on them. This was done by assigning the mode = WORKING_DIRECTORY to the files inside a moved directory. submodules however need to update their link to the git directory as well as updates to the .gitmodules file. By removing the condition of `mode != INDEX` (the remaining modes are BOTH and WORKING_DIRECTORY) for the required submodule actions, we perform these for submodules in a moved directory. Signed-off-by: Stefan Beller --- builtin/mv.c | 7 +++++-- t/t7001-mv.sh | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/builtin/mv.c b/builtin/mv.c index aeae855..65fff11 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -252,9 +252,12 @@ int cmd_mv(int argc, const char **argv, const char *prefix) int pos; if (show_only || verbose) printf(_("Renaming %s to %s\n"), src, dst); - if (!show_only && mode != INDEX) { - if (rename(src, dst) < 0 && !ignore_errors) + if (!show_only) { + if (mode != INDEX && + rename(src, dst) < 0 && + !ignore_errors) die_errno(_("renaming '%s' failed"), src); + if (submodule_gitfile[i]) { if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR) connect_work_tree_and_git_dir(dst, submodule_gitfile[i]); diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 4008fae..4a2570e 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -292,6 +292,9 @@ test_expect_success 'setup submodule' ' echo content >file && git add file && git commit -m "added sub and file" && + mkdir -p deep/directory/hierachy && + git submodule add ./. deep/directory/hierachy/sub && + git commit -m "added another submodule" && git branch submodule ' @@ -475,4 +478,17 @@ test_expect_success 'mv -k does not accidentally destroy submodules' ' git checkout . ' +test_expect_success 'moving a submodule in nested directories' ' + ( + cd deep && + git mv directory ../ && + # git status would fail if the update of linking git dir to + # work dir of the submodule failed. + git status && + git config -f ../.gitmodules submodule.deep/directory/hierachy/sub.path >../actual && + echo "directory/hierachy/sub" >../expect + ) && + test_cmp actual expect +' + test_done -- 2.8.0.26.gba39a1b.dirty ``` ## Junio C Hamano, 2016-04-18 20:54 Subject: Re: [PATCH] mv: allow moving nested submodules Message-ID: URL: https://gitlist.dev/e/xmqqk2ju4ozy.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1460998489-2155-1-git-send-email-sbeller@google.com> ``` Stefan Beller writes: > if (show_only || verbose) > printf(_("Renaming %s to %s\n"), src, dst); > - if (!show_only && mode != INDEX) { > - if (rename(src, dst) < 0 && !ignore_errors) > + if (!show_only) { > + if (mode != INDEX && > + rename(src, dst) < 0 && > + !ignore_errors) > die_errno(_("renaming '%s' failed"), src); > + If ignore-errors is set and rename fails, this would fall through and try to touch this codepath... > if (submodule_gitfile[i]) { > if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR) > connect_work_tree_and_git_dir(dst, submodule_gitfile[i]); ... but I am not sure if this thing is prepared to cope with such a case? src should have been moved to dst but if rename() failed we wouldn't see what we expect at dst, or would we? > diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh > index 4008fae..4a2570e 100755 > --- a/t/t7001-mv.sh > +++ b/t/t7001-mv.sh > @@ -292,6 +292,9 @@ test_expect_success 'setup submodule' ' > echo content >file && > git add file && > git commit -m "added sub and file" && > + mkdir -p deep/directory/hierachy && > + git submodule add ./. deep/directory/hierachy/sub && > + git commit -m "added another submodule" && > git branch submodule > ' > > @@ -475,4 +478,17 @@ test_expect_success 'mv -k does not accidentally destroy submodules' ' > git checkout . > ' > > +test_expect_success 'moving a submodule in nested directories' ' > + ( > + cd deep && > + git mv directory ../ && > + # git status would fail if the update of linking git dir to > + # work dir of the submodule failed. > + git status && > + git config -f ../.gitmodules submodule.deep/directory/hierachy/sub.path >../actual && > + echo "directory/hierachy/sub" >../expect > + ) && > + test_cmp actual expect > +' > + > test_done ``` ## Junio C Hamano, 2016-04-18 21:13 Subject: Re: [PATCH] mv: allow moving nested submodules Message-ID: URL: https://gitlist.dev/e/xmqqfuui4o4e.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > If ignore-errors is set and rename fails, this would fall through > and try to touch this codepath... > >> if (submodule_gitfile[i]) { >> if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR) >> connect_work_tree_and_git_dir(dst, submodule_gitfile[i]); > > ... but I am not sure if this thing is prepared to cope with such a > case? src should have been moved to dst but if rename() failed we > wouldn't see what we expect at dst, or would we? In other words, I was wondering if this part should read more like this: builtin/mv.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/builtin/mv.c b/builtin/mv.c index aeae855..37ed0fc 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -252,9 +252,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix) int pos; if (show_only || verbose) printf(_("Renaming %s to %s\n"), src, dst); - if (!show_only && mode != INDEX) { - if (rename(src, dst) < 0 && !ignore_errors) + if (show_only) + ; + else { + if (mode != INDEX && rename(src, dst) < 0) { + if (ignore_errors) + continue; die_errno(_("renaming '%s' failed"), src); + } if (submodule_gitfile[i]) { if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR) connect_work_tree_and_git_dir(dst, submodule_gitfile[i]); ``` ## Stefan Beller, 2016-04-18 21:26 Subject: Re: [PATCH] mv: allow moving nested submodules Message-ID: URL: https://gitlist.dev/e/CAGZ79kbVTUzKmSa9KjEJyDuRCtW5rygQmSYMpssLUKYoO1ooSw%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 18, 2016 at 2:13 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> If ignore-errors is set and rename fails, this would fall through >> and try to touch this codepath... >> >>> if (submodule_gitfile[i]) { >>> if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR) >>> connect_work_tree_and_git_dir(dst, submodule_gitfile[i]); >> >> ... but I am not sure if this thing is prepared to cope with such a >> case? src should have been moved to dst but if rename() failed we >> wouldn't see what we expect at dst, or would we? > > In other words, I was wondering if this part should read more like > this: > > builtin/mv.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/builtin/mv.c b/builtin/mv.c > index aeae855..37ed0fc 100644 > --- a/builtin/mv.c > +++ b/builtin/mv.c > @@ -252,9 +252,14 @@ int cmd_mv(int argc, const char **argv, const char *prefix) > int pos; > if (show_only || verbose) > printf(_("Renaming %s to %s\n"), src, dst); > - if (!show_only && mode != INDEX) { > - if (rename(src, dst) < 0 && !ignore_errors) > + if (show_only) > + ; > + else { > + if (mode != INDEX && rename(src, dst) < 0) { I agree until here. > + if (ignore_errors) > + continue; > die_errno(_("renaming '%s' failed"), src); This I thought would be better as: if (!ignore_errors) die_errno(...); and not continue, but continuing is the right thing I would expect. Speaking of which, connect_work_tree_and_git_dir as well as update_path_in_gitmodules need to learn about the ignore_errors flag, too. You would expect them to not die at the faintest problem, but rather honor the promise of "Skip move or rename actions which would lead to an error condition." Thanks for a starting pointer for a new patch! Stefan > + } > if (submodule_gitfile[i]) { > if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR) > connect_work_tree_and_git_dir(dst, submodule_gitfile[i]); ```