Skip to content

Commit

Permalink
Fix "--" parsing in NestedCommandLineApp
Browse files Browse the repository at this point in the history
Summary:
Adjusts the parsing order so that folly doesn't incorrectly consume multiple "--" arguments.

# -- in unix

Folly is a facebook library for many common C++ operations. It's open-source.

"--" is a unix convention ([example](https://unix.stackexchange.com/questions/11376/what-does-double-dash-mean)). It means: Anything after "--" should be parsed as a positional argument. This allows you to nest CLIs together.

For example "buck run" is a CLI, with various options. The binary that is built may have options. When you want to pass something to the underlying binary, rather than buck run, you can use "--":

```
buck run //my:binary --buck-option 1 -- --mybinary-option 2
```

This prevents buck from accidentally thinking "--mybinary-option" is an arg that should be parsed by the buck binary.

One such positional argument that can occur is "--" itself. In this case, the first "--" tells the binary to treat every subsequent arg as a positional arg, and the next "--" is interpreted **as a positional arg**:

```
buck run -- --wrapper-option -- --inner-option
```

The positional args here are:
["--wrapper-option", "--", "--inner-option"]

This allows for an arbitrary number of wrappers to be chained together.

# What's wrong / The Fix

The current logic will never allow a "--" to be passed as a positional argument, as it does not consider if it's already seen a "--". We adjust the order to:
1. If we've already seen "--" ALWAYS treat the arg as a positional arg
2. If it's the first "--", consume it. All subsequent args are positional.
3. If we haven't yet seen "--", it should be included when looking for options.

That way, we don't accidentally delete a "--" that's intended for an underlying parser.

Reviewed By: simpkins

Differential Revision: D44892191

fbshipit-source-id: 58ff1a993cd68e0a65aa039bee588bc659c8ad8d
  • Loading branch information
Julie Ganeshan authored and facebook-github-bot committed Apr 12, 2023
1 parent f09d042 commit d9b004d
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 5 deletions.
6 changes: 3 additions & 3 deletions folly/experimental/NestedCommandLineApp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -284,10 +284,10 @@ void NestedCommandLineApp::doRun(const std::vector<std::string>& args) {
std::vector<std::string> endArgs;

for (auto& na : args) {
if (na == "--") {
not_clean = true;
} else if (not_clean) {
if (not_clean) {
endArgs.push_back(na);
} else if (na == "--") {
not_clean = true;
} else {
cleanArgs.push_back(na);
}
Expand Down
13 changes: 11 additions & 2 deletions folly/experimental/test/NestedCommandLineAppTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,19 +109,28 @@ TEST(ProgramOptionsTest, DevFull) {
}

TEST(ProgramOptionsTest, CutArguments) {
// anything after -- is parsed as arguments
// anything after -- is parsed as arguments, including more --
EXPECT_EQ(
"running foo\n"
"foo global-foo 43\n"
"foo local-foo 42\n"
"foo conflict-global 42\n"
"foo conflict 42\n"
"foo arg b\n"
"foo arg --\n"
"foo arg --local-foo\n"
"foo arg 44\n"
"foo arg a\n",
callHelper(
{"foo", "--global-foo", "43", "--", "b", "--local-foo", "44", "a"}));
{"foo",
"--global-foo",
"43",
"--",
"b",
"--",
"--local-foo",
"44",
"a"}));
}

TEST(ProgramOptionsTest, Success) {
Expand Down

0 comments on commit d9b004d

Please sign in to comment.