Closed
Bug 187957
Opened 22 years ago
Closed 20 years ago
implement nsIFile.normalize for windows and os/2
Categories
(Core :: XPCOM, enhancement)
Tracking
()
RESOLVED
FIXED
People
(Reporter: timeless, Assigned: bmo)
Details
Attachments
(1 file, 2 obsolete files)
5.55 KB,
patch
|
darin.moz
:
review+
dougt
:
superreview+
|
Details | Diff | Splinter Review |
I:\build\mozilla\opt-i686-pc-cygwin\dist\bin>xpcshell js> o=Components.classes["@mozilla.org/file/local;1"].createInstance(); o instanceof Components.interfaces.nsILocalFile; o.initWithPath("i:.\\xpcshell.exe"); o.exists(); true js> o.normalize(); o.path i:.\xpcshell.exe js> quit() Expected results: o.path after o.normalize() should return I:\build\mozilla\opt-i686-pc-cygwin\dist\bin\xpcshell.exe
Comment 1•22 years ago
|
||
Yep, looks broken to me. You should get the full path to 'xpcshell.exe'. Looks like a realpath() like libc call in windows that nsLocalFileWindows ::Normalize can implement instead of just returning NS_OK might do the trick here. ;-) --pete
First cut at the patch. This function produces the following behaviour. First path is the source, second path is the results. // path normalization nFail += TestNormalize( "r:\\foobar\\foo\\..", "r:\\foobar" ); nFail += TestNormalize( "r:\\foobar\\..\\foo", "r:\\foo" ); nFail += TestNormalize( "r:\\foobar\\.\\foo", "r:\\foobar\\foo" ); nFail += TestNormalize( "r:\\foobar\\..\\.\\foo", "r:\\foo" ); nFail += TestNormalize( "r:\\foobar\\.\\.\\.\\.\\.", "r:\\foobar" ); nFail += TestNormalize( "r:\\foobar\\..\\..", "r:\\" ); nFail += TestNormalize( "r:\\foobar\\test\\..\\test\\.\\..\\test", "r:\\foobar\\test" ); nFail += TestNormalize( "r:\\foobar\\\\..\\.\\\\test\\foo\\.", "r:\\test\\foo" ); nFail += TestNormalize( "\\\\svr\\shr\\foobar\\..", "\\\\svr\\shr" ); nFail += TestNormalize( "\\\\svr\\shr\\foobar\\..\\foo", "\\\\svr\\shr\\foo" ); nFail += TestNormalize( "\\\\svr\\shr\\foobar\\.\\foo", "\\\\svr\\shr\\foobar\\foo" ); nFail += TestNormalize( "\\\\svr\\shr\\foobar\\..\\.\\foo", "\\\\svr\\shr\\foo" ); nFail += TestNormalize( "\\\\svr\\shr\\foobar\\.\\.\\.\\.\\.", "\\\\svr\\shr\\foobar" ); nFail += TestNormalize( "\\\\svr\\shr\\foobar\\..\\..", "\\\\svr\\shr" ); nFail += TestNormalize( "\\\\svr\\shr\\foobar\\test\\..\\test\\.\\..\\test", "\\\\svr\\shr\\foobar\\test" ); // path normalization using the current directory char cwd[MAX_PATH], drive[_MAX_DRIVE]; getcwd(cwd, MAX_PATH); _splitpath(cwd, drive, 0, 0, 0); nFail += TestNormalize( drive, cwd ); chdir("c:\\temp"); nFail += TestNormalize( "c:", "c:\\temp" ); nFail += TestNormalize( "c:.", "c:\\temp" ); nFail += TestNormalize( "c:.\\test", "c:\\temp\\test" ); nFail += TestNormalize( "c:.", "c:\\temp" ); chdir("c:\\"); nFail += TestNormalize( "c:", "c:\\" ); chdir(cwd);
Comment on attachment 150511 [details] [diff] [review] patch 1 Timeless: would you assign this to me please.
Attachment #150511 -
Flags: review?(timeless)
Updated•20 years ago
|
Assignee: timeless → brofield
Fix a pointer problem that I noticed and cleaned up the comments.
Attachment #150511 -
Attachment is obsolete: true
Attachment #150511 -
Flags: review?(timeless)
Attachment #150775 -
Flags: superreview?(dougt)
Attachment #150775 -
Flags: review?(timeless)
Comment on attachment 150775 [details] [diff] [review] patch 2 there's a bug recently discussing three or more dots in a row. do you want to handle that?
Attachment #150775 -
Flags: review?(timeless) → review?(timeless)
Do you have a bug number? I can't find it, so I'm not sure what behaviour you are after.
If I handle this case it will be to either error or ignore. I don't think that we should accept this as valid syntax as it is an esoteric function that few people know about or use and it's not cross platform. My vote is to ignore it (treat it like ".") when normalizing. If satisfactory then I'll update the patch and again ask for review.
Attachment #150775 -
Flags: superreview?(dougt)
Attachment #150775 -
Flags: review?(timeless)
This version ignores path segments consisting solely of > 2 dots. i.e. "c:\foo\bar\..." is normalized to "c:\foo\bar"
Attachment #150775 -
Attachment is obsolete: true
Assignee | ||
Comment 10•20 years ago
|
||
Comment on attachment 151083 [details] [diff] [review] patch 3 See testcases (TestNormalize) handled by this patch in bug 247456, attachment http://bugzilla.mozilla.org/attachment.cgi?id=151082&action=view Now can I get review?
Attachment #151083 -
Flags: superreview?(dougt)
Attachment #151083 -
Flags: review?(timeless)
Comment 11•20 years ago
|
||
Comment on attachment 151083 [details] [diff] [review] patch 3 darin, not timeless, should review this.
Attachment #151083 -
Flags: review?(timeless) → review?(darin)
Assignee | ||
Comment 12•20 years ago
|
||
This whole thing is almost moot anyhow. We can't add relative paths using .. according to the spec, so it looks like this function was added before that rule came to be. The only thing that can be done now is adding paths with . or creating paths like c:foo Is it even correct to allow paths like c:foo? Shouldn't an nsILocalFile always contain a full and validly specified path? (not necessarily one in existence, but the format should be right). If this is correct and the set/append methods were correctly implemented to prevent invalid input, then this functions entire usefulness is simply stripping /./ from paths.
Comment 13•20 years ago
|
||
Brodie: good questions. I know that under UNIX, we allow a path to be initialized with "~/", so maybe that is enough reason to allow initialization with pathes like "c:dir". my main concern is that we have some code in existence that tries to distinguish a URL from a local file by seeing if a string can be used to initialize a local file. we have to be careful not to regress browser functionality if we make changes to the input we accept. of course, we can rightfully limit ourselves to single character driver letters, so maybe it is easy to avoid the problems i'm thinking about :-/ I'd also like to know who intends to make use of this Normalize function. Fully implementing our frozen API is good in general, but without a clear idea of who will benefit, isn't this just code bloat?
Comment 14•20 years ago
|
||
By the way, I looked over your patch, and it looks very solid to me. I'd just like to know if there are any planned consumers for this code. Thanks!
Comment 15•20 years ago
|
||
(In reply to comment #13) > of > course, we can rightfully limit ourselves to single character driver letters, as long as you don't break unc paths :)
Assignee | ||
Comment 16•20 years ago
|
||
I have no idea who wants to use it. I found the bug, had some spare time and fixed it. After implementing it I started thinking that it was pretty but probably useless. Since timeless reported it and has commented a few times, I expect he should have a better idea of who if anyone would want to use it.
Reporter | ||
Comment 17•20 years ago
|
||
well, bookmarks or whatever could use it. i certainly have uses for it in testcases and other places. a consistent api means less silly corner casing code.
Assignee | ||
Comment 18•20 years ago
|
||
This implementation makes the assumption that the current working path is valid. This assumption leaves us open to problems with the current implementation of localFileWin because we don't rigorously check the paths passed in in the set/append functions. It currently assumes one of the following formats: "" Empty path "\\server\share" Valid UNC "D:\" Valid absolute local "D:" Valid relative local I know that there are paths that can be passed in in the set/append methods which will invalidate this assumption. e.g. I think these semantics currently work without error (but probably shouldn't). NS_NewLocalFile("", &file); file.Append("."); This can't be addressed until we rigorously define what the contents of mWorkingPath is allowed to be. It will however work in 100% of current use cases (since the current use is 0 :-), and if the working path matches the assumptions.
Comment 19•20 years ago
|
||
Comment on attachment 151083 [details] [diff] [review] patch 3 r=darin perhaps it would be a good idea to add testcases for this function to nsIFileTest.cpp you might also want to add a XXX comment in the code corresponding to the concern you expressed in your last comment.
Attachment #151083 -
Flags: review?(darin) → review+
Assignee | ||
Comment 20•20 years ago
|
||
Darin: I'm about to leave in a couple of days and will be away for a couple of months. I won't have time to update this or check it in before then. Testcases are already in the new version of the test harness (see comment #2 and bug 247456). There is no point modifying the old test harness because it requires a manual inspection to check the results.
Updated•20 years ago
|
Attachment #151083 -
Flags: superreview?(dougt) → superreview+
Assignee | ||
Comment 21•20 years ago
|
||
Who needs sleep? Comment added. Checked in. Checking in xpcom/io/nsLocalFileWin.cpp; /cvsroot/mozilla/xpcom/io/nsLocalFileWin.cpp,v <-- nsLocalFileWin.cpp new revision: 1.128; previous revision: 1.127 done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 22•20 years ago
|
||
> Testcases are already in the new version of the test harness (see comment #2 and
> bug 247456). There is no point modifying the old test harness because it
> requires a manual inspection to check the results.
ok.. sounds good.
You need to log in
before you can comment on or make changes to this bug.
Description
•