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

From: David Aguilar <davvid@gmail.com>
Date: Sep 25, 2008, 11:38 UTC
Message-ID: <20080925113849.GA18543@gmail.com>
In-Reply-To: <20080924161902.GT3669@spearce.org>

On  0, "Shawn O. Pearce" <spearce@spearce.org> wrote:

Show 21 quoted lines

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

Show 24 quoted lines

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