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

VSCode document selector is not working when one project name is subset of other #41724

Closed
Apoorva-GA opened this issue Jan 17, 2018 · 14 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug file-glob File glob engine verified Verification succeeded
Milestone

Comments

@Apoorva-GA
Copy link

VSCode document selector is not working when one project name is subset of other.

The glob pattern that we use to match is $projectroot/**/* which is use to differentiate between two projects in a workspace.

let clientOptions = {
        documentSelector: [{ scheme: 'file', language: 'gauge', pattern: `${folder.uri.fsPath}/**/*` }]
}

The above glob pattern seems to work with shell commands but not in VSCode

All the requests are being sent to both the language servers starting with the similar name.

Steps to Reproduce:

  1. Create a project with name test.
  2. Create an other project with name testone.
  3. Add both to a workspace.
  • VSCode Version: 1.19.2
  • OS Version: 10.13.2 (mac)
@bpasero
Copy link
Member

bpasero commented Jan 18, 2018

I can reproduce, the issue is that a glob pattern like /Users/bpasero/Development/Microsoft/monaco/src/vs/base/test/node/**/* matches on /Users/bpasero/Development/Microsoft/monaco/src/vs/base/test/node as well as /Users/bpasero/Development/Microsoft/monaco/src/vs/base/test/nodestar

@bpasero bpasero added the bug Issue identified by VS Code Team member as probable bug label Jan 18, 2018
@bpasero bpasero added this to the On Deck milestone Jan 18, 2018
@bpasero bpasero added the file-glob File glob engine label Jan 18, 2018
@bpasero bpasero changed the title VSCode document selector is not working when one project name is subset of other. Glob: the globstar (**) should not match on empty strings but require a path segment Jan 18, 2018
@bpasero
Copy link
Member

bpasero commented Jan 18, 2018

@Apoorva-GA I think a workaround would be to set pattern: ${folder.uri.fsPath}/*/**. The single * means at least one character while the ** can mean 0-N path segments. Can you try that?

I fear about breaking people if we change our glob star to require a path segment, since we have behaved like this from day one. I know that other glob libraries seem to treat ** as "at least one path segment must follow".

/cc @chrmarti @roblourens

@bpasero bpasero removed this from the On Deck milestone Jan 18, 2018
@chrmarti
Copy link
Contributor

.../node/**/* shouldn't match .../nodestar, it should at least require node as a separate path segment, like: .../node/star. I wouldn't expect a fix to this to cause much breakage since the faulty behavior is quite unexpected from what I see.

@bpasero bpasero added this to the January 2018 milestone Jan 18, 2018
@bpasero
Copy link
Member

bpasero commented Jan 19, 2018

Unfortunately I have to close this "as designed" simply because it would break too many things in our code. To give one example: Our default files.exclude setting is this:

"files.exclude": {
    "**/.git": true,
    "**/.svn": true,
    "**/.hg": true,
    "**/CVS": true,
    "**/.DS_Store": true
  }

But the matching in the files explorer for example is always relative to the root (e.g. we will match ".git" onto "**/.git"). If we change ** to be required, a top level ".git" folder would no longer match. There seem to be more examples, e.g. our extension recommendation system matching on file extensions would also break.

@bpasero bpasero closed this as completed Jan 19, 2018
@bpasero bpasero added the *as-designed Described behavior is as designed label Jan 19, 2018
@chrmarti
Copy link
Contributor

@bpasero What I suggested wouldn't break these cases, maybe I misunderstood your example from earlier?

@bpasero
Copy link
Member

bpasero commented Jan 19, 2018

@chrmarti so then maybe try to explain how "**" should behave? To give an example, the Python library implements **/*.js in a way that it will not match on something.js when I tested from the CLI.

@chrmarti
Copy link
Contributor

@bpasero I agree it should match zero or more path segments. But node/**/*.js shouldn't match node_modules/foo.js nor node_modules/bar/baz.js, it should only match with node as a path segment and not with node as a prefix of a path segment. You said the current behavior is to match in that case in #41724 (comment).

@bpasero
Copy link
Member

bpasero commented Jan 19, 2018

@chrmarti I am having a hard time to encode that logic, nor understanding it. I find it hard to understand how a leading ** can be 0 paths (so that **/*.js matches on some.js) but an embedded usage such as node/**/*.js would not match node/some.js.

Btw if we were to change it, we would also need at the trivias we added for avoiding the RegExp, we seem to hit that case too with these patterns.

@chrmarti
Copy link
Contributor

@bpasero I agree, what I'm saying is: node/**/*.js should match node/some.js, but not node_modules/some.js.

@bpasero
Copy link
Member

bpasero commented Jan 19, 2018

Good point 👍

@bpasero bpasero reopened this Jan 19, 2018
@bpasero bpasero removed the *as-designed Described behavior is as designed label Jan 19, 2018
@bpasero bpasero changed the title Glob: the globstar (**) should not match on empty strings but require a path segment VSCode document selector is not working when one project name is subset of other Jan 19, 2018
@bpasero
Copy link
Member

bpasero commented Jan 19, 2018

I think I found a fix. In the parsing code (here) we typically add a path separator regex to the segments, but not if the next segment is a "**" pattern. I am still not brave enough to always require a path separator, but the fix is to require it if there are more segments to follow.

To verify: you can use the files.exclude setting in the explorer and have 2 folders (on the same level) where one folder is the substring of another folder and then e.g. exclude all JS of the one folder with a pattern like **/folder/**/*.js and verify that only the JS files in the one folder are hidden, not in the other.

ColCh added a commit to ColCh/vscode that referenced this issue Jan 20, 2018
* master: (280 commits)
  fix issue with useAltAsMultiSelectModifier
  Log ripgrep cmd from quickopen to logservice
  Switch search output panel to logservice
  Use absolute paths for webview core resources
  Explicitly hide TS version status when in ts/js file with unknown scheme
  First attempt of search caching.
  Pick up latest TS insiders
  2018-01-19. Merged in translations from Transifex.
  Avoid double-counting same settings search query
  Use latest emmet helper that has fixes microsoft#33818
  Remove settings search @groupId filters - since group ids can now be extension guids, and nobody uses this anyway
  backups - make sure to use "utf8" as encoding and not "utf-8"
  Catch any errors that onLineData users throw
  use uuid as extensions configuration id
  fix format on save problem that occurs when a provider returns no edits
  fix microsoft#41724
  Fix microsoft#41868
  some menu polish for issue reporter
  Fix microsoft#40260
  Simplify edits handling in TextModelTokens
  ...
@Apoorva-GA
Copy link
Author

@bpasero the workaround you suggested worked for me, but looks like this is fixed now.

Can I know in which insider version the fix is available?

@bpasero
Copy link
Member

bpasero commented Jan 22, 2018

@Apoorva-GA fix will be in todays insider build, let me know if it works for you.

@Apoorva-GA
Copy link
Author

@bpasero It works for me 👍
Thanks for the fix.

@bpasero bpasero added the verified Verification succeeded label Jan 23, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Mar 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug file-glob File glob engine verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants