Re: ''git submodule sync'' should not add uninitialized submodules to .git/config

Jens Lehmann [off-list ref] writes:

I agree that 33f072f introduced a regression. One could argue if it was
a good idea to let "git submodule init" not do the clone itself but defer
it to "git submodule update" by setting the url in .git/config, but that's
the way things are done now (and maybe there was a very good reason to do
it that way I'm not aware of, because I didn't follow the list that closely
back then).

Actually, shouldn't the fix be more like this patch, which is directly on
top of 33f072f?  I think this is more in line with what the end user wants
to tell the system with "submodule init", namely "I am interested in this
submodule".

-- >8 --
Subject: submodule sync: do not auto-vivify uninteresting submodule

Earlier 33f072f (submodule sync: Update "submodule.<name>.url" for empty
directories, 2010-10-08) attempted to fix a bug where "git submodule sync"
command does not update the URL if the current superproject does not have
a checkout of the submodule.

However, it did so by unconditionally registering submodule.$name.url to
every submodule in the project, even the ones that the user has never
showed interest in at all by running 'git submodule init' command. This
caused subsequent 'git submodule update' to start cloning/updating submodules
that are not interesting to the user at all.

Update the code so that the URL is updated from the .gitmodules file only
for submodules that already have submodule.$name.url entries, i.e. the
ones the user has showed interested in having a checkout.

Signed-off-by: Junio C Hamano <redacted>
---

 git-submodule.sh          |   23 +++++++++++++----------
 t/t7403-submodule-sync.sh |   13 +++++++++++--
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index c291eed..ce65971 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -836,17 +836,20 @@ cmd_sync()
 			;;
 		esac
 
-		say "Synchronizing submodule url for '$name'"
-		git config submodule."$name".url "$url"
-
-		if test -e "$path"/.git
+		if git config "submodule.$name.url" >/dev/null 2>/dev/null
 		then
-		(
-			clear_local_git_env
-			cd "$path"
-			remote=$(get_default_remote)
-			git config remote."$remote".url "$url"
-		)
+			say "Synchronizing submodule url for '$name'"
+			git config submodule."$name".url "$url"
+
+			if test -e "$path"/.git
+			then
+			(
+				clear_local_git_env
+				cd "$path"
+				remote=$(get_default_remote)
+				git config remote."$remote".url "$url"
+			)
+			fi
 		fi
 	done
 }

diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index e5b1953..597a06f 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh

@@ -25,7 +25,8 @@ test_expect_success setup '
 	git clone super super-clone &&
 	(cd super-clone && git submodule update --init) &&
 	git clone super empty-clone &&
-	(cd empty-clone && git submodule init)
+	(cd empty-clone && git submodule init) &&
+	git clone super top-only-clone
 '
 
 test_expect_success 'change submodule' '

@@ -66,7 +67,7 @@ test_expect_success '"git submodule sync" should update submodule URLs' '
 	)
 '
 
-test_expect_success '"git submodule sync" should update submodule URLs if not yet cloned' '
+test_expect_success '"git submodule sync" should update known submodule URLs' '
 	(cd empty-clone &&
 	 git pull &&
 	 git submodule sync &&

@@ -74,4 +75,12 @@ test_expect_success '"git submodule sync" should update submodule URLs if not ye
 	)
 '
 
+test_expect_success '"git submodule sync" should not vivify uninteresting submodule' '
+	(cd top-only-clone &&
+	 git pull &&
+	 git submodule sync &&
+	 test -z "$(git config submodule.submodule.url)"
+	)
+'
+
 test_done

Am 24.06.2011 00:28, schrieb Junio C Hamano:
---8<---

Subject: submodule sync: do not auto-vivify uninteresting submodule

Earlier 33f072f (submodule sync: Update "submodule.<name>.url" for empty
directories, 2010-10-08) attempted to fix a bug where "git submodule sync"
command does not update the URL if the current superproject does not have
a checkout of the submodule.

However, it did so by unconditionally registering submodule.$name.url to
every submodule in the project, even the ones that the user has never
showed interest in at all by running 'git submodule init' command. This
caused subsequent 'git submodule update' to start cloning/updating submodules
that are not interesting to the user at all.

Update the code so that the URL is updated from the .gitmodules file only
for submodules that already have submodule.$name.url entries, i.e. the
ones the user has showed interested in having a checkout.

---8<---

I think that describes the situation pretty well and the patch looks
good. I will not be able to test it before next week though, do not wait
for me.

There are two minor issues for me left.
(1) The man page of submodule sync talks about "all submodules" and not
"registered submodules". I treated it as "subsequent init" and
personally do not restrict the list of submodules, simply because my
build would not work without them.

(2) It is confusing to have registered submodules in .git/config without
a matching gitlink. Say, you switch between branch a1 with a submodule s
with url u1, to a branch without s, you git clean -xdff (url u1 is still
in .git/config, right?), and then to a branch a2 with s pointing to url
u2. This bugged me.

Ciao,
-- andi5

Am 24.06.2011 01:10, schrieb Andreas Köhler:

There are two minor issues for me left.
(1) The man page of submodule sync talks about "all submodules" and not
"registered submodules". I treated it as "subsequent init" and
personally do not restrict the list of submodules, simply because my
build would not work without them.

While investigating this I stumbled across that too, will post a patch.

(2) It is confusing to have registered submodules in .git/config without
a matching gitlink. Say, you switch between branch a1 with a submodule s
with url u1, to a branch without s, you git clean -xdff (url u1 is still
in .git/config, right?), and then to a branch a2 with s pointing to url
u2. This bugged me.

I think that is ok, because that way git remembers that the user cares
about submodule s even when he switches to a branch where s doesn't
exist. And the documentation is pretty clear that you'll have to use
"git submodule sync" to update the url itself from u1 to u2 when needed.

From: Junio C Hamano <redacted>

Earlier 33f072f (submodule sync: Update "submodule.<name>.url" for empty
directories, 2010-10-08) attempted to fix a bug where "git submodule sync"
command does not update the URL if the current superproject does not have
a checkout of the submodule.

However, it did so by unconditionally registering submodule.$name.url to
every submodule in the project, even the ones that the user has never
showed interest in at all by running 'git submodule init' command. This
caused subsequent 'git submodule update' to start cloning/updating submodules
that are not interesting to the user at all.

Update the code so that the URL is updated from the .gitmodules file only
for submodules that already have submodule.$name.url entries, i.e. the
ones the user has showed interested in having a checkout.

Acked-by: Jens Lehmann <redacted>
Signed-off-by: Junio C Hamano <redacted>
---

Am 24.06.2011 00:28, schrieb Junio C Hamano:

Actually, shouldn't the fix be more like this patch, which is directly on
top of 33f072f?  I think this is more in line with what the end user wants
to tell the system with "submodule init", namely "I am interested in this
submodule".

Yes, I am convinced your patch is the doing Right Thing. I squashed in a
change to the git submodule sync documentation (explicitly stating that a
sync only affects submodules which have an url entry in .git/config) and
two more lines in the test you added to make sure that explicitly giving
a submodule on the command line uses the same logic.

What do you think?

(I'll send another patch shortly addressing the failing tests when
jl/submodule-add-relurl-wo-upstream is merged into this one)


 Documentation/git-submodule.txt |    4 +++-
 git-submodule.sh                |   23 +++++++++++++----------
 t/t7403-submodule-sync.sh       |   15 +++++++++++++--
 3 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 5e7a413..2b31d5f 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt

@@ -167,7 +167,9 @@ commit for each submodule.

 sync::
 	Synchronizes submodules' remote URL configuration setting
-	to the value specified in .gitmodules.  This is useful when
+	to the value specified in .gitmodules. It will only affect those
+	submodules which already have an url entry in .git/config (that is the
+	case when they are initialized or freshly added). This is useful when
 	submodule URLs change upstream and you need to update your local
 	repositories accordingly.
 +

diff --git a/git-submodule.sh b/git-submodule.sh
index d189a24..543f1d0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -874,17 +874,20 @@ cmd_sync()
 			;;
 		esac

-		say "Synchronizing submodule url for '$name'"
-		git config submodule."$name".url "$url"
-
-		if test -e "$path"/.git
+		if git config "submodule.$name.url" >/dev/null 2>/dev/null
 		then
-		(
-			clear_local_git_env
-			cd "$path"
-			remote=$(get_default_remote)
-			git config remote."$remote".url "$url"
-		)
+			say "Synchronizing submodule url for '$name'"
+			git config submodule."$name".url "$url"
+
+			if test -e "$path"/.git
+			then
+			(
+				clear_local_git_env
+				cd "$path"
+				remote=$(get_default_remote)
+				git config remote."$remote".url "$url"
+			)
+			fi
 		fi
 	done
 }

diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index d600583..95ffe34 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh

@@ -25,7 +25,8 @@ test_expect_success setup '
 	git clone super super-clone &&
 	(cd super-clone && git submodule update --init) &&
 	git clone super empty-clone &&
-	(cd empty-clone && git submodule init)
+	(cd empty-clone && git submodule init) &&
+	git clone super top-only-clone
 '

 test_expect_success 'change submodule' '

@@ -66,7 +67,7 @@ test_expect_success '"git submodule sync" should update submodule URLs' '
 	)
 '

-test_expect_success '"git submodule sync" should update submodule URLs if not yet cloned' '
+test_expect_success '"git submodule sync" should update known submodule URLs' '
 	(cd empty-clone &&
 	 git pull &&
 	 git submodule sync &&

@@ -74,4 +75,14 @@ test_expect_success '"git submodule sync" should update submodule URLs if not ye
 	)
 '

+test_expect_success '"git submodule sync" should not vivify uninteresting submodule' '
+	(cd top-only-clone &&
+	 git pull &&
+	 git submodule sync &&
+	 test -z "$(git config submodule.submodule.url)" &&
+	 git submodule sync submodule &&
+	 test -z "$(git config submodule.submodule.url)"
+	)
+'
+
 test_done

-- 
1.7.6.rc3.1.g8fd6