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 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.

@@            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     

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

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

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?

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.

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

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?)

@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;
}

@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.

Once this lands, I'll open a PR fixing the bug with --env-file.... Once that lands I'll open a PR refactoring the function for readability.

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

@redyetidev, there is one last thing still missing from the PR to fully fix #54232, which is being able to use a script argument --env-file without the -- separator.

This command will now work:

node script.js -- --env-file nonexistent-file

but it is quite unusual to require a -- as first argument of a script and node also does not require it:

Usage: node [options] [ script.js ] [arguments]

I would like to call the script like this:

node script.js --env-file nonexistent-file

or

./script.js --env-file nonexistent-file

To archive this, it should be enough to check for arguments that do not start with a hyphen everywhere where you check for --. Something like this could work:

Change

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

to

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

Change

if (*path == "--") {

to

if (*path == "--" || !(*path).starts_with("-")) {

No, the current behavior IMO is correct. Any arguments passed after the script are treated as script arguments, not arguments to Node itself.

Anything after --, in any case, won't be treated as an argument.

So this works now with your changes?

node script.js --env-file nonexistent-file

Because in the latest release it exists with

node: nonexistent-file: not found

No, that doesn't work, but I don't believe it should. Any arguments passed after the script to run are passed to the script, not to the node executable.

Landed in 880c446

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