-
Notifications
You must be signed in to change notification settings - Fork 379
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
test_git_push: make tests work on newer libgit2-s #4184
Conversation
This should make it unnecessary to disable a test in jj-vcs#4080.
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 agree that we'll need a TODO comment. And if we leave a TODO, I slightly prefer more scoped workaround (such as just disable the single failing test on Windows.)
If we want to add more practical workaround, I'll probably insert a workaround to commands::git::clone::absolute_git_source()
.
@@ -38,7 +38,8 @@ fn set_up() -> (TestEnvironment, PathBuf) { | |||
"git", | |||
"clone", | |||
"--config-toml=git.auto-local-branch=true", | |||
origin_git_repo_path.to_str().unwrap(), | |||
// Git and libgit2 > 1.8 do not allow backslashes in remote names | |||
&origin_git_repo_path.to_str().unwrap().replace("\\", "/"), |
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: might be safer to apply dunce::simplified()
first. I didn't know the canonicalized Windows path can have forward slashes.
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.
didn't know the canonicalized Windows path can have forward slashes.
It's madness. Officially, they can't. For libgit2
and Git, who knows. See also rhysd/path-slash#6.
You can see my draft of a more principled solution in #4188, but I do not like where it is going so far.
This PR might still be useful for reference, but I no longer plan on merging it. |
This should make it unnecessary to disable a test
in #4080.
See also CI run in #4183
This is a messy fix, I should probably add a TODO for doing something more reasonable when
jj git clone
gets passed a path with backslahshes.Update: The CI seems slow, but I'll mark this as ready for review. Hopefully, the CI will recover on its own.
Update 2: I don't think this is urgent, so I'll see about a more complete fix with a better error message when
jj git clone
orgit remote add
get an invalid remote name.