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

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

[David Aguilar]

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(-)

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

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

[David Aguilar]

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(-)

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

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> @@ -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?

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

[Junio C Hamano]

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.

> +#
> +# 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.

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

[Mark Levedahl]

Junio C Hamano wrote:
>>  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

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

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> 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

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

[Mark Levedahl]

Junio C Hamano wrote:
>> 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

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

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> 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.