FIX: Folder starting with the same "prefix" should not be a potential pa... #1

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

Folder starting with the same "prefix" should not be a potential parent

Owner

domenic commented Feb 27, 2014

Nice catch! Will merge and publish 1.0.1 as soon as I get some free time.

Nice, and then I appreciate the include of that in npm, it will prevent an annoying "Adding a cache directory to the cache will make the world implode." when you set the npm cache in tmp folder (also used by npm), maybe should not be good idea but I found this issue because of that.

Should I open an issue on npm project to include this fix?

Owner

domenic commented Feb 27, 2014

Nah, once I publish a new version we'll update the one in npm.

@@ -13,7 +13,11 @@ module.exports = function (thePath, potentialParent) {
potentialParent = potentialParent.toLowerCase();
}
- return thePath.indexOf(potentialParent) === 0;
@kriskowal

kriskowal Feb 27, 2014

thePath.lastIndexOf(potentialParent, 0) === 0 is O(1) in the worst case.

@pmarques

pmarques Feb 27, 2014

Sorry, I didn't understand your comment. The lines added are also O(1) each one, right?

@kriskowal

kriskowal Feb 27, 2014

The complexity of thePath.indexOf(potentialParent) is O(max(n, m)) where n and m are the path lengths assuming that indexOf does not employ a naïve algorithm. This is because it will consider substrings that do not start at index 0, which are not interesting in this case. thePath.lastIndexOf(portentialParent, 0) forces the algorithm to only consider substrings that start at index 0.

@kriskowal

kriskowal Feb 27, 2014

And, correction, taking account for both lengths, thePath.lastIndexOf(potentialParent, 0)) is O(m) where m is the length of the potentialParent, in the worst case, which happens to be any of the cases where the potential parent is in fact a parent.

@pmarques

pmarques Feb 27, 2014

Ups, sorry, my bad! I didn't saw the difference in the line, you are obviously right!

I think that you can say that on the general case m is the length shortest string,

Owner

domenic commented Mar 1, 2014

Merged as 15f3dda with @kriskowal's optimization as 9277983; released as 1.0.1, and will put it into npm shortly.

@domenic domenic closed this Mar 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment