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.

Sign up for GitHub

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

fix crash on checking excluded folders with missing project data #2276

Merged
merged 2 commits into from
May 23, 2023
+3 −2
Merged

fix crash on checking excluded folders with missing project data #2276

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
082948d
fix crash on checking excluded folders with missing project data
rchl May 20, 2023
9fccfa6
add a comment
rchl May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions plugin/core/workspace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ def __init__(self, window: sublime.Window) -> None:
self._update_exclude_patterns(self.folders)

def _update_exclude_patterns(self, folders: List[str]) -> None:
self._folders_exclude_patterns = []
# Ensure that the number of patterns matches the number of folders so that accessing by index never throws.
self._folders_exclude_patterns = [[]] * len(folders)
project_data = self._window.project_data()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would definitely put a comment on that line, or link to the issue,
it was not obvious why it needs to be multiplied by the length of the folders

Copy link
Member

@jwortmann jwortmann May 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[x] * n is a concise syntax in Python to create a list with n entries which are all x, i.e. here it will be for example [[], [], []] with 3 folders.
But yeah, I was also surprised that this bug can happen, and I never saw it with "normal" usage. The window.project_data() even returns a dict with the (sidebar) folders, if there is no project at all (iirc ST automatically creates a "hidden" project in that case), or when ST is started by double-clicking a single .sublime-workspace file of a project. It might be possible with multiple workspace files or if you edit them manually.
But with this initialization line we are certain to be on the safe side, so it looks fine to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that those lists created via [[]] * len(folders) refer to exactly the same thing. Not sure whether this is what you want but just provide a potential issue.

>>> a = [[]] * 4
>>> a[0].append('foo')
>>> a
[['foo'], ['foo'], ['foo'], ['foo']]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine, because each entry gets replaced completely, not appended to:

self._folders_exclude_patterns[i] = exclude_patterns

For example

>>> a = [[]] * 4
>>> a[1] = ['foo']
>>> a
[[], ['foo'], [], []]

Copy link
Member Author

@rchl rchl May 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw. it's expected that this creates a list of lists since exclude patterns are a list and there would be one of those for each folder.
The current code would also be fine with other value (like None) but this is more consistent with the real (correct) case.

if not isinstance(project_data, dict):
return
Expand All @@ -79,7 +80,7 @@ def _update_exclude_patterns(self, folders: List[str]) -> None:
else:
exclude_patterns.append(sublime_pattern_to_glob('//' + pattern, True, path))
exclude_patterns.append(sublime_pattern_to_glob('//**/' + pattern, True, path))
self._folders_exclude_patterns.append(exclude_patterns)
self._folders_exclude_patterns[i] = exclude_patterns

def update(self) -> bool:
new_folders = self._window.folders()
Expand Down