Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

jake.FileList fails with absolute path #1

Closed
wants to merge 5 commits into from

3 participants

@vmeurisse

Hello,

I tried to use jake.FileList with an absolute directory and it failed when jake tried to read folders that are only accessible to root.
The reason is that file.basedir returns the highest possible folder instead of the deepest one. Eg. file.basedir('/home/username/project/**') returns / while it should return /home/username/project/ which makes jake walks my entire hard-drive instead of just my project folder.

A quick search show me no usage of this method outside of jake so I guess that changing it's behavior is not too problematic (and the method add no documentation anyway).

Regards

lib/file.js
((22 lines not shown))
- // If the path has a leading asterisk, basedir is the current dir
- if (str.indexOf('*') > -1) {
- return '.';
- }
- // Otherwise it's the first segment in the path
- else {
- return str;
+ p = p || '';
+
+ var basedir = ''
+ , parts = p.split(/\\|\//)
+ , part
+ , pos = 0;
+ for (var i = 0, l = parts.length - 1; i < l; i++) {
+ part = parts[i];
+ if (part.indexOf('*') !== -1) break;
@larzconwell Collaborator

For if statements can you include brackets please?

if (blahblah) {
  break;
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/file.js
((14 lines not shown))
this.basedir = function (p) {
- var str = p || ''
- , abs = this.isAbsolute(p);
- if (abs) {
- return abs;
- }
- // Split into segments
- str = str.split(/\\|\//)[0];
- // If the path has a leading asterisk, basedir is the current dir
- if (str.indexOf('*') > -1) {
- return '.';
- }
- // Otherwise it's the first segment in the path
- else {
- return str;
+ p = p || '';
@larzconwell Collaborator

Also can you use a local variable instead of manipulating the input? So instead of doing p = p || '' just do var path = p || ''.

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

Overall this looks good, and the tests pass! Just left a few aesthetic notes for you to change, and then I'll merge it.

@mde
Owner

Good call on the formatting. Tests pass, but it looks like test conditions were changed, not just added to. I kind of want to make sure this isn't going to have any bad effects before merging. That basedir stuff was really fiddly, and took a lot of iterations to arrive at what we have now.

vmeurisse added some commits
@vmeurisse vmeurisse Fix style b1e5c98
@vmeurisse vmeurisse Add some testcases for method basedir 961a477
@vmeurisse vmeurisse Fix file.basedir with '..' ending path
For path ending with '..', the last '..' was ignored.
Eg. `file.basedir('..')` was returning './' instead of '../'.
e53cfa6
@vmeurisse

I fixed the style so it should be better now. I also added a few test cases to make sure that there is no regression.

While playing a bit, I found two bugs in the current implementation:

  • file.basedir('../..') was returning '..' which is wrong. It now returns '../../ (after the second set of commit).
  • file.basedir('name') was returning 'name' while name may be a folder or a file. It now take the safe path: it assume that it is a folder and returns './'.
@mde
Owner
mde commented

Sorry this took so long. It was more than a simple merge, but this is live and pushed to NPM in v0.0.19. Thanks very much for your help with this.

@mde mde closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Aug 18, 2012
  1. @vmeurisse

    Improve method basedir

    vmeurisse authored
Commits on Aug 30, 2012
  1. @vmeurisse
  2. @vmeurisse

    Fix style

    vmeurisse authored
  3. @vmeurisse
  4. @vmeurisse

    Fix file.basedir with '..' ending path

    vmeurisse authored
    For path ending with '..', the last '..' was ignored.
    Eg. `file.basedir('..')` was returning './' instead of '../'.
This page is out of date. Refresh to see the latest.
Showing with 67 additions and 19 deletions.
  1. +30 −12 lib/file.js
  2. +37 −7 test/file.js
View
42 lib/file.js
@@ -303,22 +303,40 @@ var fileUtils = new (function () {
}
};
+ /**
+ * Given a patern, return the base directory of it (ie. the folder
+ * that will contain all the files matching the path).
+ * eg. file.basedir('/test/**') => '/test/'
+ * Path ending by '/' are considerd as folder while other are considerd as
+ * files
+ * eg. file.basedir('/test/a/') => '/test/a/', file.basedir('/test/a') => '/test/'
+ * The returned path always end with a '/' so we have:
+ * file.basedir(file.basedir(x)) === file.basedir(x)
+ */
this.basedir = function (p) {
- var str = p || ''
- , abs = this.isAbsolute(p);
- if (abs) {
- return abs;
+ var basedir = ''
+ , parts
+ , part
+ , pos = 0
+ , path = p || '';
+
+ // always consider .. at the end as a folder and not a filename
+ if (/(?:^|\/|\\)\.\.$/.test(path.slice(-3))) {
+ path = path + '/';
}
- // Split into segments
- str = str.split(/\\|\//)[0];
- // If the path has a leading asterisk, basedir is the current dir
- if (str.indexOf('*') > -1) {
- return '.';
+ parts = path.split(/\\|\//);
+ for (var i = 0, l = parts.length - 1; i < l; i++) {
+ part = parts[i];
+ if (part.indexOf('*') !== -1) {
+ break;
+ }
+ pos += part.length + 1;
+ basedir += part + path[pos - 1];
}
- // Otherwise it's the first segment in the path
- else {
- return str;
+ if (!basedir) {
+ basedir = './';
}
+ return basedir;
};
// Search for a directory in parent directories if it can't be found in cwd
View
44 test/file.js
@@ -56,9 +56,18 @@ tests = {
fs.rmdirSync('foo');
}
-// TODO: Need Windows test with c:\\
, 'test basedir with Unix absolute path': function () {
var p = '/foo/bar/baz';
+ assert.equal('/foo/bar/', file.basedir(p));
+ }
+
+, 'test basedir with Windows absolute path': function () {
+ var p = 'C:\\foo\\bar\\baz';
+ assert.equal('C:\\foo\\bar\\', file.basedir(p));
+ }
+
+, 'test basedir with Unix root path': function () {
+ var p = '/';
assert.equal('/', file.basedir(p));
}
@@ -69,27 +78,48 @@ tests = {
, 'test basedir with leading double-asterisk': function () {
var p = '**/foo';
- assert.equal('.', file.basedir(p));
+ assert.equal('./', file.basedir(p));
}
, 'test basedir with leading asterisk': function () {
var p = '*.js';
- assert.equal('.', file.basedir(p));
+ assert.equal('./', file.basedir(p));
}
, 'test basedir with leading dot-slash and double-asterisk': function () {
var p = './**/foo';
- assert.equal('.', file.basedir(p));
+ assert.equal('./', file.basedir(p));
}
, 'test basedir with leading dirname and double-asterisk': function () {
var p = 'a/**/*.js';
- assert.equal('a', file.basedir(p));
+ assert.equal('a/', file.basedir(p));
}
, 'test basedir with leading dot-dot-slash and double-asterisk': function () {
- var p = '../test/**/*.js';
- assert.equal('..', file.basedir(p));
+ var p = '../../test/**/*.js';
+ assert.equal('../../test/', file.basedir(p));
+ }
+
+, 'test basedir with single-asterisk in dirname': function () {
+ var p = 'a/test*/file';
+ assert.equal('a/', file.basedir(p));
+ }
+
+, 'test basedir with single filename': function () {
+ var p = 'filename';
+ assert.equal('./', file.basedir(p));
+ }
+
+, 'test basedir with empty path': function () {
+ var p = '';
+ assert.equal('./', file.basedir(p));
+ assert.equal('./', file.basedir());
+ }
+
+, 'test basedir with dot-dot path': function () {
+ var p = '..';
+ assert.equal('../', file.basedir(p));
}
};
Something went wrong with that request. Please try again.