919769 - OS.Path.normalize doesn't handle forward slashes correctly on Windows

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Brandon Benvie [:benvie]]

The Windows version of OS.Path.normalize has a couple problems with handling forward slashes.

First, it only replaces the first forward slash with a back slash.

> OS.Path.normalize("a/b/c")
> > a\b/c

Second, it strips out the forward slash if it comes after the drive and doesn't replace it with a backslash.

> OS.Path.normalize("c:/a")
> > c:a

Comment 1

[Brandon Benvie [:benvie]]

Attachment: bug-919769.patch

https://tbpl.mozilla.org/?tree=Try&rev=e3d204b15d0e

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 808833 [details] [diff] [review]
bug-919769.patch

Review of attachment 808833 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/modules/ospath_win.jsm
@@ +214,5 @@
>  let normalize = function(path) {
>    let stack = [];
>  
> +  // Normalize "/" to "\\"
> +  path = path.replace(/\//g, "\\");

That's better, but doing things here will introduce another error if we have a path that looks like
 \\foo/a/b/c/d
in this case, the forward slash is a legal character inside a file name and "foo/a/b/c/d" should not be converted to "foo\\a\\b\\c\\d".

I believe it is sufficient to perform that transformation only if |path| doesn't start with "\\".

::: toolkit/components/osfile/tests/xpcshell/test_path.js
@@ +101,5 @@
> +  do_check_eq(Win.normalize("c:////a/b/c"), "c:\\a\\b\\c");
> +  do_check_eq(Win.normalize("c:/a/b/c///"), "c:\\a\\b\\c");
> +  do_check_eq(Win.normalize("c:/a/b/c/../../../d/e/f"), "c:\\d\\e\\f");
> +  do_check_eq(Win.normalize("c:a/b/c/../../../d/e/f"), "c:d\\e\\f");
> +

Could you also add the basename, winGetDrive, etc. tests here?

Comment 3

[Brandon Benvie [:benvie]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Could you also add the basename, winGetDrive, etc. tests here?

None of those currently handle forward slashes at all. The uses I've seen involve first using normalize on a path and then feeding it into those. Should I make them handle forward slashes?

Comment 4

[Brandon Benvie [:benvie]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> I believe it is sufficient to perform that transformation only if |path|
> doesn't start with "\\".

Do you mean one or two backslashes? As in, is that escaped already or not?

Comment 5

[Brandon Benvie [:benvie]]

Attachment: bug-919769.patch

Don't convert forward slashes if the path begins with two backslashes.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 809343 [details] [diff] [review]
bug-919769.patch

Review of attachment 809343 [details] [diff] [review]:
-----------------------------------------------------------------

You correctly inferred that I meant "\\\\".
Also, you are right, let's not add tests for winGetDrive, etc.

Comment 7

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/9ba4408fb621

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ba4408fb621