187957 - implement nsIFile.normalize for windows and os/2

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[timeless]

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

[Pete Collins (MDG)]

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

Comment 2

[Brodie]

Attachment: patch 1

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 3

[Brodie]

Comment on attachment 150511 [details] [diff] [review]
patch 1

Timeless: would you assign this to me please.

Comment 4

[Brodie]

Attachment: patch 2

Fix a pointer problem that I noticed and cleaned up the comments.

Comment 5

[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?

Comment 6

[Brodie]

Do you have a bug number? I can't find it, so I'm not sure what behaviour you
are after.

Comment 7

[timeless]

bug 87501

Comment 8

[Brodie]

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.

Comment 9

[Brodie]

Attachment: patch 3

This version ignores path segments consisting solely of > 2 dots.
i.e. "c:\foo\bar\..." is normalized to "c:\foo\bar"

Comment 10

[Brodie]

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?

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 151083 [details] [diff] [review]
patch 3

darin, not timeless, should review this.

Comment 12

[Brodie]

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

[Darin Fisher]

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

[Darin Fisher]

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

[Christian :Biesinger (don't email me, ping me on IRC)]

(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 :)

Comment 16

[Brodie]

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. 

Comment 17

[timeless]

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.

Comment 18

[Brodie]

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

[Darin Fisher]

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.

Comment 20

[Brodie]

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.

Comment 21

[Brodie]

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

Comment 22

[Darin Fisher]

> 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.