From b4bcc1a7ce2999de5802d6ca6eada3152869c54c Mon Sep 17 00:00:00 2001
From: Alexey Spiridonov <lesha@fb.com>
Date: Wed, 12 Apr 2017 14:43:04 -0700
Subject: [PATCH] Replace Subprocess::pipe* syntax sugar with
 Subprocess::Options().pipe*

Summary:
This is a bit too magical -- it's not clear that the thing produces an Options object. If you do know that you can chain further option setters off this thing, it's nice, but otherwise, the first impression is "what just happened?".

So, let's have one good way for doing things.

Reviewed By: yfeldblum

Differential Revision: D4863947

fbshipit-source-id: 3dfe83cfc077d47f604f47dcb21149fbaa2d2243
---
 folly/Subprocess.h                             |  8 ++------
 folly/experimental/test/ProgramOptionsTest.cpp |  2 +-
 folly/test/SubprocessTest.cpp                  | 18 +++++++++++-------
 folly/tracing/test/StaticTracepointTest.cpp    |  3 ++-
 4 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/folly/Subprocess.h b/folly/Subprocess.h
index d739288a..2afa2382 100644
--- a/folly/Subprocess.h
+++ b/folly/Subprocess.h
@@ -33,12 +33,12 @@
  * to complete, returning the exit status.
  *
  * A thread-safe [1] version of popen() (type="r", to read from the child):
- *    Subprocess proc(cmd, Subprocess::pipeStdout());
+ *    Subprocess proc(cmd, Subprocess::Options().pipeStdout());
  *    // read from proc.stdoutFd()
  *    proc.wait();
  *
  * A thread-safe [1] version of popen() (type="w", to write to the child):
- *    Subprocess proc(cmd, Subprocess::pipeStdin());
+ *    Subprocess proc(cmd, Subprocess::Options().pipeStdin());
  *    // write to proc.stdinFd()
  *    proc.wait();
  *
@@ -439,10 +439,6 @@ class Subprocess {
 #endif
   };
 
-  static Options pipeStdin() { return Options().stdinFd(PIPE); }
-  static Options pipeStdout() { return Options().stdoutFd(PIPE); }
-  static Options pipeStderr() { return Options().stderrFd(PIPE); }
-
   // Non-copiable, but movable
   Subprocess(const Subprocess&) = delete;
   Subprocess& operator=(const Subprocess&) = delete;
diff --git a/folly/experimental/test/ProgramOptionsTest.cpp b/folly/experimental/test/ProgramOptionsTest.cpp
index d5f58bc7..208c8415 100644
--- a/folly/experimental/test/ProgramOptionsTest.cpp
+++ b/folly/experimental/test/ProgramOptionsTest.cpp
@@ -55,7 +55,7 @@ std::string callHelper(ProgramOptionsStyle style,
     break;
   }
 
-  Subprocess proc(allArgs, Subprocess::pipeStdout(), nullptr, &env);
+  Subprocess proc(allArgs, Subprocess::Options().pipeStdout(), nullptr, &env);
   auto p = proc.communicate();
   EXPECT_EQ(0, proc.wait().exitStatus());
 
diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index d9763ae6..74a822e3 100644
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -222,8 +222,9 @@ TEST(SimpleSubprocessTest, FdLeakTest) {
   // Test where the exec call fails() with pipes
   checkFdLeak([] {
     try {
-      Subprocess proc(std::vector<std::string>({"/no/such/file"}),
-                      Subprocess::pipeStdout().stderrFd(Subprocess::PIPE));
+      Subprocess proc(
+          std::vector<std::string>({"/no/such/file"}),
+          Subprocess::Options().pipeStdout().stderrFd(Subprocess::PIPE));
       ADD_FAILURE() << "expected an error when running /no/such/file";
     } catch (const SubprocessSpawnError& ex) {
       EXPECT_EQ(ENOENT, ex.errnoValue());
@@ -258,7 +259,7 @@ TEST(ParentDeathSubprocessTest, ParentDeathSignal) {
 }
 
 TEST(PopenSubprocessTest, PopenRead) {
-  Subprocess proc("ls /", Subprocess::pipeStdout());
+  Subprocess proc("ls /", Subprocess::Options().pipeStdout());
   int found = 0;
   gen::byLine(File(proc.stdoutFd())) |
     [&] (StringPiece line) {
@@ -317,8 +318,9 @@ TEST(AfterForkCallbackSubprocessTest, TestAfterForkCallbackError) {
 }
 
 TEST(CommunicateSubprocessTest, SimpleRead) {
-  Subprocess proc(std::vector<std::string>{ "/bin/echo", "-n", "foo", "bar"},
-                  Subprocess::pipeStdout());
+  Subprocess proc(
+      std::vector<std::string>{"/bin/echo", "-n", "foo", "bar"},
+      Subprocess::Options().pipeStdout());
   auto p = proc.communicate();
   EXPECT_EQ("foo bar", p.first);
   proc.waitChecked();
@@ -375,7 +377,8 @@ TEST(CommunicateSubprocessTest, Duplex2) {
       "-e", "s/a test/a successful test/",
       "-e", "/^another line/w/dev/stderr",
     });
-    auto options = Subprocess::pipeStdin().pipeStdout().pipeStderr().usePath();
+    auto options =
+        Subprocess::Options().pipeStdin().pipeStdout().pipeStderr().usePath();
     Subprocess proc(cmd, options);
     auto out = proc.communicateIOBuf(std::move(input));
     proc.waitChecked();
@@ -463,7 +466,8 @@ TEST(CommunicateSubprocessTest, Chatty) {
     int wcount = 0;
     int rcount = 0;
 
-    auto options = Subprocess::pipeStdin().pipeStdout().pipeStderr().usePath();
+    auto options =
+        Subprocess::Options().pipeStdin().pipeStdout().pipeStderr().usePath();
     std::vector<std::string> cmd {
       "sed",
       "-u",
diff --git a/folly/tracing/test/StaticTracepointTest.cpp b/folly/tracing/test/StaticTracepointTest.cpp
index 7c53efd8..710c565c 100644
--- a/folly/tracing/test/StaticTracepointTest.cpp
+++ b/folly/tracing/test/StaticTracepointTest.cpp
@@ -98,7 +98,8 @@ static std::string getNoteRawContent(const std::string& fileName) {
       "section",
       ".note." + kUSDTSubsectionName,
       fileName);
-  auto subProc = folly::Subprocess(args, folly::Subprocess::pipeStdout());
+  auto subProc =
+      folly::Subprocess(args, folly::Subprocess::Options().pipeStdout());
   auto output = subProc.communicate();
   auto retCode = subProc.wait();
   CHECK(retCode.exited());
-- 
2.34.1