Re: [PATCH 2/2] Quoting paths in tests

[Jonathan del Strother <maillist@steelskies.com>]

On 18 Oct 2007, at 07:08, Johannes Sixt wrote:

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

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?

Also, from "What's cooking in git.git" :
> I have a feeling that this should have forked off of 'maint'.
> The change looks obvious and trivial, so perhaps after getting a
> testcase (hint, hint) merge to 'master' and then cherry-pick to
> 'maint' as well.
Noted


Jon del Strother