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

windows: Make git update work #21808

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CharlesChen0823
Copy link
Contributor

@CharlesChen0823 CharlesChen0823 commented Dec 10, 2024

when introduce SanitizedPath for deal with nuc path in windows platform. IMO, we can also using it in fs::canonicalize and fs::watch in windows platform. It will make git update checkout work in windows.

@SomeoneToIgnore

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Dec 10, 2024
@maxdeviant maxdeviant changed the title windows: Make git update worked in windows platform windows: Make git update work Dec 10, 2024
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

Honestly, not sure at all, mainly due to no example and description of the issue this tries to solve.

I sure see

It will make git update checkout work in windows.

but which issue does it corresponds, what actions do I need to do to retest is missing.
And that oddity:

N/A or Added/Fixed/Improved ...

looks bad for a contributor who had been here for a while.


If I start to guess, this is related to git changes outside Zed, that cause FS changes?
That code is around the Workspace which #19727 was wrapping around with the Sanitized fix.

The discussion in that PR converged to an idea, that we fix things around the Workspace near to its code, so there's some sort of a barrier API there.

In addition to that Fs methods are sort of a "library" ones, so have to be relatively abstract and might better be close to std::fs of Rust stdlib by semantics?

This seems to be the opposite of all those ideas, so I'm not personally sure we have to change everything without any context at all.

async fn canonicalize(&self, path: &Path) -> Result<PathBuf> {
Ok(smol::fs::canonicalize(path).await?)
}

#[cfg(target_os = "windows")]
async fn canonicalize(&self, path: &Path) -> Result<PathBuf> {
use util::paths::SanitizedPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

But that thing is a noop already for non-windows?
So we do not need to do that at all it seems and could have always used SanitizedPath::from.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personly, I only want using dunce::simplified, using SanitizedPath only because it is exists there.

@SomeoneToIgnore SomeoneToIgnore self-assigned this Dec 11, 2024
@CharlesChen0823
Copy link
Contributor Author

CharlesChen0823 commented Dec 11, 2024

  1. when using git checkout to switch branch, inside and outside zed with terminal will not update branch information.

That code is around the Workspace which #19727 was wrapping around with the Sanitized fix.

unc path mostly introduce by fs::canonicalize, and in fs::watch which using fs::canonicalize behind. we cannot avoid unc path only by using SanitizedPath in Workspace.

In addition to that Fs methods are sort of a "library" ones, so have to be relatively abstract and might better be close to std::fs of Rust stdlib by semantics?

I think this is right, but we cannot avoid unc path by call fs::canonicalize. and i add some comments here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants