Changed the position of the output->swap() call. This way, even if the command encounters an error, we still record the output. This is important for the case when stderr has been redirected to stdout, and we'd like to see stdout even when the exit code is non-zero.

Patch contributed by Michael Williamson.

Review URL: http://codereview.chromium.org/2665004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@49423 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 6 insertions, 10 deletions

diff --git a/base/process_util.h b/base/process_util.h
index b8bcfd4..d6aed0d 100644
--- a/base/process_util.h
+++ b/base/process_util.h
@@ -232,8 +232,7 @@ bool LaunchApp(const CommandLine& cl,
 // Executes the application specified by |cl| and wait for it to exit. Stores
 // the output (stdout) in |output|. Redirects stderr to /dev/null. Returns true
 // on success (application launched and exited cleanly, with exit code
-// indicating success). |output| is modified only when the function finished
-// successfully.
+// indicating success).
 bool GetAppOutput(const CommandLine& cl, std::string* output);
 
 #if defined(OS_POSIX)
diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
index af5ddcb..6c5b5fe 100644
--- a/base/process_util_posix.cc
+++ b/base/process_util_posix.cc
@@ -735,7 +735,6 @@ int64 TimeValToMicroseconds(const struct timeval& tv) {
 // specify the path of the application, and |envp| will be used as the
 // environment. Redirects stderr to /dev/null. Returns true on success
 // (application launched and exited cleanly, with exit code indicating success).
-// |output| is modified only when the function finished successfully.
 static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
                                  std::string* output, size_t max_output,
                                  bool do_search_path) {
@@ -807,8 +806,8 @@ static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
         // write to the pipe).
         close(pipe_fd[1]);
 
+        output->clear();
         char buffer[256];
-        std::string output_buf;
         size_t output_buf_left = max_output;
         ssize_t bytes_read = 1;  // A lie to properly handle |max_output == 0|
                                  // case in the logic below.
@@ -818,7 +817,7 @@ static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
                                     std::min(output_buf_left, sizeof(buffer))));
           if (bytes_read <= 0)
             break;
-          output_buf.append(buffer, bytes_read);
+          output->append(buffer, bytes_read);
           output_buf_left -= static_cast<size_t>(bytes_read);
         }
         close(pipe_fd[0]);
@@ -834,7 +833,6 @@ static bool GetAppOutputInternal(const CommandLine& cl, char* const envp[],
             return false;
         }
 
-        output->swap(output_buf);
         return true;
       }
   }
diff --git a/base/process_util_unittest.cc b/base/process_util_unittest.cc
index 96dca2f..15405f4 100644
--- a/base/process_util_unittest.cc
+++ b/base/process_util_unittest.cc
@@ -459,12 +459,11 @@ TEST_F(ProcessUtilTest, GetAppOutputRestricted) {
   EXPECT_TRUE(base::GetAppOutputRestricted(CommandLine(argv), &output, 100));
   EXPECT_STREQ("", output.c_str());
 
-  // On failure, should not touch |output|. As above, but for |false|.
   argv[2] = "exit 1";  // equivalent to "false"
-  output = "abc";
+  output = "before";
   EXPECT_FALSE(base::GetAppOutputRestricted(CommandLine(argv),
-                                      &output, 100));
-  EXPECT_STREQ("abc", output.c_str());
+                                            &output, 100));
+  EXPECT_STREQ("", output.c_str());
 
   // Amount of output exactly equal to space allowed.
   argv[2] = "echo 123456789";  // (the sh built-in doesn't take "-n")