threads / patch / 42064

patchmv: allow moving nested submodules

Subject: [PATCH] mv: allow moving nested submodules

## tl;dr

4 messages between Apr 18, 2016 and Apr 18, 2016. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Stefan Beller· Apr 18, 2016, 16:54 UTC · lore

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(-)

Show changes to 2 files +21 −2

builtin/mv.c, t/t7001-mv.sh

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· Apr 18, 2016, 20:54 UTC · re: Stefan Beller · lore

Re: [PATCH] mv: allow moving nested submodules

Stefan Beller <sbeller@google.com> writes:

Show 10 quoted lines

>  		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?

Show 32 quoted lines

> 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· Apr 18, 2016, 21:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] mv: allow moving nested submodules

Junio C Hamano <gitster@pobox.com> writes:

Show 10 quoted lines

> 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(-)

Show changes to builtin/mv.c +7 −2

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· Apr 18, 2016, 21:26 UTC · re: Junio C Hamano · lore

Re: [PATCH] mv: allow moving nested submodules

On Mon, Apr 18, 2016 at 2:13 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 33 quoted lines

> 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]);