define and implement smarter nsIFile::Equals for all platforms

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

10 years ago
Right now, nsIFile::Equals does not have the same behavior on all platforms and most implementations have a poor implementation. Some do a dumb path string comparison, others find the files on disk and do an inode comparison... We should standardize this comparison behavior and implement the standard behavior for all platforms.
Assignee

Comment 1

10 years ago
Posted patch fix v1.0 (obsolete) — Splinter Review
This patch implements a path comparison that does not require either path to be normalized, the path strings don't have to match to be equal, and the files aren't required to exist. Paths must be rooted however, which is already a requirement for nsIFile.

In this patch all platforms use a path comparison implementation I wrote that does not make any heap allocations for the sake of speed.

I have not tested this patch on Windows yet (I haven't verified my interpretation of Windows path requirements either), but on Mac OS X and Linux all xpcom unit and xpcshell tests pass. I need to do more testing and add some more xpcshell tests before this is ready but in the mean time any comments on my strategy would be welcome.

Comment 2

10 years ago
This is definitely wrong on Windows: you should not use mShortWorkingPath, which may be lossy or disabled; this code needs to be run using the wide-character path strings.

On file systems that support symlinks, I'm pretty sure that /foo/baz and /foo/bar/../baz can actually point to different files. I'm not sure whether that's something worth worrying about.

Alternately, we could say that /foo/baz and /foo/bar/../baz are *not* equal files, and you'd have to make sure that both files are normalized before comparing them.
(In reply to comment #2)
> Alternately, we could say that /foo/baz and /foo/bar/../baz are *not* equal
> files, and you'd have to make sure that both files are normalized before
> comparing them.

That's what I would prefer. The question is whether it's compatible with existing users of Equals.
Assignee

Comment 4

10 years ago
Posted patch fix v2.0 (obsolete) — Splinter Review
I had to fix xptiInterfaceInfoManager in order to get tests to pass with this scheme. The paths have to be normalized or they won't be considered equal. On Linux and Windows the paths happen to be normalized, on Mac OS X they are not. On Mac OS X the path for components given to us by the directory service is:

/Users/foo/mozilla/ff_192_debug/objdir-debug/xpcom/tests/../../dist/bin/components
Attachment #375572 - Attachment is obsolete: true
Attachment #386439 - Flags: review?(benjamin)

Comment 5

10 years ago
Comment on attachment 386439 [details] [diff] [review]
fix v2.0

I need this to come with tests.
Attachment #386439 - Flags: review?(benjamin) → review-
Assignee

Comment 6

10 years ago
Posted patch fix v2.0 w/tests (obsolete) — Splinter Review
Attachment #386439 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Attachment #387041 - Flags: superreview?(benjamin)

Comment 7

10 years ago
Comment on attachment 387041 [details] [diff] [review]
fix v2.0 w/tests

Josh, how dose this work with case-preserving file systems on mac, and do you care?

On Windows we do case-insensitive compares on purpose... I'm fine if Equals does case-sensitive compares, but Normalize() probably ought to normalize the case.

Updated

10 years ago
Attachment #387041 - Flags: superreview?(benjamin) → superreview+
Assignee

Comment 8

10 years ago
Posted patch fix v2.1Splinter Review
includes Windows unit test fix, pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/51bafb458d68
Attachment #387041 - Attachment is obsolete: true
Assignee

Updated

10 years ago
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee

Updated

10 years ago
Blocks: 490372
Comment on attachment 387458 [details] [diff] [review]
fix v2.1

     nsCAutoString inPath;
+    nsresult rv = inFile->GetNativePath(inPath);
(Not your fault, but auto strings are a waste in nsLocalFileUnix.cpp because all we're doing is string buffer sharing.)

>+  *_retval = !strcmp(inPath.get(), thisPath.get());
Why not use .Equals()?
Depends on: 513736
Depends on: 530793

Updated

9 years ago
Duplicate of this bug: 576111
You need to log in before you can comment on or make changes to this bug.