Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: return first folder made by mkdir recursive #31530

Closed
wants to merge 5 commits into from

Conversation

@bcoe
Copy link
Member

bcoe commented Jan 27, 2020

mkdir('/foo/bar', { recursive: true }) and mkdirSync will now return
the path of the first folder created. This matches more closely
mkdirp's behavior, and is useful for setting folder permissions.

Refs: #31481

@isaacs mind testing out this branch and seeing if it matches the functionality you were asking for, i.e., populating the path appropriately.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@nodejs-github-bot

This comment has been minimized.

@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Jan 27, 2020

@joaocgreis any thoughts on this one, the Windows tests seem to have added a strange prefix to the path I return, ////?; The path looks otherwise correct.

Copy link
Member

jasnell left a comment

LGTM as a semver-minor. Since there was not previously a return value, adding a return value shouldn't be breaking. Docs likely need to be updated also, however.

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Jan 27, 2020

the Windows tests seem to have added a strange prefix to the path I return, ////?

That's coming from the use of toNamespacedPath() in the path module (https://github.com/nodejs/node/blob/master/lib/path.js#L549). Generally, you'll want to reverse what that module is adding.

@bcoe bcoe added semver-minor and removed semver-major labels Jan 27, 2020
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

src/inspector_profiler.cc Outdated Show resolved Hide resolved
src/inspector_profiler.cc Outdated Show resolved Hide resolved
src/node_file-inl.h Outdated Show resolved Hide resolved
src/node_file.cc Outdated Show resolved Hide resolved
src/node_file.h Show resolved Hide resolved
src/node_file.h Outdated Show resolved Hide resolved
src/node_file.cc Outdated Show resolved Hide resolved
src/node_file.cc Outdated Show resolved Hide resolved
@Trott
Trott approved these changes Jan 29, 2020
mkdir('/foo/bar', { recursive: true }) and mkdirSync will now return
the path of the first folder created. This matches more closely
mkdirp's behavior, and is useful for setting folder permissions.

Refs: #31481
@bcoe bcoe force-pushed the bcoe:mkdirp-return branch to e865d43 Jan 29, 2020
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

bcoe added 3 commits Jan 30, 2020
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment has been minimized.

Copy link

nodejs-github-bot commented Jan 31, 2020

@bcoe bcoe added the author ready label Jan 31, 2020
bcoe added a commit that referenced this pull request Jan 31, 2020
mkdir('/foo/bar', { recursive: true }) and mkdirSync will now return
the path of the first folder created. This matches more closely
mkdirp's behavior, and is useful for setting folder permissions.

PR-URL: #31530
Refs: #31481
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@bcoe

This comment has been minimized.

Copy link
Member Author

bcoe commented Jan 31, 2020

Landed in 13fe56b

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Mar 9, 2020

hey @bcoe do we want to backport this to 13.x?

MylesBorins added a commit that referenced this pull request Mar 10, 2020
mkdir('/foo/bar', { recursive: true }) and mkdirSync will now return
the path of the first folder created. This matches more closely
mkdirp's behavior, and is useful for setting folder permissions.

PR-URL: #31530
Refs: #31481
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Mar 10, 2020

After some other backports landed I was able to get this to land with minimal changes 🎉

@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
MylesBorins added a commit that referenced this pull request Mar 10, 2020
Notable changes:

* [[`ff58854dbe`](ff58854)] - **(SEMVER-MINOR)** **fs**: return first folder made by mkdir recursive (Benjamin Coe) [#31530](#31530)
* [[`258a80d3cc`](258a80d)] - **(SEMVER-MINOR)** **src**: create a getter for kernel version (Juan José Arboleda) [#31732](#31732)
* [[`4d5981be96`](4d5981b)] - **(SEMVER-MINOR)** **async_hooks**: add sync enterWith to ALS (Stephen Belanger) [#31945](#31945)
* [[`dd83bd266d`](dd83bd2)] - **(SEMVER-MINOR)** **wasi**: add returnOnExit option (Colin Ihrig) [#32101](#32101)

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 11, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 11, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 12, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
MylesBorins added a commit that referenced this pull request Mar 12, 2020
Notable changes:

* async_hooks:
  - add sync enterWith to ALS (Stephen Belanger)
    #31945
* cli:
  - allow --jitless V8 flag in NODE\_OPTIONS (Andrew Neitsch)
    #32100
* fs:
  - return first folder made by mkdir recursive (Benjamin Coe)
    #31530
* n-api:
  - define release 6 (Gabriel Schulhof)
    #32058
* src:
  - create a getter for kernel version (Juan José Arboleda)
    #31732
* wasi:
  - add returnOnExit option (Colin Ihrig)
    #32101

PR-URL: #32185
@coreyfarrell

This comment has been minimized.

Copy link
Member

coreyfarrell commented Mar 12, 2020

Will this get backported to 10 or 12 after the two week delay?

addaleax added a commit to addaleax/node that referenced this pull request Mar 25, 2020
Refs: nodejs#31530
@addaleax addaleax mentioned this pull request Mar 25, 2020
3 of 3 tasks complete
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Mar 25, 2020

If this is backported to v12.x, please include #32490 (with REPLACEME as the version in the backport…)

addaleax added a commit that referenced this pull request Mar 29, 2020
Refs: #31530

PR-URL: #32490
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax added a commit that referenced this pull request Mar 30, 2020
Refs: #31530

PR-URL: #32490
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.