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

Fix crash when cwd is deleted #7185

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Conversation

yo-main
Copy link
Contributor

@yo-main yo-main commented May 30, 2023

Should solve #2527

I didn't figure out how to set an error on the editor from some places.

Where is was crashing before, it now relies on the scratch buffer instead.
In some other places (not related to buffer, like the open command or the global search ), I was able to generate a message or rely on the default value of the PathBuffer.

Let me know any improvements/suggestions you can see, I'll be more than happy to correct them :)

@kirawi kirawi added C-bug Category: This is a bug A-helix-term Area: Helix term improvements labels May 31, 2023
helix-term/src/application.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-term/src/commands/lsp.rs Outdated Show resolved Hide resolved
@the-mikedavis the-mikedavis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 13, 2023
@yo-main
Copy link
Contributor Author

yo-main commented Jun 13, 2023

Thanks for reviewing !
Changes have been done accordingly

helix-term/src/commands.rs Outdated Show resolved Hide resolved
helix-view/src/document.rs Outdated Show resolved Hide resolved
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

This is looking good, just needs the clippy lints fixed

@yo-main yo-main force-pushed the master branch 2 times, most recently from bb63440 to e027009 Compare June 16, 2023 20:37
@yo-main
Copy link
Contributor Author

yo-main commented Jun 16, 2023

should be fine now 🤞

the-mikedavis
the-mikedavis previously approved these changes Jun 17, 2023
@the-mikedavis the-mikedavis added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2023
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

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

Sorry for being a bit late here. I put off reviewing this because I thin this is actually a bit more subtle to solve. IMO it's really unfortunate that get_cwd can fail (if the directory doesn't exist anymore) ideally I would still like to obtain the path and then fail when attempting any IO (which can always fail so no special handeling required).

This PR often uses unwrap_or_default or other suboptimal fallback strategies. I would really like to avoid this as that can lead to other issues.

I think a better approach would be to track CWD in helix internally. Just store the path in a static that is intalized on startup (and if we fail to acquire the cwd on startup we can just exit with an error). The only way CWD can change at runtime is by using the command so we only need to update the static in that command.

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jun 17, 2023
@yo-main yo-main marked this pull request as draft June 18, 2023 16:55
@yo-main yo-main marked this pull request as ready for review June 18, 2023 20:36
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

It looks like we will need to do some path expansion. mkdir tmp and then either cargo r tmp or cargo r and then :cd tmp: The directory will change correctly but current_working_dir will return tmp and not the full path. This messes up consumers like file_picker that are expecting an expanded path.

I'm not totally sure where this expansion should belong. We could move

pub fn get_canonicalized_path(path: &Path) -> std::io::Result<PathBuf> {
let path = expand_tilde(path);
let path = if path.is_relative() {
std::env::current_dir().map(|current_dir| current_dir.join(path))?
} else {
path
};
Ok(get_normalized_path(path.as_path()))
}
over to helix-loader and call it from set_current_working_dir. That would shift around some dependencies and code between helix-core and helix-loader but might be the cleaner option. Or we could require that each caller of set_current_working_dir passes in an absolute path (maybe a debug_assert could work here). What do you think?

@pascalkuthe
Copy link
Member

pascalkuthe commented Jul 9, 2023

It looks like we will need to do some path expansion. mkdir tmp and then either cargo r tmp or cargo r and then :cd tmp: The directory will change correctly but current_working_dir will return tmp and not the full path. This messes up consumers like file_picker that are expecting an expanded path.

I'm not totally sure where this expansion should belong. We could move

pub fn get_canonicalized_path(path: &Path) -> std::io::Result<PathBuf> {
let path = expand_tilde(path);
let path = if path.is_relative() {
std::env::current_dir().map(|current_dir| current_dir.join(path))?
} else {
path
};
Ok(get_normalized_path(path.as_path()))
}

over to helix-loader and call it from set_current_working_dir. That would shift around some dependencies and code between helix-core and helix-loader but might be the cleaner option. Or we could require that each caller of set_current_working_dir passes in an absolute path (maybe a debug_assert could work here). What do you think?

Good catch! .we can actually use dunce::cannonicolize derctly here I think. That canonicalization function only makes a difference if the path doesn't currently exits (wich is not the case here since the working directory must exist). I wouldn't want to move all the code over but just adding that one extra dependency seems fine to me

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Jul 9, 2023
@yo-main yo-main force-pushed the master branch 2 times, most recently from 63fed09 to def2505 Compare July 10, 2023 19:12
@yo-main
Copy link
Contributor Author

yo-main commented Jul 10, 2023

updated ! thanks for spotting :)

@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 10, 2023
pascalkuthe
pascalkuthe previously approved these changes Jul 10, 2023
The call to std::env::current_dir might fail
if the cwd has been deleted. As a way to prevent
this from happening, we keep track internally of
the current working directory and display an error
message when any I/O is attempted.
Copy link
Member

Choose a reason for hiding this comment

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

For the two changes here we will probably want some feedback:

if !cwd.exists() {
    editor.set_error("Current working directory does not exist");
    return;
}

since the file_picker_in_current_directory won't show any files and global_search won't be able to find any matches if the cwd doesn't exist.

Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

Looks good 👍

I like the "deleted" message in the show_current_directory too, that's a nice touch

@pascalkuthe pascalkuthe merged commit 8afc028 into helix-editor:master Jul 11, 2023
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants