Re: [PATCH v3 03/10] submodule sync: use submodule--helper is-active

From: Brandon Williams <bmwill@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, sbeller@google.com
Subject: Re: [PATCH v3 03/10] submodule sync: use submodule--helper is-active
Date: Tue, 14 Mar 2017 16:50:36 -0700	[thread overview]
Message-ID: <20170314235036.GF168037@google.com> (raw)
In-Reply-To: <xmqq8to7kamv.fsf@gitster.mtv.corp.google.com>

On 03/14, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  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