Fix handling of -- in 'eden start'

Summary:
When giving arguments to the edenfs program, 'eden start' only strips `--` if it is the first positional argument. The position of `--` shouldn't matter as long (as it's before any options).

In other words, given the following command invocation:

  $ eden start --opt-a arg-b -- --opt-c arg-d

Current behavior:

* `--opt-a` is interpreted as an option for the 'eden start' command.
* `arg-b -- --opt-c arg-d` is passed to edenfs as four arguments.

Desired behavior:

* `--opt-a` is interpreted as an option for the 'eden start' command.
* `arg-b --opt-c arg-d` is passed to edenfs as three arguments.
* The `--` argument is not passed to edenfs.

Fix 'eden start' by stripping `--` regardless of its position.

Reviewed By: chadaustin

Differential Revision: D10863987

fbshipit-source-id: 094da3f3674e775fe3e7eb8441ec95c37c34ff05

eden/cli/daemon.py

@@ -195,8 +195,11 @@ def _get_daemon_args(
 
     # If the user put an "--" argument before the edenfs args, argparse passes
     # that through to us.  Strip it out.
-    if edenfs_args and edenfs_args[0] == "--":
-        edenfs_args = edenfs_args[1:]
+    if edenfs_args is not None:
+        try:
+            edenfs_args.remove("--")
+        except ValueError:
+            pass
 
     return instance.get_edenfs_start_cmd(
         valid_daemon_binary,

eden/integration/clone_test.py

@@ -7,19 +7,25 @@
 # LICENSE file in the root directory of this source tree. An additional grant
 # of patent rights can be found in the PATENTS file in the same directory.
 
-import errno
 import json
 import os
+import pathlib
 import stat
 import subprocess
+import sys
 from textwrap import dedent
-from typing import Optional, Set
+from typing import Optional, Sequence, Set
 
+import pexpect
 from eden.cli import util
 from eden.integration.lib.hgrepo import HgRepository
 
 from .lib import edenclient, testcase
+from .lib.fake_edenfs import read_fake_edenfs_argv_file
 from .lib.find_executables import FindExe
+from .lib.pexpect import PexpectAssertionMixin, wait_for_pexpect_process
+from .lib.service_test_case import ServiceTestCaseBase, service_test
+from .lib.temporary_directory import TemporaryDirectoryMixin
 
 
 # This is the name of the default repository created by EdenRepoTestBase.
@@ -401,3 +407,60 @@ echo -n "$1" >> "{scratch_file}"
         self.eden.run_cmd("unmount", new_mount)
         self.assertFalse(os.path.exists(os.path.join(new_mount, "hello")))
         self.assertEqual(custom_readme_text, util.read_all(bytes(readme_path, "utf-8")))
+
+
+@service_test
+class CloneFakeEdenFSTest(
+    ServiceTestCaseBase, PexpectAssertionMixin, TemporaryDirectoryMixin
+):
+    def setUp(self):
+        super().setUp()
+        self.eden_dir = pathlib.Path(self.make_temporary_directory())
+
+    def test_daemon_command_arguments_should_forward_to_edenfs(self) -> None:
+        argv_file = self.eden_dir / "argv"
+        assert not argv_file.exists()
+        repo = self.make_dummy_hg_repo()
+        mount_path = pathlib.Path(self.make_temporary_directory())
+
+        extra_daemon_args = ["--commandArgumentsLogFile", str(argv_file), "hello world"]
+        clone_process = self.spawn_clone(
+            repo_path=pathlib.Path(repo.path),
+            mount_path=mount_path,
+            extra_args=["--daemon-args"] + extra_daemon_args,
+        )
+        wait_for_pexpect_process(clone_process)
+
+        argv = read_fake_edenfs_argv_file(argv_file)
+        self.assertEquals(
+            argv[-len(extra_daemon_args) :],
+            extra_daemon_args,
+            f"fake_edenfs should have received arguments verbatim\nargv: {argv}",
+        )
+
+    def make_dummy_hg_repo(self) -> HgRepository:
+        repo = HgRepository(path=self.make_temporary_directory())
+        repo.init()
+        repo.write_file("hello", "")
+        repo.commit("Initial commit.")
+        return repo
+
+    def spawn_clone(
+        self,
+        repo_path: pathlib.Path,
+        mount_path: pathlib.Path,
+        extra_args: Sequence[str] = (),
+    ) -> "pexpect.spawn[str]":
+        args = [
+            "--config-dir",
+            str(self.eden_dir),
+            "clone",
+            "--daemon-binary",
+            FindExe.FAKE_EDENFS,
+            str(repo_path),
+            str(mount_path),
+        ]
+        args.extend(extra_args)
+        return pexpect.spawn(
+            FindExe.EDEN_CLI, args, encoding="utf-8", logfile=sys.stderr
+        )

eden/integration/helpers/fake_edenfs.cpp

@@ -14,10 +14,15 @@
 #include <gflags/gflags.h>
 #include <signal.h>
 #include <thrift/lib/cpp2/server/ThriftServer.h>
+#include <algorithm>
 #include <array>
 #include <chrono>
+#include <fstream>
+#include <iterator>
 #include <optional>
+#include <string>
 #include <thread>
+#include <vector>
 
 #include "eden/fs/eden-config.h"
 #include "eden/fs/fuse/privhelper/UserInfo.h"
@@ -38,6 +43,10 @@ DEFINE_string(
     cleanShutdownFile,
     "",
     "If set, create the given file when shutting down cleanly");
+DEFINE_string(
+    commandArgumentsLogFile,
+    "",
+    "If set, write argv to the given file before starting up");
 DEFINE_bool(
     exitWithoutCleanupOnStop,
     false,
@@ -81,6 +90,8 @@ std::string prettyPrint(
     std::chrono::duration<Rep, Ratio> duration,
     bool addSpace = true);
 
+void writeCommandArgumentsToLogIfNecessary(const std::vector<std::string>&);
+
 enum class StopBehavior {
   DoNothing,
   ExitWithoutCleanup,
@@ -295,8 +306,13 @@ int main(int argc, char** argv) {
   auto identity = UserInfo::lookup();
   identity.dropPrivileges();
 
+  auto originalCommandArguments =
+      std::vector<std::string>{&argv[0], &argv[argc]};
+
   auto init = folly::Init(&argc, &argv);
 
+  writeCommandArgumentsToLogIfNecessary(originalCommandArguments);
+
 #if EDEN_HAVE_SYSTEMD
   if (FLAGS_experimentalSystemd) {
     XLOG(INFO) << "Running in experimental systemd mode";
@@ -368,4 +384,19 @@ std::string prettyPrint(
       durationInSeconds.count(), folly::PRETTY_TIME, addSpace);
 }
 
+void writeCommandArgumentsToLogIfNecessary(
+    const std::vector<std::string>& originalCommandArguments) {
+  const auto& path = FLAGS_commandArgumentsLogFile;
+  if (path.empty()) {
+    return;
+  }
+  auto file = std::ofstream{};
+  file.exceptions(std::ofstream::badbit | std::ofstream::failbit);
+  file.open(path);
+  std::copy(
+      originalCommandArguments.begin(),
+      originalCommandArguments.end(),
+      std::ostream_iterator<std::string>{file, "\n"});
+}
+
 } // namespace

eden/integration/lib/fake_edenfs.py

@@ -62,3 +62,12 @@ class FakeEdenFS(typing.ContextManager[int]):
         with contextlib.suppress(ProcessLookupError):
             os.kill(self.process_id, signal.SIGTERM)
         return None
+
+
+def read_fake_edenfs_argv_file(argv_file: pathlib.Path) -> typing.List[str]:
+    try:
+        return list(argv_file.read_text().splitlines())
+    except FileNotFoundError as e:
+        raise Exception(
+            "fake_edenfs should have recognized the --commandArgumentsLogFile argument"
+        ) from e

eden/integration/start_test.py

@@ -9,6 +9,7 @@
 
 import os
 import pathlib
+import subprocess
 import sys
 import typing
 
@@ -18,8 +19,9 @@ from eden.cli.util import HealthStatus
 from fb303.ttypes import fb_status
 
 from .lib import testcase
+from .lib.fake_edenfs import read_fake_edenfs_argv_file
 from .lib.find_executables import FindExe
-from .lib.pexpect import PexpectAssertionMixin
+from .lib.pexpect import PexpectAssertionMixin, wait_for_pexpect_process
 from .lib.service_test_case import ServiceTestCaseBase, service_test
 from .lib.temporary_directory import TemporaryDirectoryMixin
 
@@ -117,6 +119,64 @@ class StartFakeEdenFSTest(
             f"The edenfs process should have separate process IDs",
         )
 
+    def test_daemon_command_arguments_should_forward_to_edenfs(self) -> None:
+        argv_file = self.eden_dir / "argv"
+        assert not argv_file.exists()
+
+        extra_daemon_args = [
+            "--commandArgumentsLogFile",
+            str(argv_file),
+            "--",
+            "hello world",
+            "--ignoredOption",
+        ]
+        start_process = self.spawn_start(daemon_args=extra_daemon_args)
+        wait_for_pexpect_process(start_process)
+
+        argv = read_fake_edenfs_argv_file(argv_file)
+        self.assertEquals(
+            argv[-len(extra_daemon_args) :],
+            extra_daemon_args,
+            f"fake_edenfs should have received arguments verbatim\nargv: {argv}",
+        )
+
+    def test_daemon_command_arguments_should_forward_to_edenfs_without_leading_dashdash(
+        self
+    ) -> None:
+        argv_file = self.eden_dir / "argv"
+        assert not argv_file.exists()
+
+        subprocess.check_call(
+            [
+                FindExe.EDEN_CLI,
+                "--config-dir",
+                str(self.eden_dir),
+                "start",
+                "--daemon-binary",
+                FindExe.FAKE_EDENFS,
+                "hello world",
+                "another fake_edenfs argument",
+                "--",
+                "--commandArgumentsLogFile",
+                str(argv_file),
+                "arg_after_dashdash",
+            ]
+        )
+
+        expected_extra_daemon_args = [
+            "hello world",
+            "another fake_edenfs argument",
+            "--commandArgumentsLogFile",
+            str(argv_file),
+            "arg_after_dashdash",
+        ]
+        argv = read_fake_edenfs_argv_file(argv_file)
+        self.assertEquals(
+            argv[-len(expected_extra_daemon_args) :],
+            expected_extra_daemon_args,
+            f"fake_edenfs should have received extra arguments\nargv: {argv}",
+        )
+
     def test_eden_start_fails_if_edenfs_is_already_running(self) -> None:
         with self.spawn_fake_edenfs(self.eden_dir) as daemon_pid:
             start_process = self.spawn_start()
@@ -150,3 +210,10 @@ class StartFakeEdenFSTest(
         return pexpect.spawn(
             FindExe.EDEN_CLI, args, encoding="utf-8", logfile=sys.stderr
         )
+
+    def __read_argv_file(self, argv_file: pathlib.Path) -> typing.List[str]:
+        self.assertTrue(
+            argv_file.exists(),
+            f"fake_edenfs should have recognized the --commandArgumentsLogFile argument",
+        )
+        return list(argv_file.read_text().splitlines())