Factor out command-line quoting code on Windows.

Factor out command-line quoting code on Windows.

Our command line code is not very good with respect to getting
quoting right.  Because of this, callers will sometimes themselves
attempt to quote arguments, though some of them get it wrong(!) by
just surrounding with quotes.

The first step is to add a quoting function that we think is correct
and update the unit test to test it.

(Note that most of this is Windows-specific, because on POSIX
we can pass arguments to commands as a vector -- trying to do
quoting on POSIX is usually wrong.)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55591

[cpu_(ooo_6.6-7.5), 2010-08-06 20:09:04]

http://codereview.chromium.org/3007038/diff/2001/3001
File base/command_line.cc (right):

http://codereview.chromium.org/3007038/diff/2001/3001#newcode399
base/command_line.cc:399: return arg;
I am lost on that first if(). So arg = L"ab cd" does not need quoting?

I would expect ab cd  => "ab cd"

[Evan Martin, 2010-08-06 20:10:59]

http://codereview.chromium.org/3007038/diff/2001/3001
File base/command_line.cc (right):

http://codereview.chromium.org/3007038/diff/2001/3001#newcode399
base/command_line.cc:399: return arg;
On 2010/08/06 20:09:04, cpu wrote:
> I am lost on that first if(). So arg = L"ab cd" does not need quoting?
> 
> I would expect ab cd  => "ab cd"

It's saying: if we *don't* find any spaces \ or " then we don't need quoting. 
So spaces do need quoting.

(I actually got this wrong when writing this patch, but the test caught this!)

[Evan Martin, 2010-08-06 20:11:38]

On 2010/08/06 20:10:59, Evan Martin wrote:
> http://codereview.chromium.org/3007038/diff/2001/3001
> File base/command_line.cc (right):
> 
> http://codereview.chromium.org/3007038/diff/2001/3001#newcode399
> base/command_line.cc:399: return arg;
> On 2010/08/06 20:09:04, cpu wrote:
> > I am lost on that first if(). So arg = L"ab cd" does not need quoting?
> > 
> > I would expect ab cd  => "ab cd"
> 
> It's saying: if we *don't* find any spaces \ or " then we don't need quoting. 
> So spaces do need quoting.
> 
> (I actually got this wrong when writing this patch, but the test caught this!)

Search for switch3 in the diff for the unittest, it has spaces only

[Evan Martin, 2010-08-06 20:54:35]

Ping.  Chris, don't be afraid to review too -- it's security-sensitive code.  :)

[cevans, 2010-08-06 21:01:35]

[+jschuh]

I think Justin is more familiar with the nuances of Windows command line
parsing?

On Fri, Aug 6, 2010 at 1:54 PM, <evan@chromium.org> wrote:

> Ping.  Chris, don't be afraid to review too -- it's security-sensitive
> code.  :)
>
>
> http://codereview.chromium.org/3007038/show
>

[cpu_(ooo_6.6-7.5), 2010-08-07 01:00:27]

http://codereview.chromium.org/3007038/diff/2001/3001
File base/command_line.cc (right):

http://codereview.chromium.org/3007038/diff/2001/3001#newcode399
base/command_line.cc:399: return arg;
Got it.

What confused me what the check for ("), but it makes sense. But now should
arg[0]== " == arg[size()-1] be checked as to no escape again?

In other words "ab cd" does not get quoted again.



On 2010/08/06 20:10:59, Evan Martin wrote:
> On 2010/08/06 20:09:04, cpu wrote:
> > I am lost on that first if(). So arg = L"ab cd" does not need quoting?
> > 
> > I would expect ab cd  => "ab cd"
> 
> It's saying: if we *don't* find any spaces \ or " then we don't need quoting. 
> So spaces do need quoting.
> 
> (I actually got this wrong when writing this patch, but the test caught this!)

[Evan Martin, 2010-08-09 17:12:27]

On 2010/08/07 01:00:27, cpu wrote:
I'll use [] to delimit the ends of a string to reduce confusion.  :)

> In other words "ab cd" does not get quoted again.

If the thing you're trying to pass as an argument is [ab cd], then it should be
quoted as ["ab cd"].  But the the argument is actually ["ab cd"] with the
quotes, then the proper quoting is ["\"ab cd\""] so that the quotes aren't lost.
 That is to say, "quoted again" can only happen if you call the function twice
-- there's no such thing as a string being "already" quoted.

[cpu_(ooo_6.6-7.5), 2010-08-09 23:00:33]

http://codereview.chromium.org/3007038/diff/2001/3001
File base/command_line.cc (left):

http://codereview.chromium.org/3007038/diff/2001/3001#oldcode399
base/command_line.cc:399: (value_string[0] != L'"') &&
my last comment was due to line 399 here. I guess that current callers use
AppendLooseValue? 

Anyhow, lgtm