From b95c51056ab4d992468e03603bd31ba47be1421b Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 7 Dec 2023 23:26:03 +0900 Subject: [PATCH 1/5] index: extract as_composite() to trait method The revset engine will accept abstract AsCompositeIndex type, and the evaluated revset can be 'static if the index is behind Arc. --- cli/src/commands/debug.rs | 2 +- lib/src/default_index/composite.rs | 26 +++++++++++++++++++ lib/src/default_index/mod.rs | 2 +- lib/src/default_index/mutable.rs | 12 +++++---- lib/src/default_index/readonly.rs | 6 +++-- lib/src/default_revset_engine.rs | 2 +- .../test_default_revset_graph_iterator.rs | 2 +- lib/tests/test_index.rs | 2 +- 8 files changed, 42 insertions(+), 12 deletions(-) diff --git a/cli/src/commands/debug.rs b/cli/src/commands/debug.rs index c6f3cb8f5c..fe5bf02f41 100644 --- a/cli/src/commands/debug.rs +++ b/cli/src/commands/debug.rs @@ -18,7 +18,7 @@ use std::io::Write as _; use clap::Subcommand; use jj_lib::backend::ObjectId; -use jj_lib::default_index::{DefaultIndexStore, DefaultReadonlyIndex}; +use jj_lib::default_index::{AsCompositeIndex as _, DefaultIndexStore, DefaultReadonlyIndex}; use jj_lib::local_working_copy::LocalWorkingCopy; use jj_lib::revset; use jj_lib::working_copy::WorkingCopy; diff --git a/lib/src/default_index/composite.rs b/lib/src/default_index/composite.rs index 97551fee04..88cdc0dd80 100644 --- a/lib/src/default_index/composite.rs +++ b/lib/src/default_index/composite.rs @@ -63,6 +63,26 @@ pub(super) trait IndexSegment: Send + Sync { fn segment_entry_by_pos(&self, pos: IndexPosition, local_pos: u32) -> IndexEntry; } +/// Abstraction over owned and borrowed types that can be cheaply converted to +/// a `CompositeIndex` reference. +pub trait AsCompositeIndex { + /// Returns reference wrapper that provides global access to this index. + fn as_composite(&self) -> CompositeIndex<'_>; +} + +impl AsCompositeIndex for &T { + fn as_composite(&self) -> CompositeIndex<'_> { + ::as_composite(self) + } +} + +impl AsCompositeIndex for &mut T { + fn as_composite(&self) -> CompositeIndex<'_> { + ::as_composite(self) + } +} + +/// Reference wrapper that provides global access to nested index segments. #[derive(Clone, Copy)] pub struct CompositeIndex<'a>(&'a dyn IndexSegment); @@ -288,6 +308,12 @@ impl<'a> CompositeIndex<'a> { } } +impl AsCompositeIndex for CompositeIndex<'_> { + fn as_composite(&self) -> CompositeIndex<'_> { + *self + } +} + impl Index for CompositeIndex<'_> { /// Suppose the given `commit_id` exists, returns the minimum prefix length /// to disambiguate it. The length to be returned is a number of hexadecimal diff --git a/lib/src/default_index/mod.rs b/lib/src/default_index/mod.rs index 4db14c9277..674eaffd97 100644 --- a/lib/src/default_index/mod.rs +++ b/lib/src/default_index/mod.rs @@ -21,7 +21,7 @@ mod readonly; mod rev_walk; mod store; -pub use self::composite::{CompositeIndex, IndexLevelStats, IndexStats}; +pub use self::composite::{AsCompositeIndex, CompositeIndex, IndexLevelStats, IndexStats}; pub use self::entry::{IndexEntry, IndexPosition}; pub use self::mutable::DefaultMutableIndex; pub use self::readonly::DefaultReadonlyIndex; diff --git a/lib/src/default_index/mutable.rs b/lib/src/default_index/mutable.rs index 11c12c6365..de8cf9b858 100644 --- a/lib/src/default_index/mutable.rs +++ b/lib/src/default_index/mutable.rs @@ -30,7 +30,7 @@ use itertools::Itertools; use smallvec::SmallVec; use tempfile::NamedTempFile; -use super::composite::{CompositeIndex, IndexSegment}; +use super::composite::{AsCompositeIndex, CompositeIndex, IndexSegment}; use super::entry::{IndexEntry, IndexPosition, SmallIndexPositionsVec}; use super::readonly::{DefaultReadonlyIndex, ReadonlyIndexSegment}; use super::store::IndexLoadError; @@ -403,10 +403,6 @@ impl DefaultMutableIndex { DefaultMutableIndex(mutable_segment) } - pub fn as_composite(&self) -> CompositeIndex { - self.0.as_composite() - } - #[cfg(test)] pub(crate) fn add_commit_data( &mut self, @@ -422,6 +418,12 @@ impl DefaultMutableIndex { } } +impl AsCompositeIndex for DefaultMutableIndex { + fn as_composite(&self) -> CompositeIndex<'_> { + self.0.as_composite() + } +} + impl Index for DefaultMutableIndex { fn shortest_unique_commit_id_prefix_len(&self, commit_id: &CommitId) -> usize { self.as_composite() diff --git a/lib/src/default_index/readonly.rs b/lib/src/default_index/readonly.rs index 10b822627e..e08895ca0f 100644 --- a/lib/src/default_index/readonly.rs +++ b/lib/src/default_index/readonly.rs @@ -25,7 +25,7 @@ use std::sync::Arc; use byteorder::{LittleEndian, ReadBytesExt}; use smallvec::SmallVec; -use super::composite::{CompositeIndex, IndexSegment}; +use super::composite::{AsCompositeIndex, CompositeIndex, IndexSegment}; use super::entry::{IndexEntry, IndexPosition, SmallIndexPositionsVec}; use super::mutable::DefaultMutableIndex; use super::store::IndexLoadError; @@ -384,8 +384,10 @@ impl DefaultReadonlyIndex { pub(super) fn as_segment(&self) -> &Arc { &self.0 } +} - pub fn as_composite(&self) -> CompositeIndex { +impl AsCompositeIndex for DefaultReadonlyIndex { + fn as_composite(&self) -> CompositeIndex<'_> { self.0.as_composite() } } diff --git a/lib/src/default_revset_engine.rs b/lib/src/default_revset_engine.rs index 3e91ab37ca..1071a44834 100644 --- a/lib/src/default_revset_engine.rs +++ b/lib/src/default_revset_engine.rs @@ -978,7 +978,7 @@ fn has_diff_from_parent( mod tests { use super::*; use crate::backend::{ChangeId, CommitId, ObjectId}; - use crate::default_index::DefaultMutableIndex; + use crate::default_index::{AsCompositeIndex as _, DefaultMutableIndex}; /// Generator of unique 16-byte ChangeId excluding root id fn change_id_generator() -> impl FnMut() -> ChangeId { diff --git a/lib/tests/test_default_revset_graph_iterator.rs b/lib/tests/test_default_revset_graph_iterator.rs index 95b412de7d..dbd6a5ef79 100644 --- a/lib/tests/test_default_revset_graph_iterator.rs +++ b/lib/tests/test_default_revset_graph_iterator.rs @@ -14,7 +14,7 @@ use itertools::Itertools; use jj_lib::commit::Commit; -use jj_lib::default_index::DefaultReadonlyIndex; +use jj_lib::default_index::{AsCompositeIndex as _, DefaultReadonlyIndex}; use jj_lib::default_revset_engine::{evaluate, RevsetImpl}; use jj_lib::repo::{ReadonlyRepo, Repo as _}; use jj_lib::revset::ResolvedExpression; diff --git a/lib/tests/test_index.rs b/lib/tests/test_index.rs index 99b410dd2d..e1f8b79680 100644 --- a/lib/tests/test_index.rs +++ b/lib/tests/test_index.rs @@ -18,7 +18,7 @@ use jj_lib::backend::CommitId; use jj_lib::commit::Commit; use jj_lib::commit_builder::CommitBuilder; use jj_lib::default_index::{ - CompositeIndex, DefaultMutableIndex, DefaultReadonlyIndex, IndexPosition, + AsCompositeIndex as _, CompositeIndex, DefaultMutableIndex, DefaultReadonlyIndex, IndexPosition, }; use jj_lib::index::Index as _; use jj_lib::repo::{MutableRepo, ReadonlyRepo, Repo}; From 163b81c6800c10154bab081ae51420ee448d5379 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 7 Dec 2023 23:51:44 +0900 Subject: [PATCH 2/5] revset: abstract evaluated RevsetImpl over owned/borrowed index types --- lib/src/default_revset_engine.rs | 53 ++++++++++--------- .../test_default_revset_graph_iterator.rs | 7 ++- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/lib/src/default_revset_engine.rs b/lib/src/default_revset_engine.rs index 1071a44834..b7855a2e41 100644 --- a/lib/src/default_revset_engine.rs +++ b/lib/src/default_revset_engine.rs @@ -24,7 +24,7 @@ use std::sync::Arc; use itertools::Itertools; use crate::backend::{ChangeId, CommitId, MillisSinceEpoch}; -use crate::default_index::{CompositeIndex, IndexEntry, IndexPosition}; +use crate::default_index::{AsCompositeIndex, CompositeIndex, IndexEntry, IndexPosition}; use crate::default_revset_graph_iterator::RevsetGraphIterator; use crate::id_prefix::{IdIndex, IdIndexSource, IdIndexSourceEntry}; use crate::index::{HexPrefix, PrefixResolution}; @@ -69,29 +69,26 @@ trait InternalRevset: fmt::Debug + ToPredicateFn { Self: 'a; } -pub struct RevsetImpl<'index> { +pub struct RevsetImpl { inner: Box, - index: CompositeIndex<'index>, + index: I, } -impl<'index> RevsetImpl<'index> { - fn new(revset: Box, index: CompositeIndex<'index>) -> Self { - Self { - inner: revset, - index, - } +impl RevsetImpl { + fn new(inner: Box, index: I) -> Self { + Self { inner, index } } - fn entries(&self) -> Box> + '_> { - self.inner.iter(self.index) + fn entries(&self) -> Box> + '_> { + self.inner.iter(self.index.as_composite()) } - pub fn iter_graph_impl(&self) -> RevsetGraphIterator<'_, 'index> { - RevsetGraphIterator::new(self.index, self.entries()) + pub fn iter_graph_impl(&self) -> RevsetGraphIterator<'_, '_> { + RevsetGraphIterator::new(self.index.as_composite(), self.entries()) } } -impl fmt::Debug for RevsetImpl<'_> { +impl fmt::Debug for RevsetImpl { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("RevsetImpl") .field("inner", &self.inner) @@ -99,7 +96,11 @@ impl fmt::Debug for RevsetImpl<'_> { } } -impl<'index> Revset<'index> for RevsetImpl<'index> { +impl<'index, I> Revset<'index> for RevsetImpl +where + // Clone + Send + Sync for change_id_index() + I: AsCompositeIndex + Clone + Send + Sync + 'index, +{ fn iter(&self) -> Box + '_> { Box::new(self.entries().map(|index_entry| index_entry.commit_id())) } @@ -122,7 +123,7 @@ impl<'index> Revset<'index> for RevsetImpl<'index> { pos_by_change.insert(&entry.change_id(), entry.position()); } Box::new(ChangeIdIndexImpl { - index: self.index, + index: self.index.clone(), pos_by_change: pos_by_change.build(), }) } @@ -136,21 +137,21 @@ impl<'index> Revset<'index> for RevsetImpl<'index> { } } -struct ChangeIdIndexImpl<'index> { - index: CompositeIndex<'index>, +struct ChangeIdIndexImpl { + index: I, pos_by_change: IdIndex, } -impl ChangeIdIndex for ChangeIdIndexImpl<'_> { +impl ChangeIdIndex for ChangeIdIndexImpl { fn resolve_prefix(&self, prefix: &HexPrefix) -> PrefixResolution> { self.pos_by_change - .resolve_prefix_with(self.index, prefix, |entry| entry.commit_id()) + .resolve_prefix_with(self.index.as_composite(), prefix, |entry| entry.commit_id()) .map(|(_, commit_ids)| commit_ids) } fn shortest_unique_prefix_len(&self, change_id: &ChangeId) -> usize { self.pos_by_change - .shortest_unique_prefix_len(self.index, change_id) + .shortest_unique_prefix_len(self.index.as_composite(), change_id) } } @@ -576,14 +577,14 @@ impl<'index, I1: Iterator>, I2: Iterator( +pub fn evaluate( expression: &ResolvedExpression, store: &Arc, - index: CompositeIndex<'index>, -) -> Result, RevsetEvaluationError> { + index: I, +) -> Result, RevsetEvaluationError> { let context = EvaluationContext { store: store.clone(), - index, + index: index.as_composite(), }; let internal_revset = context.evaluate(expression)?; Ok(RevsetImpl::new(internal_revset, index)) @@ -978,7 +979,7 @@ fn has_diff_from_parent( mod tests { use super::*; use crate::backend::{ChangeId, CommitId, ObjectId}; - use crate::default_index::{AsCompositeIndex as _, DefaultMutableIndex}; + use crate::default_index::DefaultMutableIndex; /// Generator of unique 16-byte ChangeId excluding root id fn change_id_generator() -> impl FnMut() -> ChangeId { diff --git a/lib/tests/test_default_revset_graph_iterator.rs b/lib/tests/test_default_revset_graph_iterator.rs index dbd6a5ef79..cfa502facf 100644 --- a/lib/tests/test_default_revset_graph_iterator.rs +++ b/lib/tests/test_default_revset_graph_iterator.rs @@ -14,7 +14,7 @@ use itertools::Itertools; use jj_lib::commit::Commit; -use jj_lib::default_index::{AsCompositeIndex as _, DefaultReadonlyIndex}; +use jj_lib::default_index::DefaultReadonlyIndex; use jj_lib::default_revset_engine::{evaluate, RevsetImpl}; use jj_lib::repo::{ReadonlyRepo, Repo as _}; use jj_lib::revset::ResolvedExpression; @@ -25,13 +25,12 @@ use testutils::{CommitGraphBuilder, TestRepo}; fn revset_for_commits<'index>( repo: &'index ReadonlyRepo, commits: &[&Commit], -) -> RevsetImpl<'index> { +) -> RevsetImpl<&'index DefaultReadonlyIndex> { let index = repo .readonly_index() .as_any() .downcast_ref::() - .unwrap() - .as_composite(); + .unwrap(); let expression = ResolvedExpression::Commits(commits.iter().map(|commit| commit.id().clone()).collect()); evaluate(&expression, repo.store(), index).unwrap() From 4205a1c3cb738097a9f38e38124ba86120c90a08 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 8 Dec 2023 00:22:48 +0900 Subject: [PATCH 3/5] index: add 'static version of evaluate_revset() to ReadonlyIndex We'll probably need a better abstraction, but a separate method is good enough to remove unsafe code from ReadonlyRepo. I'm not sure if this is feasible for the other backends, but I guess there would be less lifetimed variables than DefaultReadonlyIndex. --- lib/src/default_index/readonly.rs | 12 +++++++++++- lib/src/index.rs | 8 ++++++++ lib/tests/test_default_revset_graph_iterator.rs | 8 ++++---- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/lib/src/default_index/readonly.rs b/lib/src/default_index/readonly.rs index e08895ca0f..94d5426ee9 100644 --- a/lib/src/default_index/readonly.rs +++ b/lib/src/default_index/readonly.rs @@ -30,6 +30,7 @@ use super::entry::{IndexEntry, IndexPosition, SmallIndexPositionsVec}; use super::mutable::DefaultMutableIndex; use super::store::IndexLoadError; use crate::backend::{ChangeId, CommitId, ObjectId}; +use crate::default_revset_engine; use crate::index::{HexPrefix, Index, MutableIndex, PrefixResolution, ReadonlyIndex}; use crate::revset::{ResolvedExpression, Revset, RevsetEvaluationError}; use crate::store::Store; @@ -373,7 +374,7 @@ impl IndexSegment for ReadonlyIndexSegment { } /// Commit index backend which stores data on local disk. -#[derive(Debug)] +#[derive(Clone, Debug)] pub struct DefaultReadonlyIndex(Arc); impl DefaultReadonlyIndex { @@ -440,6 +441,15 @@ impl ReadonlyIndex for DefaultReadonlyIndex { self } + fn evaluate_revset_static( + &self, + expression: &ResolvedExpression, + store: &Arc, + ) -> Result>, RevsetEvaluationError> { + let revset_impl = default_revset_engine::evaluate(expression, store, self.clone())?; + Ok(Box::new(revset_impl)) + } + fn start_modification(&self) -> Box { Box::new(DefaultMutableIndex::incremental(self.0.clone())) } diff --git a/lib/src/index.rs b/lib/src/index.rs index cf8514e61b..2f7cfff21f 100644 --- a/lib/src/index.rs +++ b/lib/src/index.rs @@ -75,6 +75,14 @@ pub trait ReadonlyIndex: Send + Sync { fn as_index(&self) -> &dyn Index; + // TODO: might be better to split Index::evaluate_revset() to + // Readonly/MutableIndex::evaluate_static(). + fn evaluate_revset_static( + &self, + expression: &ResolvedExpression, + store: &Arc, + ) -> Result>, RevsetEvaluationError>; + fn start_modification(&self) -> Box; } diff --git a/lib/tests/test_default_revset_graph_iterator.rs b/lib/tests/test_default_revset_graph_iterator.rs index cfa502facf..aac8c0abd9 100644 --- a/lib/tests/test_default_revset_graph_iterator.rs +++ b/lib/tests/test_default_revset_graph_iterator.rs @@ -22,10 +22,10 @@ use jj_lib::revset_graph::RevsetGraphEdge; use test_case::test_case; use testutils::{CommitGraphBuilder, TestRepo}; -fn revset_for_commits<'index>( - repo: &'index ReadonlyRepo, +fn revset_for_commits( + repo: &ReadonlyRepo, commits: &[&Commit], -) -> RevsetImpl<&'index DefaultReadonlyIndex> { +) -> RevsetImpl { let index = repo .readonly_index() .as_any() @@ -33,7 +33,7 @@ fn revset_for_commits<'index>( .unwrap(); let expression = ResolvedExpression::Commits(commits.iter().map(|commit| commit.id().clone()).collect()); - evaluate(&expression, repo.store(), index).unwrap() + evaluate(&expression, repo.store(), index.clone()).unwrap() } fn direct(commit: &Commit) -> RevsetGraphEdge { From a11711aa4959ed11fbc5892450bdc8a1a229fed8 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 8 Dec 2023 00:33:34 +0900 Subject: [PATCH 4/5] repo: remove unsafe lifetime hack from change_id_index() --- lib/src/repo.rs | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 9b4b35779c..a65474c567 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -19,7 +19,6 @@ use std::fmt::{Debug, Formatter}; use std::io::ErrorKind; use std::ops::Deref; use std::path::{Path, PathBuf}; -use std::pin::Pin; use std::sync::Arc; use std::{fs, slice}; @@ -49,7 +48,7 @@ use crate::operation::Operation; use crate::refs::{ diff_named_ref_targets, diff_named_remote_refs, merge_ref_targets, merge_remote_refs, }; -use crate::revset::{self, ChangeIdIndex, Revset, RevsetExpression}; +use crate::revset::{ChangeIdIndex, RevsetExpression}; use crate::rewrite::{DescendantRebaser, RebaseOptions}; use crate::settings::{RepoSettings, UserSettings}; use crate::signing::{SignInitError, Signer}; @@ -60,7 +59,7 @@ use crate::submodule_store::SubmoduleStore; use crate::transaction::Transaction; use crate::tree::TreeMergeError; use crate::view::View; -use crate::{backend, dag_walk, op_store}; +use crate::{backend, dag_walk, op_store, revset}; pub trait Repo { fn store(&self) -> &Arc; @@ -97,8 +96,7 @@ pub struct ReadonlyRepo { settings: RepoSettings, index_store: Arc, submodule_store: Arc, - index: OnceCell>>, - // Declared after `change_id_index` since it must outlive it on drop. + index: OnceCell>, change_id_index: OnceCell>, // TODO: This should eventually become part of the index and not be stored fully in memory. view: View, @@ -255,28 +253,25 @@ impl ReadonlyRepo { pub fn readonly_index(&self) -> &dyn ReadonlyIndex { self.index .get_or_init(|| { - Box::into_pin( - self.index_store - .get_index_at_op(&self.operation, &self.store), - ) + self.index_store + .get_index_at_op(&self.operation, &self.store) }) .deref() } - fn change_id_index<'a>(&'a self) -> &'a (dyn ChangeIdIndex + 'a) { - let change_id_index: &'a (dyn ChangeIdIndex + 'a) = self - .change_id_index + fn change_id_index(&self) -> &dyn ChangeIdIndex { + self.change_id_index .get_or_init(|| { - let revset: Box> = - RevsetExpression::all().evaluate_programmatic(self).unwrap(); - let change_id_index: Box = revset.change_id_index(); - // evaluate() above only borrows the index, not the whole repo - let change_id_index: Box = - unsafe { std::mem::transmute(change_id_index) }; - change_id_index + // TODO: maybe add abstraction over 'static/'index revset + // evaluation, and use it. + let expression = RevsetExpression::all().resolve_programmatic(self); + let revset = self + .readonly_index() + .evaluate_revset_static(&expression, self.store()) + .unwrap(); + revset.change_id_index() }) - .as_ref(); - change_id_index + .as_ref() } pub fn op_heads_store(&self) -> &Arc { @@ -693,7 +688,7 @@ impl RepoLoader { settings: self.repo_settings.clone(), index_store: self.index_store.clone(), submodule_store: self.submodule_store.clone(), - index: OnceCell::with_value(Box::into_pin(index)), + index: OnceCell::with_value(index), change_id_index: OnceCell::new(), view, }; From 824fcb31b88a1e7e2b023bba9978dbd433b28fa1 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 13 Dec 2023 13:16:23 +0900 Subject: [PATCH 5/5] lib: forbid unsafe_code at all --- lib/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/src/lib.rs b/lib/src/lib.rs index 3f09ec6f4a..4b5dd5e384 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -16,6 +16,7 @@ #![warn(missing_docs)] #![deny(unused_must_use)] +#![forbid(unsafe_code)] #[macro_use] pub mod content_hash;