Last Comment Bug 707696 - [OS.File] Path handling
: [OS.File] Path handling
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: David Rajchenbach-Teller [:Yoric] (please use "needinfo")
:
Mentors:
Depends on:
Blocks: 707694 OS.File 764436 769310
  Show dependency treegraph
 
Reported: 2011-12-05 08:11 PST by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2012-12-13 09:52 PST (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1. The API (10.91 KB, patch)
2011-12-14 09:52 PST, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Unix test suite (6.64 KB, patch)
2012-06-19 07:30 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Unix back-end (7.14 KB, patch)
2012-06-19 07:33 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Review
Shared code (922 bytes, patch)
2012-06-19 07:37 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Unix test suite (3.01 KB, patch)
2012-06-19 15:35 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
taras.mozilla: review+
Details | Diff | Review
Unix back-end (4.22 KB, patch)
2012-06-19 15:36 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Review
Windows test suite (5.15 KB, patch)
2012-06-19 18:12 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Review
Windows back-end (5.71 KB, patch)
2012-06-19 18:15 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Review
Windows test suite (5.12 KB, patch)
2012-06-21 07:26 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Windows back-end (9.06 KB, patch)
2012-06-21 07:30 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Review
Windows back-end (9.34 KB, patch)
2012-06-21 08:25 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
taras.mozilla: review-
Details | Diff | Review
Windows test suite (5.06 KB, patch)
2012-06-21 08:25 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
taras.mozilla: review+
Details | Diff | Review
Unix back-end (4.25 KB, patch)
2012-06-22 05:55 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Review
Windows back-end (9.62 KB, patch)
2012-06-22 06:01 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
taras.mozilla: review+
Details | Diff | Review
Windows back-end (9.62 KB, patch)
2012-07-03 07:20 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Review
Unix back-end (4.25 KB, patch)
2012-07-03 07:21 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Review
Unix test suite (2.97 KB, patch)
2012-07-03 07:21 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Review
Windows test suite (4.99 KB, patch)
2012-07-03 07:22 PDT, David Rajchenbach-Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-05 08:11:58 PST
We need a module comparable to Python os.path.
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2011-12-14 09:52:52 PST
Created attachment 581686 [details] [diff] [review]
v1. The API

The API. Implementation for Unix should be sufficient, implementation for Windows is incomplete. Also missing a number of well-known directories.
Comment 2 Dave Herman [:dherman] 2011-12-20 22:28:52 PST
Comment on attachment 581686 [details] [diff] [review]
v1. The API

i'm not the right person to review; I'd ask one of the module owners of toolkit:

https://wiki.mozilla.org/Modules/Toolkit

Dave
Comment 3 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-19 07:30:08 PDT
Created attachment 634414 [details] [diff] [review]
Unix test suite
Comment 4 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-19 07:33:33 PDT
Created attachment 634416 [details] [diff] [review]
Unix back-end
Comment 5 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-19 07:37:36 PDT
Created attachment 634417 [details] [diff] [review]
Shared code
Comment 6 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-19 07:42:33 PDT
Comment on attachment 634416 [details] [diff] [review]
Unix back-end

Taras, I have found myself blocked a few times because we have no portable way to represent paths in OS.File, so I have put together a prototype implementation. Could you take a look?

This implements only the data structure. To make it useful, I intend to add a function |Path.to| that 

The idea is that once we have this OS.Path to add a function |Path.to| that will hook to the DirectoryProvider through some native code.
Comment 7 (dormant account) 2012-06-19 09:53:24 PDT
Comment on attachment 634416 [details] [diff] [review]
Unix back-end

I prefer the python way over OO here. See http://docs.python.org/library/os.path.html
Comment 8 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-19 15:35:03 PDT
Created attachment 634623 [details] [diff] [review]
Unix test suite
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-19 15:36:05 PDT
Created attachment 634624 [details] [diff] [review]
Unix back-end

Can't sleep, so here is a revised version.
Taras, can you tell me what you think of this one? If it works for you, I will start working on the dreaded Windows implementation.
Comment 10 (dormant account) 2012-06-19 17:56:28 PDT
Comment on attachment 634624 [details] [diff] [review]
Unix back-end

i could if bugzilla dns wasn't busted :(
Comment 11 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-19 18:12:35 PDT
Created attachment 634694 [details] [diff] [review]
Windows test suite
Comment 12 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-19 18:15:28 PDT
Created attachment 634696 [details] [diff] [review]
Windows back-end

Ah well, still can't sleep. Here is a prototype of the Windows back-end.

I still believe that we should introduce a data structure for paths. In particular, this would let us ensure that we only |join| and |split| normalized paths, without performance penalty.
Comment 13 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-19 18:22:12 PDT
We can even combine both. Leave these back-ends with the rest of the back-ends for users who prefer working with strings, and add a thin front-end to wrap them in safer/more abstract OO code.
Comment 14 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-20 02:50:29 PDT
Of course, I forgot to mention, but the main reason to add a little abstraction on top of the API is to ensure that users concatenate paths with |join| and not manually, which is quite brittle.
Comment 15 (dormant account) 2012-06-20 15:30:04 PDT
Comment on attachment 634694 [details] [diff] [review]
Windows test suite

Really? OS.Unix.Path in the windows testsuite? This works?
Comment 16 (dormant account) 2012-06-20 15:45:07 PDT
Comment on attachment 634696 [details] [diff] [review]
Windows back-end


exports.OS.Unix.Path <-- do not use Unix on a windows platform. If for whatever OS.Path sure, but not OS.lie.Path.


+         basename: function basename(path) {
+           return path.slice(Math.max(path.lastIndexOf("\\"), path.lastIndexOf(":")) + 1);
+         },

This and other path handling functions need to explicitly block paths(ie UNC ones) that start with '\\'(make a utility). Make a utility function that throws an exception if a UNC path is detected.
 Otherwise you get weird string exceptions for paths without ':' in them. Should also check errors when looking for "\\" position in path strings and throw proper exceptions.

+ let noDrive = (options && options.winNoDrive);

what the heck is .winNoDrive? no docs for it.

Need to document what you are doing in join with regexps, general strategy, etc. I generally don't require comments in the body if the code is clear, it's not in this case.
Comment 17 (dormant account) 2012-06-20 15:46:05 PDT
Comment on attachment 634624 [details] [diff] [review]
Unix back-end

+           return path.slice(path.lastIndexOf("/") + 1);

as in other patch, please check errors from string functions and throw appropriate exceptions.
Comment 18 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-20 15:50:06 PDT
(In reply to Taras Glek (:taras) from comment #15)
> Comment on attachment 634694 [details] [diff] [review]
> Windows test suite
> 
> Really? OS.Unix.Path in the windows testsuite? This works?

Sorry, that's an artifact of testing the Windows version on a Unix computer. I'll fix this.
Comment 19 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-21 07:19:32 PDT
(In reply to Taras Glek (:taras) from comment #16)
> Comment on attachment 634696 [details] [diff] [review]
> Windows back-end
> 
> 
> exports.OS.Unix.Path <-- do not use Unix on a windows platform. If for
> whatever OS.Path sure, but not OS.lie.Path.

Well, it is not a lie, it is an error, but yes, it is still my fault for uploading at 3am.

> 
> 
> +         basename: function basename(path) {
> +           return path.slice(Math.max(path.lastIndexOf("\\"),
> path.lastIndexOf(":")) + 1);
> +         },
> 
> This and other path handling functions need to explicitly block paths(ie UNC
> ones) that start with '\\'(make a utility). Make a utility function that
> throws an exception if a UNC path is detected.
>
>  Otherwise you get weird string exceptions for paths without ':' in them.
> Should also check errors when looking for "\\" position in path strings and
> throw proper exceptions.

Will do. I still think that this check would be better served as part of a data structure, though. Do you mind if I open a bug for that?

> 
> + let noDrive = (options && options.winNoDrive);
> 
> what the heck is .winNoDrive? no docs for it.

I will document this.

> Need to document what you are doing in join with regexps, general strategy,
> etc. I generally don't require comments in the body if the code is clear,
> it's not in this case.

Ok.
Comment 20 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-21 07:25:52 PDT
(In reply to Taras Glek (:taras) from comment #17)
> Comment on attachment 634624 [details] [diff] [review]
> Unix back-end
> 
> +           return path.slice(path.lastIndexOf("/") + 1);
> 
> as in other patch, please check errors from string functions and throw
> appropriate exceptions.

The only way these functions can fail is if |path| is |null|. Is this what you want me to check?
Comment 21 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-21 07:26:36 PDT
Created attachment 635302 [details] [diff] [review]
Windows test suite
Comment 22 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-21 07:30:55 PDT
Created attachment 635308 [details] [diff] [review]
Windows back-end

Updating the Windows back-end: more comments, more doc, and checks that we are not
Comment 23 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-21 08:25:02 PDT
Created attachment 635325 [details] [diff] [review]
Windows back-end
Comment 24 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-21 08:25:45 PDT
Created attachment 635326 [details] [diff] [review]
Windows test suite
Comment 25 (dormant account) 2012-06-21 14:00:31 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #20)
> (In reply to Taras Glek (:taras) from comment #17)
> > Comment on attachment 634624 [details] [diff] [review]
> > Unix back-end
> > 
> > +           return path.slice(path.lastIndexOf("/") + 1);
> > 
> > as in other patch, please check errors from string functions and throw
> > appropriate exceptions.
> 
> The only way these functions can fail is if |path| is |null|. Is this what
> you want me to check?

good point, this is a false alarm. I just want to ensure we don't throw string exceptions out of os.file.
Comment 26 (dormant account) 2012-06-21 14:11:57 PDT
Comment on attachment 635325 [details] [diff] [review]
Windows back-end

This is getting close to landing, a few minor cleanups.

+ return (!noDrive && this.winGetDrive(path)) || ".";

This is too clever. Please use an if statement here.

+           // Ignore any occurrence of "\\: immediately before that one
typo

+             start = (this.winGetDrive(path) || "").length;
too clever. if please

+ for each(let i in arguments) {

s/i/segment, component, whatever  you like/ i implies integer-like thing

+           let result = paths.join("\\");
+           if (absolute) {
+             result = "\\" + result;
+           }
I'm not a fan of prepending strings. Instead of a prepend, do an append..ie add drive letter first, result after.

Same with if(root) below.

+         winGetDrive: function winHasDrive(path) { <-- that's bad
Name indicates boolean, return is a string/null. call it winGetDrive

similar thing with  winIsAbsolute -> winEnsureAbsolute
Comment 27 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-21 15:56:42 PDT
(In reply to Taras Glek (:taras) from comment #26)
> Comment on attachment 635325 [details] [diff] [review]
> Windows back-end
> 
> This is getting close to landing, a few minor cleanups.
> 
> + return (!noDrive && this.winGetDrive(path)) || ".";
> 
> This is too clever. Please use an if statement here.

ok

> 
> +           // Ignore any occurrence of "\\: immediately before that one
> typo

oops

> 
> +             start = (this.winGetDrive(path) || "").length;
> too clever. if please
> 
> + for each(let i in arguments) {
> 
> s/i/segment, component, whatever  you like/ i implies integer-like thing

Good idea. Even if we are not writing in Fortran :)

> 
> +           let result = paths.join("\\");
> +           if (absolute) {
> +             result = "\\" + result;
> +           }
> I'm not a fan of prepending strings. Instead of a prepend, do an append..ie
> add drive letter first, result after.
> 
> Same with if(root) below.

Ok. Out of curiosity, what is wrong with prepending strings?

> 
> +         winGetDrive: function winHasDrive(path) { <-- that's bad
> Name indicates boolean, return is a string/null. call it winGetDrive

Sorry, typo. As you see, field name is already winGetDrive.

> 
> similar thing with  winIsAbsolute -> winEnsureAbsolute

This one, I don't understand. The method returns a boolean, why should I call it "ensure"?
Comment 28 (dormant account) 2012-06-21 16:32:57 PDT
> 
> > 
> > +           let result = paths.join("\\");
> > +           if (absolute) {
> > +             result = "\\" + result;
> > +           }
> > I'm not a fan of prepending strings. Instead of a prepend, do an append..ie
> > add drive letter first, result after.
> > 
> > Same with if(root) below.
> 
> Ok. Out of curiosity, what is wrong with prepending strings?

Nothing. It's slightly more straightforward to assemble the string in the order that it ends up.

> 
> > 
> > +         winGetDrive: function winHasDrive(path) { <-- that's bad
> > Name indicates boolean, return is a string/null. call it winGetDrive
> 
> Sorry, typo. As you see, field name is already winGetDrive.

Right :)
> 
> > 
> > similar thing with  winIsAbsolute -> winEnsureAbsolute
> 
> This one, I don't understand. The method returns a boolean, why should I
> call it "ensure"?

Sorry, misread. I seem to be suffering a bout of dyslexia today.
Comment 29 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-22 05:55:23 PDT
Created attachment 635712 [details] [diff] [review]
Unix back-end
Comment 30 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-22 06:01:16 PDT
Created attachment 635717 [details] [diff] [review]
Windows back-end

Applied the requested back-end changes.
Comment 31 (dormant account) 2012-06-25 09:50:02 PDT
David, I likely wont be able to get to these reviews this week. Either ask someone else for review or hold tight :)
Comment 32 Doug Turner (:dougt) 2012-06-26 14:12:22 PDT
Comment on attachment 634623 [details] [diff] [review]
Unix test suite

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js
@@ +5,5 @@
>    dump("WORKER "+text+"\n");
>  }
>  
>  function send(message) {
> +//  log("Sending " + JSON.stringify(message) + "errno: " + ctypes.errno);

just remove it, right?

@@ +208,5 @@
> +function test_path()
> +{
> +  ok(true, "test_path: starting");
> +  is(OS.Unix.Path.basename("a/b"), "b", "basename of a/b");
> +  is(OS.Unix.Path.basename("a/b/"), "", "basename of a/b/");

is basename really "" for b/?  Shouldn't it be 'b'?
Comment 33 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-26 15:25:01 PDT
(In reply to Doug Turner (:dougt) from comment #32)
> Comment on attachment 634623 [details] [diff] [review]
> Unix test suite
> 
> Review of attachment 634623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js
> @@ +5,5 @@
> >    dump("WORKER "+text+"\n");
> >  }
> >  
> >  function send(message) {
> > +//  log("Sending " + JSON.stringify(message) + "errno: " + ctypes.errno);
> 
> just remove it, right?
> 
> @@ +208,5 @@
> > +function test_path()
> > +{
> > +  ok(true, "test_path: starting");
> > +  is(OS.Unix.Path.basename("a/b"), "b", "basename of a/b");
> > +  is(OS.Unix.Path.basename("a/b/"), "", "basename of a/b/");
> 
> is basename really "" for b/?  Shouldn't it be 'b'?

Interestingly, this depends on whether we follow Unix conventions ('b') or Python conventions (""). I have followed the second here, and I should probably document this.
Comment 34 (dormant account) 2012-07-02 10:37:09 PDT
Comment on attachment 635717 [details] [diff] [review]
Windows back-end

stripBackslashes -> trimBackslashes might be better
 ensureNotUNC should probably ensure that the path has a drive: component so functions like basename do not throw string exceptions


+         /**
+          * Return |true| if the path is absolute, |false| otherwise.
+          *
+          * We consider that a path is absolute if it starts with "\\"
+          * or "driveletter:\\".
+          */

That seems wrong. paths that start with \ are relative to current drive. That's in no way absolute.

Please address above comments in a followup patch.
Comment 35 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-07-03 07:12:38 PDT
(In reply to Taras Glek (:taras) from comment #34)
> Comment on attachment 635717 [details] [diff] [review]
> Windows back-end
> 
> stripBackslashes -> trimBackslashes might be better
ok

>  ensureNotUNC should probably ensure that the path has a drive: component so
> functions like basename do not throw string exceptions

Well, none of the functions throws if there is no drive component, so this does not really seem useful. Do you really want that? I believe that this will only make |basename| and |dirname| less useful.

> 
> 
> +         /**
> +          * Return |true| if the path is absolute, |false| otherwise.
> +          *
> +          * We consider that a path is absolute if it starts with "\\"
> +          * or "driveletter:\\".
> +          */
> 
> That seems wrong. paths that start with \ are relative to current drive.
> That's in no way absolute.

Well, I am sticking with the definition of MSDN:
« A file name is relative to the current directory if it does not begin with one of the following: [...] A single backslash, for example, "\directory" or "\file.txt". This is also referred to as an absolute path. »

Are you sure that you want me to change this?

> Please address above comments in a followup patch.

Ok.
Comment 36 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-07-03 07:20:25 PDT
Created attachment 638710 [details] [diff] [review]
Windows back-end
Comment 37 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-07-03 07:21:05 PDT
Created attachment 638711 [details] [diff] [review]
Unix back-end
Comment 38 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-07-03 07:21:52 PDT
Created attachment 638712 [details] [diff] [review]
Unix test suite
Comment 39 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-07-03 07:22:37 PDT
Created attachment 638714 [details] [diff] [review]
Windows test suite

Note You need to log in before you can comment on or make changes to this bug.