From 22c5cc0982d006442e8095ab4f5e13efd835dfce Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 8 Jun 2024 13:59:42 -0700 Subject: [PATCH 1/2] repo/workspace: drop support for old repo formats It's been more than 6 months since we added support for dynamically selecting the working copy implementation. This patch drops support for selecting the default implementation of that and other stores. --- CHANGELOG.md | 3 +++ lib/src/repo.rs | 53 ++++++----------------------------------- lib/src/workspace.rs | 10 +++----- lib/tests/test_index.rs | 15 ++---------- 4 files changed, 15 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c46c04463f..3c07e878f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 modifier. Surround with parentheses if it should be parsed as string/file pattern. +* Dropped support for automatic upgrade of repo formats used by versions before + 0.12.0. + ### Deprecations ### New features diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 15ab992c7b..fdc4489b87 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -17,7 +17,6 @@ use std::collections::hash_map::Entry; use std::collections::{HashMap, HashSet}; use std::fmt::{Debug, Formatter}; -use std::io::ErrorKind; use std::ops::Deref; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -478,19 +477,7 @@ impl StoreFactories { settings: &UserSettings, store_path: &Path, ) -> Result, StoreLoadError> { - // For compatibility with existing repos. TODO: Delete in 0.8+. - if store_path.join("backend").is_file() { - fs::rename(store_path.join("backend"), store_path.join("type")) - .expect("Failed to rename 'backend' file to 'type'"); - } - // For compatibility with existing repos. TODO: Delete default in 0.8+. - let backend_type = read_store_type_compat("commit", store_path.join("type"), || { - if store_path.join("git_target").is_file() { - GitBackend::name() - } else { - LocalBackend::name() - } - })?; + let backend_type = read_store_type("commit", store_path.join("type"))?; let backend_factory = self.backend_factories.get(&backend_type).ok_or_else(|| { StoreLoadError::UnsupportedType { store: "commit", @@ -509,9 +496,7 @@ impl StoreFactories { settings: &UserSettings, store_path: &Path, ) -> Result, StoreLoadError> { - // For compatibility with existing repos. TODO: Delete default in 0.8+. - let op_store_type = - read_store_type_compat("operation", store_path.join("type"), SimpleOpStore::name)?; + let op_store_type = read_store_type("operation", store_path.join("type"))?; let op_store_factory = self.op_store_factories.get(&op_store_type).ok_or_else(|| { StoreLoadError::UnsupportedType { store: "operation", @@ -531,12 +516,7 @@ impl StoreFactories { settings: &UserSettings, store_path: &Path, ) -> Result, StoreLoadError> { - // For compatibility with existing repos. TODO: Delete default in 0.8+. - let op_heads_store_type = read_store_type_compat( - "operation heads", - store_path.join("type"), - SimpleOpHeadsStore::name, - )?; + let op_heads_store_type = read_store_type("operation heads", store_path.join("type"))?; let op_heads_store_factory = self .op_heads_store_factories .get(&op_heads_store_type) @@ -556,9 +536,7 @@ impl StoreFactories { settings: &UserSettings, store_path: &Path, ) -> Result, StoreLoadError> { - // For compatibility with existing repos. TODO: Delete default in 0.9+ - let index_store_type = - read_store_type_compat("index", store_path.join("type"), DefaultIndexStore::name)?; + let index_store_type = read_store_type("index", store_path.join("type"))?; let index_store_factory = self .index_store_factories .get(&index_store_type) @@ -579,13 +557,7 @@ impl StoreFactories { settings: &UserSettings, store_path: &Path, ) -> Result, StoreLoadError> { - // For compatibility with repos without repo/submodule_store. - // TODO Delete default in TBD version - let submodule_store_type = read_store_type_compat( - "submodule_store", - store_path.join("type"), - DefaultSubmoduleStore::name, - )?; + let submodule_store_type = read_store_type("submodule_store", store_path.join("type"))?; let submodule_store_factory = self .submodule_store_factories .get(&submodule_store_type) @@ -598,23 +570,12 @@ impl StoreFactories { } } -pub fn read_store_type_compat( +pub fn read_store_type( store: &'static str, path: impl AsRef, - default: impl FnOnce() -> &'static str, ) -> Result { let path = path.as_ref(); - let read_or_write_default = || match fs::read_to_string(path) { - Ok(content) => Ok(content), - Err(err) if err.kind() == ErrorKind::NotFound => { - let default_type = default(); - fs::create_dir(path.parent().unwrap()).ok(); - fs::write(path, default_type)?; - Ok(default_type.to_owned()) - } - Err(err) => Err(err), - }; - read_or_write_default() + fs::read_to_string(path) .context(path) .map_err(|source| StoreLoadError::ReadError { store, source }) } diff --git a/lib/src/workspace.rs b/lib/src/workspace.rs index 6b3e6e4b29..c4bd4c63ae 100644 --- a/lib/src/workspace.rs +++ b/lib/src/workspace.rs @@ -31,7 +31,7 @@ use crate::local_backend::LocalBackend; use crate::local_working_copy::{LocalWorkingCopy, LocalWorkingCopyFactory}; use crate::op_store::{OperationId, WorkspaceId}; use crate::repo::{ - read_store_type_compat, BackendInitializer, CheckOutCommitError, IndexStoreInitializer, + read_store_type, BackendInitializer, CheckOutCommitError, IndexStoreInitializer, OpHeadsStoreInitializer, OpStoreInitializer, ReadonlyRepo, Repo, RepoInitError, RepoLoader, StoreFactories, StoreLoadError, SubmoduleStoreInitializer, }; @@ -484,12 +484,8 @@ impl WorkspaceLoader { &self, working_copy_factories: &'a WorkingCopyFactories, ) -> Result<&'a dyn WorkingCopyFactory, StoreLoadError> { - // For compatibility with existing repos. TODO: Delete default in 0.17+ - let working_copy_type = read_store_type_compat( - "working copy", - self.working_copy_state_path.join("type"), - LocalWorkingCopy::name, - )?; + let working_copy_type = + read_store_type("working copy", self.working_copy_state_path.join("type"))?; if let Some(factory) = working_copy_factories.get(&working_copy_type) { Ok(factory.as_ref()) diff --git a/lib/tests/test_index.rs b/lib/tests/test_index.rs index 50bf815e4d..fdd7d2244d 100644 --- a/lib/tests/test_index.rs +++ b/lib/tests/test_index.rs @@ -727,29 +727,18 @@ fn test_reindex_missing_commit() { assert_matches!(err, DefaultIndexStoreError::IndexCommits { op_id, .. } if op_id == *bad_op_id); } -/// Test that .jj/repo/index/type is created when the repo is created, and that -/// it is created when an old repo is loaded. +/// Test that .jj/repo/index/type is created when the repo is created. #[test] fn test_index_store_type() { - let settings = testutils::user_settings(); let test_repo = TestRepo::init(); let repo = &test_repo.repo; assert_eq!(as_readonly_composite(repo).num_commits(), 1); let index_store_type_path = repo.repo_path().join("index").join("type"); assert_eq!( - std::fs::read_to_string(&index_store_type_path).unwrap(), - "default" - ); - // Remove the file to simulate an old repo. Loading the repo should result in - // the file being created. - std::fs::remove_file(&index_store_type_path).unwrap(); - let repo = load_repo_at_head(&settings, repo.repo_path()); - assert_eq!( - std::fs::read_to_string(&index_store_type_path).unwrap(), + std::fs::read_to_string(index_store_type_path).unwrap(), "default" ); - assert_eq!(as_readonly_composite(&repo).num_commits(), 1); } #[test] From d729d668464de38626b1e134dd788d2f8631f4d8 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 7 Jun 2024 17:02:36 -0700 Subject: [PATCH 2/2] lib: make git support optional via crate feature I've wanted to make the Git support optional for a long time. However, since everyone uses the Git backend (and we want to support it even in the custom binary at Google), there hasn't been much practical reason to make Git support optional. Since we now use jj-lib on the server at Google, it does make sense to have the server not include Git support. In addition to making the server binary smaller, it would make it easier for us (jj team at Googlle) to prove that our server is not affected by some libgit2 or Gitoxide vulnerability. But to be honest, neither of those problems have come up, so it's more of an excuse to make the Git support optional at this point. It turned out to be much simpler than I expected to make Git support in the lib crate optional. We have done a pretty good job of keeping Git-related logic separated there. If we make Git support optional in the lib crate, it's going to make it a bit harder to move logic from the CLI crate into the lib crate (as we have planned to do). Maybe that's good, though, since it helps remind us to keep Git-related logic separated. --- .github/workflows/build.yml | 14 ++++++++ lib/Cargo.toml | 7 ++-- lib/src/lib.rs | 2 ++ lib/src/repo.rs | 10 ++++-- lib/src/revset.rs | 17 +++++++-- lib/src/simple_op_store.rs | 27 +++++++++----- lib/src/workspace.rs | 72 +++++++++++++++++++++---------------- 7 files changed, 102 insertions(+), 47 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 7fbbd8f8a1..b8200f0116 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -76,6 +76,20 @@ jobs: env: RUST_BACKTRACE: 1 + build-no-git: + name: Build jj-lib without Git support + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 + + - name: Install Rust + uses: dtolnay/rust-toolchain@1482605bfc5719782e1267fd0c0cc350fe7646b8 + with: + toolchain: 1.76 + - name: Build + run: cargo build -p jj-lib --no-default-features --verbose + check-protos: name: Check protos runs-on: ubuntu-latest diff --git a/lib/Cargo.toml b/lib/Cargo.toml index d7a68f6a13..7d5436dbc3 100644 --- a/lib/Cargo.toml +++ b/lib/Cargo.toml @@ -42,8 +42,8 @@ config = { workspace = true } digest = { workspace = true } either = { workspace = true } futures = { workspace = true } -git2 = { workspace = true } -gix = { workspace = true } +git2 = { workspace = true, optional = true } +gix = { workspace = true, optional = true } glob = { workspace = true } hex = { workspace = true } ignore = { workspace = true } @@ -91,7 +91,8 @@ testutils = { workspace = true } tokio = { workspace = true, features = ["full"] } [features] -default = [] +default = ["git"] +git = ["dep:git2", "dep:gix"] vendored-openssl = ["git2/vendored-openssl"] watchman = ["dep:tokio", "dep:watchman_client"] testing = [] diff --git a/lib/src/lib.rs b/lib/src/lib.rs index 200735cd6b..149956ad2b 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -44,7 +44,9 @@ pub mod fileset; mod fileset_parser; pub mod fmt_util; pub mod fsmonitor; +#[cfg(feature = "git")] pub mod git; +#[cfg(feature = "git")] pub mod git_backend; pub mod gitignore; pub mod gpg_signing; diff --git a/lib/src/repo.rs b/lib/src/repo.rs index fdc4489b87..1382dc9224 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -37,7 +37,6 @@ use crate::commit_builder::CommitBuilder; use crate::default_index::{DefaultIndexStore, DefaultMutableIndex}; use crate::default_submodule_store::DefaultSubmoduleStore; use crate::file_util::{IoResultExt as _, PathError}; -use crate::git_backend::GitBackend; use crate::index::{ChangeIdIndex, Index, IndexStore, MutableIndex, ReadonlyIndex}; use crate::local_backend::LocalBackend; use crate::object_id::{HexPrefix, ObjectId, PrefixResolution}; @@ -378,9 +377,14 @@ impl Default for StoreFactories { LocalBackend::name(), Box::new(|_settings, store_path| Ok(Box::new(LocalBackend::load(store_path)))), ); + #[cfg(feature = "git")] factories.add_backend( - GitBackend::name(), - Box::new(|settings, store_path| Ok(Box::new(GitBackend::load(settings, store_path)?))), + crate::git_backend::GitBackend::name(), + Box::new(|settings, store_path| { + Ok(Box::new(crate::git_backend::GitBackend::load( + settings, store_path, + )?)) + }), ); #[cfg(feature = "testing")] factories.add_backend( diff --git a/lib/src/revset.rs b/lib/src/revset.rs index cdacad54f5..a4d82ca204 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -43,7 +43,7 @@ pub use crate::revset_parser::{ }; use crate::store::Store; use crate::str_util::StringPattern; -use crate::{dsl_util, git, revset_parser}; +use crate::{dsl_util, revset_parser}; /// Error occurred during symbol resolution. #[derive(Debug, Error)] @@ -1313,7 +1313,8 @@ pub fn walk_revs<'index>( fn resolve_remote_branch(repo: &dyn Repo, name: &str, remote: &str) -> Option> { let view = repo.view(); let target = match (name, remote) { - ("HEAD", git::REMOTE_NAME_FOR_LOCAL_GIT_REPO) => view.git_head(), + #[cfg(feature = "git")] + ("HEAD", crate::git::REMOTE_NAME_FOR_LOCAL_GIT_REPO) => view.git_head(), (name, remote) => &view.get_remote_branch(name, remote).target, }; target @@ -1605,7 +1606,17 @@ fn resolve_commit_ref( let commit_ids = repo .view() .remote_branches_matching(branch_pattern, remote_pattern) - .filter(|&((_, remote_name), _)| remote_name != git::REMOTE_NAME_FOR_LOCAL_GIT_REPO) + .filter(|&((_, remote_name), _)| { + #[cfg(feature = "git")] + { + remote_name != crate::git::REMOTE_NAME_FOR_LOCAL_GIT_REPO + } + #[cfg(not(feature = "git"))] + { + let _ = remote_name; + true + } + }) .flat_map(|(_, remote_ref)| remote_ref.target.added_ids()) .cloned() .collect(); diff --git a/lib/src/simple_op_store.rs b/lib/src/simple_op_store.rs index 1b43841c5d..7a9765471e 100644 --- a/lib/src/simple_op_store.rs +++ b/lib/src/simple_op_store.rs @@ -36,7 +36,7 @@ use crate::op_store::{ OpStore, OpStoreError, OpStoreResult, Operation, OperationId, OperationMetadata, RefTarget, RemoteRef, RemoteRefState, RemoteView, View, ViewId, WorkspaceId, }; -use crate::{dag_walk, git, op_store}; +use crate::{dag_walk, op_store}; // BLAKE2b-512 hash length in bytes const OPERATION_ID_LENGTH: usize = 64; @@ -528,8 +528,11 @@ fn branch_views_from_proto_legacy( // than deleted but yet-to-be-pushed local branch. Alternatively, we could read // git.auto-local-branch setting here, but that wouldn't always work since the // setting could be toggled after the branch got merged. + #[cfg(feature = "git")] let is_git_tracking = - remote_branch.remote_name == git::REMOTE_NAME_FOR_LOCAL_GIT_REPO; + remote_branch.remote_name == crate::git::REMOTE_NAME_FOR_LOCAL_GIT_REPO; + #[cfg(not(feature = "git"))] + let is_git_tracking = false; let default_state = if is_git_tracking || local_target.is_present() { RemoteRefState::Tracking } else { @@ -578,13 +581,21 @@ fn migrate_git_refs_to_remote(view: &mut View) { git_view.branches.insert(name.to_owned(), remote_ref); } } - view.remote_views - .insert(git::REMOTE_NAME_FOR_LOCAL_GIT_REPO.to_owned(), git_view); + #[cfg(feature = "git")] + { + view.remote_views.insert( + crate::git::REMOTE_NAME_FOR_LOCAL_GIT_REPO.to_owned(), + git_view, + ); - // jj < 0.9 might have imported refs from remote named "git" - let reserved_git_ref_prefix = format!("refs/remotes/{}/", git::REMOTE_NAME_FOR_LOCAL_GIT_REPO); - view.git_refs - .retain(|name, _| !name.starts_with(&reserved_git_ref_prefix)); + // jj < 0.9 might have imported refs from remote named "git" + let reserved_git_ref_prefix = format!( + "refs/remotes/{}/", + crate::git::REMOTE_NAME_FOR_LOCAL_GIT_REPO + ); + view.git_refs + .retain(|name, _| !name.starts_with(&reserved_git_ref_prefix)); + } } fn ref_target_to_proto(value: &RefTarget) -> Option { diff --git a/lib/src/workspace.rs b/lib/src/workspace.rs index c4bd4c63ae..624a80d935 100644 --- a/lib/src/workspace.rs +++ b/lib/src/workspace.rs @@ -23,10 +23,9 @@ use std::sync::Arc; use thiserror::Error; -use crate::backend::{Backend, BackendInitError, MergedTreeId}; +use crate::backend::{BackendInitError, MergedTreeId}; use crate::commit::Commit; -use crate::file_util::{self, IoResultExt as _, PathError}; -use crate::git_backend::{canonicalize_git_repo_path, GitBackend}; +use crate::file_util::{IoResultExt as _, PathError}; use crate::local_backend::LocalBackend; use crate::local_working_copy::{LocalWorkingCopy, LocalWorkingCopyFactory}; use crate::op_store::{OperationId, WorkspaceId}; @@ -157,36 +156,44 @@ impl Workspace { /// Initializes a workspace with a new Git backend and bare Git repo in /// `.jj/repo/store/git`. + #[cfg(feature = "git")] pub fn init_internal_git( user_settings: &UserSettings, workspace_root: &Path, ) -> Result<(Self, Arc), WorkspaceInitError> { - let backend_initializer: &BackendInitializer = - &|settings, store_path| Ok(Box::new(GitBackend::init_internal(settings, store_path)?)); + let backend_initializer: &BackendInitializer = &|settings, store_path| { + Ok(Box::new(crate::git_backend::GitBackend::init_internal( + settings, store_path, + )?)) + }; let signer = Signer::from_settings(user_settings)?; Self::init_with_backend(user_settings, workspace_root, backend_initializer, signer) } /// Initializes a workspace with a new Git backend and Git repo that shares /// the same working copy. + #[cfg(feature = "git")] pub fn init_colocated_git( user_settings: &UserSettings, workspace_root: &Path, ) -> Result<(Self, Arc), WorkspaceInitError> { let backend_initializer = |settings: &UserSettings, store_path: &Path| - -> Result, _> { + -> Result, _> { // TODO: Clean up path normalization. store_path is canonicalized by // ReadonlyRepo::init(). workspace_root will be canonicalized by // Workspace::new(), but it's not yet here. let store_relative_workspace_root = if let Ok(workspace_root) = workspace_root.canonicalize() { - file_util::relative_path(store_path, &workspace_root) + crate::file_util::relative_path(store_path, &workspace_root) } else { workspace_root.to_owned() }; - let backend = - GitBackend::init_colocated(settings, store_path, &store_relative_workspace_root)?; + let backend = crate::git_backend::GitBackend::init_colocated( + settings, + store_path, + &store_relative_workspace_root, + )?; Ok(Box::new(backend)) }; let signer = Signer::from_settings(user_settings)?; @@ -197,33 +204,38 @@ impl Workspace { /// /// The `git_repo_path` usually ends with `.git`. It's the path to the Git /// repo directory, not the working directory. + #[cfg(feature = "git")] pub fn init_external_git( user_settings: &UserSettings, workspace_root: &Path, git_repo_path: &Path, ) -> Result<(Self, Arc), WorkspaceInitError> { - let backend_initializer = - |settings: &UserSettings, store_path: &Path| -> Result, _> { - // If the git repo is inside the workspace, use a relative path to it so the - // whole workspace can be moved without breaking. - // TODO: Clean up path normalization. store_path is canonicalized by - // ReadonlyRepo::init(). workspace_root will be canonicalized by - // Workspace::new(), but it's not yet here. - let store_relative_git_repo_path = match ( - workspace_root.canonicalize(), - canonicalize_git_repo_path(git_repo_path), - ) { - (Ok(workspace_root), Ok(git_repo_path)) - if git_repo_path.starts_with(&workspace_root) => - { - file_util::relative_path(store_path, &git_repo_path) - } - _ => git_repo_path.to_owned(), - }; - let backend = - GitBackend::init_external(settings, store_path, &store_relative_git_repo_path)?; - Ok(Box::new(backend)) + let backend_initializer = |settings: &UserSettings, + store_path: &Path| + -> Result, _> { + // If the git repo is inside the workspace, use a relative path to it so the + // whole workspace can be moved without breaking. + // TODO: Clean up path normalization. store_path is canonicalized by + // ReadonlyRepo::init(). workspace_root will be canonicalized by + // Workspace::new(), but it's not yet here. + let store_relative_git_repo_path = match ( + workspace_root.canonicalize(), + crate::git_backend::canonicalize_git_repo_path(git_repo_path), + ) { + (Ok(workspace_root), Ok(git_repo_path)) + if git_repo_path.starts_with(&workspace_root) => + { + crate::file_util::relative_path(store_path, &git_repo_path) + } + _ => git_repo_path.to_owned(), }; + let backend = crate::git_backend::GitBackend::init_external( + settings, + store_path, + &store_relative_git_repo_path, + )?; + Ok(Box::new(backend)) + }; let signer = Signer::from_settings(user_settings)?; Self::init_with_backend(user_settings, workspace_root, &backend_initializer, signer) }