-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Revert "fix(cli): deno upgrade file permission (#18427)" #18467
Conversation
This reverts commit 0742ea1.
@@ -265,15 +265,13 @@ pub async fn upgrade( | |||
) -> Result<(), AnyError> { | |||
let ps = ProcState::build(flags).await?; | |||
let current_exe_path = std::env::current_exe()?; | |||
let output_exe_path = | |||
upgrade_flags.output.as_ref().unwrap_or(¤t_exe_path); | |||
let metadata = fs::metadata(output_exe_path)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just change this to never throw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. The fix is easier than the revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand how to change that... metadata
and permissions
by extension are used further down in the file. How should that be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least a quick scroll down says that metadata is only used to check for permissions and root ownership. All of that is unnecessary if the file doesn't exist yet and thus could be handled in a if let Some()
branch I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to land this one for the release, but we should fix this properly in the future.
This reverts commit 0742ea1.
Closes #18466
CC @rottenpen