Closed Bug 766194 Opened 10 years ago Closed 10 years ago

[OS.File] Get file information

Categories

(Core :: Networking: File, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(8 files, 26 obsolete files)

2.93 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
4.46 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
8.88 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
3.61 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
854 bytes, patch
Yoric
: review+
Details | Diff | Splinter Review
6.02 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
10.05 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
7.40 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch Unix back-end (obsolete) — Splinter Review
Assignee: nobody → dteller
Attached patch Windows back-end (obsolete) — Splinter Review
Attachment #635780 - Attachment is obsolete: true
Attached patch Unix back-end (obsolete) — Splinter Review
Attachment #635779 - Attachment is obsolete: true
Attached patch Unix test suite (obsolete) — Splinter Review
Attachment #635781 - Attachment is obsolete: true
Attached patch Unix constants (obsolete) — Splinter Review
Attachment #635783 - Attachment is obsolete: true
Attached patch Shared code (obsolete) — Splinter Review
Attachment #638975 - Flags: review?(taras.mozilla)
Attached patch Constants (obsolete) — Splinter Review
Attachment #638751 - Attachment is obsolete: true
Attachment #638976 - Flags: review?(taras.mozilla)
Attached patch Unix back-end (obsolete) — Splinter Review
Attachment #638749 - Attachment is obsolete: true
Attachment #638977 - Flags: review?(taras.mozilla)
Attachment #638748 - Flags: review?(taras.mozilla)
Attachment #638750 - Flags: review?(taras.mozilla)
Attached patch Unix front-end (obsolete) — Splinter Review
Same one, minus debugging code.
Attachment #638748 - Attachment is obsolete: true
Attachment #638748 - Flags: review?(taras.mozilla)
Attachment #638980 - Flags: review?(taras.mozilla)
Attached patch Companion testsuite (obsolete) — Splinter Review
Attachment #638752 - Attachment is obsolete: true
Attachment #638981 - Flags: review?(taras.mozilla)
Attached patch Windows test suite (obsolete) — Splinter Review
Attachment #638747 - Attachment is obsolete: true
Attachment #639287 - Flags: review?(taras.mozilla)
Attached patch Windows back-end (obsolete) — Splinter Review
Attachment #638746 - Attachment is obsolete: true
Attachment #639288 - Flags: review?(taras.mozilla)
Attachment #638745 - Flags: review?(taras.mozilla)
Attached patch Companion testsuite (obsolete) — Splinter Review
Attachment #638981 - Attachment is obsolete: true
Attachment #638981 - Flags: review?(taras.mozilla)
Attachment #639290 - Flags: review?(taras.mozilla)
David please include order of the patch in the queue in the name. Ie patch 1a: windows backend, patch 1b: unix backend.
Otherwise it's not immediately clear where 'Shared Code' fits, etc.
Comment on attachment 638975 [details] [diff] [review]
Shared code

r+ i guess. withName seems like abuse of the prototype stuff
Attachment #638975 - Flags: review?(taras.mozilla) → review+
Comment on attachment 638976 [details] [diff] [review]
Constants

ewww, but ok
Attachment #638976 - Flags: review?(taras.mozilla) → review+
(In reply to Taras Glek (:taras) from comment #23)
> Comment on attachment 638975 [details] [diff] [review]
> Shared code
> 
> r+ i guess. withName seems like abuse of the prototype stuff

Actually, given what you use it for, that's pretty awesome :)
Comment on attachment 638977 [details] [diff] [review]
Unix back-end

Eww, but this is as neat as possible in this case...i think.
Attachment #638977 - Flags: review?(taras.mozilla) → review+
Comment on attachment 638980 [details] [diff] [review]
Unix front-end

+         Object.defineProperty(this, "creationDate", {
+           value: date
+         });


why not this.creationDate = {value:date}?

i'm not sure i like .getInfo vs .stat. getInfo doesn't quite convey the severity of doing a stat.
Comment on attachment 638750 [details] [diff] [review]
Unix test suite

+  ok(change.getTime() - start.getTime() >= -10000, "test_stat: file has changed after the start of the test - " + change + ", " + start);
+  ok(change.getTime() - start.getTime() <= 10000, "test_stat: file has changed less than 10 seconds after the start of the test");
+  if (stat.st_btime) {
+    let birth = new Date(stat.st_btime * 1000);
+    ok(birth.getTime() - start.getTime() >= -10000, "test_stat: file was created after the start of the test - " + change);
+    ok(birth.getTime() - start.getTime() <= 10000, "test_stat: file was created less than 10 seconds after the start of the test");
+  }

This is bad. Magic 10second mark has got to go. Use change >= start.
Attachment #638750 - Flags: review?(taras.mozilla) → review+
Comment on attachment 638750 [details] [diff] [review]
Unix test suite

Sorry, meant to r-
Attachment #638750 - Flags: review+ → review-
(In reply to Taras Glek (:taras) from comment #28)
> Comment on attachment 638750 [details] [diff] [review]
> Unix test suite
> 
> +  ok(change.getTime() - start.getTime() >= -10000, "test_stat: file has
> changed after the start of the test - " + change + ", " + start);
> +  ok(change.getTime() - start.getTime() <= 10000, "test_stat: file has
> changed less than 10 seconds after the start of the test");
> +  if (stat.st_btime) {
> +    let birth = new Date(stat.st_btime * 1000);
> +    ok(birth.getTime() - start.getTime() >= -10000, "test_stat: file was
> created after the start of the test - " + change);
> +    ok(birth.getTime() - start.getTime() <= 10000, "test_stat: file was
> created less than 10 seconds after the start of the test");
> +  }
> 
> This is bad. Magic 10second mark has got to go. Use change >= start.

Note the best thing to do here is to do relative comparisons. Ie create two files in a row and check that fileA.creation <= fileB.creation. you can also do a strict comparison against well-known file like /bin/sh
You can then compare .modifcationTime against .creation
Comment on attachment 639287 [details] [diff] [review]
Windows test suite

r- for using magic 10seconds
Attachment #639287 - Flags: review?(taras.mozilla) → review-
Comment on attachment 639288 [details] [diff] [review]
Windows back-end

windows is so neat and tidy compared to unix :)
Attachment #639288 - Flags: review?(taras.mozilla) → review+
Comment on attachment 638745 [details] [diff] [review]
Windows front-end

Where is getInfo here? no call to GetFileInformationByHandle either
Attachment #638745 - Flags: review?(taras.mozilla) → review-
Comment on attachment 639290 [details] [diff] [review]
Companion testsuite

magic 10s again
Attachment #639290 - Flags: review?(taras.mozilla) → review-
> > r+ i guess. withName seems like abuse of the prototype stuff
> 
> Actually, given what you use it for, that's pretty awesome :)

Thanks :)
(In reply to Taras Glek (:taras) from comment #30)
> (In reply to Taras Glek (:taras) from comment #28)
> > Comment on attachment 638750 [details] [diff] [review]
> > Unix test suite
> > 
> > +  ok(change.getTime() - start.getTime() >= -10000, "test_stat: file has
> > changed after the start of the test - " + change + ", " + start);
> > +  ok(change.getTime() - start.getTime() <= 10000, "test_stat: file has
> > changed less than 10 seconds after the start of the test");
> > +  if (stat.st_btime) {
> > +    let birth = new Date(stat.st_btime * 1000);
> > +    ok(birth.getTime() - start.getTime() >= -10000, "test_stat: file was
> > created after the start of the test - " + change);
> > +    ok(birth.getTime() - start.getTime() <= 10000, "test_stat: file was
> > created less than 10 seconds after the start of the test");
> > +  }
> > 
> > This is bad. Magic 10second mark has got to go. Use change >= start.
> 
> Note the best thing to do here is to do relative comparisons. Ie create two
> files in a row and check that fileA.creation <= fileB.creation. you can also
> do a strict comparison against well-known file like /bin/sh
> You can then compare .modifcationTime against .creation

Note that |change >= start|does not work, as the file system and JS have different precisions. 

However, you are right, I should get rid of this. I have something simpler in mind, though.
(In reply to Taras Glek (:taras) from comment #27)
> Comment on attachment 638980 [details] [diff] [review]
> Unix front-end
> 
> +         Object.defineProperty(this, "creationDate", {
> +           value: date
> +         });
> 
> 
> why not this.creationDate = {value:date}?

I assume you mean
  |this.creationDate = date|

This works in all of cases in non-strict mode, but only in some cases in strict mode. After having been bitten a few times, I opted for the method that is harder to read but always works.

More precisely, in strict mode, this works if you have defined the getter during the definition of the object, but not if you have added the getter to an existing object, e.g. whenever any inheritance is involved.

> 
> i'm not sure i like .getInfo vs .stat. getInfo doesn't quite convey the
> severity of doing a stat.

I appreciate that. In JS, method names are always verbs, though, so perhaps |accessStat| or |accessInfo|?
(In reply to David Rajchenbach Teller [:Yoric] from comment #37)
> (In reply to Taras Glek (:taras) from comment #27)
> > Comment on attachment 638980 [details] [diff] [review]
> > Unix front-end
> > 
> > +         Object.defineProperty(this, "creationDate", {
> > +           value: date
> > +         });
> > 
> > 
> > why not this.creationDate = {value:date}?
> 
> I assume you mean
>   |this.creationDate = date|
> 
> This works in all of cases in non-strict mode, but only in some cases in
> strict mode. After having been bitten a few times, I opted for the method
> that is harder to read but always works.
> 
> More precisely, in strict mode, this works if you have defined the getter
> during the definition of the object, but not if you have added the getter to
> an existing object, e.g. whenever any inheritance is involved.

Where are you running into strict mode issues?

> 
> > 
> > i'm not sure i like .getInfo vs .stat. getInfo doesn't quite convey the
> > severity of doing a stat.
> 
> I appreciate that. In JS, method names are always verbs, though, so perhaps
> |accessStat| or |accessInfo|?

I suspect stat() is a verb in UNIX meaning retrieveStats.
Comment on attachment 638980 [details] [diff] [review]
Unix front-end

Waldo explained the defineProperty business.

Object.defineProperty(this, "creationDate", {value: date });has same effect as passing { value: date, enumerable: false, configurable: false, writable: false, etc } means the property is readonly, non-enumerable, non-configurable

Still not clear on what this has to do with strict mode. Until I get a better justification, r-
Attachment #638980 - Flags: review?(taras.mozilla) → review-
(In reply to Taras Glek (:taras) from comment #39)
> Comment on attachment 638980 [details] [diff] [review]
> Unix front-end
> 
> Waldo explained the defineProperty business.
> 
> Object.defineProperty(this, "creationDate", {value: date });has same effect
> as passing { value: date, enumerable: false, configurable: false, writable:
> false, etc } means the property is readonly, non-enumerable, non-configurable

Indeed.

> Still not clear on what this has to do with strict mode.

Actually, you may be right, this may be not related to strict mode.

> Until I get a
> better justification, r-

Not convinced it's a good idea, but as you wish.
(In reply to Taras Glek (:taras) from comment #38)
> > I appreciate that. In JS, method names are always verbs, though, so perhaps
> > |accessStat| or |accessInfo|?
> 
> I suspect stat() is a verb in UNIX meaning retrieveStats.

Ok.
Attached patch Companion testsuite (obsolete) — Splinter Review
I attach a new version of the test suite, without the 10s magic. Also, I think we can probably get around without the back-end test suite, since we have a front-end testsuite, so let's remove them for the moment.
Attachment #638750 - Attachment is obsolete: true
Attachment #639287 - Attachment is obsolete: true
Attachment #639290 - Attachment is obsolete: true
Attachment #641493 - Flags: review?(taras.mozilla)
I double-checked and I confirm that I need to call |defineProperty|, see attached demo. The presence/absence of "use strict" only switches between exception or silent error if I do not use |defineProperty|.
Attachment #641495 - Flags: feedback?(taras.mozilla)
Attached patch Unix front-end v2 (obsolete) — Splinter Review
Attaching a new version of the Unix front-end. This version changes the following:
- "stat" is now a verb :)
- more comments and documentation;
- option |noFollowLinks| is now |unixNoFollowLinks|;
- |get size| is now a lazy getter, uses |projectValue| and may return |NaN|.

I have tried rewriting lazy getters without |Object.defineProperty| and this just fails. See the previous attachment for more details.

I will attach a new version of the Windows front-end once we have agreed on the Unix back-end.
Attachment #638980 - Attachment is obsolete: true
Attachment #641496 - Flags: review?(taras.mozilla)
(In reply to Taras Glek (:taras) from comment #26)
> Comment on attachment 638977 [details] [diff] [review]
> Unix back-end
> 
> Eww, but this is as neat as possible in this case...i think.

Btw, I have experimented with alternatives:
- pointer arithmetics using the current brand of pointer arithmetics available in js-ctypes (much uglier);
- helper getters written in C (splits the code across four files and two languages instead of one, not more readable, most likely slower, plus requires unreviewed patches);
- pointer arithmetics based on bug 771928 (compatable readability, most likely slower, plus requires unreviewed patches).

I'd rather not pursue the alternatives in this bug, though. Perhaps in a following bug.
Comment on attachment 641493 [details] [diff] [review]
Companion testsuite

This is more reasonable. If it random oranges we'll have to do an approach that does not depend on non-filesystem time
Attachment #641493 - Flags: review?(taras.mozilla) → review+
Comment on attachment 641495 [details]
(read this before the front-ends) Lazy getter argumentation

sold. Thanks for the clear testcase
Attachment #641495 - Flags: feedback?(taras.mozilla) → feedback+
Comment on attachment 641496 [details] [diff] [review]
Unix front-end v2

Looks ok. 

I'm not clear on what projectValue is why a NaN would appear. But we can address that in an incremental patch, rest looks good.
Attachment #641496 - Flags: review?(taras.mozilla) → review+
(In reply to Taras Glek (:taras) from comment #48)
> Comment on attachment 641496 [details] [diff] [review]
> Unix front-end v2
> 
> Looks ok. 
> 
> I'm not clear on what projectValue is why a NaN would appear. But we can
> address that in an incremental patch, rest looks good.

If I understand correctly, there are two questions here:
- a |NaN| may appear on a OS with 64-bits |stat| if the size cannot be represented with 32 bits;
- |projectValue| is a utility I wrote to uniformize the transformation of js-ctypes integers into JS.
Attached patch Windows front-end (obsolete) — Splinter Review
Now let's move to the Windows front-end.
My apologies for the previous submission, I had marked the patch as "r?" by error.
Attachment #638745 - Attachment is obsolete: true
Attachment #641853 - Flags: review?(taras.mozilla)
(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #49)
> (In reply to Taras Glek (:taras) from comment #48)
> > Comment on attachment 641496 [details] [diff] [review]
> > Unix front-end v2
> > 
> > Looks ok. 
> > 
> > I'm not clear on what projectValue is why a NaN would appear. But we can
> > address that in an incremental patch, rest looks good.
> 
> If I understand correctly, there are two questions here:
> - a |NaN| may appear on a OS with 64-bits |stat| if the size cannot be
> represented with 32 bits;
> - |projectValue| is a utility I wrote to uniformize the transformation of
> js-ctypes integers into JS.

does that not go through a double conversion. So you'd only get a nan if you can't represent a 64bit int with a double?
Comment on attachment 641853 [details] [diff] [review]
Windows front-end

get isLink() { <-- missed that before

is link term ambigious. reparse-things are junctions, which are a type of symlink, so call this isSymLink. Same in the unix code(there is no way simple way to detect a hardlink).
Attachment #641853 - Flags: review?(taras.mozilla) → review+
Attached patch Windows front-end v2 (obsolete) — Splinter Review
Attachment #641853 - Attachment is obsolete: true
Attachment #642936 - Flags: review?(taras.mozilla)
Comment on attachment 642936 [details] [diff] [review]
Windows front-end v2

Not sure what happened.
Attachment #642936 - Flags: review?(taras.mozilla) → review+
Attached patch Renaming link to symLink (obsolete) — Splinter Review
Here we go.
Attachment #642938 - Flags: review?(taras.mozilla)
Attachment #642938 - Flags: review?(taras.mozilla) → review+
No longer blocks: 754671
Attached patch Shared codeSplinter Review
Attachment #638975 - Attachment is obsolete: true
Attachment #644288 - Flags: review+
Attached patch Unix constantsSplinter Review
Attachment #638976 - Attachment is obsolete: true
Attachment #644289 - Flags: review+
Attached patch Unix back-endSplinter Review
Attachment #638977 - Attachment is obsolete: true
Attachment #644290 - Flags: review+
Attached patch Windows back-endSplinter Review
Attachment #639288 - Attachment is obsolete: true
Attachment #644291 - Flags: review+
Attached patch Unix front-endSplinter Review
Attachment #641496 - Attachment is obsolete: true
Attachment #644293 - Flags: review+
Attachment #642936 - Attachment is obsolete: true
Attachment #644294 - Flags: review+
Attachment #641493 - Attachment is obsolete: true
Attachment #644295 - Flags: review+
Attachment #641495 - Attachment is obsolete: true
Attachment #642938 - Attachment is obsolete: true
Just had a little fun toying with Windows APIs.

For future reference:
- Windows presents the current hour in UTC, JS and Unix in local time;
- to determine if a file is a directory with |GetInformationByFileHandle|, you have to open the handle with so-called "backup semantics";
- under Windows, creating a file sets the file creation date _except_ if you have just removed a file with the same name and the file system hasn't flushed the buffer yet.

Running the last tests, and I hope we can checkin this tomorrow.
Will push it.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.