threads / patch / 29576

patch, 2 partssubmodules: Use relative paths to gitdir and work tree

Subject: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree

## tl;dr

21 messages between Feb 8, 2012 and Feb 28, 2012. Diffs are folded; open one to read it.

replies: 20people: 4as markdown or json

Jens Lehmann· Feb 8, 2012, 22:08 UTC · lore

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
Jens Lehmann· Feb 8, 2012, 22:11 UTC · re: Jens Lehmann · lore

[PATCH 1/2] submodules: always use a relative path to gitdir

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.

Reported-by: Antony Male <antony.male@gmail.com>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 git-submodule.sh           |   11 ++++-------
 t/t7400-submodule-basic.sh |    2 ++
 2 files changed, 6 insertions(+), 7 deletions(-)
Show changes to 2 files +6 −7

git-submodule.sh, t/t7400-submodule-basic.sh

diff --git a/git-submodule.sh b/git-submodule.sh
index 9bb2e13..2a93c61 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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"
 }
 
 #
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 695f7af..2b70b22 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -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
Jens Lehmann· Feb 9, 2012, 08:18 UTC · re: Jens Lehmann · lore

[PATCH v2 1/2] submodules: always use a relative path to gitdir

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.

Reported-by: Antony Male <antony.male@gmail.com>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
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(-)
Show changes to 2 files +6 −7

git-submodule.sh, t/t7400-submodule-basic.sh

diff --git a/git-submodule.sh b/git-submodule.sh
index 9bb2e13..2a93c61 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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"
 }

 #
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 695f7af..2b70b22 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -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
Junio C Hamano· Feb 9, 2012, 19:40 UTC · re: Jens Lehmann · lore

Re: [PATCH v2 1/2] submodules: always use a relative path to gitdir

Jens Lehmann <Jens.Lehmann@web.de> writes:
> Since recently a submodule with name <name> has its git directory in the

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

> The first version was whitespace damaged, please use this one instead.
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.

Junio C Hamano· Feb 9, 2012, 19:52 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 1/2] submodules: always use a relative path to gitdir

Junio C Hamano <gitster@pobox.com> writes:
> 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.

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.
Jens Lehmann· Feb 9, 2012, 20:13 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 1/2] submodules: always use a relative path to gitdir

Am 09.02.2012 20:40, schrieb Junio C Hamano:
Show 6 quoted lines
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> Since recently a submodule with name <name> has its git directory in the
> 
> I understand that "recently" refers to efc5fb6a77 and this is potentially
> a maint material for v1.7.8.X; am I correct?
That's correct.
Show 7 quoted lines
>> The first version was whitespace damaged, please use this one instead.
> 
> 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.

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

Jens Lehmann· Feb 8, 2012, 22:17 UTC · re: Jens Lehmann · lore

[PATCH 2/2] submodules: always use a relative path from gitdir to work tree

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.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
 git-submodule.sh           |    9 +++++++++
 t/t7400-submodule-basic.sh |   20 ++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)
Show changes to 2 files +29 −0

git-submodule.sh, t/t7400-submodule-basic.sh

diff --git a/git-submodule.sh b/git-submodule.sh
index 2a93c61..3463d6d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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")
 }
 
 #
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 2b70b22..b377a7a 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -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
Jens Lehmann· Feb 9, 2012, 08:18 UTC · re: Jens Lehmann · lore

[PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree

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.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---
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(-)
Show changes to 2 files +29 −0

git-submodule.sh, t/t7400-submodule-basic.sh

diff --git a/git-submodule.sh b/git-submodule.sh
index 2a93c61..3463d6d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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")
 }

 #
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 2b70b22..b377a7a 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -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
Junio C Hamano· Feb 13, 2012, 19:59 UTC · re: Jens Lehmann · lore

Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree

Jens Lehmann <Jens.Lehmann@web.de> writes:
> The first version was whitespace damaged, please use this instead.
Thanks.  This one looks somewhat iffy (1/2 looked perfectly fine, though).
Show 9 quoted lines
> +	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")
>  }

The style ([ "$b" ] vs "test -n "$b") aside, it strikes me odd that you only check $b; it is unclear what guarantees that "$a" is always longer. Maybe there is a reason but then a one-line comment here would not hurt?

Jens Lehmann· Feb 14, 2012, 17:36 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree

Am 13.02.2012 20:59, schrieb Junio C Hamano:
Show 15 quoted lines
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> +	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")
>>  }
> 
> 
> The style ([ "$b" ] vs "test -n "$b") aside, it strikes me odd that you
> only check $b; it is unclear what guarantees that "$a" is always longer.
> Maybe there is a reason but then a one-line comment here would not hurt?

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?

Show changes to git-submodule.sh +4 −2
diff --git a/git-submodule.sh b/git-submodule.sh
index 3463d6d..ed76ce2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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")
Junio C Hamano· Feb 14, 2012, 20:24 UTC · re: Jens Lehmann · lore

Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree

Jens Lehmann <Jens.Lehmann@web.de> writes:
> 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?
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.
Show 18 quoted lines
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3463d6d..ed76ce2 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -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")
Junio C Hamano· Feb 14, 2012, 20:34 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>> 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?
>
> 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.

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.

Show 25 quoted lines
> 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.
>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 3463d6d..ed76ce2 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -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')

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

>>         (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
this will refer to "/$b" from the root?
Jens Lehmann· Feb 15, 2012, 22:18 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree

Am 14.02.2012 21:34, schrieb Junio C Hamano:
Show 17 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Jens Lehmann <Jens.Lehmann@web.de> writes:
>>
>>> 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?
>>
>> 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.
> 
> 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.

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.

Show 7 quoted lines
> 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.
Yes, they should differ somewhere in any sane setup I can imagine.
Show 32 quoted lines
>> 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.
>>
>>> diff --git a/git-submodule.sh b/git-submodule.sh
>>> index 3463d6d..ed76ce2 100755
>>> --- a/git-submodule.sh
>>> +++ b/git-submodule.sh
>>> @@ -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')
> 
> Perhaps aseert that $a never becomes empty before this line (or set it
> explicitly to "." when $a is empty), as otherwise
> 
>>>         (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
> 
> this will refer to "/$b" from the root?

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?

Johannes Sixt· Feb 26, 2012, 17:38 UTC · re: Jens Lehmann · lore

Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree

Am 08.02.2012 23:08, schrieb Jens Lehmann:
Show 11 quoted lines
> 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

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?
Show changes to git-submodule.sh +4 −4
diff --git a/git-submodule.sh b/git-submodule.sh
index 3463d6d..f37745e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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
Jens Lehmann· Feb 26, 2012, 19:58 UTC · re: Johannes Sixt · lore

Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree

Am 26.02.2012 18:38, schrieb Johannes Sixt:
Show 18 quoted lines
> Am 08.02.2012 23:08, schrieb Jens Lehmann:
>> 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
> 
> 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.

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

Show 14 quoted lines
> 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).

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/,'; }
Show 17 quoted lines
> Any other ideas?
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 3463d6d..f37745e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -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

Hmm, here the path which starts with "c:/" is returned by the "git rev-parse --git-dir" which is used to initialize the $gitdir variable a few lines up.

Show 11 quoted lines
> @@ -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#*/};

I don't understand why you need this. Does "pwd" sometimes return a path starting with "c:/" and sometimes "/c/" depending on what form you use when you cd into that directory? If not, does the following help you on windows? (If that is the case, you might need the pwd redefinition too to make that work)

-----------------8<--------------------
Show changes to git-submodule.sh +1 −1
diff --git a/git-submodule.sh b/git-submodule.sh
index 5deabf6..5bb8109 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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"
Johannes Sixt· Feb 27, 2012, 21:19 UTC · re: Jens Lehmann · lore

Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree

Am 26.02.2012 20:58, schrieb Jens Lehmann:
Show 12 quoted lines
> Am 26.02.2012 18:38, schrieb Johannes Sixt:
>> -	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#*/};
> 
> I don't understand why you need this. Does "pwd" sometimes return a
> path starting with "c:/" and sometimes "/c/" depending on what form
> you use when you cd into that directory?

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.

> -       gitdir=$(git rev-parse --git-dir)
> +       gitdir=$(git rev-parse --git-dir | sed -e 's,^\([a-z]\):/,/\1/,')

I don't like pipelines of this kind because they fork yet another process. But it looks like there are not that many alternatives...

-- Hannes
Johannes Schindelin· Feb 27, 2012, 21:41 UTC · re: Johannes Sixt · lore

Re: [msysGit] Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree

Hi,
On Mon, 27 Feb 2012, Johannes Sixt wrote:
Show 7 quoted lines
> Am 26.02.2012 20:58, schrieb Jens Lehmann:
> 
> > -       gitdir=$(git rev-parse --git-dir)
> > +       gitdir=$(git rev-parse --git-dir | sed -e 's,^\([a-z]\):/,/\1/,')
> 
> I don't like pipelines of this kind because they fork yet another
> process. But it looks like there are not that many alternatives...
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

Johannes Sixt· Feb 28, 2012, 18:58 UTC · re: Johannes Sixt · lore

Re: [msysGit] Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree

Am 27.02.2012 22:19, schrieb Johannes Sixt:
Show 8 quoted lines
> Am 26.02.2012 20:58, schrieb Jens Lehmann:
>> I don't understand why you need this. Does "pwd" sometimes return a
>> path starting with "c:/" and sometimes "/c/" depending on what form
>> you use when you cd into that directory?
> 
> 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.
pwd -L doesn't make a difference, either.
Show 5 quoted lines
>> -       gitdir=$(git rev-parse --git-dir)
>> +       gitdir=$(git rev-parse --git-dir | sed -e 's,^\([a-z]\):/,/\1/,')
> 
> I don't like pipelines of this kind because they fork yet another
> process. But it looks like there are not that many alternatives...

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.
Show changes to git-submodule.sh +3 −0
diff --git a/git-submodule.sh b/git-submodule.sh
index e1984e0..953ca5e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -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
Junio C Hamano· Feb 28, 2012, 19:14 UTC · re: Johannes Sixt · lore

Re: [msysGit] Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree

Johannes Sixt <j6t@kdbg.org> writes:
Show 5 quoted lines
> 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.
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?

Show 14 quoted lines
> diff --git a/git-submodule.sh b/git-submodule.sh
> index e1984e0..953ca5e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -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
Jens Lehmann· Feb 28, 2012, 19:33 UTC · re: Junio C Hamano · lore

Re: [msysGit] Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree

Am 28.02.2012 20:14, schrieb Junio C Hamano:
Show 13 quoted lines
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> 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.
> 
> 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?

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.

Show 15 quoted lines
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index e1984e0..953ca5e 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -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
> 
Jens Lehmann· Feb 28, 2012, 19:21 UTC · re: Johannes Sixt · lore

Re: [msysGit] Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree

Am 28.02.2012 19:58, schrieb Johannes Sixt:
Show 13 quoted lines
> Am 27.02.2012 22:19, schrieb Johannes Sixt:
>> Am 26.02.2012 20:58, schrieb Jens Lehmann:
>>> -       gitdir=$(git rev-parse --git-dir)
>>> +       gitdir=$(git rev-parse --git-dir | sed -e 's,^\([a-z]\):/,/\1/,')
>>
>> I don't like pipelines of this kind because they fork yet another
>> process. But it looks like there are not that many alternatives...
> 
> 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.

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.

Show 15 quoted lines
> diff --git a/git-submodule.sh b/git-submodule.sh
> index e1984e0..953ca5e 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -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
> 

← back to recent threads