-
Notifications
You must be signed in to change notification settings - Fork 343
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
fixes for paths on Windows #4188
base: main
Are you sure you want to change the base?
Conversation
…sanitize_git_url_if_path`
It seems like an oversight that the algorithm from `jj git clone` wasn't used in them before
@@ -1164,7 +1198,7 @@ pub fn set_remote_url( | |||
})?; | |||
|
|||
git_repo | |||
.remote_set_url(remote_name, new_remote_url) | |||
.remote_set_url(remote_name, &sanitize_git_url_if_path(cwd, new_remote_url)) |
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.
nit: this should be handled by CLI because it's UI issue to resolve relative path.
(BTW, I think it's good idea to move the path conversion function to library.)
// original source is a non-UTF Windows path. For Windows UNC paths, we | ||
// could use dunce, though see also https://gitlab.com/kornelski/dunce/-/issues/7 | ||
// TODO: double-check about UNC paths; what does Git do? | ||
cwd.join(source).to_slash().map_or_else( |
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't we simply do replace MAIN_SEPARATOR
to slash (if cfg!(windows)
)? path-slash
might be optimized if we have to deal with Path
/OsStr
, but here we build String
anyway.
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.
This is on my list to try and see if it makes things saner (that is, makes it easier to make the merge with #4080 work).
My main worry (and the reason I didn't start with search-and-replace) is about UNC paths, especially the ones appearing in our tests. I would have to find out experimentally whether I can get rid of them via dunce
, and/or I'd have to find out how libgit2 deals with paths like:
\\?\C:/Path/
//?/C:/Path/
Neither of these is correct according to Microsoft, but I think libgit2
definitely chokes on the correct \\?\C:\Path\
and I have a mild suspicion that it might prefer one f the incorrect options. I wish I knew what Git did; I think we'd have to ask or read the source to find out.
(As for dependency on path-slash, that library hasn't been updated in years and it's probably pretty simple, so we could re-implement it if needed)
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 think something like dunce::simplified(..).replace(MAIN_SEPARATOR, "/")
is good as a workaround. It might not work if the path couldn't be simplified, but that wouldn't be worse than the current state.
This is my attempt to replace #4184 with something more principled. It is, however, not so principled as to replace
fs::canonicalize
withdunce::canonicalize
everywhere.I tried to improve (what used to be)
absolute_git_source
in general, but I may have introduced some bugs.It feels a bit like whack-a-mole. I couldn't find rules on what kind of Windows paths Git or libgit2 accept in clone URLs so far. I'm also introducing more and more ways the same path can look in a test.
I'm still looking into it, but I am running out of patience (though perhaps I'll get some better ideas later). Advice is welcome. Do you think using
dunce
everywhere would help, for example? Also, my Windows setup is awkward; I'm running it in a VM and it's not very fast and runs out of memory or causes the main system to run out of memory occasionally. Perhaps because of that, or perhaps it's on a USB drive, it also freezes occasionally.At the moment, it fails a different test when merged with
git2
upgrade that #4080 does, see #4183. (There are also more trivial but annoying test failures with TEST_ENV)