From 9a6e5d4097a820a2128397286efc77c1196bfd29 Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Sat, 13 Jul 2024 20:29:13 +0800 Subject: [PATCH 1/7] cargo: bump `git2` to 0.19.0 This includes a bump of `libgit2` to v1.8.1. --- Cargo.lock | 8 ++++---- Cargo.toml | 2 +- cli/tests/test_git_push.rs | 2 ++ lib/src/git.rs | 7 ++++++- 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1dbaf1d956..422c0328a6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -967,9 +967,9 @@ checksum = "40ecd4077b5ae9fd2e9e169b102c6c330d0605168eb0e8bf79952b256dbefffd" [[package]] name = "git2" -version = "0.18.3" +version = "0.19.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "232e6a7bfe35766bf715e55a88b39a700596c0ccfd88cd3680b4cdb40d66ef70" +checksum = "b903b73e45dc0c6c596f2d37eccece7c1c8bb6e4407b001096387c63d0d93724" dependencies = [ "bitflags 2.6.0", "libc", @@ -2019,9 +2019,9 @@ checksum = "97b3888a4aecf77e811145cadf6eef5901f4782c53886191b2f693f24761847c" [[package]] name = "libgit2-sys" -version = "0.16.2+1.7.2" +version = "0.17.0+1.8.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ee4126d8b4ee5c9d9ea891dd875cfdc1e9d0950437179104b183d7d8a74d24e8" +checksum = "10472326a8a6477c3c20a64547b0059e4b0d086869eee31e6d7da728a8eb7224" dependencies = [ "cc", "libc", diff --git a/Cargo.toml b/Cargo.toml index 8e6e514b28..60740704f8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,7 +48,7 @@ dunce = "1.0.4" either = "1.13.0" esl01-renderdag = "0.3.0" futures = "0.3.30" -git2 = "0.18.3" +git2 = "0.19.0" gix = { version = "0.63.0", default-features = false, features = [ "index", "max-performance-safe", diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index 8126e24442..7e3cfe75f1 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -1235,6 +1235,8 @@ fn test_git_push_moved_sideways_untracked() { } #[test] +// TODO: This test fails with libgit2 v1.8.1 on Windows. +#[cfg(not(target_os = "windows"))] fn test_git_push_to_remote_named_git() { let (test_env, workspace_root) = set_up(); let git_repo = { diff --git a/lib/src/git.rs b/lib/src/git.rs index d7066a22f7..f4030a7ca2 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1265,7 +1265,12 @@ pub fn fetch( tracing::debug!("remote.prune"); remote.prune(None)?; tracing::debug!("remote.update_tips"); - remote.update_tips(None, false, git2::AutotagOption::Unspecified, None)?; + remote.update_tips( + None, + git2::RemoteUpdateFlags::empty(), + git2::AutotagOption::Unspecified, + None, + )?; // TODO: We could make it optional to get the default branch since we only care // about it on clone. let mut default_branch = None; From 91f3d5ef2cd6c812ae4307f37800086fd14976d1 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Sun, 28 Jul 2024 21:37:10 -0700 Subject: [PATCH 2/7] test_git_push: make tests work on newer libgit2-s This should make it unnecessary to disable a test in #4080. --- cli/tests/test_git_push.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index 8126e24442..c923cd5b18 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -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("\\", "/"), "local", ], ); From 7b97d969ed62d8c0c4ff8ae6c41cc442dc6fb700 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 30 Jul 2024 20:49:03 -0700 Subject: [PATCH 3/7] backout of commit 91f3d5ef2cd6c812ae4307f37800086fd14976d1 --- cli/tests/test_git_push.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index c923cd5b18..8126e24442 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -38,8 +38,7 @@ fn set_up() -> (TestEnvironment, PathBuf) { "git", "clone", "--config-toml=git.auto-local-branch=true", - // Git and libgit2 > 1.8 do not allow backslashes in remote names - &origin_git_repo_path.to_str().unwrap().replace("\\", "/"), + origin_git_repo_path.to_str().unwrap(), "local", ], ); From bb01575cd721f6d81e6bdf4b35bfd1af735096ab Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 30 Jul 2024 20:36:51 -0700 Subject: [PATCH 4/7] path_slash --- Cargo.lock | 7 +++++++ Cargo.toml | 1 + cli/Cargo.toml | 1 + cli/src/commands/git/clone.rs | 16 +++++++++++----- cli/tests/common/mod.rs | 8 +++++--- 5 files changed, 25 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1dbaf1d956..cf10983eb6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1879,6 +1879,7 @@ dependencies = [ "maplit", "minus", "once_cell", + "path-slash", "pest", "pest_derive", "pollster", @@ -2341,6 +2342,12 @@ version = "1.0.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "57c0d7b74b563b49d38dae00a0c37d4d6de9b432382b2892f0574ddcae73fd0a" +[[package]] +name = "path-slash" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e91099d4268b0e11973f036e885d652fb0b21fedcf69738c627f94db6a44f42" + [[package]] name = "pathdiff" version = "0.2.1" diff --git a/Cargo.toml b/Cargo.toml index 8e6e514b28..a91efcc0a7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -67,6 +67,7 @@ maplit = "1.0.2" minus = { version = "5.6.1", features = ["dynamic_output", "search"] } num_cpus = "1.16.0" once_cell = "1.19.0" +path-slash = "0.2.1" pest = "2.7.11" pest_derive = "2.7.11" pollster = "0.3.0" diff --git a/cli/Cargo.toml b/cli/Cargo.toml index ffb41e2997..e8c6656c9f 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -74,6 +74,7 @@ jj-lib = { workspace = true } maplit = { workspace = true } minus = { workspace = true } once_cell = { workspace = true } +path-slash = { workspace = true } pest = { workspace = true } pest_derive = { workspace = true } pollster = { workspace = true } diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index b7cfdf8101..018b5d3391 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -20,6 +20,7 @@ use jj_lib::git::{self, GitFetchError, GitFetchStats}; use jj_lib::repo::Repo; use jj_lib::str_util::StringPattern; use jj_lib::workspace::Workspace; +use path_slash::PathBufExt; use crate::cli_util::{CommandHelper, WorkspaceCommandHelper}; use crate::command_error::{cli_error, user_error, user_error_with_message, CommandError}; @@ -53,12 +54,17 @@ fn absolute_git_source(cwd: &Path, source: &str) -> String { // be tedious to copy the exact git (or libgit2) behavior, we simply assume a // source containing ':' is a URL, SSH remote, or absolute path with Windows // drive letter. + // TODO: Match "This syntax is only recognized if there are no slashes before the first colon" from https://git-scm.com/docs/git-clone#_git_urls + // TODO: Pass paths like `c:\qq` to path_slash if !source.contains(':') && Path::new(source).exists() { - // It's less likely that cwd isn't utf-8, so just fall back to original source. - cwd.join(source) - .into_os_string() - .into_string() - .unwrap_or_else(|_| source.to_owned()) + // TODO: This won't work for Windows UNC path or (less importantly) if the + // 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 + cwd.join(source).to_slash().map_or_else( + // It's less likely that cwd isn't utf-8, so just fall back to original source. + || source.to_owned(), + |s| s.to_string(), + ) } else { source.to_owned() } diff --git a/cli/tests/common/mod.rs b/cli/tests/common/mod.rs index 72f4eb5a51..32ce520b7f 100644 --- a/cli/tests/common/mod.rs +++ b/cli/tests/common/mod.rs @@ -17,6 +17,7 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; use itertools::Itertools as _; +use path_slash::PathBufExt; use regex::{Captures, Regex}; use tempfile::TempDir; @@ -320,13 +321,14 @@ impl TestEnvironment { pub fn normalize_output(&self, text: &str) -> String { let text = text.replace("jj.exe", "jj"); let regex = Regex::new(&format!( - r"{}(\S+)", - regex::escape(&self.env_root.display().to_string()) + r"({}|{})(\S+)", + regex::escape(&self.env_root.display().to_string()), + regex::escape(&self.env_root.to_slash_lossy()) )) .unwrap(); regex .replace_all(&text, |caps: &Captures| { - format!("$TEST_ENV{}", caps[1].replace('\\', "/")) + format!("$TEST_ENV{}", caps[2].replace('\\', "/")) }) .to_string() } From 8bc865946674ba96ad697762d5f9609909866d74 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 30 Jul 2024 21:05:36 -0700 Subject: [PATCH 5/7] looks_like_a_path_to_git --- cli/src/commands/git/clone.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index 018b5d3391..ffb5fbc3ca 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -48,15 +48,24 @@ pub struct GitCloneArgs { colocate: bool, } +fn looks_like_a_path_to_git(source: &str) -> bool { + source.chars().nth(1) == Some(':') || // Looks like a Windows C:... path + // Match Git's behavior, the URL syntax [is only recognized if there are no + // slashes before the first + // colon](https://git-scm.com/docs/git-clone#_git_urls) + !source + .split_once("/") + .map_or(source, |(first, _)| first) + .contains(":") +} + fn absolute_git_source(cwd: &Path, source: &str) -> String { // Git appears to turn URL-like source to absolute path if local git directory // exits, and fails because '$PWD/https' is unsupported protocol. Since it would // be tedious to copy the exact git (or libgit2) behavior, we simply assume a // source containing ':' is a URL, SSH remote, or absolute path with Windows // drive letter. - // TODO: Match "This syntax is only recognized if there are no slashes before the first colon" from https://git-scm.com/docs/git-clone#_git_urls - // TODO: Pass paths like `c:\qq` to path_slash - if !source.contains(':') && Path::new(source).exists() { + if looks_like_a_path_to_git(source) && Path::new(source).exists() { // TODO: This won't work for Windows UNC path or (less importantly) if the // 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 From f87f89f36f5513ec607a50ebd041f8725281ebb8 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 30 Jul 2024 21:31:58 -0700 Subject: [PATCH 6/7] git clone: move `absolute_git_source` from cli to lib --- Cargo.lock | 1 + cli/src/commands/git/clone.rs | 36 ++--------------------------------- lib/Cargo.toml | 1 + lib/src/git.rs | 34 ++++++++++++++++++++++++++++++++- 4 files changed, 37 insertions(+), 35 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cf10983eb6..2dfdd0e696 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1932,6 +1932,7 @@ dependencies = [ "maplit", "num_cpus", "once_cell", + "path-slash", "pest", "pest_derive", "pollster", diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index ffb5fbc3ca..9c5a4b9ed9 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -16,11 +16,10 @@ use std::io::Write; use std::path::{Path, PathBuf}; use std::{fs, io}; -use jj_lib::git::{self, GitFetchError, GitFetchStats}; +use jj_lib::git::{self, sanitize_git_url_if_path, GitFetchError, GitFetchStats}; use jj_lib::repo::Repo; use jj_lib::str_util::StringPattern; use jj_lib::workspace::Workspace; -use path_slash::PathBufExt; use crate::cli_util::{CommandHelper, WorkspaceCommandHelper}; use crate::command_error::{cli_error, user_error, user_error_with_message, CommandError}; @@ -48,37 +47,6 @@ pub struct GitCloneArgs { colocate: bool, } -fn looks_like_a_path_to_git(source: &str) -> bool { - source.chars().nth(1) == Some(':') || // Looks like a Windows C:... path - // Match Git's behavior, the URL syntax [is only recognized if there are no - // slashes before the first - // colon](https://git-scm.com/docs/git-clone#_git_urls) - !source - .split_once("/") - .map_or(source, |(first, _)| first) - .contains(":") -} - -fn absolute_git_source(cwd: &Path, source: &str) -> String { - // Git appears to turn URL-like source to absolute path if local git directory - // exits, and fails because '$PWD/https' is unsupported protocol. Since it would - // be tedious to copy the exact git (or libgit2) behavior, we simply assume a - // source containing ':' is a URL, SSH remote, or absolute path with Windows - // drive letter. - if looks_like_a_path_to_git(source) && Path::new(source).exists() { - // TODO: This won't work for Windows UNC path or (less importantly) if the - // 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 - cwd.join(source).to_slash().map_or_else( - // It's less likely that cwd isn't utf-8, so just fall back to original source. - || source.to_owned(), - |s| s.to_string(), - ) - } else { - source.to_owned() - } -} - fn clone_destination_for_source(source: &str) -> Option<&str> { let destination = source.strip_suffix(".git").unwrap_or(source); let destination = destination.strip_suffix('/').unwrap_or(destination); @@ -104,7 +72,7 @@ pub fn cmd_git_clone( return Err(cli_error("--at-op is not respected")); } let remote_name = "origin"; - let source = absolute_git_source(command.cwd(), &args.source); + let source = sanitize_git_url_if_path(command.cwd(), &args.source); let wc_path_str = args .destination .as_deref() diff --git a/lib/Cargo.toml b/lib/Cargo.toml index 5750331785..b0a7189de8 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -52,6 +52,7 @@ itertools = { workspace = true } jj-lib-proc-macros = { workspace = true } maplit = { workspace = true } once_cell = { workspace = true } +path-slash = { workspace = true } pest = { workspace = true } pest_derive = { workspace = true } pollster = { workspace = true } diff --git a/lib/src/git.rs b/lib/src/git.rs index d7066a22f7..f1e2d6a38d 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -18,11 +18,12 @@ use std::borrow::Cow; use std::collections::{BTreeMap, HashMap, HashSet}; use std::default::Default; use std::io::Read; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::{fmt, str}; use git2::Oid; use itertools::Itertools; +use path_slash::PathBufExt; use tempfile::NamedTempFile; use thiserror::Error; @@ -1143,6 +1144,37 @@ pub fn rename_remote( Ok(()) } +fn looks_like_a_path_to_git(source: &str) -> bool { + source.chars().nth(1) == Some(':') || // Looks like a Windows C:... path + // Match Git's behavior, the URL syntax [is only recognized if there are no + // slashes before the first + // colon](https://git-scm.com/docs/git-clone#_git_urls) + !source + .split_once("/") + .map_or(source, |(first, _)| first) + .contains(":") +} + +pub fn sanitize_git_url_if_path(cwd: &Path, source: &str) -> String { + // Git appears to turn URL-like source to absolute path if local git directory + // exits, and fails because '$PWD/https' is unsupported protocol. Since it would + // be tedious to copy the exact git (or libgit2) behavior, we simply assume a + // source containing ':' is a URL, SSH remote, or absolute path with Windows + // drive letter. + if looks_like_a_path_to_git(source) && Path::new(source).exists() { + // TODO: This won't work for Windows UNC path or (less importantly) if the + // 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 + cwd.join(source).to_slash().map_or_else( + // It's less likely that cwd isn't utf-8, so just fall back to original source. + || source.to_owned(), + |s| s.to_string(), + ) + } else { + source.to_owned() + } +} + pub fn set_remote_url( git_repo: &git2::Repository, remote_name: &str, From 3b4701fdfdb858451023a000f26719c82d985982 Mon Sep 17 00:00:00 2001 From: Ilya Grigoriev Date: Tue, 30 Jul 2024 21:39:17 -0700 Subject: [PATCH 7/7] Use `sanitize_git_url_if_path` in `jj git remote add/set-url` --- cli/src/commands/git/remote/add.rs | 8 ++++++-- cli/src/commands/git/remote/set_url.rs | 2 +- lib/src/git.rs | 3 ++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/cli/src/commands/git/remote/add.rs b/cli/src/commands/git/remote/add.rs index a66a7faac8..986b35cad5 100644 --- a/cli/src/commands/git/remote/add.rs +++ b/cli/src/commands/git/remote/add.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use jj_lib::git; +use jj_lib::git::{self, sanitize_git_url_if_path}; use jj_lib::repo::Repo; use crate::cli_util::CommandHelper; @@ -37,6 +37,10 @@ pub fn cmd_git_remote_add( let workspace_command = command.workspace_helper(ui)?; let repo = workspace_command.repo(); let git_repo = get_git_repo(repo.store())?; - git::add_remote(&git_repo, &args.remote, &args.url)?; + git::add_remote( + &git_repo, + &args.remote, + &sanitize_git_url_if_path(command.cwd(), &args.url), + )?; Ok(()) } diff --git a/cli/src/commands/git/remote/set_url.rs b/cli/src/commands/git/remote/set_url.rs index cee2da0559..ca386dbb2b 100644 --- a/cli/src/commands/git/remote/set_url.rs +++ b/cli/src/commands/git/remote/set_url.rs @@ -37,6 +37,6 @@ pub fn cmd_git_remote_set_url( let workspace_command = command.workspace_helper(ui)?; let repo = workspace_command.repo(); let git_repo = get_git_repo(repo.store())?; - git::set_remote_url(&git_repo, &args.remote, &args.url)?; + git::set_remote_url(&git_repo, &args.remote, &args.url, command.cwd())?; Ok(()) } diff --git a/lib/src/git.rs b/lib/src/git.rs index f1e2d6a38d..57fc4d1a4c 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -1179,6 +1179,7 @@ pub fn set_remote_url( git_repo: &git2::Repository, remote_name: &str, new_remote_url: &str, + cwd: &Path, ) -> Result<(), GitRemoteManagementError> { if remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO { return Err(GitRemoteManagementError::RemoteReservedForLocalGitRepo); @@ -1196,7 +1197,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)) .map_err(GitRemoteManagementError::InternalGitError)?; Ok(()) }