git commit/push can fail silently when clone omits ".git"

Ladies and Gentlemen,

I'm running git 1.7.9.5 on Ubuntu 12.04.1 LTS

I got bitten by what follows. Yes, it's an edge case. Yes I now understand
why it does what it does. Yes the right answer is "Don't do that, Jeff." :-)

Still, it took me a little time to figure out what I'd done wrong because
the failure is silent, so I thought I'd document it. Perhaps there's even
some way to issue an error message for cases like this.

The attached test script shows the issue in detail, but here's the basic
failure:

$ ls
hello.git
$ git clone hello # *Mistake!* Succeeds, but should have cloned "hello.git"
or into something else.
$ cd hello; touch foo; git add foo; git commit -am"add a new file"
$ git status # says I'm a rev ahead of the origin
$ git push # nothing pushed
$ git status # says everything's okay

At this point hello/foo still exists, there's nothing to commit, git diff
origin/master reports nothing, yet foo was never pushed to hello.git.

HTH!

--
Jeffrey Haemer [off-list ref]
720-837-8908 [cell], http://seejeffrun.blogspot.com [blog],
http://www.youtube.com/user/goyishekop [vlog]
פרייהייט? דאס איז יאַנג דינען וואָרט.

On Sun, Nov 04, 2012 at 12:50:58PM -0700, Jeffrey S. Haemer wrote:

I got bitten by what follows. Yes, it's an edge case. Yes I now understand
why it does what it does. Yes the right answer is "Don't do that, Jeff." :-)

Still, it took me a little time to figure out what I'd done wrong because
the failure is silent, so I thought I'd document it. Perhaps there's even
some way to issue an error message for cases like this.

The attached test script shows the issue in detail, but here's the basic
failure:

$ ls
hello.git
$ git clone hello # *Mistake!* Succeeds, but should have cloned "hello.git"
or into something else.

It does clone hello.git into "hello", but it sets remote.origin.url in
the cloned repository to "/path/to/hello". I.e., to itself, rather than
the correct hello.git.

The reason is that "clone" sets the config from the repo name you gave
it, not the path it finds on disk. The name you gave was not ambiguous
at the time of clone, but it became so during the clone. I am tempted to
say that we should set the config to the path we found on disk, not what
the user gave us. That includes the ugly "/.git" for non-bare repos, but
we should be able to safely strip that off without adding any ambiguity
(i.e, it is only "foo" versus "foo.git" that is ambiguous).

Unfortunately, the patch below which does that seems to make t7407 very
unhappy. It looks like the submodule test uses "git clone ." and
"git-submodule add" expects the "/." to still be at the end of the
configured URL when processing relative submodule paths. I'm not sure if
that is important, or an unnecessary brittleness in the submodule code.

Jens, Heiko?

---

diff --git a/builtin/clone.c b/builtin/clone.c
index 0d663e3..687d5c0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -713,8 +713,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);
-	if (path)
-		repo = xstrdup(absolute_path(repo_name));
+	if (path) {
+		if (!suffixcmp(path, "/.git"))
+			repo = xstrndup(path, strlen(path) - 5);
+		else
+			repo = xstrdup(path);
+	}
 	else if (!strchr(repo_name, ':'))
 		die(_("repository '%s' does not exist"), repo_name);
 	else

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 67869b4..0eeeb2d 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh

@@ -280,4 +280,20 @@ test_expect_success 'clone checking out a tag' '
 	test_cmp fetch.expected fetch.actual
 '
 
+test_expect_success 'clone does not create ambiguous config' '
+	git init --bare ambiguous.git &&
+	git clone ambiguous &&
+	(
+		cd ambiguous &&
+		test_commit one &&
+		git push --all
+	) &&
+	echo one >expect &&
+	(
+		cd ambiguous.git &&
+		git log -1 --format=%s
+	) >actual &&
+	test_cmp expect actual
+'
+
 test_done

Hi,

On Thu, Nov 08, 2012 at 01:56:43PM -0500, Jeff King wrote:

Unfortunately, the patch below which does that seems to make t7407 very
unhappy. It looks like the submodule test uses "git clone ." and
"git-submodule add" expects the "/." to still be at the end of the
configured URL when processing relative submodule paths. I'm not sure if
that is important, or an unnecessary brittleness in the submodule code.

Jens, Heiko?

After some analysis it seems to me that the test deviates from the
expected behavior. For relative urls we have documented that if we have
a remote in the superproject a relative submodule path is relative to that
remotes url.

In the test super has been cloned from ".". So the tests root directory
should be the directory the submodule path is relative to. That would
be ./submodule (since submodule is also in the root directory) and not
../submodule.

Before your patch a "/." was added to the origin of super and "/." is
currently counted as a path component.

So we have another corner case here: When your superproject was cloned
from "." the urls you currently have to specify with submodule add are
wrong (one ".." to much).

Since this is a change in behaviour I would like to further think about
the implications this brings if we fix this. Not sure how many people
clone from ".". The correct behavior (as documented) is the one you
introduce with your patch. If we decide to fix this we should also correct
the path calculation in git-submodule.sh.

Cheers Heiko

Hi,

On Fri, Nov 09, 2012 at 07:42:26PM +0100, Heiko Voigt wrote:

Since this is a change in behaviour I would like to further think about
the implications this brings if we fix this. Not sure how many people
clone from ".". The correct behavior (as documented) is the one you
introduce with your patch. If we decide to fix this we should also correct
the path calculation in git-submodule.sh.

Ok I think this corner case is not that commonly used since most people
work with remote remotes which you can not cd into to clone from ".".

Here is a patch series to clean this handling up.

Cheers Heiko

Heiko Voigt (3):
  Fix relative submodule setup of submodule tests
  ensure that relative submodule url needs ./ or ../
  fix corner case for relative submodule path calculation

 git-submodule.sh             | 22 +++++++++++++++++
 t/t7400-submodule-basic.sh   | 56 ++++++++++++++++++++++++++++++++++++++++++++
 t/t7403-submodule-sync.sh    |  2 ++
 t/t7406-submodule-update.sh  |  2 ++
 t/t7407-submodule-foreach.sh |  2 ++
 t/t7506-status-submodule.sh  |  2 ++
 6 files changed, 86 insertions(+)

-- 
1.8.0.3.gaed4666

If a remote is configured in a superproject relative submodule urls
should be relative to that remote. Since we have a bug in relative
path calculation for superproject paths that contain a "/." using
../submodule was accepted here. We are going to fix this behavior so
we first need to correct these tests.

Later tests expect the submodules origin to be in a directory underneath
the tests root. Lets remove the origin from super (which points directly
at the tests root directory) to keep these tests expectations.

Signed-off-by: Heiko Voigt <redacted>
---
 t/t7403-submodule-sync.sh    | 2 ++
 t/t7406-submodule-update.sh  | 2 ++
 t/t7407-submodule-foreach.sh | 2 ++
 t/t7506-status-submodule.sh  | 2 ++
 4 files changed, 8 insertions(+)

diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 524d5c1..b310a58 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh

@@ -18,6 +18,8 @@ test_expect_success setup '
 	git clone . super &&
 	git clone super submodule &&
 	(cd super &&
+	 # relative submodule urls relate to this folder not the remotes
+	 git remote rm origin &&
 	 git submodule add ../submodule submodule &&
 	 test_tick &&
 	 git commit -m "submodule"

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 1542653..f3628c9 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh

@@ -32,6 +32,8 @@ test_expect_success 'setup a submodule tree' '
 	git clone super merging &&
 	git clone super none &&
 	(cd super &&
+	 # relative submodule urls relate to this folder not the remotes
+	 git remote rm origin &&
 	 git submodule add ../submodule submodule &&
 	 test_tick &&
 	 git commit -m "submodule" &&

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 9b69fe2..99956a6 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh

@@ -21,6 +21,8 @@ test_expect_success 'setup a submodule tree' '
 	git clone super submodule &&
 	(
 		cd super &&
+		# relative submodule urls relate to this folder not the remotes
+		git remote rm origin &&
 		git submodule add ../submodule sub1 &&
 		git submodule add ../submodule sub2 &&
 		git submodule add ../submodule sub3 &&

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34d..9021b1a 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh

@@ -203,6 +203,8 @@ test_expect_success 'status with merge conflict in .gitmodules' '
 	test_create_repo_with_commit sub2 &&
 	(
 		cd super &&
+		# relative submodule urls relate to this folder not the remotes
+		git remote rm origin &&
 		prev=$(git rev-parse HEAD) &&
 		git checkout -b add_sub1 &&
 		git submodule add ../sub1 &&

-- 
1.8.0.3.gaed4666

Even though a relative path can be without them the
documentation explicitely talks about them. Lets ensure
that behavior with a test.

Signed-off-by: Heiko Voigt <redacted>
---
 t/t7400-submodule-basic.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5397037..3c2afa6 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh

@@ -506,6 +506,18 @@ test_expect_success 'set up for relative path tests' '
 	)
 '
 
+test_expect_success 'subrepo is NOT considered a relative path"' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config -f .gitmodules submodule.sub.url "subrepo" &&
+		git config remote.origin.url "$submodurl" &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = subrepo
+	)
+'
+
 test_expect_success '../subrepo works with URL - ssh://hostname/repo' '
 	(
 		cd reltest &&

-- 
1.8.0.3.gaed4666

A trailing /. for the superprojects origin is treated as
a full path component. This is wrong. Lets add a test and
fix this.

Signed-off-by: Heiko Voigt <redacted>
---
 git-submodule.sh           | 22 ++++++++++++++++++++++
 t/t7400-submodule-basic.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index ab6b110..9f61a9c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -69,6 +69,28 @@ resolve_relative_url ()
 		;;
 	esac
 
+	# strip one dot path components
+	tempurl="$remoteurl"
+	remoteurl=
+	sep=
+	while test -n "$tempurl"
+	do
+		case "$tempurl" in
+		*/.)
+			tempurl="${tempurl%/.}"
+			;;
+		?*/*)
+			remoteurl="${tempurl##*/}$sep$remoteurl"
+			tempurl="${tempurl%/*}"
+			sep=/
+		;;
+		*)
+			remoteurl="$tempurl$sep$remoteurl"
+			tempurl=
+			;;
+		esac
+	done
+
 	while test -n "$url"
 	do
 		case "$url" in

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 3c2afa6..1b4cc00 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh

@@ -518,6 +518,50 @@ test_expect_success 'subrepo is NOT considered a relative path"' '
 	)
 '
 
+test_expect_success '../subrepo works with absolute local path - "$submodurl/repo/."' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url "$submodurl/repo/." &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = "$submodurl/subrepo"
+	)
+'
+
+test_expect_success '../subrepo works with absolute local path - "$submodurl/repo/./"' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url "$submodurl/repo/./" &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = "$submodurl/subrepo"
+	)
+'
+
+test_expect_success '../subrepo works with absolute local path - "$submodurl/./repo/."' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url "$submodurl/./repo/." &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = "$submodurl/subrepo"
+	)
+'
+
+test_expect_success '../subrepo works with absolute local path - "$submodurl/././repo/."' '
+	(
+		cd reltest &&
+		cp pristine-.git-config .git/config &&
+		cp pristine-.gitmodules .gitmodules &&
+		git config remote.origin.url "$submodurl/././repo/." &&
+		git submodule init &&
+		test "$(git config submodule.sub.url)" = "$submodurl/subrepo"
+	)
+'
+
 test_expect_success '../subrepo works with URL - ssh://hostname/repo' '
 	(
 		cd reltest &&

-- 
1.8.0.3.gaed4666