53596 - leaf not working on relative paths[FIX][REVIEW]

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Pete Collins (MDG)]

This use to work fine . . .

If you use a relative path name GetLeafNameRaw ASSERTS . . .

I have a patch that works for me but i aint no c hacker.

--pete

Comment 1

[Pete Collins (MDG)]

Attachment: diff that works for me . . .

Comment 2

[Pete Collins (MDG)]

adding myself to for spam.

Comment 3

[Pete Collins (MDG)]

By the way relatave paths *do* work if i initialize it like this "./io.js" 

check this out: 
  

js> load('io.js'); 
js> var f=new File(); 
js> var p='./io.js'; 
js> f.exists(p); 
true 
js> f.leaf(p); 
io.js 
js> 

Comment 4

[Brendan Eich [:brendan]]

Right, relative paths with one or more /'s in them are cool.  Hmm.  I'll look 
into this fast, for blizzard.

/be

Comment 5

[Pete Collins (MDG)]

I was playing around w/ python last night and it uses relative paths also.
So it seems like perl, php and ptyhon all allow the use of relative paths w/
I/O.

So i would hope that nsIFile users are not going to be forced to go against
common convention and have to use full paths. That would be very "un-open
sourcey"

--pete

Comment 6

[Stephen P. Morse]

*** Bug 62512 has been marked as a duplicate of this bug. ***

Comment 7

[Stephen P. Morse]

Oops, ignore the dup notation above.  It was meant for bug 53956 and I 
transposed two digits.  Sorry.

Comment 8

[Pete Collins (MDG)]

Need an r= and sr=. This is a minor patch.

--pete

Comment 9

[Pete Collins (MDG)]

Asigning to myself.

--pete

Comment 10

[Pete Collins (MDG)]

Attachment: updating patch

Comment 11

[Brendan Eich [:brendan]]

You know what I'm gonna say :-)

+    leafName ? *_retval=leafName+1 : *_retval=mPath;

First, factor assignment of conditional so the left-hand side is stated once:

+    *_retval = leafName ? leafName+1 : mPath;

I like spaces around =, and often around + -- so does prevailing style.

But more important, why worry about the null strrchr return value case?  It
"can't happen", and the existing code is there only as a paranoid backstop.  If
we can prove (it's only one file, let's do it!) that mPath always must point to
a string with a '/' in it, then we should not have any tests, if, ?: or otherwise.

/be

/be

Comment 12

[Pete Collins (MDG)]

Here is an example of strrchr being null. 

js> f;
[xpconnect wrapped nsILocalFile @ 0x81f7440]
js> f.path;
chrome
js> f.exists();
true
js> f.leafName;
chrome
js> 

So in this case we need the test so we can return the un '/' inited path to the
caller which would be the same as leafName proper.

The above example is using the patch in order to work.

--pete

Comment 13

[Pete Collins (MDG)]

Attachment: cleaned up patch

Comment 14

[Brendan Eich [:brendan]]

How was f created?  I thought (from memory, didn't look) that the Init* methods
all made sure that mPath was absolute.

/be

Comment 15

[Pete Collins (MDG)]

const File = new Components.Constructor("@mozilla.org/file/local;1",
"nsILocalFile", "initWithPath")
;
var f = new File('chrome');

No, you can init w/ or w/out a '/'.  I don't think that should change.

--pete

Comment 16

[Pete Collins (MDG)]

this will be resolved then bug 94323 is sompleted.

--pete

Comment 17

[Pete Collins (MDG)]

pushing back

Comment 18

[Pete Collins (MDG)]

I would like to check in patch 44510 as a fix against the current nsIFile
implementation for the time being.

CAn i get an r= and sr= please.

Thanks

--pete

Comment 19

[Conrad Carlen (not reading bugmail)]

InitWithPath(), according to the comments in nsILocalFile.idl, requires that the
path is a full path. Only the Unix impl doesn't enforce this - Mac and Windows
do. I'm not sure why this rule was made. Doug? Whatever the rule is, we've gotta
be consistent amongst platforms.

Comment 20

[Doug Turner (:dougt)]

The rule is correct.  We didn't want to have relative path.  For example, on Mac
a relative patch would probably be relative to the cwd, and on windows it may be
to c:\.  To simplify things, we voted to enforce a full path.

Comment 21

[Pete Collins (MDG)]

Doug, just about all environments (runtime anyway), allow the use of relative
paths. All web environments also allow relative paths. Keeping it from local
file i/o seems to break conventional methods. 

Why can't we just prepend "MozBinD" and have the path be relative to that in the
case where a relative path is used as an inializer?

This bug is more relevant when using nsIFile from script.

I think we should have relative initialization of file paths across platforms.

--pete

Comment 22

[Doug Turner (:dougt)]

If you really want a xp application to behave consistantly, we can not have this
kind of platform/environment dependancy.  In a script, if you mean something
relative to a given directory, say so.  

Comment 23

[Pete Collins (MDG)]

Alright, i'll mark this WONTFIX and fix the unix implementation of initWithPath,
so relative paths can not be used.

I'll open a bug on that if one desn't exist already.

--pete

Comment 24

[Brendan Eich [:brendan]]

I suspect we'll now cause clients of nsIFile to reinvent the current working
directory wheel.  Maybe that's for the best, and we can reunify later, but it is
not farfetched or anti-cross-platform to want a relative pathname to work when
given to initWithPath -- so long as it works consistently, across platforms and
on any given platform.

/be

Comment 25

[Doug Turner (:dougt)]

a CWD key should be added to the nsIDirectoryService if it is not already there.
Lets not have a special meaning to relative paths - just not a fun place to be.

Comment 26

[Brendan Eich [:brendan]]

>a CWD key should be added to the nsIDirectoryService if it is not already there.

That's a fine idea, but not crucial (it's not there now, AFAICT), and not my point.

>Lets not have a special meaning to relative paths - just not a fun place to be.

Relative paths are special already -- you're treating them as errors.  If we can
agree on what they're relative to, and if lots of clients (or even a few) all go
through the same procedure to make an absolute path from a relative one (as OSes
do in roughly the same way -- are you sure windows would make a relative path
absolute by prepending C:\ ?), then nsIFile's impls can do the same thing and
consolidate redundant code in clients.  Again, in an XP way.

What's not fun about that?

/be

Comment 27

[Doug Turner (:dougt)]

I really don't care.  if we want to do relative paths, lets just make sure the
result is defined.

Comment 28

[Pete Collins (MDG)]

Relative path initialization:

If nsIFile is initialized with a relative path, it will be prepended with the
local path to the mozilla bin dir.

reopening. 

I would really prefer to get some review on bug #92329, so that i can check
something useful into the trunk instead of letting all these patches rot away
and having the bug dependencies keep rising . . .

--pete

Comment 29

[Conrad Carlen (not reading bugmail)]

This may be a tangent, but what I want is not a relative path but a relative
descriptor. Not a path because paths are always in platform-native format. What
I want is something in canonical format, the same on all platforms, and the
nsLocalFile impl would be responsible for turning it into a native path.

You would have:
string getRelativeDescriptor(in nsIFile fromFile);
void initWithRelativeDescriptor(in string descriptor, in nsIFile fromFile);

The advantage is that you can make the path relative to anything, not just the
bin dir. Since it's XP, it means that relative descs written as strings in
prefs.js (among others) will be portable instead of what we have now with
absolute, native paths.  Assuming the relative descs in profile files are
relative to the current profile dir, moving a profile is a breeze.

This was discussed on porkjockeys months ago.

Comment 30

[Pete Collins (MDG)]

Sounds fine to me.

However, if a user manually moves his profile dir (say to another drive because
his mail dir is 6G's or something). How will the descriptor for the new anchor
point be manifested when the app is started up?

PrefD is now somewhere else now. PrefD is pulled from the system environment i
beleive. 

--pete

Comment 31

[Conrad Carlen (not reading bugmail)]

It will do what 4.x did - put up a file picker and ask you where it is.

"PrefD" a.k.a. NS_APP_PREFS_50_DIR is a profile relative location. See:
http://lxr.mozilla.org/mozilla/source/xpcom/io/nsAppDirectoryServiceDefs.h#75
for others.

Comment 32

[Pete Collins (MDG)]

Yea, ok that's what i thought. 

Anyone else noticing double emails from bugzilla?

--pete

Comment 33

[timeless]

brendan: relative paths on windows are fun.

suppose for example

e:\winnt> cd d:\BCC
e:\winnt> C:\program files\mozilla\bin\mozilla.exe
so if i ask for the relative path 'e:system32\' i'd expect e:\winnt\system32. 
Even though this skirts the fact that CWD should be c:\program files\mozilla\bin
 [ assuming Comments From pete collins 2001-10-29 13:34].

similarly i can try to open d:bin\bcc.exe if i like.

fwiw As i windows user, i wouldn't expect opening '..\boot.ini' to fail and
would be rather shocked that it decided to use c:\program files\mozilla\bin.

otoh that's almost a nonissue, these days windows encourages CWD to be set to my
documents.

Comment 34

[Doug Turner (:dougt)]

Comment on attachment 44510 [details] [diff] [review]
cleaned up patch

r=dougt.

Comment 35

[Pete Collins (MDG)]

Brendan, an sr= here?

This patch fixes this big and has nothing to do w/ what all this discussion is
about which is initWithPath from a relative path using CWD.

I'll open a bug for that if there isn't one already.


--pete

Comment 36

[Pete Collins (MDG)]

Brendan, can i get an sr for this simple patch.

Thanks

--pete

Comment 37

[Brendan Eich [:brendan]]

Comment on attachment 44510 [details] [diff] [review]
cleaned up patch

sr=[e-mail]

Comment 38

[Pete Collins (MDG)]

js> f.initWithPath('chrome');
js> f.path;
chrome
js> f.leafName;
chrome
js> f.exists();
true
js>