fixes for paths on Windows #4188
Conversation
…sanitize_git_url_if_path`
It seems like an oversight that the algorithm from `jj git clone` wasn't used in them before
ilyagr
changed the title
| @@ -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.
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.
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.
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.
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::canonicalizewithdunce::canonicalizeeverywhere.I tried to improve (what used to be)
absolute_git_sourcein 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
dunceeverywhere 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
git2upgrade that #4080 does, see #4183. (There are also more trivial but annoying test failures with TEST_ENV)