fix: Query Sync: don't check for validity of excludes that are system… #6841
Conversation
…-excludes (bazel-bin, .ijwb, etc.)
github-actions
bot
added
product: CLion
| @@ -85,6 +90,11 @@ public QuerySpec deriveQuerySpec(Context<?> context, Path workspaceRoot) throws | |||
| } | |||
| } | |||
| for (Path exclude : projectExcludes()) { | |||
| if(systemExcludes().contains(exclude.toString())){ | |||
| // We don't have to check if these dirs are valid for queries, because they also don't fall into //... wildcard | |||
| continue; | |||
There was a problem hiding this comment.
but do we still need to exclude them via result.excludePath(exclude); or not?
There was a problem hiding this comment.
I'm not sure, can investigate it later. The whole point of this PR is to avoid isValidPathForQuery for these paths which might be super expensive
There was a problem hiding this comment.
So that's basically an optimization, it doesn't change the behavior
There was a problem hiding this comment.
what happens if we added result.excludePath(exclude); instead of continue? if we get the same performance improvement then it seems like the right thing to do
There was a problem hiding this comment.
I haven't tried but I think it could go to the query, as a subtraction unconditionally, without validity check and hence could result in a crash
There was a problem hiding this comment.
I see that makes sense, I can merge it as is to release for 2024.3 but we may come up with a better optimization later
github-actions
bot
removed
the
awaiting-review
…-excludes (bazel-bin, .ijwb, etc.)
Demo for bazelbuild/bazel:
Screen.Recording.2024-10-03.at.23.15.04.mov
Checklist
Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.
Discussion thread for this change
Issue number:
<please reference the issue number or url here>Description of this change