path: resolve normalize drive letter to lower case

[dead-horse]

make path.resolve work the same as path.normalize.

see https://github.com/joyent/node/blob/v0.12/lib/path.js#L180

[dead-horse]

here make things inconsistency because process.pwd() and __dirname do not change to lower case. and actually i don't use windows, and dunno why we need to change device to lower case in path.normalize here,

also i think this will break somebody's code like:

var root = path.dirname(__dirname); // root = 'C:\\\\path'

var filepath = 'to/where';
var resolvedPath = path.join(root, filepath); // filepath = 'c:\\\\path\\to\\where';
if (resolvedPath.indexOf(root) !== 0) throw new Error('permission denied');

if we made this change, we should make all these things change together, and notify people in the document or at least in the changelog.

[dead-horse]

ping @trevnorris, @refack added this one dead-horse#1

[refack]

♬Than merge it ohh @dead-horse ♬ ohh @dead-horse ♬ ohh @dead-horse, ♯ merge it into your's ♭ . it'll propagate to here...

[trevnorris]

I'm pretty sure the exceeds the 80 char limit. Also, drop the { }.

[trevnorris]

@tjfontaine @orangemocha Either one of you mind taking a look? I'm not the one to give a +1 on this type of Windows change.

[dead-horse]

@refack merged

[refack]

👍

[orangemocha]

What is the point of this change? process.cwd() makes no promise of normalizing the result, and I think this change might not be necessary. I'd rather keep it case-preserving unless there is a good reason to change that.

[refack]

If we're normalizing paths, then as mentioned before, we need to normalize cwd() output, and thus reach consistency.
Anyway drive letter case is arbitrary, and depends only on the process's environment.

C:\>node -e console.log(process.cwd())
C:\

C:\>cmd /k cd "c:\temp"

c:\temp>node -e console.log(process.cwd())
c:\temp

It is semantically equivalent, but breaks allot of path comparisons as they are string based and case sensitive.

[orangemocha]

The part about making path.resolve normalize the drive letter looks good to me. I an not convinced we should be normalizing the drive letter in process.cwd().

[orangemocha]

change this to
cwd = path.normalize(cwd);
?

[dead-horse]

@refack sorry i revert your commit. maybe better to create another PR with that commit.

@orangemocha done

[orangemocha]

LGTM

[refack]

@orangemocha RE: my commit comments. Don't you agree we should normalize across the board?

[orangemocha]

@refack, your change addresses the scenario process.cwd() === path.resolve('.') but it doesn't address all the scenarios in which case-sensitive comparisons could be broken on Windows. You can set arbitrary casing in the process current directory by passing it to CreateProcess, and it will be reflected verbatim in process.cwd(). Doing a case sensitive comparison against anything coming from readdir will fail.

The bottom line is that case-sensitive path comparison is not the correct way of comparing paths on windows. And this arbitrary conversion to lower-case of the drive letter mitigates the problem only partially.

The first part of the pull request is a no brainer, because the docs state that path.resolve will normalize the result, and that is currently broken. But this other part raises all sorts of questions about how to do path comparisons correctly on Windows, for which I would like us to think of a more robust long-term solution.

@tjfontaine : thoughts? In terms of a more robust solution, I can think of the following:
a) Introducing an api for path comparison, eg path.equals() and path.startsWith(), that would do the right thing on all platforms. This is the truly portable solution, but it needs a new API.
b) Making path.normalize convert everything to lower-case (or upper-case). Not as robust as b), but it doesn't need a new API.

[refack]

Agreed.
I thought about suggesting a new path comparison method, but who has the energy....

[orangemocha]

@refack this is causing a test failure: http://jenkins.nodejs.org/view/node/job/node-review-windows/166/DESTCPU=ia32,label=windows/tapTestReport/simple.tap-432/

See https://github.com/joyent/node/blob/master/test/simple/test-module-nodemodulepaths.js#L32
I think the value of 'file' should go through path.normalize()

[orangemocha]

sorry, the last comment was meant for @dead-horse

[dead-horse]

my bad. @orangemocha done

[orangemocha]

Thank you, landed in f6e5740

[UltCombo]

@dead-horse @orangemocha Indeed, the driver letter casing inconsistency did break the minimatch matching in JSCS (jscs-dev/node-jscs#703).

Sorry if I'm late to the party, but when/why was it decided that drive letters should be normalized to lowercase? As far as I can see, this has mostly been just breaking existing code.

[dead-horse]

This patch is because path.normalize normalize drive letter to lower case but path.resolve didn't. This bugged me in some situation.

[UltCombo]

@dead-horse makes sense. However, path.dirname still returns an uppercase drive letter (at least in Node 0.11.14) and that's causing a real issue for me, similar to the one you outlined in this comment.

[orangemocha]

@UltCombo for lack for a better solution, I would suggest your normalize the result of path.dirname, or use case-insensitive comparison on Windows.

[UltCombo]

@orangemocha those were my thoughts as well, thanks. =]

[piscisaureus]

The patch a05f973 was landed to make some tests pass, but I can't really reconstruct what the motivation was. What I can tell is that the test failure wasn't really related to path.normalize() and path.resolve() [not] changing the drive letter case.

This patch has caused a lot of issues, including #7031 and #7806 and lots of bikeshedding about whether uppercase is the right case or lowercase.

I propose reverting the offending patch, and just doing what node does everywhere else (which is: path functions don't touch the case) and take another look at the cause of those test failures.

[UltCombo]

@piscisaureus +1 for reverting and not touching the case.

[llafuente]

We should use the same case as __dirname or process.pwd(), I'm not sure but I think I never saw it uppercased so assume lowercase.

Also means, always lowercase everywhere. No different behaviour between normalizer, resolve, dirname, __dirname, etc...

Not touching the case could lead to problems between modules, or even, your own code.

Path is windows are case-insensitive so developers should handle it properly.

mypath.toLowerCase().indexOf(test.path.toLowerCase())

I think this is a PITA, so I would even sugest to lowercase everything...

as @UltCombo suggest, I temporary revert and fix all the problem as a whole.

[orangemocha]

I am not opposed to reverting a05f973, as it seems to have done more harm than good. However, I think that the real solution to all these issues is introducing a cross-platform path comparison API, and make it case-insensitive on Windows. I will work on a proposal (though likely not for another couple of weeks).

[UltCombo]

@llafuente @orangemocha worth noting that the require.cache is case sensitive, even on platforms that have case-insensitive paths. E.g.:

require('./a');
require('./A');

Both requires will resolve to the same file on Windows, initialize it twice and generate two entries in the require cache. The same issue would happen with different drive letter casing, or, actually, any casing discrepancy in the path.

I believe we can keep the original path casing, and do path comparisons in a case-insensitive manner in Windows, through a new cross-platform path comparison API as suggested by @orangemocha.