Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relative paths should be invalid #20

Closed
alberto opened this issue Apr 16, 2016 · 3 comments
Closed

Relative paths should be invalid #20

alberto opened this issue Apr 16, 2016 · 3 comments
Labels
need-discuss need further discussion need-spec

Comments

@alberto
Copy link

alberto commented Apr 16, 2016

I think node-ignore should not filter relative paths starting with ./ or ../

const ignore = require('ignore');
var ig = ignore().add(['.*']);
ig.filter(['foo', '.bar', './baz', '../quuz']);

Outputs: [ 'foo' ]

Similarly, relative ignore paths should be discarded as invalid:

const ignore = require('ignore')
var ig = ignore().add(['../*'])
ig.filter(['foo', '.bar', './baz', '../quuz'])
[ 'foo', '.bar', './baz' ]
@alberto alberto changed the title Ignore relative paths in filter Relative paths should be invalid Apr 16, 2016
@kaelzhang
Copy link
Owner

kaelzhang commented Apr 17, 2016

Let's discuss this first.

  1. relative paths like ../* should always be ignored.
  2. relatvie paths like ./foo shoule be converted to foo first (or not?)
  3. the conversion from ./foo -> foo should be done by user or by node-ignore?
path.join('./foo') // foo

@alberto
Copy link
Author

alberto commented Apr 17, 2016

Note you can do things like ./../foo so you are not guaranteed to always get a path in a subdirectory.

Ideally the conversion should by done by the user but node-ignore should not filter those if they don't, IMO.

Maybe you could try to get the relative path and exclude from filtering if they are outside the current directory, I don't know.

Here is another simplified example not mixing with relative ignore patters:

const ignore = require('ignore')
var ig = ignore().add(['foo.js'])
ig.filter(['foo.js', 'bar.js', '../foo.js'])

[ 'bar.js' ]

I don't think it should filter ../foo.js

@kaelzhang
Copy link
Owner

kaelzhang commented Oct 18, 2018

@alberto

I've tried to upgrade node-ignore to 5.x for eslint, but I found that it was harder than I expected.

There are lots of runtime logics that make node-ignore to filter relative paths in eslint, which I thought is not robust because node-ignore can not ensure idempotence for these cases.


With ignore@5.0.2, if we filter a relative path such as ./foo or ../bar, there will be an error thrown.

gfyoung added a commit to forking-repos/node-ignore that referenced this issue Nov 3, 2021
Blocking the acceptance of relative paths by throwing errors
is a relatively harsh change that has made it difficult for
downstream libraries (e.g., eslint) to upgrade.

Given that relative paths have undefined treatment in ".gitignore"
and the usage of this library beyond ".gitignore" behavior, it seems
fair to give downstream users the chance to customize the handling
of these cases for their own cases.

xref: kaelzhang#20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-discuss need further discussion need-spec
Projects
None yet
Development

No branches or pull requests

3 participants
@alberto @kaelzhang and others