[v3] submodule: teach set-url subcommand
diff mbox series

Message ID f5f44812af2b43fe3f7ea837f8b07c4747eedcc0.1572368447.git.liu.denton@gmail.com
State New
Headers show
Series
  • [v3] submodule: teach set-url subcommand
Related show

Commit Message

Denton Liu Oct. 29, 2019, 5:01 p.m. UTC
Currently, in the event that a submodule's upstream URL changes, users
have to manually alter the URL in the .gitmodules file then run
`git submodule sync`. Let's make that process easier.

Teach submodule the set-url subcommand which will automatically change
the `submodule.$name.url` property in the .gitmodules file and then run
`git submodule sync` to complete the process.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Thanks for the review, Danh. I've taken all of your suggestions.

Range-diff against v2:
1:  d8841c6009 ! 1:  f5f44812af submodule: teach set-url subcommand
    @@ Documentation/git-submodule.txt: SYNOPSIS
      'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
      'git submodule' [--quiet] update [<options>] [--] [<path>...]
      'git submodule' [--quiet] set-branch [<options>] [--] <path>
    -+'git submodule' [--quiet] set-url [<options>] [--] <path> <newurl>
    ++'git submodule' [--quiet] set-url [--] <path> <newurl>
      'git submodule' [--quiet] summary [<options>] [--] [<path>...]
      'git submodule' [--quiet] foreach [--recursive] <command>
      'git submodule' [--quiet] sync [--recursive] [--] [<path>...]
    @@ t/t7420-submodule-set-url.sh (new)
     +		cd super &&
     +		test_must_fail git submodule update --remote &&
     +		git submodule set-url submodule ../newsubmodule &&
    -+		grep "url = \.\./newsubmodule" .gitmodules &&
    ++		grep -F "url = ../newsubmodule" .gitmodules &&
     +		git submodule update --remote
     +	) &&
     +	git -C super/submodule show >actual &&

 Documentation/git-submodule.txt        |  6 +++
 contrib/completion/git-completion.bash |  2 +-
 git-submodule.sh                       | 52 +++++++++++++++++++++++-
 t/t7420-submodule-set-url.sh           | 55 ++++++++++++++++++++++++++
 4 files changed, 113 insertions(+), 2 deletions(-)
 create mode 100755 t/t7420-submodule-set-url.sh

Comments

Junio C Hamano Oct. 30, 2019, 5:54 a.m. UTC | #1
Denton Liu <liu.denton@gmail.com> writes:

> Currently, in the event that a submodule's upstream URL changes, users
> have to manually alter the URL in the .gitmodules file then run
> `git submodule sync`. Let's make that process easier.

Right now, submodule.<name>.url might be the only thing that gets
synched down to .git/config of the submodule; we may not learn any
more things that needs "edit in .gitmodules and then run sync".  Can
we sately say that "sync" is now obsolete and what it has been used
for can now be done with "set-url"?  In other words, does "set-url"
makes "sync" unnecessary and deprecated?

Or is it expected that we would learn more things end users can edit
in .gitmodules and run sync to propagate necessary pieces of
information down?  If so, do we want to add an extra command like
set-url for each of these new things, or do we tell users "if you
are editing url, use set-url, otherwise edit .gitmodules and run
sync"?  If the former, that would make the set of subcommands quite
noisy, and if the latter, the users need to learn two things,
i.e. it is not making it easier but harder to learn the system.

There is nothing _wrong_ to introduce the new subcommand per-se, but
given that the URL should not change that often, and due to above
concerns, I am not sure if I want to back this change.
Denton Liu Oct. 30, 2019, 5:58 p.m. UTC | #2
Hi Junio,

On Wed, Oct 30, 2019 at 02:54:58PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > Currently, in the event that a submodule's upstream URL changes, users
> > have to manually alter the URL in the .gitmodules file then run
> > `git submodule sync`. Let's make that process easier.
> 
> Right now, submodule.<name>.url might be the only thing that gets
> synched down to .git/config of the submodule; we may not learn any
> more things that needs "edit in .gitmodules and then run sync".  Can
> we sately say that "sync" is now obsolete and what it has been used
> for can now be done with "set-url"?  In other words, does "set-url"
> makes "sync" unnecessary and deprecated?

"sync" isn't deprecated quite yet. If upstream edits the .gitmodules and
we pull in the change, then we still need to run "sync" to update our
.git/config.

> 
> Or is it expected that we would learn more things end users can edit
> in .gitmodules and run sync to propagate necessary pieces of
> information down?  If so, do we want to add an extra command like
> set-url for each of these new things, or do we tell users "if you
> are editing url, use set-url, otherwise edit .gitmodules and run
> sync"?  If the former, that would make the set of subcommands quite
> noisy, and if the latter, the users need to learn two things,
> i.e. it is not making it easier but harder to learn the system.

I was running under the assumption that we weren't planning on adding
new information to .gitmodules. I don't think we'll be making changes to
the submodule file format without careful thought so _if_ that ever
comes up, we should discuss it then but I don't think it's a discussion
that's going to happen any time soon, if ever.

Either way, though, the sync workflow isn't going away so users still
need to know how to do that (unfortunately). We'll just add a
convenience function on top to make people's lives easier but it's not
strictly _necessary_ to use "set-url".

> 
> There is nothing _wrong_ to introduce the new subcommand per-se, but
> given that the URL should not change that often, and due to above
> concerns, I am not sure if I want to back this change.

I think it changes often enough to cause problems. Seems like at least
936 people on StackOverflow agree with me[1].

I think that the problem is that it's a common enough use-case but not
common enough that people often forget to "sync" after editing the
.gitmodules. I know that it's bitten me the few times I've done this and
I've had to go back to that SO post each time.

[1]: https://stackoverflow.com/questions/913701/how-to-change-the-remote-repository-for-a-git-submodule
Junio C Hamano Nov. 2, 2019, 4:25 a.m. UTC | #3
Denton Liu <liu.denton@gmail.com> writes:

> I think that the problem is that it's a common enough use-case but not
> common enough that people often forget to "sync" after editing the
> .gitmodules.

Which means that far from deprecating "sync", we should strive for
imprinting the need for the use of the subcommand deeper in users'
minds.  I wonder if it makes sense to make sure that .gitmodules
file starts with something like

	# Note that after updating this file, you may want to
	# run "git submodule sync" so that the URL for the remote
	# repository etc. are propagated down.

when we create it (via, say "git submodule add")?  Or alternatively,
warn the users when "git status" notices a local modification, or
something along that line?

Patch
diff mbox series

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 1f46380af2..285486d0a8 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,6 +16,7 @@  SYNOPSIS
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] <path>...)
 'git submodule' [--quiet] update [<options>] [--] [<path>...]
 'git submodule' [--quiet] set-branch [<options>] [--] <path>
+'git submodule' [--quiet] set-url [--] <path> <newurl>
 'git submodule' [--quiet] summary [<options>] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
 'git submodule' [--quiet] sync [--recursive] [--] [<path>...]
@@ -180,6 +181,11 @@  set-branch (-d|--default) [--] <path>::
 	`--default` option removes the submodule.<name>.branch configuration
 	key, which causes the tracking branch to default to 'master'.
 
+set-url [--] <path> <newurl>::
+	Sets the URL of the specified submodule to <newurl>. Then, it will
+	automatically synchronize the submodule's new remote URL
+	configuration.
+
 summary [--cached|--files] [(-n|--summary-limit) <n>] [commit] [--] [<path>...]::
 	Show commit summary between the given commit (defaults to HEAD) and
 	working tree/index. For a submodule in question, a series of commits
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 00fbe6c03d..88c7446414 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2779,7 +2779,7 @@  _git_submodule ()
 {
 	__git_has_doubledash && return
 
-	local subcommands="add status init deinit update set-branch summary foreach sync absorbgitdirs"
+	local subcommands="add status init deinit update set-branch set-url summary foreach sync absorbgitdirs"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
 		case "$cur" in
diff --git a/git-submodule.sh b/git-submodule.sh
index c7f58c5756..f7374ddbd6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -12,6 +12,7 @@  USAGE="[--quiet] [--cached]
    or: $dashless [--quiet] deinit [-f|--force] (--all| [--] <path>...)
    or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] [--reference <repository>] [--recursive] [--] [<path>...]
    or: $dashless [--quiet] set-branch (--default|--branch <branch>) [--] <path>
+   or: $dashless [--quiet] set-url [--] <path> <newurl>
    or: $dashless [--quiet] summary [--cached|--files] [--summary-limit <n>] [commit] [--] [<path>...]
    or: $dashless [--quiet] foreach [--recursive] <command>
    or: $dashless [--quiet] sync [--recursive] [--] [<path>...]
@@ -760,6 +761,55 @@  cmd_set_branch() {
 	fi
 }
 
+#
+# Configures a submodule's remote url
+#
+# $@ = requested path, requested url
+#
+cmd_set_url() {
+	while test $# -ne 0
+	do
+		case "$1" in
+		-q|--quiet)
+			GIT_QUIET=1
+			;;
+		--)
+			shift
+			break
+			;;
+		-*)
+			usage
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+
+	if test $# -ne 2
+	then
+		usage
+	fi
+
+	# we can't use `git submodule--helper name` here because internally, it
+	# hashes the path so a trailing slash could lead to an unintentional no match
+	name="$(git submodule--helper list "$1" | cut -f2)"
+	if test -z "$name"
+	then
+		exit 1
+	fi
+
+	url="$2"
+	if test -z "$url"
+	then
+		exit 1
+	fi
+
+	git submodule--helper config submodule."$name".url "$url"
+	git submodule--helper sync ${GIT_QUIET:+--quiet} "$name"
+}
+
 #
 # Show commit summary for submodules in index or working tree
 #
@@ -1059,7 +1109,7 @@  cmd_absorbgitdirs()
 while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add | foreach | init | deinit | update | set-branch | status | summary | sync | absorbgitdirs)
+	add | foreach | init | deinit | update | set-branch | set-url | status | summary | sync | absorbgitdirs)
 		command=$1
 		;;
 	-q|--quiet)
diff --git a/t/t7420-submodule-set-url.sh b/t/t7420-submodule-set-url.sh
new file mode 100755
index 0000000000..ef0cb6e8e1
--- /dev/null
+++ b/t/t7420-submodule-set-url.sh
@@ -0,0 +1,55 @@ 
+#!/bin/sh
+#
+# Copyright (c) 2019 Denton Liu
+#
+
+test_description='Test submodules set-url subcommand
+
+This test verifies that the set-url subcommand of git-submodule is working
+as expected.
+'
+
+TEST_NO_CREATE_REPO=1
+. ./test-lib.sh
+
+test_expect_success 'submodule config cache setup' '
+	mkdir submodule &&
+	(
+		cd submodule &&
+		git init &&
+		echo a >file &&
+		git add file &&
+		git commit -ma
+	) &&
+	mkdir super &&
+	(
+		cd super &&
+		git init &&
+		git submodule add ../submodule &&
+		git commit -m "add submodule"
+	)
+'
+
+test_expect_success 'test submodule set-url' '
+	# add a commit and move the submodule (change the url)
+	(
+		cd submodule &&
+		echo b >>file &&
+		git add file &&
+		git commit -mb
+	) &&
+	mv submodule newsubmodule &&
+
+	git -C newsubmodule show >expect &&
+	(
+		cd super &&
+		test_must_fail git submodule update --remote &&
+		git submodule set-url submodule ../newsubmodule &&
+		grep -F "url = ../newsubmodule" .gitmodules &&
+		git submodule update --remote
+	) &&
+	git -C super/submodule show >actual &&
+	test_cmp expect actual
+'
+
+test_done