submodules: Use relative paths to gitdir and work tree

This patch series replaces all absolute paths pointing from submodule work trees to its gitdir and back with relative paths as discussed in $gmane/187785.

The motivation is to make superprojects movable again. They lost this ability with the move of the git directory of submodules into the .git/modules directory of the superproject. While fixing that a bug which would hit when moving the submodule inside the superproject was also fixed.

Jens Lehmann (2):
submodules: always use a relative path to gitdir
submodules: always use a relative path from gitdir to work tree

 git-submodule.sh           |   23 ++++++++++++++++-------
 t/t7400-submodule-basic.sh |   22 ++++++++++++++++++++++
 2 files changed, 38 insertions(+), 7 deletions(-)

1.7.9.190.gb17a42.dirty

Since recently a submodule with name <name> has its git directory in the .git/modules/<name> directory of the superproject while the work tree contains a gitfile pointing there. When the submodule git directory needs to be cloned because it is not found in .git/modules/<name> the clone command will write an absolute path into the gitfile. When no clone is necessary the git directory will be reactivated by the git-submodule.sh script by writing a relative path into the gitfile.

This is inconsistent, as the behavior depends on the submodule having been cloned before into the .git/modules of the superproject. A relative path is preferable here because it allows the superproject to be moved around without invalidating the gitfile. We do that by always writing the relative path into the gitfile, which overwrites the absolute path the clone command may have written there.

This is only the first step to make superprojects movable again like they were before the separate-git-dir approach was introduced. The second step is to use a relative path in core.worktree too.

Enhance t7400 to ensure that future versions won't re-add absolute paths by accident.

While at it also replace an if/else construct evaluating the presence of the 'reference' option with a single line of bash code.

---
 git-submodule.sh           |   11 ++++-------
 t/t7400-submodule-basic.sh |    2 ++
 2 files changed, 6 insertions(+), 7 deletions(-)

index 9bb2e13..2a93c61 100755
@@ -160,18 +160,15 @@ module_clone()
     if test -d "$gitdir"
     then
         mkdir -p "$path"
-        echo "gitdir: $rel_gitdir" >"$path/.git"
         rm -f "$gitdir/index"
     else
         mkdir -p "$gitdir_base"
-        if test -n "$reference"
-        then
-            git-clone $quiet "$reference" -n "$url" "$path" --separate-git-dir "$gitdir"
-        else
-            git-clone $quiet -n "$url" "$path" --separate-git-dir "$gitdir"
-        fi ||
+        git clone $quiet -n ${reference:+"$reference"} \
+            --separate-git-dir "$gitdir" "$url" "$path" ||
         die "$(eval_gettext "Clone of '\$url' into submodule path '\$path' failed")"
     fi
+
+    echo "gitdir: $rel_gitdir" >"$path/.git"
 }
 
 #

index 695f7af..2b70b22 100755
@@ -79,6 +79,8 @@ test_expect_success 'submodule add' '
         cd addtest &&
         git submodule add -q "$submodurl" submod >actual &&
         test ! -s actual &&
+        echo "gitdir: ../.git/modules/submod" >expect &&
+        test_cmp expect submod/.git &&
         git submodule init
     ) &&
 

1.7.9.190.gb17a42.dirty

Since recently a submodule with name <name> has its git directory in the .git/modules/<name> directory of the superproject while the work tree contains a gitfile pointing there. When the submodule git directory needs to be cloned because it is not found in .git/modules/<name> the clone command will write an absolute path into the gitfile. When no clone is necessary the git directory will be reactivated by the git-submodule.sh script by writing a relative path into the gitfile.

This is inconsistent, as the behavior depends on the submodule having been cloned before into the .git/modules of the superproject. A relative path is preferable here because it allows the superproject to be moved around without invalidating the gitfile. We do that by always writing the relative path into the gitfile, which overwrites the absolute path the clone command may have written there.

This is only the first step to make superprojects movable again like they were before the separate-git-dir approach was introduced. The second step is to use a relative path in core.worktree too.

Enhance t7400 to ensure that future versions won't re-add absolute paths by accident.

While at it also replace an if/else construct evaluating the presence of the 'reference' option with a single line of bash code.

---

The first version was whitespace damaged, please use this one instead.

 git-submodule.sh           |   11 ++++-------
 t/t7400-submodule-basic.sh |    2 ++
 2 files changed, 6 insertions(+), 7 deletions(-)

index 9bb2e13..2a93c61 100755
@@ -160,18 +160,15 @@ module_clone()
 	if test -d "$gitdir"
 	then
 		mkdir -p "$path"
-		echo "gitdir: $rel_gitdir" >"$path/.git"
 		rm -f "$gitdir/index"
 	else
 		mkdir -p "$gitdir_base"
-		if test -n "$reference"
-		then
-			git-clone $quiet "$reference" -n "$url" "$path" --separate-git-dir "$gitdir"
-		else
-			git-clone $quiet -n "$url" "$path" --separate-git-dir "$gitdir"
-		fi ||
+		git clone $quiet -n ${reference:+"$reference"} \
+			--separate-git-dir "$gitdir" "$url" "$path" ||
 		die "$(eval_gettext "Clone of '\$url' into submodule path '\$path' failed")"
 	fi
+
+	echo "gitdir: $rel_gitdir" >"$path/.git"
 }
 
 #

index 695f7af..2b70b22 100755
@@ -79,6 +79,8 @@ test_expect_success 'submodule add' '
 		cd addtest &&
 		git submodule add -q "$submodurl" submod >actual &&
 		test ! -s actual &&
+		echo "gitdir: ../.git/modules/submod" >expect &&
+		test_cmp expect submod/.git &&
 		git submodule init
 	) &&
 

1.7.9.190.g0a6c2

I understand that "recently" refers to efc5fb6a77 and this is potentially a maint material for v1.7.8.X; am I correct?

Thanks.

When applied to v1.7.8.x maintenance track, this failed the first step in t5526, but with the earlier jl/submodule-re-add topic everything seems to pass just fine.

Am 09.02.2012 20:40, schrieb Junio C Hamano:

That's correct.

Ah, I only tested these patches on current master. But you are right, this belongs on top of jl/submodule-re-add for maint.

What I meant to say is that "this and the other topic need to be merged to that maintenance track together"; I didn't mean to say there is any breakage in this patch.

Sorry for a potentially confusing statement.

Since recently a submodule with name <name> has its git directory in the .git/modules/<name> directory of the superproject while the work tree contains a gitfile pointing there. To make that work the git directory has the core.worktree configuration set in its config file to point back to the work tree.

That core.worktree is an absolute path set by the initial clone of the submodule. A relative path is preferable here because it allows the superproject to be moved around without invalidating that setting, so compute and set that relative path after cloning or reactivating the submodule.

This also fixes a bug when moving a submodule around inside the superproject, as the current code forgot to update the setting to the new submodule work tree location.

Enhance t7400 to ensure that future versions won't re-add absolute paths by accident and that moving a superproject won't break submodules.

---
 git-submodule.sh           |    9 +++++++++
 t/t7400-submodule-basic.sh |   20 ++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

index 2a93c61..3463d6d 100755
@@ -169,6 +169,15 @@ module_clone()
     fi
 
     echo "gitdir: $rel_gitdir" >"$path/.git"
+
+    a=$(cd "$gitdir" && pwd)
+    b=$(cd "$path" && pwd)
+    while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
+    do
+        a=${a#*/} b=${b#*/};
+    done
+    rel=$(echo $a | sed -e 's|[^/]*|..|g')
+    (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
 }
 
 #

index 2b70b22..b377a7a 100755
@@ -81,6 +81,13 @@ test_expect_success 'submodule add' '
         test ! -s actual &&
         echo "gitdir: ../.git/modules/submod" >expect &&
         test_cmp expect submod/.git &&
+        (
+            cd submod &&
+            git config core.worktree >actual &&
+            echo "../../../submod" >expect &&
+            test_cmp expect actual &&
+            rm -f actual expect
+        ) &&
         git submodule init
     ) &&
 

@@ -500,4 +507,17 @@ test_expect_success 'relative path works with user@host:path' '
     )
 '
 
+test_expect_success 'moving the superproject does not break submodules' '
+    (
+        cd addtest &&
+        git submodule status >expect
+    )
+    mv addtest addtest2 &&
+    (
+        cd addtest2 &&
+        git submodule status >actual &&
+        test_cmp expect actual
+    )
+'
+
 test_done

1.7.9.190.gf8e73.dirty

Since recently a submodule with name <name> has its git directory in the .git/modules/<name> directory of the superproject while the work tree contains a gitfile pointing there. To make that work the git directory has the core.worktree configuration set in its config file to point back to the work tree.

That core.worktree is an absolute path set by the initial clone of the submodule. A relative path is preferable here because it allows the superproject to be moved around without invalidating that setting, so compute and set that relative path after cloning or reactivating the submodule.

This also fixes a bug when moving a submodule around inside the superproject, as the current code forgot to update the setting to the new submodule work tree location.

Enhance t7400 to ensure that future versions won't re-add absolute paths by accident and that moving a superproject won't break submodules.

---

The first version was whitespace damaged, please use this instead.

 git-submodule.sh           |    9 +++++++++
 t/t7400-submodule-basic.sh |   20 ++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

index 2a93c61..3463d6d 100755
@@ -169,6 +169,15 @@ module_clone()
 	fi
 
 	echo "gitdir: $rel_gitdir" >"$path/.git"
+
+	a=$(cd "$gitdir" && pwd)
+	b=$(cd "$path" && pwd)
+	while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
+	do
+		a=${a#*/} b=${b#*/};
+	done
+	rel=$(echo $a | sed -e 's|[^/]*|..|g')
+	(clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
 }
 
 #

index 2b70b22..b377a7a 100755
@@ -81,6 +81,13 @@ test_expect_success 'submodule add' '
 		test ! -s actual &&
 		echo "gitdir: ../.git/modules/submod" >expect &&
 		test_cmp expect submod/.git &&
+		(
+			cd submod &&
+			git config core.worktree >actual &&
+			echo "../../../submod" >expect &&
+			test_cmp expect actual &&
+			rm -f actual expect
+		) &&
 		git submodule init
 	) &&
 

@@ -500,4 +507,17 @@ test_expect_success 'relative path works with user@host:path' '
 	)
 '
 
+test_expect_success 'moving the superproject does not break submodules' '
+	(
+		cd addtest &&
+		git submodule status >expect
+	)
+	mv addtest addtest2 &&
+	(
+		cd addtest2 &&
+		git submodule status >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

1.7.9.190.g0a6c2

Thanks. This one looks somewhat iffy (1/2 looked perfectly fine, though).

Am 13.02.2012 20:59, schrieb Junio C Hamano:

I just copied that while loop from a few lines up, but you are right about the style and logic issues (Will send a cleanup patch for the other loop too when we agree on how to write this one).

After adding a comment, using test instead of [], testing both $a and $b and assigning each variable on it's own line I get the following interdiff. Does that make more sense?

index 3463d6d..ed76ce2 100755
@@ -172,9 +172,11 @@ module_clone()
 
        a=$(cd "$gitdir" && pwd)
        b=$(cd "$path" && pwd)
-       while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
+       # Remove all common leading directories
+       while test -n "$a" && test -n "$b" && test "${a%%/*}" = "${b%%/*}"
        do
-               a=${a#*/} b=${b#*/};
+               a=${a#*/}
+               b=${b#*/}
        done
        rel=$(echo $a | sed -e 's|[^/]*|..|g')
        (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")

My earlier request for comment was to say

# $a is always longer than $b for such and such reasons

to explain why testing $b without testing $a was sufficient.

It is obvious (at least to me) that the loop continues as long as $a and $b begin with the same string before their first '/' and removes that common segment from both of them, so I do not think the new comment is absolutely necessary, but it would not hurt to have it, especially it is short enough and to the point.

Thanks.

Heh, after I follow the entire module_clone, $gitdir is defined in earlier parts of the function to be "$(rev-parse --git-dir)/modules/$path", so it is clear that it is longer than $path. Unless "cd $there && pwd" does not result in a funny situation (such as $something/modules is a symbolic link to another place that is much closer to the root of the filesystem), that is.

And in such a case, the prefix part of $a and $b would be different from the very beginning hopefully.

Perhaps aseert that $a never becomes empty before this line (or set it explicitly to "." when $a is empty), as otherwise

this will refer to "/$b" from the root?

Am 14.02.2012 21:34, schrieb Junio C Hamano:

Unfortunately only by accident. The usage of $path is not correct here, $name should be used instead (I have a patch in the making to correct that, but as that hits the same code area as these fixes I'll post that later together with some tests moving submodules around inside a superproject). Then the result of "$(rev-parse --git-dir)/modules/$name" can be shorter than "$path" when a submodule is renamed into a higher directory level.

Yes, they should differ somewhere in any sane setup I can imagine.

I think neither $a nor $b should be empty after that. But thinking deeper about that while loop I suspect the real problem here is doing "a=${a#*/}" or "b=${b#*/}" on a string that doesn't contain a slash anymore. We'll happily remove a leading directory on the other path while the one without slash will stay untouched, leading to a bogus result which is off by one directory level.

AFAICS that will only happen when one path is a prefix of the other, which is a pretty pathological case. So I'll whip up a new version asserting that beforehand and dropping the -n test in the while loop, ok?

Am 08.02.2012 23:08, schrieb Jens Lehmann:

This series, with the tip at e3307adaba in Junio's repo causes major headaches on Windows.

First, a check for an absolute path must be extended to take Windows-style paths into account.

Second, the a's and b's are filled with different forms of absolute paths (/c/there vs. c:/there), and as a consequence the subsequent loops do not find a suitable relative path.

The below is a minimal hack that passes all t/*submod* tests, but it works only on Windows, where the pwd utility has an option -W that prints a Windows style absolute path.

How would you have this solved? One option would be to introduce a function

pwd() { builtin pwd -W "$@"; }

in git-sh-setup conditionally on Windows (but that would affect other shell scripts, too).

Any other ideas?

index 3463d6d..f37745e 100755
@@ -139,8 +139,8 @@ module_clone()
 	gitdir="$gitdir/modules/$path"
 
 	case $gitdir in
-	/*)
-		a="$(cd_to_toplevel && pwd)/"
+	/* | [a-z]:/*)
+		a="$(cd_to_toplevel && pwd -W)/"
 		b=$gitdir
 		while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
 		do

@@ -170,8 +170,8 @@ module_clone()
 
 	echo "gitdir: $rel_gitdir" >"$path/.git"
 
-	a=$(cd "$gitdir" && pwd)
-	b=$(cd "$path" && pwd)
+	a=$(cd "$gitdir" && pwd -W)
+	b=$(cd "$path" && pwd -W)
 	while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
 	do
 		a=${a#*/} b=${b#*/};

1.7.8.216.g2e426

Am 26.02.2012 18:38, schrieb Johannes Sixt:

Okay, but that check is not part of my series (it was introduced by 501770e1 "Move git-dir for submodules", which is in Git since 1.7.8), so that looks like it would need to be fixed for msysgit even without my patches, right?

But I'm not so happy about the two code paths there anyway, so I prepared a patch to replace them with a single code path based upon the paths computed in the last patch of this series. Please see the always-use-relative-gitdir branch in my github repo https://github.com/jlehmann/git-submod-enhancements

I suspect other shell scripts might be less affected when non-Windows paths are forced (at least when they aren't developed under Windows only). What about something like this:

pwd() { builtin pwd -W "$@" | sed -e 's,^\([a-z]\):/,/\1/,'; }

index 5deabf6..5bb8109 100755
@@ -134,7 +134,7 @@ module_clone()
        test -n "$name" || name="$path"
        base_path=$(dirname "$path")
 
-       gitdir=$(git rev-parse --git-dir)
+       gitdir=$(git rev-parse --git-dir | sed -e 's,^\([a-z]\):/,/\1/,')
        gitdir_base="$gitdir/modules/$base_path"
        gitdir="$gitdir/modules/$path"

Am 26.02.2012 20:58, schrieb Jens Lehmann:

It looks like this is the case. I was surprised as well. I hoped that pwd -P would fix it, but it makes no difference. I should have tested pwd -L as well, but I forgot.

Am 27.02.2012 22:19, schrieb Johannes Sixt:

pwd -L doesn't make a difference, either.

With the following patch on top of your always-use-relative-gitdir branch from https://github.com/jlehmann/git-submod-enhancements the tests pass on Windows.

Thanks, Dscho, for pointing out the obvious.

index e1984e0..953ca5e 100755
@@ -151,6 +151,9 @@ module_clone()
 
 	a=$(cd "$gitdir" && pwd)
 	b=$(cd "$path" && pwd)
+	# normalize Windows-style absolute paths to POSIX-style absolute paths
+	case $a in [a-zA-Z]:/*) a=/${a%%:*}${a#*:} esac
+	case $b in [a-zA-Z]:/*) b=/${b%%:*}${b#*:} esac
 	# Remove all common leading directories
 	while test -n "$a" && test -n "$b" && test "${a%%/*}" = "${b%%/*}"
 	do

Am 28.02.2012 19:58, schrieb Johannes Sixt:

Thanks for helping to test and fix that on the Windows side. Do you want to post a commit based on the the interdiff below so I can apply it on top of my branch? Then I would make this a four patch series in the next round.

The patch looks unintrusive and sane.

Thanks all three of you for looking into this. Should I wait for a patch with nice write-up from one of you, or should I just come up with a random message and apply it locally avoiding roundtrip cost?

Am 28.02.2012 20:14, schrieb Junio C Hamano:

Thanks, but that interdiff needs all three patches from my branch to work properly, while I only posted the first two here so far (without the third one the gitfile still might contain the "c:/" notation even with J6t's diff applied). I still need to remove the iffiness of my 2/2 patch and the third one needs a test case too before I can repost that series.

Hi,

Not many, but some rather straight-forward ones. E.g.

	# quoted to preserve tabs & newslines (Windows users are creative)
	gitdir="$(git rev-parse --git-dir)"
	case "$gitdir" in
	[A-Za-z]:*)
		gitdir="/${gitdir%%:*}${gitdir#?:}"
		;;
	esac

Note: untested.

Note2: I did not add error handling, either.

Note3: yes, ${...} is available in bash and it does not fork. Neither does
case ... esac

Ciao,
Johannes