threads / patch / 15154

patch, 2 partsgit-submodule: replace duplicated code with a module_list function

Subject: [PATCH 1/2] git-submodule: replace duplicated code with a module_list function

## tl;dr

8 messages between Aug 22, 2008 and Aug 23, 2008. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

David Aguilar· Aug 22, 2008, 07:30 UTC · lore

Several call sites in git-submodule.sh used the same idiom for getting submodule information:

	git ls-files --stage -- "$@" | grep '^160000 '
This patch removes this duplication by introducing a module_list function.
Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-submodule.sh |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)
Show changes to git-submodule.sh +13 −4
diff --git a/git-submodule.sh b/git-submodule.sh
index 2d57d60..2a3a197 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -54,6 +54,15 @@ resolve_relative_url ()
 }
 
 #
+# Get submodule info for registered submodules
+# $@ = path to limit submodule list
+#
+module_list()
+{
+	git ls-files --stage -- "$@" | grep '^160000 '
+}
+
+#
 # Map submodule path to submodule name
 #
 # $1 = path
@@ -206,7 +215,7 @@ cmd_add()
 #
 cmd_foreach()
 {
-	git ls-files --stage | grep '^160000 ' |
+	module_list |
 	while read mode sha1 stage path
 	do
 		if test -e "$path"/.git
@@ -246,7 +255,7 @@ cmd_init()
 		shift
 	done
 
-	git ls-files --stage -- "$@" | grep '^160000 ' |
+	module_list "$@" |
 	while read mode sha1 stage path
 	do
 		# Skip already registered paths
@@ -304,7 +313,7 @@ cmd_update()
 		esac
 	done
 
-	git ls-files --stage -- "$@" | grep '^160000 ' |
+	module_list "$@" |
 	while read mode sha1 stage path
 	do
 		name=$(module_name "$path") || exit
@@ -569,7 +578,7 @@ cmd_status()
 		shift
 	done
 
-	git ls-files --stage -- "$@" | grep '^160000 ' |
+	module_list "$@" |
 	while read mode sha1 stage path
 	do
 		name=$(module_name "$path") || exit

-- 
1.6.0.90.g436ed
David Aguilar· Aug 22, 2008, 07:30 UTC · re: David Aguilar · lore

[PATCH 2/2] git-submodule: add "sync" command

When a submodule's URL changes upstream, existing submodules will be out of sync since their remote.origin.url will still be set to the old value.

This change adds a "git submodule sync" command that reads the submodule URLs from .gitmodules and updates any existing submodules accordingly.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Documentation/git-submodule.txt |    7 ++++++
 git-submodule.sh                |   45 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 2 deletions(-)
Show changes to 2 files +50 −2

Documentation/git-submodule.txt, git-submodule.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index abbd5b7..a229032 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -15,6 +15,7 @@ SYNOPSIS
 'git submodule' [--quiet] update [--init] [--] [<path>...]
 'git submodule' [--quiet] summary [--summary-limit <n>] [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach <command>
+'git submodule' [--quiet] sync [--] [<path>...]
 
 
 DESCRIPTION
@@ -139,6 +140,12 @@ foreach::
 As an example, "git submodule foreach 'echo $path `git rev-parse HEAD`' will
 show the path and currently checked out commit for each submodule.
 
+sync::
+	Synchronizes each submodule's remote.origin.url configuration
+	setting to match that of the corresponding submodule.*.url
+	value as specified in .gitmodules.  This is useful if your
+	submodule URLs have changed upstream and you want to update your
+	local repositories accordingly.
 
 OPTIONS
 -------
diff --git a/git-submodule.sh b/git-submodule.sh
index 2a3a197..46739de 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -6,7 +6,7 @@
 
 USAGE="[--quiet] [--cached] \
 [add <repo> [-b branch] <path>]|[status|init|update [-i|--init]|summary [-n|--summary-limit <n>] [<commit>]] \
-[--] [<path>...]|[foreach <command>]"
+[--] [<path>...]|[foreach <command>]|[sync [<path>...]]"
 OPTIONS_SPEC=
 . git-sh-setup
 require_work_tree
@@ -602,6 +602,47 @@ cmd_status()
 		fi
 	done
 }
+#
+# Sync git urls for submodules
+# This makes the value for remote.origin.url match the value
+# specified in .gitmodules.
+#
+cmd_sync()
+{
+	while test $# -ne 0
+	do
+		case "$1" in
+		-q|--quiet)
+			quiet=1
+			;;
+		-*)
+			usage
+			;;
+		--)
+			shift
+			break
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done
+
+	cd_to_toplevel
+
+	module_list "$@" |
+	while read mode sha1 stage path
+	do
+		! test -f "$path"/.git/config &&
+		echo "Warn: submodule at path '$path' does not exist."
+		test -f "$path"/.git/config || continue
+		name=$(module_name "$path")
+		url=$(git config -f .gitmodules --get submodule."$name".url)
+		say "Synchronizing submodule url for '$name'"
+		git config -f "$path"/.git/config remote.origin.url "$url"
+	done
+}
 
 # This loop parses the command line arguments to find the
 # subcommand name to dispatch.  Parsing of the subcommand specific
@@ -612,7 +653,7 @@ cmd_status()
 while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add | foreach | init | update | status | summary)
+	add | foreach | init | update | status | summary | sync)
 		command=$1
 		;;
 	-q|--quiet)

-- 
1.6.0.90.g436ed
Junio C Hamano· Aug 22, 2008, 23:13 UTC · re: David Aguilar · lore

Re: [PATCH 2/2] git-submodule: add "sync" command

David Aguilar <davvid@gmail.com> writes:
> When a submodule's URL changes upstream, existing submodules
> will be out of sync since their remote.origin.url will still
> be set to the old value.
Ok.
Show 41 quoted lines
> +#
> +# Sync git urls for submodules
> +# This makes the value for remote.origin.url match the value
> +# specified in .gitmodules.
> +#
> +cmd_sync()
> +{
> +	while test $# -ne 0
> +	do
> +		case "$1" in
> +		-q|--quiet)
> +			quiet=1
> +			;;
> +		-*)
> +			usage
> +			;;
> +		--)
> +			shift
> +			break
> +			;;
> +		*)
> +			break
> +			;;
> +		esac
> +		shift
> +	done
> +
> +	cd_to_toplevel
> +
> +	module_list "$@" |
> +	while read mode sha1 stage path
> +	do
> +		! test -f "$path"/.git/config &&
> +		echo "Warn: submodule at path '$path' does not exist."
> +		test -f "$path"/.git/config || continue
> +		name=$(module_name "$path")
> +		url=$(git config -f .gitmodules --get submodule."$name".url)
> +		say "Synchronizing submodule url for '$name'"
> +		git config -f "$path"/.git/config remote.origin.url "$url"
> +	done
> +}
Hmm.  I do not quite like this.

We allow using a gitfile for submodule $GIT_DIR; checking for file existence of "$path/.git/config" does not work for such a configuration.

Neither does 'git config -f "$path/.git/config"' work. I think it should be more like:

    ( unset GIT_DIR; cd "$path" && git config remote.origin.url "$url" )

It is a norm not to have any interest in a submodule that appears in the index of a superproject. You shouldn't get as many warnings as you have submodules in such a repository. You should just skip them (which you already do, but see above), or at least honor "$quiet".

I also think the warning should be sent to the standard error stream.
Junio C Hamano· Aug 22, 2008, 22:53 UTC · re: David Aguilar · lore

Re: [PATCH 1/2] git-submodule: replace duplicated code with a module_list function

David Aguilar <davvid@gmail.com> writes:
Show 22 quoted lines
> @@ -54,6 +54,15 @@ resolve_relative_url ()
>  }
>  
>  #
> +# Get submodule info for registered submodules
> +# $@ = path to limit submodule list
> +#
> +module_list()
> +{
> +	git ls-files --stage -- "$@" | grep '^160000 '
> +}
> +
> +#
>  # Map submodule path to submodule name
>  #
>  # $1 = path
> @@ -206,7 +215,7 @@ cmd_add()
>  #
>  cmd_foreach()
>  {
> -	git ls-files --stage | grep '^160000 ' |
> +	module_list |
Thanks.

I think the original "foreach" implementation does not pay attention to "$@" not by design but by mistake, and we should pass "$@" here as well.

Other than that I do not see anything obviously wrong with the patch.
Mark?
Mark Levedahl· Aug 23, 2008, 00:01 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] git-submodule: replace duplicated code with a module_list function

Junio C Hamano wrote:
Show 16 quoted lines
>>  cmd_foreach()
>>  {
>> -	git ls-files --stage | grep '^160000 ' |
>> +	module_list |
>>     
>
> Thanks.
>
> I think the original "foreach" implementation does not pay attention to
> "$@" not by design but by mistake, and we should pass "$@" here as well.
>
> Other than that I do not see anything obviously wrong with the patch.
>
> Mark?
>
>

Actually, this was by design, not mistake, though we did not discuss this at all. I'm not sure what the semantics would / should be: first of all, some part of "$@" is the command to be executed in each submodule, and as written "$@" in its entirety is what is used. Also, as written and documented, foreach operates in each checked out submodule, not a subset. I guess the basic questions are: a) What specific option or options to git ls-files makes sense here? b) How do we distinguish the ls-files options from the command to be executed?

Mark
Junio C Hamano· Aug 23, 2008, 00:08 UTC · re: Mark Levedahl · lore

Re: [PATCH 1/2] git-submodule: replace duplicated code with a module_list function

Mark Levedahl <mlevedahl@gmail.com> writes:
Show 9 quoted lines
> Actually, this was by design, not mistake, though we did not discuss
> this at all. I'm not sure what the semantics would / should be: first
> of all, some part of "$@" is the command to be executed in each
> submodule, and as written "$@" in its entirety is what is used. Also,
> as written and documented, foreach operates in each checked out
> submodule, not a subset. I guess the basic questions are:
> a)  What specific option or options to git ls-files makes sense here?
> b) How do we distinguish the ls-files options from the command to be
> executed?

Ah, I was blind. For (a) I do not see any need for "option" but pathspecs; and (b) I agree is a real problem. We of course could do something like:

    $ git submodule foreach -c 'your command here' your pathspec here
Mark Levedahl· Aug 23, 2008, 00:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] git-submodule: replace duplicated code with a module_list function

Junio C Hamano wrote:
Show 12 quoted lines
>> a)  What specific option or options to git ls-files makes sense here?
>> b) How do we distinguish the ls-files options from the command to be
>> executed?
>>     
>
> Ah, I was blind.  For (a) I do not see any need for "option" but
> pathspecs; and (b) I agree is a real problem.  We of course could do
> something like:
>
>     $ git submodule foreach -c 'your command here' your pathspec here
>
>

As the command is required, while pathspec is optional, the latter should require the option, not the former. How about...

    $ git submodule foreach [-l pathspec] 'command'
or
    $ git submodule foreach [<pathspec> --] 'command'
Mark
Junio C Hamano· Aug 23, 2008, 01:01 UTC · re: Mark Levedahl · lore

Re: [PATCH 1/2] git-submodule: replace duplicated code with a module_list function

Mark Levedahl <mlevedahl@gmail.com> writes:
Show 8 quoted lines
> As the command is required, while pathspec is optional, the latter
> should require the option, not the former. How about...
>
>    $ git submodule foreach [-l pathspec] 'command'
>
> or
>
>    $ git submodule foreach [<pathspec> --] 'command'

Neither would fly well. For all normal git command, pathspec comes after the double dash.

And I think it is probably a mistake to think "the command is required" as anything holy. "submodule foreach" looks very much like an index aware "find". Perhaps we could even make the lack of 'command' to default to "echo its name", just like "find ." and "find . -print" are equivalent (I am not seriously suggesting this, though. Read on).

Having said all that, I think you can have best of both worlds by doing something like this:

 - If you do not have -c 'command' or any option, then everything is
   command and you cannot limit with pathspecs.  This is the original
   syntax.
 - If you do have options to "foreach", then -c 'command' string is the
   command, and perhaps the foreach subcommand will gain other options
   over time, including possible -- pathspec separator.  The remainder of
   the command line after options are processed are pathspecs that limit
   the ls-files.

I do not think it is a defect that we do not allow pathspec limited foreach subcommand. Teaching foreach to take pathspecs is an independent topic if somebody has such an itch.

As a conclusion of this discussion, I think David's [1/2] is fine as-is.
Thanks.

← back to recent threads