Factor out command-line quoting code on Windows.

base/command_line.cc

Index: base/command_line.cc
diff --git a/base/command_line.cc b/base/command_line.cc
index 8d36e16d0e21786a809c9b59adc8853f68f7d0ba..295ddd474bb5cde434e1d85df62e4964a19d57a8 100644
--- a/base/command_line.cc
+++ b/base/command_line.cc
@@ -114,6 +114,7 @@ CommandLine CommandLine::FromString(const std::wstring& command_line) {
 CommandLine::CommandLine(const FilePath& program) {
   if (!program.empty()) {
     program_ = program.value();
+    // TODO(evanm): proper quoting here.
     command_line_string_ = L'"' + program.value() + L'"';
   }
 }
@@ -388,24 +389,55 @@ void CommandLine::AppendSwitchASCII(const std::string& switch_string,
   AppendSwitchNative(switch_string, ASCIIToWide(value_string));
 }
 
-void CommandLine::AppendSwitchNative(const std::string& switch_string,
-                                     const std::wstring& value_string) {
-  std::wstring value_string_edit;
-
-  // NOTE(jhughes): If the value contains a quotation mark at one
-  //                end but not both, you may get unusable output.
-  if (!value_string.empty() &&
-      (value_string.find(L" ") != std::wstring::npos) &&
-      (value_string[0] != L'"') &&

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

my last comment was due to line 399 here. I guess that current callers use
AppendLooseValue? 

Anyhow, lgtm

-      (value_string[value_string.length() - 1] != L'"')) {
-    // need to provide quotes
-    value_string_edit = StringPrintf(L"\"%ls\"", value_string.c_str());
-  } else {
-    value_string_edit = value_string;
+// Quote a string if necessary, such that CommandLineToArgvW() will
+// always process it as a single argument.
+static std::wstring WindowsStyleQuote(const std::wstring& arg) {
+  // We follow the quoting rules of CommandLineToArgvW.
+  // http://msdn.microsoft.com/en-us/library/17w5ykft.aspx
+  if (arg.find_first_of(L" \\\"") == std::wstring::npos) {
+    // No quoting necessary.
+    return arg;

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

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]

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

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

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:

   }
 
+  std::wstring out;
+  out.push_back(L'"');
+  for (size_t i = 0; i < arg.size(); ++i) {
+    if (arg[i] == '\\') {
+      // Find the extent of this run of backslashes.
+      int start = i, end = start + 1;
+      for (; end < arg.size() && arg[end] == '\\'; ++end)
+        /* empty */;
+      int backslash_count = end - start;
+
+      // Backslashes are escapes only if the run is followed by a double quote.
+      // Since we also will end the string with a double quote, we escape for
+      // either a double quote or the end of the string.
+      if (end == static_cast<int>(arg.size()) || arg[end] == '"') {
+        // To quote, we need to output 2x as many backslashes.
+        backslash_count *= 2;
+      }
+      for (size_t j = 0; j < backslash_count; ++j)
+        out.push_back('\\');
+
+      // Advance i to one before the end to balance i++ in loop.
+      i = end - 1;
+    } else if (arg[i] == '"') {
+      out.push_back('\\');
+      out.push_back('"');
+    } else {
+      out.push_back(arg[i]);
+    }
+  }
+  out.push_back('"');
+
+  return out;
+}
+
+void CommandLine::AppendSwitchNative(const std::string& switch_string,
+                                     const std::wstring& value_string) {
+  std::wstring quoted_value_string = WindowsStyleQuote(value_string);
   std::wstring combined_switch_string =
-      PrefixedSwitchStringWithValue(switch_string, value_string_edit);
+      PrefixedSwitchStringWithValue(switch_string, quoted_value_string);
 
   command_line_string_.append(L" ");
   command_line_string_.append(combined_switch_string);
@@ -414,7 +446,8 @@ void CommandLine::AppendSwitchNative(const std::string& switch_string,
 }
 
 void CommandLine::AppendLooseValue(const std::wstring& value) {
-  // TODO(evan): quoting?
+  // TODO(evan): the quoting here is wrong, but current callers rely on it
+  // being wrong.  I have another branch which fixes all the callers.
   command_line_string_.append(L" ");
   command_line_string_.append(value);
   args_.push_back(value);