From: Johannes Sixt <j.sixt@viscovery.net>
Date: Fri, 12 Oct 2007 06:43:29 GMT
Subject: Re: [PATCH] Fixing path quoting issues
Message-ID: <470F1791.9010601@viscovery.net>
In-Reply-To: <63D5CE5B-51DD-4017-B2E2-2ADC5DCBE849@steelskies.com>

Jonathan del Strother schrieb:
> 
> On 11 Oct 2007, at 21:53, David Kastrup wrote:
> 
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>>
>>> Jonathan del Strother schrieb:
>>>> How are you going to test that git works on paths with spaces if the
>>>> test suite doesn't run there?
>>>
>>> By writing a specific test?
>>
>> This is going to be much less thorough.  And it does no harm if the
>> test scripts demonstrate defensive programming.
> 
> 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.

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