From: Johannes Sixt <j.sixt@viscovery.net>
Date: Thu, 18 Oct 2007 06:08:09 GMT
Subject: Re: [PATCH 2/2] Quoting paths in tests
Message-ID: <4716F849.3090102@viscovery.net>
In-Reply-To: <6E65762D-FBC4-4A7C-97A9-20F6744E25DE@steelskies.com>

Jonathan del Strother schrieb:
> 
> On 17 Oct 2007, at 12:32, Johannes Sixt wrote:
> 
>> Jonathan del Strother schrieb:
>>> --- 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 --fs-type fsfs/, \"$svnrepo\") == 0 or 
>>> exit(41);
>>
>> 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 double-quotes.

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"

i.e. you pass the repository name as an argument to the scriptlet.

>> May I recommend that you run the test suite in a directory named like 
>> this:
>>
>>     $ mkdir \"\ \$GIT_DIR\ \'
>>     $ ls
>>     " $GIT_DIR '
> 
> 
> 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//$/\\\$}

This is a bashism - that's a big no-no.

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

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