[PATCH] Avoid unportable nested double- and backquotes in shell scripts.

[PATCH] Avoid unportable nested double- and backquotes in shell scripts.

[Ralf Wildenhues]

Some shells parse them wrongly, esp. pdksh.  On the other hand,
the right-hand side of assignments is not word-split, so they
can be used as workarounds.

Signed-off-by: Ralf Wildenhues <Ralf.Wildenhues@gmx.de>
---
 contrib/examples/git-fetch.sh |    2 +-
 t/t9107-git-svn-migrate.sh    |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/examples/git-fetch.sh b/contrib/examples/git-fetch.sh
index a314273..06caf6b 100755
--- a/contrib/examples/git-fetch.sh
+++ b/contrib/examples/git-fetch.sh
@@ -67,7 +67,7 @@ do
 		keep='-k -k'
 		;;
 	--depth=*)
-		shallow_depth="--depth=`expr "z$1" : 'z-[^=]*=\(.*\)'`"
+		shallow_depth=--depth=`expr "z$1" : 'z-[^=]*=\(.*\)'`
 		;;
 	--depth)
 		shift
diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
index 289fc31..3d2ae3e 100755
--- a/t/t9107-git-svn-migrate.sh
+++ b/t/t9107-git-svn-migrate.sh
@@ -94,7 +94,7 @@ test_expect_success 'migrate --minimize on old inited layout' '
 		echo "$svnrepo"$path > "$GIT_DIR"/svn/$ref/info/url ) || exit 1;
 	done &&
 	git svn migrate --minimize &&
-	test -z "`git config -l | grep "^svn-remote\.git-svn\."`" &&
+	! git config -l | grep "^svn-remote\.git-svn\." &&
 	git config --get-all svn-remote.svn.fetch > fetch.out &&
 	grep "^trunk:refs/remotes/trunk$" fetch.out &&
 	grep "^branches/a:refs/remotes/a$" fetch.out &&
-- 
1.7.4.rc1

Re: [PATCH] Avoid unportable nested double- and backquotes in shell scripts.

[Jonathan Nieder]

Ralf Wildenhues wrote:

> [Subject: Avoid unportable nested double- and backquotes in shell scripts]
>
> Some shells parse them wrongly, esp. pdksh.

How does it treat $( ) command substitutions?  (We use those more
heavily and they are easier on the eyes anyway.)

> --- a/t/t9107-git-svn-migrate.sh
> +++ b/t/t9107-git-svn-migrate.sh
> @@ -94,7 +94,7 @@ test_expect_success 'migrate --minimize on old inited layout' '
>  		echo "$svnrepo"$path > "$GIT_DIR"/svn/$ref/info/url ) || exit 1;
>  	done &&
>  	git svn migrate --minimize &&
> -	test -z "`git config -l | grep "^svn-remote\.git-svn\."`" &&
> +	! git config -l | grep "^svn-remote\.git-svn\." &&

I thought I remembered portability problems with the

	! a | b

construct but it seems I am wrong; t7810-grep.sh uses that
construct without trouble, at least.

Re: [PATCH] Avoid unportable nested double- and backquotes in shell scripts.

[Ralf Wildenhues]

* Jonathan Nieder wrote on Sat, Jan 08, 2011 at 05:14:41PM CET:
> Ralf Wildenhues wrote:
> 
> > [Subject: Avoid unportable nested double- and backquotes in shell scripts]
> >
> > Some shells parse them wrongly, esp. pdksh.
> 
> How does it treat $( ) command substitutions?  (We use those more
> heavily and they are easier on the eyes anyway.)

Better (except for the usual problems when 'case ...)' comes into play).
But git makes heavy use of "no quoting needed on RHS of assignment"
anyway, so it seems like this would be a good move nonetheless.  And the
testsuite uses backticks a lot, it seems a move away from that should be
done more uniformly?

Anyway, I'll be happy to respin in whatever form is acceptable.

> > --- a/t/t9107-git-svn-migrate.sh
> > +++ b/t/t9107-git-svn-migrate.sh
> > @@ -94,7 +94,7 @@ test_expect_success 'migrate --minimize on old inited layout' '
> >  		echo "$svnrepo"$path > "$GIT_DIR"/svn/$ref/info/url ) || exit 1;
> >  	done &&
> >  	git svn migrate --minimize &&
> > -	test -z "`git config -l | grep "^svn-remote\.git-svn\."`" &&
> > +	! git config -l | grep "^svn-remote\.git-svn\." &&
> 
> I thought I remembered portability problems with the
> 
> 	! a | b
> 
> construct but it seems I am wrong; t7810-grep.sh uses that
> construct without trouble, at least.

Some non-Posix-conforming shells have problems with that too, e.g.,
Solaris /bin/sh, but I figured git wouldn't cater to them as I also saw
other such uses in the tree.

Cheers,
Ralf

Re: [PATCH] Avoid unportable nested double- and backquotes in shell scripts.

[Jonathan Nieder]

Ralf Wildenhues wrote:

> But git makes heavy use of "no quoting needed on RHS of assignment"
> anyway, so it seems like this would be a good move nonetheless.

No disagreement there.

> And the
> testsuite uses backticks a lot,

>From a quick grep, it seems you are right:

 $ git grep -c -F -e '`' -- 't/*.sh' | cut -d: -f2 | sum
 65126     1
 $ git grep -c -F -e '$(' -- 't/*.sh' | cut -d: -f2 | sum
 64807     1
 $ git grep -c -F -e '`' -- '*.sh' | cut -d: -f2 | sum
 13350     1
 $ git grep -c -F -e '$(' -- '*.sh' | cut -d: -f2 | sum
 07810     1

Documentation/CodingGuidelines 

 - We prefer $( ... ) for command substitution; unlike ``, it
   properly nests.  It should have been the way Bourne spelled
   it from day one, but unfortunately isn't.

> it seems a move away from that should be
> done more uniformly?

I don't see why. :)  In fact, I personally would not be happy at all
to see such a high-churn patch as that, while using the $( ... )
form in new code and as part of clarifications to other parts of the
same lines would seem to me to be a welcome thing.

Having said all that, I have no strong investment in this.  Feel
free to do what works best for you.

Thanks,
Jonathan

Re: [PATCH] Avoid unportable nested double- and backquotes in shell scripts.

[Jonathan Nieder]

Jonathan Nieder wrote:

> From a quick grep, it seems you are right:
> 
>  $ git grep -c -F -e '`' -- 't/*.sh' | cut -d: -f2 | sum
>  65126     1
>  $ git grep -c -F -e '$(' -- 't/*.sh' | cut -d: -f2 | sum
>  64807     1
>  $ git grep -c -F -e '`' -- '*.sh' | cut -d: -f2 | sum
>  13350     1
>  $ git grep -c -F -e '$(' -- '*.sh' | cut -d: -f2 | sum
>  07810     1

sum does something totally different than I expected.  With [1]
comes the more reasonable

 $ git grep -c -F -e '`' -- 't/*.sh' | cut -d: -f2 | addup
 485
 $ git grep -c -F -e '$(' -- 't/*.sh' | cut -d: -f2 | addup
 2620
 $ git grep -c -F -e '`' -- '*.sh' | cut -d: -f2 | addup
 594
 $ git grep -c -F -e '$(' -- '*.sh' | cut -d: -f2 | addup
 3133

So the code bloat and use of backticks are less dire than I feared.

[1] 
	addup () {
		sum=0
		while read term
		do
			: $((sum = $sum + $term))
		done
		echo $sum
	}

Re: [PATCH] Avoid unportable nested double- and backquotes in shell scripts.

[Junio C Hamano]

Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:

> diff --git a/contrib/examples/git-fetch.sh b/contrib/examples/git-fetch.sh
> index a314273..06caf6b 100755
> --- a/contrib/examples/git-fetch.sh
> +++ b/contrib/examples/git-fetch.sh
> @@ -67,7 +67,7 @@ do
>  		keep='-k -k'
>  		;;
>  	--depth=*)
> -		shallow_depth="--depth=`expr "z$1" : 'z-[^=]*=\(.*\)'`"
> +		shallow_depth=--depth=`expr "z$1" : 'z-[^=]*=\(.*\)'`
>  		;;
>  	--depth)
>  		shift

I do not very much like the idea of updating contrib/examples, one of
whose purposes is to document the historical implementation, to ship a
version that has never been battle tested in the field.

There also seem to be a few more uses of `` (the majority used $() even
back then) that are still left behind.  To make it more useful to serve as
an example (which is the other purpose of contrib/examples), it would make
more sense to update them all at once.

> diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh
> index 289fc31..3d2ae3e 100755
> --- a/t/t9107-git-svn-migrate.sh
> +++ b/t/t9107-git-svn-migrate.sh
> @@ -94,7 +94,7 @@ test_expect_success 'migrate --minimize on old inited layout' '
>  		echo "$svnrepo"$path > "$GIT_DIR"/svn/$ref/info/url ) || exit 1;
>  	done &&
>  	git svn migrate --minimize &&
> -	test -z "`git config -l | grep "^svn-remote\.git-svn\."`" &&
> +	! git config -l | grep "^svn-remote\.git-svn\." &&
>  	git config --get-all svn-remote.svn.fetch > fetch.out &&
>  	grep "^trunk:refs/remotes/trunk$" fetch.out &&
>  	grep "^branches/a:refs/remotes/a$" fetch.out &&