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

Better handling of symlinks #2718

Merged
merged 1 commit into from
Jun 15, 2022
Merged

Better handling of symlinks #2718

merged 1 commit into from
Jun 15, 2022

Conversation

zen3ger
Copy link
Contributor

@zen3ger zen3ger commented Jun 8, 2022

  • Add file-picker.follow-symlinks configuration option (default is true), this
    also controls if filename and directory completers follow symlinks.

  • Update FilePicker to set editor error if opening a file fails, instead of
    panicing.

Fix #1548
Fix #2246

Comment on lines 164 to 156
// Only list symlink if points to a file which we have permisson for
match std::fs::metadata(entry.path()).map(|meta| meta.is_file()) {
Err(_) | Ok(false) => None,
Ok(true) => Some(entry.into_path()),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe just setting follow_links(true) on the builder in this case would be better, would fix #2246 as well...

Copy link
Member

@archseer archseer Jun 9, 2022

Choose a reason for hiding this comment

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

I think so, fs::metadata is way too costly to run per-entry (out of potentially thousands). I specifically removed code calling fs::metadata from the completion functions 78fba86#diff-a2f72be0dae9186774ac716ae568f5216a5223dbf91e6ac8a25f62617c4fa55cL144

Actually, wouldn't this make more sense to do inside editor.open? Resolve the symlink, then either set_error "Not allowed to open the file" or open the file. I don't think files with insufficient permissions should just get hidden, it makes it seem like they're not there at all

Copy link
Contributor Author

@zen3ger zen3ger Jun 12, 2022

Choose a reason for hiding this comment

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

I've removed fs::metadata calls and changed to use follow_symlinks() instead. Added a configuration option to FilePicker to enable/disable following symlinks, so if it would lead to performance issues it can at least be toggled. Also error is now reported the same way like in :cd and :open if file picker fails. Hope I covered everything you mentioned.

- Add file-picker.follow-symlinks configuration option (default is true), this
  also controls if filename and directory completers follow symlinks.

- Update FilePicker to set editor error if opening a file fails, instead of
  panicing.

Fix helix-editor#1548
Fix helix-editor#2246
@archseer archseer merged commit c2cc203 into helix-editor:master Jun 15, 2022
lazytanuki pushed a commit to lazytanuki/helix that referenced this pull request Jun 21, 2022
- Add file-picker.follow-symlinks configuration option (default is true), this
  also controls if filename and directory completers follow symlinks.

- Update FilePicker to set editor error if opening a file fails, instead of
  panicing.

Fix helix-editor#1548
Fix helix-editor#2246
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when opening a symlink :change-current-directory does not autocomplete for symlinks to directories
2 participants