Re: [BUG] git-submodule has bash-ism?

From: John Keeping <john@keeping.me.uk>
Date: Jun 1, 2016, 18:31 UTC
Message-ID: <20160601183100.GN1355@john.keeping.me.uk>
In-Reply-To: <20160601163747.GA10721@sigill.intra.peff.net>

On Wed, Jun 01, 2016 at 12:37:47PM -0400, Jeff King wrote:

Show 27 quoted lines

> On Wed, Jun 01, 2016 at 09:19:06AM -0700, Junio C Hamano wrote:
> > diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
> > index 79bc135..5503ec0 100755
> > --- a/t/t7403-submodule-sync.sh
> > +++ b/t/t7403-submodule-sync.sh
> > @@ -62,13 +62,13 @@ test_expect_success 'change submodule' '
> >  '
> >  
> >  reset_submodule_urls () {
> > -	local root
> > -	root=$(pwd) &&
> >  	(
> > +		root=$(pwd) &&
> >  		cd super-clone/submodule &&
> >  		git config remote.origin.url "$root/submodule"
> >  	) &&
> >  	(
> > +		root=$(pwd) &&
> >  		cd super-clone/submodule/sub-submodule &&
> >  		git config remote.origin.url "$root/submodule"
> 
> Hmm. Isn't $root always just going to be $TRASH_DIRECTORY here? There's
> only one caller, which appears to pass an argument which is ignored (?).
> 
> It's probably worth doing the minimal thing now and leaving further
> cleanup for a separate patch, though. Cc-ing John Keeping, the author of
> 091a6eb0feed, which added this code.

I can't shed any light on what this is trying to do; I had a look through the mailing list and this arrived in the final version of the series without any comment.

Looking at it now I can't see why this is a separate function (that is called with a parameter it never uses). I wonder if my original approach was to call this via test_when_finished from the two tests following this function definition, but that's pure speculation now.

Junio's change is obviously correct as a minimal fix.

I wonder if it's relevant that the "local root" line isn't &&-chained? Is it possible that on some shells we ignore an error but everything still works?