[PATCH] mv: allow moving nested submodules

[PATCH] mv: allow moving nested submodules

[Stefan Beller]

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 <sbeller@google.com>
---

> Can you do only the 2/2 on top of maint (or maint-2.6) for now then?

This is developed on maint-2.6.

 builtin/mv.c  | 22 +++++++++++++---------
 t/t7001-mv.sh | 16 ++++++++++++++++
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index d1d4316..77f3ec5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -251,15 +251,19 @@ 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)
-				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]);
-				if (!update_path_in_gitmodules(src, dst))
-					gitmodules_modified = 1;
-			}
+		if (show_only)
+			continue;
+		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]);
+			if (!update_path_in_gitmodules(src, dst))
+				gitmodules_modified = 1;
 		}
 
 		if (mode == WORKING_DIRECTORY)
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 7b56081..fcfc953 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.6.6.dirty

[PATCH] mv: allow moving nested submodules

[Stefan Beller]

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 <sbeller@google.com>
---
 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

Re: [PATCH] mv: allow moving nested submodules

[Junio C Hamano]

Stefan Beller <sbeller@google.com> 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

Re: [PATCH] mv: allow moving nested submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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]);

Re: [PATCH] mv: allow moving nested submodules

[Stefan Beller]

On Mon, Apr 18, 2016 at 2:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> 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]);

Re: 'git mv' doesn't move submodule if it's in a subdirectory

[Stefan Beller]

On Fri, Apr 15, 2016 at 11:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> I think I can reproduce the problem. A regression test (which currently fails)
>> could look like
>
> Thanks.  I however do not think this is a regression.
>
> Changes around 0656781f (mv: update the path entry in .gitmodules
> for moved submodules, 2013-08-06) did introduce "git mv dir1 dir2"
> when 'dir1' is a submodule, but I do not think it went beyond that.
> I do not see any effort to treat a submodule that is discovered by
> scanning a directory that was given from the command line,
> i.e. prepare_move_submodule() is not called for them, and the
> entries in the submodule_gitfile[] array that correspond to them are
> explicitly set to NULL in the loop.

Also I just realize this is not exactly the same bug as reported.
Albin complains about the .gitmodules file not being adjusted, whereas
the test case I wrote breaks commands in your superproject, i.e. `git status`
or `git diff` just dies.

(Manually inspecting the .gitmodules file turns out it is not adjusted as well.)

>
>
>> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
>> index 4008fae..3b96a9a 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,14 @@ test_expect_success 'mv -k does not accidentally
>> destroy submodules' '
>>         git checkout .
>>  '
>>
>> +test_expect_failure 'moving a submodule in nested directories' '
>> +       (
>> +               cd deep &&
>> +               git mv directory ../ &&
>> +               git status
>> +               # currently git status exits with 128
>> +               # fatal: Not a git repository:
>> directory/hierachy/sub/../../../../.git/modules/deep/directory/hierachy/sub
>> +       )
>> +'
>> +
>>  test_done

[PATCH] mv: allow moving nested submodules

[Stefan Beller]

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 <sbeller@google.com>
---

Albin,
I think this would fix your problem.

Developed on origin/master (it may apply on older versions, though I did not test)

Thanks,
Stefan

 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.1.211.g630c034