diff --git a/CHANGELOG.md b/CHANGELOG.md index 299894fa70..e1444c566b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Note to packagers + +* `jj` now links `libgit2` statically by default. To use dynamic linking, you + need to set the environment variable `LIBGIT2_NO_VENDOR=1` while compiling. + ([#4163](https://github.com/martinvonz/jj/pull/4163)) + ### Breaking changes * `jj rebase --skip-empty` has been renamed to `jj rebase --skip-emptied` diff --git a/Cargo.lock b/Cargo.lock index 422c0328a6..2c65e3175a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1879,6 +1879,7 @@ dependencies = [ "maplit", "minus", "once_cell", + "path-slash", "pest", "pest_derive", "pollster", @@ -1931,6 +1932,7 @@ dependencies = [ "maplit", "num_cpus", "once_cell", + "path-slash", "pest", "pest_derive", "pollster", @@ -2341,6 +2343,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 60740704f8..f78e71ce52 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,7 +48,16 @@ dunce = "1.0.4" either = "1.13.0" esl01-renderdag = "0.3.0" futures = "0.3.30" -git2 = "0.19.0" +git2 = { version = "0.19.0", features = [ + # Do *not* disable this feature even if you'd like dynamic linking. Instead, + # set the environment variable `LIBGIT2_NO_VENDOR=1` if dynamic linking must + # be used (this will override the Cargo feature), and allow static linking + # in other cases. Rationale: If neither the feature nor the environment + # variable are set, `git2` may still decide to vendor `libgit2` if it + # doesn't find a version of `libgit2` to link to dynamically. See also + # https://github.com/rust-lang/git2-rs/commit/3cef4119f + "vendored-libgit2" +] } gix = { version = "0.63.0", default-features = false, features = [ "index", "max-performance-safe", @@ -67,6 +76,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..9c5a4b9ed9 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -16,7 +16,7 @@ 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; @@ -47,23 +47,6 @@ pub struct GitCloneArgs { colocate: bool, } -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 !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()) - } 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); @@ -89,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/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/cli/src/commands/workspace.rs b/cli/src/commands/workspace.rs index fde6de92eb..56e4661eb8 100644 --- a/cli/src/commands/workspace.rs +++ b/cli/src/commands/workspace.rs @@ -171,6 +171,15 @@ fn cmd_workspace_add( "Created workspace in \"{}\"", file_util::relative_path(command.cwd(), &destination_path).display() )?; + // Show a warning if the user passed a path without a separator, since they + // may have intended the argument to only be the name for the workspace. + if !args.destination.contains(std::path::is_separator) { + writeln!( + ui.warning_default(), + r#"Workspace created inside current directory. If this was unintentional, delete the "{}" directory and run `jj workspace forget {name}` to remove it."#, + args.destination + )?; + } // Copy sparse patterns from workspace where the command was run let mut new_workspace_command = command.for_workable_repo(ui, new_workspace, repo)?; diff --git a/cli/tests/common/mod.rs b/cli/tests/common/mod.rs index 72f4eb5a51..c028923fd2 100644 --- a/cli/tests/common/mod.rs +++ b/cli/tests/common/mod.rs @@ -14,9 +14,11 @@ use std::cell::RefCell; use std::collections::HashMap; +use std::fs; use std::path::{Path, PathBuf}; use itertools::Itertools as _; +use path_slash::PathBufExt; use regex::{Captures, Regex}; use tempfile::TempDir; @@ -53,7 +55,7 @@ impl Default for TestEnvironment { testutils::hermetic_libgit2(); let tmp_dir = testutils::new_temp_dir(); - let env_root = tmp_dir.path().canonicalize().unwrap(); + let env_root = dunce::canonicalize(tmp_dir.path()).unwrap(); let home_dir = env_root.join("home"); std::fs::create_dir(&home_dir).unwrap(); let config_dir = env_root.join("config"); @@ -320,13 +322,20 @@ 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()), + regex::escape( + &fs::canonicalize(&self.env_root) + .unwrap() + .display() + .to_string() + ) )) .unwrap(); regex .replace_all(&text, |caps: &Captures| { - format!("$TEST_ENV{}", caps[1].replace('\\', "/")) + format!("$TEST_ENV{}", caps[2].replace('\\', "/")) }) .to_string() } diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index 7e3cfe75f1..8126e24442 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -1235,8 +1235,6 @@ 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/cli/tests/test_immutable_commits.rs b/cli/tests/test_immutable_commits.rs index ed806a13fa..f100f851f0 100644 --- a/cli/tests/test_immutable_commits.rs +++ b/cli/tests/test_immutable_commits.rs @@ -136,30 +136,31 @@ fn test_immutable_heads_set_to_working_copy() { #[test] fn test_new_wc_commit_when_wc_immutable_multi_workspace() { let test_env = TestEnvironment::default(); - test_env.jj_cmd_ok(test_env.env_root(), &["git", "init"]); - test_env.jj_cmd_ok(test_env.env_root(), &["branch", "create", "main"]); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); + let repo_path = test_env.env_root().join("repo"); + test_env.jj_cmd_ok(&repo_path, &["branch", "create", "main"]); test_env.add_config(r#"revset-aliases."immutable_heads()" = "main""#); - test_env.jj_cmd_ok(test_env.env_root(), &["new", "-m=a"]); - test_env.jj_cmd_ok(test_env.env_root(), &["workspace", "add", "workspace1"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-m=a"]); + test_env.jj_cmd_ok(&repo_path, &["workspace", "add", "../workspace1"]); let workspace1_envroot = test_env.env_root().join("workspace1"); - test_env.jj_cmd_ok(workspace1_envroot.as_path(), &["edit", "default@"]); - let (_, stderr) = test_env.jj_cmd_ok(test_env.env_root(), &["branch", "set", "main"]); + test_env.jj_cmd_ok(&workspace1_envroot, &["edit", "default@"]); + let (_, stderr) = test_env.jj_cmd_ok(&repo_path, &["branch", "set", "main"]); insta::assert_snapshot!(stderr, @r###" - Moved 1 branches to kkmpptxz 40cbbd52 main | a + Moved 1 branches to kkmpptxz 7796c4df main | (empty) a Warning: The working-copy commit in workspace 'default' became immutable, so a new commit has been created on top of it. Warning: The working-copy commit in workspace 'workspace1' became immutable, so a new commit has been created on top of it. - Working copy now at: royxmykx 5bcb7da6 (empty) (no description set) - Parent commit : kkmpptxz 40cbbd52 main | a + Working copy now at: royxmykx 896465c4 (empty) (no description set) + Parent commit : kkmpptxz 7796c4df main | (empty) a "###); - test_env.jj_cmd_ok(workspace1_envroot.as_path(), &["workspace", "update-stale"]); - let (stdout, _) = test_env.jj_cmd_ok(workspace1_envroot.as_path(), &["log", "--no-graph"]); + test_env.jj_cmd_ok(&workspace1_envroot, &["workspace", "update-stale"]); + let (stdout, _) = test_env.jj_cmd_ok(&workspace1_envroot, &["log", "--no-graph"]); insta::assert_snapshot!(stdout, @r###" - nppvrztz test.user@example.com 2001-02-03 08:05:11 workspace1@ 44082ceb + nppvrztz test.user@example.com 2001-02-03 08:05:11 workspace1@ ee0671fd (empty) (no description set) - royxmykx test.user@example.com 2001-02-03 08:05:12 default@ 5bcb7da6 + royxmykx test.user@example.com 2001-02-03 08:05:12 default@ 896465c4 (empty) (no description set) - kkmpptxz test.user@example.com 2001-02-03 08:05:12 main 40cbbd52 - a + kkmpptxz test.user@example.com 2001-02-03 08:05:09 main 7796c4df + (empty) a zzzzzzzz root() 00000000 "###); } diff --git a/cli/tests/test_workspaces.rs b/cli/tests/test_workspaces.rs index 4bd653ac16..cd045ad485 100644 --- a/cli/tests/test_workspaces.rs +++ b/cli/tests/test_workspaces.rs @@ -374,6 +374,61 @@ fn test_workspaces_add_workspace_from_subdir() { "###); } +#[test] +fn test_workspaces_add_workspace_in_current_workspace() { + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "main"]); + let main_path = test_env.env_root().join("main"); + + std::fs::write(main_path.join("file"), "contents").unwrap(); + test_env.jj_cmd_ok(&main_path, &["commit", "-m", "initial"]); + + // Try to create workspace using name instead of path + let (stdout, stderr) = test_env.jj_cmd_ok(&main_path, &["workspace", "add", "secondary"]); + insta::assert_snapshot!(stdout.replace('\\', "/"), @""); + insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" + Created workspace in "secondary" + Warning: Workspace created inside current directory. If this was unintentional, delete the "secondary" directory and run `jj workspace forget secondary` to remove it. + Working copy now at: pmmvwywv 0a77a39d (empty) (no description set) + Parent commit : qpvuntsm 751b12b7 initial + Added 1 files, modified 0 files, removed 0 files + "###); + + // Workspace created despite warning + let stdout = test_env.jj_cmd_success(&main_path, &["workspace", "list"]); + insta::assert_snapshot!(stdout, @r###" + default: rlvkpnrz 46d9ba8b (no description set) + secondary: pmmvwywv 0a77a39d (empty) (no description set) + "###); + + // Use explicit path instead (no warning) + let (stdout, stderr) = test_env.jj_cmd_ok(&main_path, &["workspace", "add", "./third"]); + insta::assert_snapshot!(stdout.replace('\\', "/"), @""); + insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" + Created workspace in "third" + Working copy now at: zxsnswpr 64746d4b (empty) (no description set) + Parent commit : qpvuntsm 751b12b7 initial + Added 1 files, modified 0 files, removed 0 files + "###); + + // Both workspaces created + let stdout = test_env.jj_cmd_success(&main_path, &["workspace", "list"]); + insta::assert_snapshot!(stdout, @r###" + default: rlvkpnrz 477c647f (no description set) + secondary: pmmvwywv 0a77a39d (empty) (no description set) + third: zxsnswpr 64746d4b (empty) (no description set) + "###); + + // Can see files from the other workspaces in main workspace, since they are + // child directories and will therefore be snapshotted + let stdout = test_env.jj_cmd_success(&main_path, &["file", "list"]); + insta::assert_snapshot!(stdout.replace('\\', "/"), @r###" + file + secondary/file + third/file + "###); +} + /// Test making changes to the working copy in a workspace as it gets rewritten /// from another workspace #[test] 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 f4030a7ca2..c5be8c9240 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,10 +1144,43 @@ 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 + // TODO: double-check about UNC paths; what does Git do? + 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, new_remote_url: &str, + cwd: &Path, ) -> Result<(), GitRemoteManagementError> { if remote_name == REMOTE_NAME_FOR_LOCAL_GIT_REPO { return Err(GitRemoteManagementError::RemoteReservedForLocalGitRepo); @@ -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)) .map_err(GitRemoteManagementError::InternalGitError)?; Ok(()) }