Factor out command-line quoting code on Windows.

base/command_line.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/command_line.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shellapi.h>
 #elif defined(OS_POSIX)
@@ skipping 96 matching lines @@
 // static
 CommandLine CommandLine::FromString(const std::wstring& command_line) {
   CommandLine cmd;
   cmd.ParseFromString(command_line);
   return cmd;
 }
 
 CommandLine::CommandLine(const FilePath& program) {
   if (!program.empty()) {
     program_ = program.value();
+    // TODO(evanm): proper quoting here.
     command_line_string_ = L'"' + program.value() + L'"';
   }
 }
 
 #elif defined(OS_POSIX)
 CommandLine::CommandLine(ArgumentsOnly args_only) {
   // Push an empty argument, because we always assume argv_[0] is a program.
   argv_.push_back("");
 }
 
@@ skipping 254 matching lines @@
   command_line_string_.append(L" ");
   command_line_string_.append(prefixed_switch_string);
   switches_[switch_string] = L"";
 }
 
 void CommandLine::AppendSwitchASCII(const std::string& switch_string,
                                     const std::string& value_string) {
   AppendSwitchNative(switch_string, ASCIIToWide(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 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;
-  }
-
+  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);
 
   switches_[switch_string] = value_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);
 }
 
 void CommandLine::AppendArguments(const CommandLine& other,
                                   bool include_program) {
   // Verify include_program is used correctly.
   // Logic could be shorter but this is clearer.
   DCHECK_EQ(include_program, !other.GetProgram().empty());
@@ skipping 71 matching lines @@
     if (source.HasSwitch(switches[i])) {
       StringType value = source.GetSwitchValueNative(switches[i]);
       AppendSwitchNative(switches[i], value);
     }
   }
 }
 
 // private
 CommandLine::CommandLine() {
 }