git-rebase and a number of tests didn't properly quote paths, leading to problems when run from a path with a space in.

Signed-off-by: Jonathan del Strother <jon.delStrother@bestbefore.tv>
---
 git-rebase.sh                            |   26 +++++-----
 t/t1020-subdirectory.sh                  |   22 ++++----
 t/t3050-subprojects-fetch.sh             |    2 +-
 t/t3404-rebase-interactive.sh            |    2 +-
 t/t5500-fetch-pack.sh                    |    2 +-
 t/t5700-clone-reference.sh               |    2 +-
 t/t7003-filter-branch.sh                 |    2 +-
 t/t7501-commit.sh                        |   74 +++++++++++++++---------------
 t/t9100-git-svn-basic.sh                 |   18 ++++----
 t/t9101-git-svn-props.sh                 |    6 +-
 t/t9102-git-svn-deep-rmdir.sh            |    6 +-
 t/t9104-git-svn-follow-parent.sh         |   50 ++++++++++----------
 t/t9105-git-svn-commit-diff.sh           |   10 ++--
 t/t9106-git-svn-commit-diff-clobber.sh   |   14 +++---
 t/t9107-git-svn-migrate.sh               |   40 ++++++++--------
 t/t9108-git-svn-glob.sh                  |    8 ++--
 t/t9110-git-svn-use-svm-props.sh         |    8 ++--
 t/t9111-git-svn-use-svnsync-props.sh     |    8 ++--
 t/t9112-git-svn-md5less-file.sh          |    4 +-
 t/t9113-git-svn-dcommit-new-file.sh      |    6 +-
 t/t9114-git-svn-dcommit-merge.sh         |    4 +-
 t/t9115-git-svn-dcommit-funky-renames.sh |    4 +-
 t/t9116-git-svn-log.sh                   |    4 +-
 t/test-lib.sh                            |    2 +-
 24 files changed, 162 insertions(+), 162 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 1583402..b48397e 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -59,7 +59,7 @@ continue_merge () {
 		die "$RESOLVEMSG"
 	fi
 
-	cmt=`cat $dotest/current`
+	cmt=`cat "$dotest/current"`
 	if ! git diff-index --quiet HEAD
 	then
 		if ! git-commit -C "$cmt"
@@ -84,14 +84,14 @@ continue_merge () {
 }
 
 call_merge () {
-	cmt="$(cat ...
[ message continues ]

... there are shells out there in the wild that will get badly confused by 
this sort of quoting and escaping. Butter use


Huh? This looks very wrong. What are the extra quotes needed for? If they 


... and you break it. More of these follow. Don't do that, it makes patch 
review unnecessarily hard.

I question the usefulness of this patch. Why only fix breakage due to spaces 
in the path? What about single-quotes, double-quotes? IMHO, it's not too 
much of a burden for developers to require "sane" build directory paths.

-- Hannes

-

[ message continues ]

It is correct, modulo breaking when there are single quotes in the



Double quotes would work.  Single quotes wouldn't.  You can do
something like

visualpath="$(pwd)"
export visualpath
VISUAL='"$visualpath/fake-editor.sh"'

and this should work in all circumstances where VISUAL is interpreted
as intended (which at the current point of time does not include git's

For a normal user, the only writable directories might be of the
"C:\Programs and Data\User settings\Karl"
variety.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
-

[ message continues ]

I whole-heartedly agree about the part of the patch that fixes 
git-rebase.sh. This should be a separate patch.

But the reset of the patch is about running the test suite, and it is much 
more difficult to fix because of the 'eval' that is going on. And, yes, I do 
think that we can expect that contributors, including this handful of people 
on Windows, have a "sane" build directory.

-- Hannes
-

[ message continues ]

How are you going to test that git works on paths with spaces if the  
test suite doesn't run there?
-

[ message continues ]

By writing a specific test?

-- Hannes
-

[ message continues ]

This is going to be much less thorough.  And it does no harm if the
test scripts demonstrate defensive programming.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
-

[ message continues ]

I would also point out that most tests have already been written to  
handle this case - ones that don't quote their paths are in the  
minority.
-

[ message continues ]

Hi,


We do not have _extensive_ tests.  We want to do some coding in addition 

That might very well be the case, and your goal is laudable.  However, I 
have to agree that most devs (indeed, since you are the first to try to 
fix it, _all_ except for you) do not care that deeply about spaces in the 
path, and having a _single_ test for this would be the logical solution.

I mean, we do not force our main developers to run the most obscure setups 
all the time just to make sure that it runs fine.  Otherwise none of us 
could run Linux, but a couple would be coerced into running Windows, for 
example.

Ciao,
Dscho

-

[ message continues ]

A good reason for not requiring special tests just for spaces.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
-

[ message continues ]

Actually, reconsidering your proposed patch, there are only a handful of 
problematic cases, namely those where the test script is quoted with 
double-quotes, like this:

  test_expect_success 'load repository with strange names' "
-	svnadmin load -q $rawsvnrepo < ../t9115/funky-names.dump &&
+	svnadmin load -q '$rawsvnrepo' < ../t9115/funky-names.dump &&
  	start_httpd
  	"

The problem is that here $rawsvnrepo will be expanded before the entire test 
script is passed as argument to test_expect_success. Consider the case where 
$rawsvnrepo contains a single-quote (say, a directory named "Joe's git"): 
then the 'eval' inside test_expect_success sees a syntax error. The proper 
change is:

  test_expect_success 'load repository with strange names' "
-	svnadmin load -q $rawsvnrepo < ../t9115/funky-names.dump &&
+	svnadmin load -q \"\$rawsvnrepo\" < ../t9115/funky-names.dump &&
  	start_httpd
  	"

So, what I think you should do is:

1. Submit the change to git-rebase.sh in a separate patch.
2. Fix the patch for the double-quoted test scriptlets.

That should remove all my concerns.

-- Hannes
-

[ message continues ]

+1: especially in this case, where it really is "defensive" and not  
"paranoiac".

Cheers,
Wincent


-

[ message continues ]

Hi,


I am all for it, _iff_ the guilty parties (and by that, I mean _you_) do 
it and keep maintaining it.  See?  Discussion closed already.

Ciao,
Dscho

-

[ message continues ]

How am *I* the guilty party? I'm merely endorsing David's comment  
that a modicum of defensive programming isn't a bad thing; an  
eminently reasonable position which is somewhat difficult to argue  
against.

Cheers,
Wincent

-

[ message continues ]

Hi,


All I'm saying: let patches speak.  Talk is cheap.

Ciao,
Dscho

-

[ message continues ]

I'm just preparing to release this patch... was that "don't break  
whitespace", or "don't try to fix whitespace in a patch that's has  
nothing to do with whitespacing-fixing" ?

And while I'm here - tabs are preferred, are they?  There seem to be  
a mixture of tabs & 4 space indentation.
-

[ message continues ]

1 hard tab / level of indent, but use spaces for continuation alignment.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231
-

[ message continues ]

OK, second attempt at this.  First patch fixes quoting in git rebase.  Second patch allows you to run tests from a path with a space in.  The third allows you to run tests from a path with an apostrophe in (and is where things start to get a bit ugly, hence the separate patch).
-

[ message continues ]

From: Jonathan del Strother <jon.delStrother@bestbefore.tv>

git-rebase used to fail when run from a path with a space in.

Signed-off-by: Jonathan del Strother <jon.delStrother@bestbefore.tv>
---
 git-rebase.sh |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 1583402..9995d9d 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -59,7 +59,7 @@ continue_merge () {
 		die "$RESOLVEMSG"
 	fi
 
-	cmt=`cat $dotest/current`
+	cmt=`cat "$dotest/current"`
 	if ! git diff-index --quiet HEAD
 	then
 		if ! git-commit -C "$cmt"
@@ -84,14 +84,14 @@ continue_merge () {
 }
 
 call_merge () {
-	cmt="$(cat $dotest/cmt.$1)"
+	cmt="$(cat "$dotest/cmt.$1")"
 	echo "$cmt" > "$dotest/current"
 	hd=$(git rev-parse --verify HEAD)
 	cmt_name=$(git symbolic-ref HEAD)
-	msgnum=$(cat $dotest/msgnum)
-	end=$(cat $dotest/end)
+	msgnum=$(cat "$dotest/msgnum")
+	end=$(cat "$dotest/end")
 	eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
-	eval GITHEAD_$hd='"$(cat $dotest/onto_name)"'
+	eval GITHEAD_$hd='"$(cat \"$dotest/onto_name\")"'
 	export GITHEAD_$cmt GITHEAD_$hd
 	git-merge-$strategy "$cmt^" -- "$hd" "$cmt"
 	rv=$?
@@ -140,10 +140,10 @@ do
 		}
 		if test -d "$dotest"
 		then
-			prev_head="`cat $dotest/prev_head`"
-			end="`cat $dotest/end`"
-			msgnum="`cat $dotest/msgnum`"
-			onto="`cat $dotest/onto`"
+			prev_head=$(cat "$dotest/prev_head")
+			end=$(cat "$dotest/end")
+			msgnum=$(cat "$dotest/msgnum")
+			onto=$(cat "$dotest/onto")
 			continue_merge
 			while test "$msgnum" -le "$end"
 			do
@@ -160,11 +160,11 @@ do
 		if test -d "$dotest"
 		then
 			git rerere clear
-			prev_head="`cat $dotest/prev_head`"
-			end="`cat $dotest/end`"
-			msgnum="`cat $dotest/msgnum`"
+			prev_head=$(cat "$dotest/prev_head")
+			end=$(cat "$dotest/end")
+			msgnum=$(cat "$dotest/msgnum")
 			msgnum=$(($msgnum + 1))
-			onto="`cat $dotest/onto`"
+			onto=$(cat ...
[ message continues ]

I believe this is not correct. It should be this way:

	eval GITHEAD_$hd='$(cat "$dotest/onto_name")'

You can test it with a conflicting git-rebase -m. It only affects what the 
conflict markers look like. The test suite does not test it.

The rest looks good.

-- Hannes
-

[ message continues ]

Thanks for catching that.  I'll post revised patches this morning
-

[ message continues ]

These patches attempt to fix things up for people who like to have awkward directory paths including spaces & apostrophes.  First patch fixes git-rebase (all other git tools handle these paths fine), second patch fixes up the tests so they can be run from spaced directories.
Third time lucky?
-

[ message continues ]

From: Jonathan del Strother <jon.delStrother@bestbefore.tv>

git-rebase used to fail when run from a path with a space in.

Signed-off-by: Jonathan del Strother <jon.delStrother@bestbefore.tv>
---
 git-rebase.sh |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 1583402..224cca9 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -59,7 +59,7 @@ continue_merge () {
 		die "$RESOLVEMSG"
 	fi
 
-	cmt=`cat $dotest/current`
+	cmt=`cat "$dotest/current"`
 	if ! git diff-index --quiet HEAD
 	then
 		if ! git-commit -C "$cmt"
@@ -84,14 +84,14 @@ continue_merge () {
 }
 
 call_merge () {
-	cmt="$(cat $dotest/cmt.$1)"
+	cmt="$(cat "$dotest/cmt.$1")"
 	echo "$cmt" > "$dotest/current"
 	hd=$(git rev-parse --verify HEAD)
 	cmt_name=$(git symbolic-ref HEAD)
-	msgnum=$(cat $dotest/msgnum)
-	end=$(cat $dotest/end)
+	msgnum=$(cat "$dotest/msgnum")
+	end=$(cat "$dotest/end")
 	eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
-	eval GITHEAD_$hd='"$(cat $dotest/onto_name)"'
+	eval GITHEAD_$hd='$(cat "$dotest/onto_name")'
 	export GITHEAD_$cmt GITHEAD_$hd
 	git-merge-$strategy "$cmt^" -- "$hd" "$cmt"
 	rv=$?
@@ -140,10 +140,10 @@ do
 		}
 		if test -d "$dotest"
 		then
-			prev_head="`cat $dotest/prev_head`"
-			end="`cat $dotest/end`"
-			msgnum="`cat $dotest/msgnum`"
-			onto="`cat $dotest/onto`"
+			prev_head=$(cat "$dotest/prev_head")
+			end=$(cat "$dotest/end")
+			msgnum=$(cat "$dotest/msgnum")
+			onto=$(cat "$dotest/onto")
 			continue_merge
 			while test "$msgnum" -le "$end"
 			do
@@ -160,11 +160,11 @@ do
 		if test -d "$dotest"
 		then
 			git rerere clear
-			prev_head="`cat $dotest/prev_head`"
-			end="`cat $dotest/end`"
-			msgnum="`cat $dotest/msgnum`"
+			prev_head=$(cat "$dotest/prev_head")
+			end=$(cat "$dotest/end")
+			msgnum=$(cat "$dotest/msgnum")
 			msgnum=$(($msgnum + 1))
-			onto="`cat $dotest/onto`"
+			onto=$(cat "$dotest/onto")
 ...
[ message continues ]

From: Jonathan del Strother <jon.delStrother@bestbefore.tv>

Double-quoting all paths so the tests can be run from inside directories with spaces and apostrophes

Signed-off-by: Jonathan del Strother <jon.delStrother@bestbefore.tv>
---
 t/lib-git-svn.sh                         |    2 +-
 t/t1020-subdirectory.sh                  |   22 ++++++------
 t/t3050-subprojects-fetch.sh             |    2 +-
 t/t3404-rebase-interactive.sh            |    2 +-
 t/t5500-fetch-pack.sh                    |    2 +-
 t/t5700-clone-reference.sh               |    2 +-
 t/t7003-filter-branch.sh                 |    2 +-
 t/t7501-commit.sh                        |    4 +-
 t/t9100-git-svn-basic.sh                 |   54 +++++++++++++++---------------
 t/t9101-git-svn-props.sh                 |    6 ++--
 t/t9102-git-svn-deep-rmdir.sh            |    6 ++--
 t/t9104-git-svn-follow-parent.sh         |   50 ++++++++++++++--------------
 t/t9105-git-svn-commit-diff.sh           |   12 +++---
 t/t9106-git-svn-commit-diff-clobber.sh   |   14 ++++----
 t/t9107-git-svn-migrate.sh               |   40 +++++++++++-----------
 t/t9108-git-svn-glob.sh                  |    8 ++--
 t/t9110-git-svn-use-svm-props.sh         |    8 ++--
 t/t9111-git-svn-use-svnsync-props.sh     |    8 ++--
 t/t9112-git-svn-md5less-file.sh          |    4 +-
 t/t9113-git-svn-dcommit-new-file.sh      |    6 ++--
 t/t9114-git-svn-dcommit-merge.sh         |    4 +-
 t/t9115-git-svn-dcommit-funky-renames.sh |    4 +-
 t/t9116-git-svn-log.sh                   |    4 +-
 t/t9500-gitweb-standalone-no-errors.sh   |    4 +-
 t/test-lib.sh                            |    2 +-
 25 files changed, 136 insertions(+), 136 deletions(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 8d4a447..cde3053 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -25,7 +25,7 @@ perl -w -e "
 use SVN::Core;
 use SVN::Repos;
 \$SVN::Core::VERSION gt '1.1.0' or exit(42);
-system(qw/svnadmin create --fs-type fsfs/, '$svnrepo') == 0 or ...
[ message continues ]

Here you have to work harder: The reason is that this is part of a perl 
expression (as opposed to an eval'd string), which does not have access to 
$svnrepo of the shell by which it is invoked. The original version failed if 
there were single-quotes in $svnrepo, the new version fails if it contains 

This must be

	svn import -m 'import for git-svn' . \"\$svnrepo\" >/dev/null &&

to be safe. Your version would break with names with double-quotes, because 
$svnrepo would be expanded and then eval'd inside test_expect_*. This error 
recurs numerous times until the end of the patch.

May I recommend that you run the test suite in a directory named like this:

	$ mkdir \"\ \$GIT_DIR\ \'
	$ ls

I assume $path is under control of the test script, otherwise it must be 

Why is this trickery with find needed? Isn't it easier to put the whole test 
case in single-quotes instead?

-- Hannes

-

[ message continues ]

Eww.  I'm struggling a bit with paths this perverse, actually.

For instance, git_editor in git-sh-setup expects the editor path to be  
pre-quoted.  So in t3404, you need to produce escaped double quotes &  
dollar signs, resulting in unpleasantness like this :

VISUAL="`pwd`/fake-editor.sh"
VISUAL=${VISUAL//\"/\\\"}
VISUAL=${VISUAL//$/\\\$}
VISUAL=\"$VISUAL\"
export VISUAL


And I'm struggling to come up with neat ways of rewriting things like,  
eg, this bit from t5500 -
test_expect_success "clone shallow" "git-clone --depth 2 \"file:// 
`pwd`/.\" shallow"
- to handle paths like that properly.


Suggestions?

Jon
-

[ message continues ]

You can rewrite this expression as
     perl -w -e '$svnrepo = shift;
	...
	$SVN::Core::Version gt "1.1.0" ...
	system(qw/svnadmin create --fs-type fsfs/, $svnrepo) == 0 ...
	...
     ' >&3 2>&4 "$svnrepo"



These examples expand `pwd` too early. Can't you just put everything inside 
single-quotes? Although I'm not sure about VISUAL: Is it invoked with $PWD 
that is different from $PWD when VISUAL is defined? If so, then you can 
hardly delay `pwd`...

I know I'm a bit anal with my criticism. I reviewed your patch because I 
think fixing for paths with whitespace is worthwhile. However, I also think 
any fix should go the full way and not only shift the problems into a 
different corner. Maybe a word from $maintainer would be in order ;)

-- Hannes
-

[ message continues ]

In theory, I agree that the tests should properly handle perverse  
paths, but it's beginning to stretch my shell scripting skills.
So now our esteemed leader is back in business, any thoughts on how  
hard we want to work to quote things?

Noted


Jon del Strother

-

[ message continues ]

Looks good and works. Thanks.

-- Hannes

-

[ message continues ]

From: Jonathan del Strother <jon.delStrother@bestbefore.tv>

Double-quoting all paths so the tests can be run from inside a directory named "Joe's git"

Signed-off-by: Jonathan del Strother <jon.delStrother@bestbefore.tv>
---
 t/t9100-git-svn-basic.sh                 |   54 +++++++++++++++---------------
 t/t9101-git-svn-props.sh                 |    6 ++--
 t/t9102-git-svn-deep-rmdir.sh            |    6 ++--
 t/t9104-git-svn-follow-parent.sh         |   50 ++++++++++++++--------------
 t/t9105-git-svn-commit-diff.sh           |   12 +++---
 t/t9106-git-svn-commit-diff-clobber.sh   |   14 ++++----
 t/t9107-git-svn-migrate.sh               |   28 ++++++++--------
 t/t9108-git-svn-glob.sh                  |    8 ++--
 t/t9110-git-svn-use-svm-props.sh         |    8 ++--
 t/t9111-git-svn-use-svnsync-props.sh     |    8 ++--
 t/t9112-git-svn-md5less-file.sh          |    4 +-
 t/t9113-git-svn-dcommit-new-file.sh      |    6 ++--
 t/t9114-git-svn-dcommit-merge.sh         |    4 +-
 t/t9115-git-svn-dcommit-funky-renames.sh |    4 +-
 t/t9116-git-svn-log.sh                   |    4 +-
 15 files changed, 108 insertions(+), 108 deletions(-)

diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index c3585da..1d802a8 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -31,16 +31,16 @@ test_expect_success \
 	echo 'zzz' > bar/zzz &&
 	echo '#!/bin/sh' > exec.sh &&
 	chmod +x exec.sh &&
-	svn import -m 'import for git-svn' . '$svnrepo' >/dev/null &&
+	svn import -m 'import for git-svn' . \"$svnrepo\" >/dev/null &&
 	cd .. &&
 	rm -rf import &&
-	git-svn init '$svnrepo'"
+	git-svn init \"$svnrepo\""
 
 test_expect_success \
     'import an SVN revision into git' \
     'git-svn fetch'
 
-test_expect_success "checkout from svn" "svn co '$svnrepo' '$SVN_TREE'"
+test_expect_success "checkout from svn" "svn co \"$svnrepo\" \"$SVN_TREE\""
 
 name='try a deep --rmdir with a commit'
 test_expect_success "$name" "
@@ -51,8 +51,8 @@ test_expect_success ...
[ message continues ]

From: Jonathan del Strother <jon.delStrother@bestbefore.tv>

Add quoting to various test paths so they can be run from a path with a space in

Signed-off-by: Jonathan del Strother <jon.delStrother@bestbefore.tv>
---
 t/lib-git-svn.sh                         |    2 +-
 t/t1020-subdirectory.sh                  |   22 ++++++------
 t/t3050-subprojects-fetch.sh             |    2 +-
 t/t3404-rebase-interactive.sh            |    2 +-
 t/t5500-fetch-pack.sh                    |    2 +-
 t/t5700-clone-reference.sh               |    2 +-
 t/t7003-filter-branch.sh                 |    2 +-
 t/t7501-commit.sh                        |    4 +-
 t/t9100-git-svn-basic.sh                 |   18 +++++-----
 t/t9101-git-svn-props.sh                 |    6 ++--
 t/t9102-git-svn-deep-rmdir.sh            |    6 ++--
 t/t9104-git-svn-follow-parent.sh         |   50 +++++++++++++++---------------
 t/t9105-git-svn-commit-diff.sh           |   10 +++---
 t/t9106-git-svn-commit-diff-clobber.sh   |   14 ++++----
 t/t9107-git-svn-migrate.sh               |   40 ++++++++++++------------
 t/t9108-git-svn-glob.sh                  |    8 ++--
 t/t9110-git-svn-use-svm-props.sh         |    8 ++--
 t/t9111-git-svn-use-svnsync-props.sh     |    8 ++--
 t/t9112-git-svn-md5less-file.sh          |    4 +-
 t/t9113-git-svn-dcommit-new-file.sh      |    6 ++--
 t/t9114-git-svn-dcommit-merge.sh         |    4 +-
 t/t9115-git-svn-dcommit-funky-renames.sh |    4 +-
 t/t9116-git-svn-log.sh                   |    4 +-
 t/t9500-gitweb-standalone-no-errors.sh   |    4 +-
 t/test-lib.sh                            |    2 +-
 25 files changed, 117 insertions(+), 117 deletions(-)

diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index 8d4a447..cde3053 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -25,7 +25,7 @@ perl -w -e "
 use SVN::Core;
 use SVN::Repos;
 \$SVN::Core::VERSION gt '1.1.0' or exit(42);
-system(qw/svnadmin create --fs-type fsfs/, '$svnrepo') == 0 or exit(41);
+system(qw/svnadmin create ...
[ message continues ]

I don't see the point in changing an incorrect quoting to a different 
incorrect quoting that you fix up in a follow-up patch. It's *two* large 
patches to review instead of just one. I'm stopping the review here.

-- Hannes

-

[ message continues ]

If we want to support apostrophed paths in tests, I'll flatten 2 & 3  
into a single patch.  I thought I'd make the apostrophe part optional  
since there seemed to be some resistance to having to bother about  
quoting & escaping in tests..

-

[ message continues ]

You could also make a patch that reverses the quoting in t9100-* (and 
probably others), i.e. instead of

	"... '$foo'..." (which is incorrect)
or
	"... \"$foo\"..."
make it
	'... "$foo" ...'

It will be a large patch, too, but the result should be easier to read.

-- Hannes
-

[ message continues ]