src: don't match after `--` in `Dotenv::GetPathFromArgs` #54237

OnlyAviv · 2024-08-07T00:11:34Z

anonrig

Tests are needed

OnlyAviv · 2024-08-07T01:10:27Z

Is there is a specific file I should put the tests in? If not, I'll make a new file for them, but I wanted to make sure first.

codecov · 2024-08-07T01:28:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.10%. Comparing base (3ed9f98) to head (498011e).
Report is 460 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54237      +/-   ##
==========================================
- Coverage   87.10%   87.10%   -0.01%     
==========================================
  Files         647      647              
  Lines      181739   181755      +16     
  Branches    34886    34886              
==========================================
+ Hits       158307   158317      +10     
- Misses      16740    16742       +2     
- Partials     6692     6696       +4

Files with missing lines	Coverage Δ
src/node_dotenv.cc	`81.70% <100.00%> (+0.11%)`	⬆️

... and 28 files with indirect coverage changes

tniessen · 2024-08-07T14:21:55Z

optimizes the Dotenv::GetPathFromArgs function to not rely on string copying (strncmp)

Could you elaborate on that? strncmp itself does not copy anything.

OnlyAviv · 2024-08-07T14:30:12Z

strncmp itself does not copy anything.

I don't know too much about CPP internals, so I may have got that part wrong. From what I understood, it's faster to use .find than find_if. I can do a quick benchmark to verify if you'd like?

tniessen · 2024-08-07T14:34:49Z

I am not familiar with this part of the code base, but any iteration here is just going over the few, typically very short strings that were passed as command-line arguments, right? If this patch actually prevents copying unnecessarily, please explain that and I'm probably going to be in favor of such a change. Optimizing iteration over a few hundred characters, on the other hand, seems like it will be of little benefit.

src/node_dotenv.cc

xduugu · 2024-08-07T21:17:15Z

Thanks a lot for working on a fix, @RedYetiDev.

I just noticed I also included the -- separator in the commands that execute a script file in my original bug report (I just edited it). However, the separator should not be required in these cases:

node works.js --env-file nonexistent-file

So the argument processing need to stop when this condition is true:

arg == "--" || !arg.starts_with("-")

When looking at the code, I noticed another bug: every argument that starts with --env-file is recognized as --env-file, for example --env-file-asdf:

node --env-file-asdf nonexistent-file
node --env-file-asdf=nonexistent-file

both exit with

node: nonexistent-file: not found

OnlyAviv · 2024-08-07T21:23:07Z

When looking at the code, I noticed another bug: every argument that starts with --env-file is recognized as --env-file, for example --env-file-asdf:

That is indeed an issue, @anonrig do you want me to patch it here or in another PR?
(I can patch it here in a seperate commit?)

OnlyAviv · 2024-08-07T21:25:21Z

src/node_dotenv.cc

@@ -14,21 +14,23 @@ using v8::String;
 std::vector<std::string> Dotenv::GetPathFromArgs(
    const std::vector<std::string>& args) {
  const auto find_match = [](const std::string& arg) {
-    const std::string_view flag = "--env-file";
-    return strncmp(arg.c_str(), flag.data(), flag.size()) == 0;
+    return arg == "--" || arg.starts_with("--env-file");


Suggested change

return arg == "--" || arg.starts_with("--env-file");

return arg == "--" || arg == "--env-file" || arg.starts_with("--env-file=");

This is the patch for the behavior described:

(before) └─$ node --env-file-ABCD TEST node: TEST: not found (after) └─$ node --env-file-ABCD TEST node: bad option: --env-file-ABCD

xduugu · 2024-08-07T21:33:12Z

@anonrig The for loop @RedYetiDev used was much easier to understand for me. It might be less sophisticated (and a little less performant) as the current code, but wouldn't something like the following be a lot more readable? (probably no valid c++ code)

std::vector<std::string> Dotenv::GetPathFromArgs(const std::vector<std::string>& args) {
    const std::string_view flag = "--env-file";
    std::vector<std::string> paths;

    for (size_t i = 0; i < args.size(); ++i) {
        const std::string_view arg = args[i];

        // argument separator or script file
        if (arg == "--" || !arg.starts_with("-")) {
            break;
        }

        // --env-file <file>
        if (arg == flag) {
            if (i + 1 < args.size()) {
                paths.push_back(args[i + 1]);
                ++i;
            }
            // TODO else error?
        // --env-file=<file>
        } else if (arg.starts_with(flag + "=") {
            paths.push_back(std::string(arg.substr(flag.size() + 1)));
        }
    }

    return paths;
}

OnlyAviv · 2024-08-07T21:38:37Z

@anonrig The for loop @RedYetiDev used was much easier to understand for me. It might be less sophisticated (and a little less performant) as the current code, but wouldn't something like the following be a lot more readable? (probably no valid c++ code)

I'm planning to re-try the loop again in a new PR once this lands.

src/node_dotenv.cc

Co-Authored-By: Cedric Staniewski <[email protected]>

nodejs-github-bot · 2024-08-08T15:12:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/60978/

anonrig · 2024-08-08T15:19:31Z

@anonrig is there a reason this function is separate from the parsing of other, similar arguments? (Like -e and -p)?

--env-file supports NODE_OPTIONS. therefore, it needs to be executed/analyzed before V8 is initialized.

OnlyAviv · 2024-08-08T15:49:37Z

FWIW once this lands, I'll open a seperate PR re-writing the function in a way that fixes all these little bugs mentioned:

std::vector<std::string> Dotenv::GetPathFromArgs(
    const std::vector<std::string>& args) {
  std::vector<std::string> paths;
  for (size_t i = 1; i < args.size(); ++i) {
    const auto& arg = args[i];

    if (arg == "--" || arg[0] != '-') {
      break;
    }

    if (arg.starts_with("--env-file=")) {
      paths.push_back(arg.substr(11));  // Directly extract the path
    } else if (arg == "--env-file" && i + 1 < args.size()) {
      paths.push_back(args[++i]);  // Advance to the next argument
    }
  }
  return paths;
}

OnlyAviv · 2024-08-08T18:57:48Z

author ready?:

Passing CI (unrelated failure)
Approving Review
No requests for changes

nodejs-github-bot · 2024-08-10T19:46:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61004/

OnlyAviv · 2024-08-12T02:10:40Z

Hey, I think this is ready to land. No objections and a passing CI :-)

No rush tho

nodejs-github-bot · 2024-08-12T12:21:23Z

Commit Queue failed

- Loading data for nodejs/node/pull/54237
✔  Done loading data for nodejs/node/pull/54237
----------------------------------- PR info ------------------------------------
Title      src: don't match after `--` in `Dotenv::GetPathFromArgs` (#54237)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     RedYetiDev:dotenv-fix -> nodejs:main
Labels     c++, needs-ci, dotenv
Commits    1
 - src: don't match after `--` in `Dotenv::GetPathFromArgs`
Committers 1
 - RedYetiDev <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54237
Reviewed-By: Yagiz Nizipli <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54237
Reviewed-By: Yagiz Nizipli <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 07 Aug 2024 00:11:34 GMT
   ✔  Approvals: 1
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54237#pullrequestreview-2226545215
   ✘  This PR needs to wait 35 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-08-10T19:46:38Z: https://ci.nodejs.org/job/node-test-pull-request/61004/
- Querying data for job/node-test-pull-request/61004/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10351730728

BoscoDomingo · 2024-08-13T22:32:29Z

FWIW once this lands, I'll open a seperate PR re-writing the function in a way that fixes all these little bugs mentioned:

std::vector<std::string> Dotenv::GetPathFromArgs(
    const std::vector<std::string>& args) {
  std::vector<std::string> paths;
  for (size_t i = 1; i < args.size(); ++i) {
    const auto& arg = args[i];

    if (arg == "--" || arg[0] != '-') {
      break;
    }

    if (arg.starts_with("--env-file=")) {
      paths.push_back(arg.substr(11));  // Directly extract the path
    } else if (arg == "--env-file" && i + 1 < args.size()) {
      paths.push_back(args[++i]);  // Advance to the next argument
    }
  }
  return paths;
}

I already did change that piece of code in #53060. Might want to add there?

nodejs-github-bot · 2024-08-13T22:53:14Z

Commit Queue failed

- Loading data for nodejs/node/pull/54237
✔  Done loading data for nodejs/node/pull/54237
----------------------------------- PR info ------------------------------------
Title      src: don't match after `--` in `Dotenv::GetPathFromArgs` (#54237)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     RedYetiDev:dotenv-fix -> nodejs:main
Labels     c++, needs-ci, dotenv
Commits    1
 - src: don't match after `--` in `Dotenv::GetPathFromArgs`
Committers 1
 - RedYetiDev <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54237
Reviewed-By: Yagiz Nizipli <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54237
Reviewed-By: Yagiz Nizipli <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 07 Aug 2024 00:11:34 GMT
   ✔  Approvals: 1
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54237#pullrequestreview-2226545215
   ✘  This PR needs to wait 1 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-08-12T12:21:23Z: https://ci.nodejs.org/job/node-test-pull-request/61004/
- Querying data for job/node-test-pull-request/61004/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10378367203

nodejs-github-bot · 2024-08-13T23:56:58Z

Landed in 880c446

Co-Authored-By: Cedric Staniewski <[email protected]> PR-URL: #54237 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

targos · 2024-09-21T12:21:52Z

This would need a backport to land on v22.x as it uses C++20 features.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 7, 2024

OnlyAviv added the dotenv Issues and PRs related to .env file parsing label Aug 7, 2024

OnlyAviv force-pushed the dotenv-fix branch from 450e8a4 to 7a55485 Compare August 7, 2024 00:15

anonrig reviewed Aug 7, 2024

View reviewed changes

anonrig requested changes Aug 7, 2024

View reviewed changes

src/node_dotenv.cc Show resolved Hide resolved

OnlyAviv changed the title ~~src: optimize Dotenv::GetPathFromArgs~~ src: don't match after -- in Dotenv::GetPathFromArgs Aug 7, 2024

OnlyAviv force-pushed the dotenv-fix branch from 7a55485 to 8acc506 Compare August 7, 2024 20:04

OnlyAviv requested a review from anonrig August 7, 2024 20:04

OnlyAviv force-pushed the dotenv-fix branch from 8acc506 to 073daa7 Compare August 7, 2024 21:21

OnlyAviv commented Aug 7, 2024

View reviewed changes

anonrig reviewed Aug 7, 2024

View reviewed changes

src/node_dotenv.cc Show resolved Hide resolved

src/node_dotenv.cc Outdated Show resolved Hide resolved

OnlyAviv force-pushed the dotenv-fix branch 3 times, most recently from efb86ba to fc3c121 Compare August 7, 2024 21:44

OnlyAviv requested a review from anonrig August 7, 2024 21:45

OnlyAviv mentioned this pull request Aug 7, 2024

dotenv::GetPathFromArgs matches --env-file* #54255

Open

src: don't match after -- in Dotenv::GetPathFromArgs

498011e

Co-Authored-By: Cedric Staniewski <[email protected]>

OnlyAviv force-pushed the dotenv-fix branch from fc3c121 to 498011e Compare August 8, 2024 01:09

anonrig approved these changes Aug 8, 2024

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 8, 2024

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 12, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 12, 2024

BoscoDomingo mentioned this pull request Aug 13, 2024

src: add --env-file-if-exists flag #53060

Merged

anonrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 13, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 13, 2024

aduh95 approved these changes Aug 13, 2024

View reviewed changes

aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 13, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 13, 2024

nodejs-github-bot merged commit 880c446 into nodejs:main Aug 13, 2024
70 checks passed

OnlyAviv mentioned this pull request Aug 14, 2024

src: use for loop in Dotenv::GetPathFromArgs #54385

Closed

RafaelGSS mentioned this pull request Aug 19, 2024

v22.7.0 proposal #54452

Merged

targos added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label Sep 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: don't match after `--` in `Dotenv::GetPathFromArgs` #54237

src: don't match after `--` in `Dotenv::GetPathFromArgs` #54237

OnlyAviv commented Aug 7, 2024 •

edited

Loading

anonrig left a comment

OnlyAviv commented Aug 7, 2024

codecov bot commented Aug 7, 2024 •

edited

Loading

tniessen commented Aug 7, 2024

OnlyAviv commented Aug 7, 2024

tniessen commented Aug 7, 2024

xduugu commented Aug 7, 2024

OnlyAviv commented Aug 7, 2024 •

edited

Loading

OnlyAviv Aug 7, 2024 •

edited

Loading

xduugu commented Aug 7, 2024 •

edited

Loading

OnlyAviv commented Aug 7, 2024

nodejs-github-bot commented Aug 8, 2024

anonrig commented Aug 8, 2024

OnlyAviv commented Aug 8, 2024

OnlyAviv commented Aug 8, 2024

nodejs-github-bot commented Aug 10, 2024

OnlyAviv commented Aug 12, 2024 •

edited

Loading

nodejs-github-bot commented Aug 12, 2024

BoscoDomingo commented Aug 13, 2024

nodejs-github-bot commented Aug 13, 2024

nodejs-github-bot commented Aug 13, 2024

targos commented Sep 21, 2024

	return arg == "--" \|\| arg.starts_with("--env-file");
	return arg == "--" \|\| arg == "--env-file" \|\| arg.starts_with("--env-file=");

src: don't match after -- in Dotenv::GetPathFromArgs #54237

src: don't match after -- in Dotenv::GetPathFromArgs #54237

Conversation

OnlyAviv commented Aug 7, 2024 • edited Loading

anonrig left a comment

Choose a reason for hiding this comment

OnlyAviv commented Aug 7, 2024

codecov bot commented Aug 7, 2024 • edited Loading

Codecov Report

tniessen commented Aug 7, 2024

OnlyAviv commented Aug 7, 2024

tniessen commented Aug 7, 2024

xduugu commented Aug 7, 2024

OnlyAviv commented Aug 7, 2024 • edited Loading

OnlyAviv Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

xduugu commented Aug 7, 2024 • edited Loading

OnlyAviv commented Aug 7, 2024

nodejs-github-bot commented Aug 8, 2024

anonrig commented Aug 8, 2024

OnlyAviv commented Aug 8, 2024

OnlyAviv commented Aug 8, 2024

nodejs-github-bot commented Aug 10, 2024

OnlyAviv commented Aug 12, 2024 • edited Loading

nodejs-github-bot commented Aug 12, 2024

BoscoDomingo commented Aug 13, 2024

nodejs-github-bot commented Aug 13, 2024

nodejs-github-bot commented Aug 13, 2024

targos commented Sep 21, 2024

src: don't match after `--` in `Dotenv::GetPathFromArgs` #54237

src: don't match after `--` in `Dotenv::GetPathFromArgs` #54237

OnlyAviv commented Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 7, 2024 •

edited

Loading

OnlyAviv commented Aug 7, 2024 •

edited

Loading

OnlyAviv Aug 7, 2024 •

edited

Loading

xduugu commented Aug 7, 2024 •

edited

Loading

OnlyAviv commented Aug 12, 2024 •

edited

Loading