On 03/14, Junio C Hamano wrote: > Brandon Williams <bmwill@xxxxxxxxxx> writes: > > > Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx> > > --- > > git-submodule.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/git-submodule.sh b/git-submodule.sh > > index ab233712d..e2d08595f 100755 > > --- a/git-submodule.sh > > +++ b/git-submodule.sh > > @@ -1111,7 +1111,7 @@ cmd_sync() > > ;; > > esac > > > > - if git config "submodule.$name.url" >/dev/null 2>/dev/null > > + if git submodule--helper is-active "$sm_path" > > then > > displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix") > > say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")" > > This is not a problem this patch introduces, but the loop this hunk > is in seems a bit inefficient. It maps the sm_path to its name and > then asks .gitmodules the URL the upstream suggests to clone it from, > munges it with a large case statement and discards all of that if > the module is not active. > > Adding this patch on top would be a way to remove the inefficiency > and one level of nesting while at it, I think, but I may have missed > something, so please double check, and if you agree that this is a > good way to go, please do so as a preparatory clean-up. > > Thanks. Yeah you're right, some of that work can be avoided. I'll do the code cleanup in a prep patch and then convert submodule sync to use the is-active subcommand. > > git-submodule.sh | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/git-submodule.sh b/git-submodule.sh > index e2d08595f0..dcdd36fa64 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -1089,6 +1089,12 @@ cmd_sync() > while read mode sha1 stage sm_path > do > die_if_unmatched "$mode" "$sha1" > + > + if ! git submodule--helper is-active "$sm_path" > + then > + continue > + fi > + > name=$(git submodule--helper name "$sm_path") > url=$(git config -f .gitmodules --get submodule."$name".url) > > @@ -1111,14 +1117,12 @@ cmd_sync() > ;; > esac > > - if git submodule--helper is-active "$sm_path" > - then > - displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix") > - say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")" > - git config submodule."$name".url "$super_config_url" > + displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix") > + say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")" > + git config submodule."$name".url "$super_config_url" > > - if test -e "$sm_path"/.git > - then > + if test -e "$sm_path"/.git > + then > ( > sanitize_submodule_env > cd "$sm_path" > @@ -1131,7 +1135,6 @@ cmd_sync() > eval cmd_sync > fi > ) > - fi > fi > done > } -- Brandon Williams