Re: [PATCH] Fix submodule sync with relative submodule URLs

[David Aguilar <davvid@gmail.com>]

On  0, "Shawn O. Pearce" <spearce@spearce.org> wrote:
> Johan Herland <johan@herland.net> wrote:
> > On Wednesday 24 September 2008, David Aguilar wrote:
> > > Instead of just doing an "|| exit" shouldn't it report an explanation
> > > of the error?
> > > Other than that, it looks good to me.
> > 
> > Fixed. Thanks.
> 
> OK, time for the drive-by patch commenting.  I've largely stayed
> out of git-submodule related code, but I just looked at in the
> context of applying this patch.
> 
> There are three callers to resolve_relative_url in master and next.
> All three callers just "|| exit" when resolve_relative_url fails.
> 
> The only reason resolve_relative_url can fail is when there is no
> remote.$remote.url configuration option set for the current default
> remote ("origin"?).
> 
> I guess I'm unclear about why cmd_sync is different from the
> existing callers.

Right, it's not.  resolve_relative_url() is already calling
die() when that error condition is met, so the "|| exit" thing
can be removed entirely.  I sent a patch on top of Johan's
first patch to remove the "|| exit" calls in all callers of
resolve_relative_url.  That seems like the right thing to do;
in the very least it makes it easier to read.  What do you
think?




> 
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 1c39b59..f89bdbe 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -634,6 +634,15 @@ cmd_sync()
> >  	do
> >  		name=$(module_name "$path")
> >  		url=$(git config -f .gitmodules --get submodule."$name".url)
> > +
> > +		# Possibly a url relative to parent
> > +		case "$url" in
> > +		./*|../*)
> > +			url=$(resolve_relative_url "$url") ||
> > +				die "failed to resolve relative submodule url for '$name'"
> > +			;;
> > +		esac
> > +
> >  		if test -e "$path"/.git
> >  		then
> >  		(
> 
> -- 
> Shawn.

-- 

	David