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

readdirSync of drive letter lists items of cwd #9378

Closed
cosmo0 opened this issue Oct 31, 2016 · 29 comments
Closed

readdirSync of drive letter lists items of cwd #9378

cosmo0 opened this issue Oct 31, 2016 · 29 comments
Labels
confirmed-bug Issues with confirmed bugs. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors. windows Issues and PRs related to the Windows platform.

Comments

@cosmo0
Copy link

cosmo0 commented Oct 31, 2016

  • Version: v6.9.1
  • Platform: Windows 8.1 x64

ReaddirSync behavior depends on the current working directory (cwd/pwd) if we pass a drive letter with or without a trailing "".

It's very easy to reproduce. Here, I am on "d:" drive, and I have a "d:\Temp" folder, containing "A" and "B" sub-folders.

D:\Temp>  gci | % { $_.Name }
A
B

D:\Temp> node
> var fs = require('fs');
undefined
> fs.readdirSync('d:')
[ 'A', 'B' ]
> fs.readdirSync('d:\\')
[ '$RECYCLE.BIN',
  'Cache',
  'DEV',
  'Installers',
  'log',
  'pagefile.sys',
  'System Volume Information',
  'Temp',
  'VM' ]
> .exit
D:\Temp>

It's a very suprising behavior, and more importantly, it's not documented if expected.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Oct 31, 2016

Can reproduce on Win 7 x64 with Node.js 7 with additional observation: fs.readdirSync('d:') != fs.readdirSync('d:\\') only if d: is a current disk and cwd != d:\.

However, if d: is a current disk, fs.readdirSync('c:') still = fs.readdirSync('c:\\').

@cosmo0
Copy link
Author

cosmo0 commented Oct 31, 2016

Yep, forgot to mention that it's only when doing readdirSync on the "current" disk (the one on which node has been launched).

I haven't tested, but I assume the problem also exists with readdir.

@vsemozhetbyt
Copy link
Contributor

1

@vsemozhetbyt
Copy link
Contributor

I haven't tested, but I assume the problem also exists with readdir.

Yep.

2

@Fishrock123 Fishrock123 added fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform. labels Oct 31, 2016
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Nov 1, 2016

It seems that dir command has the same quirk: if the drive letter is not followed by a path (\ at least), the cwd content is displayed.

J:\temp>dir . /b
-arch
-mus
-v
Lv

J:\temp>dir j: /b
-arch
-mus
-v
Lv

J:\temp>dir j:\ /b
DAT
temp

I can't find this in the dir documentation, maybe this is documented in some other place.

@cosmo0
Copy link
Author

cosmo0 commented Nov 1, 2016

Oh, it makes sense then. The dir and cd commands behave weirdly with multiple drives (like the cd on another drive that doesn't change the drive letter).

I would say that it's still a surprising behavior, and thus should be fixed.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Nov 1, 2016

Maybe the \ path should be added to a bare letter path argument in the beginning of the both functions before the binding.readdir call, if we want consistent with POSIX behavior. Or the quirk could be documented.

@addaleax
Copy link
Member

addaleax commented Nov 1, 2016

So, I’m not a frequent Windows user, but from the little experience I have with drive-specific working dirs, it seems like Node is behaving as expected? Maybe this should just be mentioned in the docs?

@vsemozhetbyt
Copy link
Contributor

I am not sure users expect from Node fs functions the same quirks as from the shell with the old DOS roots. Maybe I am wrong.

@addaleax
Copy link
Member

addaleax commented Nov 1, 2016

If I’m reading this issue correctly, there’s nothing shell-specific about this, which is kind of why Node is having this behaviour? I mean, yes, drive-specific wds are not very intuitive when first encountering them, but at least they are consistent…

Also, two questions:

  • If somebody relies on/wants the current behaviour of fs.readdir('d:'), how would they replicate that when d: is translated to d:\?
  • If this is about cross-platform consistency, would libuv be the right place to raise this?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Nov 1, 2016

It seems you are right. So maybe this just could be added to fs documentation. However, it is unclear for me how wide the impact is and how many fs methods could be affected.

@cosmo0
Copy link
Author

cosmo0 commented Nov 1, 2016

I agree with @vsemozhetbyt .
The current behavior may be consistent with how batch work, but I'm pretty sure most developers these days are not familiar with the horrors that lurk in batch scripts.
Keeping a weirdness that will trip people up just to be consistent with a 30 years old tech seems... wrong to me.

I would also argue, more importantly, that it's dependent on the drive you were on when starting node. So the behavior will not be the same for everyone.
For instance, if I distribute my app through NW.js (which I will) or Electron, depending on the drive the app is installed on, it may or may not work.
You might also have multiple servers with multiple configurations (it's not a good idea, but we're talking bad-ideas-that-should-work here), and your web site will work on one and not the other.

@S-YOU
Copy link

S-YOU commented Nov 1, 2016

I believe it is not specific to particular command, but windows built-in behavior since ever.

if you just type d: in command prompt its change the drive and keep last cwd in that drive.
You can copy files across drives without specifying path, if you know you were in that folder.

@joaocgreis
Copy link
Member

This is surprising Windows behavior, not Node behavior. What is surprising is that drive letters are not exactly like directory paths, and cannot be expected to work exactly the same. Drives (like C:) are very Windows specific, so they should have Windows behavior. I do not see a problem of consistency because directories (like C:\) behave consistently.

Node is correct as it is: https://github.com/nodejs/node/blob/3c1e5b366f/lib/path.js#L170-L175 . Highlighting this in the docs would be great, I believe this is often unexpected for people without Windows background.

@joaocgreis joaocgreis added the good first issue Issues that are suitable for first-time contributors. label Nov 3, 2016
@cosmo0
Copy link
Author

cosmo0 commented Nov 4, 2016

I insist on the fact that the behavior depends on the drive the user was when running node.
Running the set of fs commands after

C:\temp> node

yields different results than after

D:\temp> node

Same if I have an electron/nw.js app that users installed on different drives, or if I set a different cwd on my shortcut to this app : my users will have different behaviors with the same codebase.

That Windows is broken in the same way seems of little importance to me. Replicating the Windows behavior will break things for many people, and I'm pretty sure (almost) nobody relies on it. Of course, I may be wrong, but I doubt it.

I believe this is often unexpected for people without Windows background.

I've been working on Windows for more than 10 years and have never encountered this (I knew about cd weirdness but not dir).
Plus, most Windows coders don't ever use the console.

@cosmo0
Copy link
Author

cosmo0 commented Nov 4, 2016

I just found out that Unix also has weirdnesses when using ls without trailing slash, so maybe it's much more widely known that I think.

Here using oh-my-zsh on MacOS Sierra:

> ls -l /tmp
lrwxr-xr-x@ 1 root  wheel  11 10 oct 15:13 /tmp -> private/tmp
> ls -l /tmp/
total 8
drwxr-xr-x@ 6 tbroust  wheel  204  4 nov 09:37 VSCode Crashes
drwx------  3 tbroust  wheel  102  4 nov 09:32 com.apple.launchd.0LqMMQjDgk
drwx------  3 tbroust  wheel  102  4 nov 09:32 com.apple.launchd.0vXKPX7vRQ

With this additional information, I agree that just adding a warning in the doc is more pertinent.

@addaleax
Copy link
Member

addaleax commented Nov 4, 2016

@cosmo0 I think that doesn’t affect readdir, though. (Also, I think the ls behaviour makes sense here, but that’s obviously subjective, too. 😄)

@S-YOU
Copy link

S-YOU commented Nov 4, 2016

> ls -l /tmp
lrwxr-xr-x@ 1 root  wheel  11 10 oct 15:13 /tmp -> private/tmp

Link itself is a file, so that is the correct behavior, imo.

@joaocgreis
Copy link
Member

@cosmo0 Can you show what you get when you run node from different drives? Before running node, make sure the cwd is set for both, by changing to the drive and cd to the directory. Then run env and look for the !C:= and !D:= lines on the top.

@cosmo0
Copy link
Author

cosmo0 commented Nov 7, 2016

Running readdirSync('c:') then readdirSync('d:') without trailing slash from c:\temp then d:\temp :

I am using Powershell, not cmd. More on that below.

From c:\temp, we get the current directory of c: then the root of d:

C:\temp> node
> require('fs').readdirSync('c:')
[ X', 'Y' ]
> require('fs').readdirSync('d:')
[ '$RECYCLE.BIN',
  'DEV',
  'pagefile.sys',
  'System Volume Information',
  'Temp',
  'VM' ]

then from d:\temp we get the root of c: and the current directory of d:

D:\temp> node
> require('fs').readdirSync('d:')
[ 'A', 'B' ]
> require('fs').readdirSync('c:')
[ '$Recycle.Bin',
  'BOOTNXT',
  'Config.Msi',
  'Documents and Settings',
  'hiberfil.sys',
  'MSOCache',
  'PerfLogs',
  'Program Files',
  'Program Files (x86)',
  'ProgramData',
  'swapfile.sys',
  'System Volume Information',
  'temp',
  'Users',
  'Windows' ]

As you can see, depending on where you run node from, the result of the same command is different.


Your request of env result has led me to realize that it also depends on which shell I use. The result above is from Powershell. cmd "remembers" different cwd for each drives, but Powershell doesn't.

In Powershell:

D:\temp> env
!::=::\
ALLUSERSPROFILE=C:\ProgramData
...
D:\temp> cd c:\temp
C:\temp> env
!::=::\
ALLUSERSPROFILE=C:\ProgramData
...
C:\temp>

Which leads to yet another behavior when using cmd to run node:

[d:\Temp]
> node
> require('fs').readdirSync('c:')
[ '$Recycle.Bin',
  'BOOTNXT',
  'Config.Msi',
  'Documents and Settings',
  'hiberfil.sys',
  'MSOCache',
  'PerfLogs',
  'Program Files',
  'Program Files (x86)',
  'ProgramData',
  'swapfile.sys',
  'System Volume Information',
  'temp',
  'Users',
  'Windows' ]
> require('fs').readdirSync('d:')
[ 'A', 'B' ]
> .exit

[d:\Temp]
> cd /d c:\temp

[c:\temp]
> env
!::=::\
!C:=c:\temp
!D:=d:\Temp
...

[c:\temp]
> node
> require('fs').readdirSync('c:')
[ 'X', 'Y' ]
> require('fs').readdirSync('d:')
[ 'A', 'B' ]
> .exit

@joaocgreis
Copy link
Member

The cmd behavior is correct. PowerShell is indeed not working as expected. I'll try to fix this but it may take me a while, so if someone else wants to give it a try, please do.

Note: good first contribution refers to the doc clarification above, fixing this might be harder.

@cosmo0 cosmo0 added confirmed-bug Issues with confirmed bugs. and removed confirmed-bug Issues with confirmed bugs. labels Nov 7, 2016
@joaocgreis
Copy link
Member

joaocgreis commented Nov 7, 2016

I added the confirmed-bug label just now, not @cosmo0 (GitHub bug?).

@cosmo0
Copy link
Author

cosmo0 commented Nov 7, 2016

Apparently I both added and deleted the label 😄

@richardlau
Copy link
Member

According to this link changing location in PowerShell doesn't actually change the cwd for non-PowerShell commands.

@sam-github sam-github added doc Issues and PRs related to the documentations. and removed doc Issues and PRs related to the documentations. docs-requested labels Dec 1, 2016
@XadillaX
Copy link
Contributor

Has this issue been resolved?

@gibfahn
Copy link
Member

gibfahn commented May 20, 2017

Has this issue been resolved?

Don't think so, the behaviour still needs documenting.

@XadillaX
Copy link
Contributor

@gibfahn But in my computer, readdirSync("c:") under D: seems return my home dir but not root of C now.

Eg. If my username is xadillax, readdirSync("c:") returns the list of c:\Users\XadillaX\.

@gibfahn
Copy link
Member

gibfahn commented May 20, 2017

Yes, that's the expected behaviour. When you use C: without the \ from a different drive, you get the current working directory of C:, not the root.

C:          :: cd to the current working directory in C:, in your case c:\Users\XadillaX\
cd Desktop  :: Changes the C: cwd to c:\Users\XadillaX\Desktop
D:          :: Changes to the current working directory in D:
dir C:      :: Shows the contents of the cwd in C: , now c:\Users\XadillaX\Desktop
dir C:\     :: Shows the contents of the root of C

It should be documented, but it's a Windows oddity, not a node one. You just have to remember that C:\ means "the root of drive C", and C: means "the last directory you were in in drive C".

D:\> C:

C:\Users\gib> cd Desktop

C:\Users\gib\Desktop> D:

D:\> dir C:
 Volume in drive C has no label.
 Volume Serial Number is E0D4-8E7B

 Directory of C:\Users\gib\Desktop

04/05/2017  11:46 AM    <DIR>          .
04/05/2017  11:46 AM    <DIR>          ..
               0 File(s)              0 bytes
               2 Dir(s)  71,552,552,960 bytes free

D:\> dir C:\

 Directory of C:\
04/12/2017  10:12 AM    <DIR>          Users
05/03/2017  10:32 AM    <DIR>          Windows
               2 File(s)          4,112 bytes
              12 Dir(s)  71,552,552,960 bytes free

bzoz added a commit to JaneaSystems/node that referenced this issue May 31, 2017
Add note to fs.md and path.md about Windows using per-drive current
working directory.

Fixes: nodejs#9378
@bzoz
Copy link
Contributor

bzoz commented May 31, 2017

I've created a PR for doc update here: #13330

I think that node behaves here correctly. It gives the same results (on both cmd and powershell) as this simple prog:

#include <windows.h>
#include <iostream>

int main() {
int main() {
  TCHAR  buffer[1024]; 
  GetFullPathName("C:", 1024, buffer, nullptr);
  std::cout << "c: " << buffer << "\n";
  GetFullPathName("D:", 1024, buffer, nullptr);
  std::cout << "d: " << buffer << "\n";
}

@bzoz bzoz closed this as completed in 422722f Jun 6, 2017
jasnell pushed a commit that referenced this issue Jun 7, 2017
Add note to fs.md and path.md about Windows using per-drive current
working directory.

Fixes: #9378
PR-URL: #13330
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.