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

fixes for paths on Windows #4188

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

fixes for paths on Windows #4188

wants to merge 5 commits into from

Conversation

ilyagr
Copy link
Collaborator

@ilyagr ilyagr commented Jul 31, 2024

This is my attempt to replace #4184 with something more principled. It is, however, not so principled as to replace fs::canonicalize with dunce::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)

ilyagr added 5 commits July 30, 2024 21:43
@ilyagr ilyagr changed the title Help wanted: fixes for paths on Windows fixes for paths on Windows Jul 31, 2024
@ilyagr ilyagr added the help wanted Extra attention is needed label Jul 31, 2024
@@ -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))
Copy link
Collaborator

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(
Copy link
Collaborator

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.

Copy link
Collaborator Author

@ilyagr ilyagr Aug 1, 2024

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)

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants