From 3341bffb18e02d451a14ec4f38b9be62ec38fa11 Mon Sep 17 00:00:00 2001 From: dploch Date: Thu, 11 Apr 2024 14:58:38 -0400 Subject: [PATCH] cli_util: support multiple extensions consistently If we ever implement some sort of ABI for dynamic extension loading, we'll need these underlying APIs to support multiple extensions, so we might as well do that first. --- .vscode/settings.json | 3 + cli/examples/custom-backend/main.rs | 4 +- cli/examples/custom-commit-templater/main.rs | 2 +- .../custom-operation-templater/main.rs | 2 +- cli/examples/custom-working-copy/main.rs | 6 +- cli/src/cli_util.rs | 92 ++++++++++--------- cli/src/commands/operation.rs | 2 +- cli/src/commit_templater.rs | 7 +- cli/src/operation_templater.rs | 9 +- lib/src/repo.rs | 28 ++++++ lib/src/workspace.rs | 14 +-- 11 files changed, 102 insertions(+), 67 deletions(-) create mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000000..4d9636b559 --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,3 @@ +{ + "rust-analyzer.showUnlinkedFileNotification": false +} \ No newline at end of file diff --git a/cli/examples/custom-backend/main.rs b/cli/examples/custom-backend/main.rs index b636cf17f5..beacdf8537 100644 --- a/cli/examples/custom-backend/main.rs +++ b/cli/examples/custom-backend/main.rs @@ -40,7 +40,7 @@ enum CustomCommand { } fn create_store_factories() -> StoreFactories { - let mut store_factories = StoreFactories::default(); + let mut store_factories = StoreFactories::empty(); // Register the backend so it can be loaded when the repo is loaded. The name // must match `Backend::name()`. store_factories.add_backend( @@ -73,7 +73,7 @@ fn run_custom_command( fn main() -> std::process::ExitCode { CliRunner::init() - .set_store_factories(create_store_factories()) + .add_store_factories(create_store_factories()) .add_subcommand(run_custom_command) .run() } diff --git a/cli/examples/custom-commit-templater/main.rs b/cli/examples/custom-commit-templater/main.rs index 6bbfe97865..181438dff1 100644 --- a/cli/examples/custom-commit-templater/main.rs +++ b/cli/examples/custom-commit-templater/main.rs @@ -131,6 +131,6 @@ impl CommitTemplateLanguageExtension for HexCounter { fn main() -> std::process::ExitCode { CliRunner::init() - .set_commit_template_extension(Box::new(HexCounter)) + .add_commit_template_extension(Box::new(HexCounter)) .run() } diff --git a/cli/examples/custom-operation-templater/main.rs b/cli/examples/custom-operation-templater/main.rs index 2c4a29b0bf..a51fd60469 100644 --- a/cli/examples/custom-operation-templater/main.rs +++ b/cli/examples/custom-operation-templater/main.rs @@ -89,6 +89,6 @@ impl OperationTemplateLanguageExtension for HexCounter { fn main() -> std::process::ExitCode { CliRunner::init() - .set_operation_template_extension(Box::new(HexCounter)) + .add_operation_template_extension(Box::new(HexCounter)) .run() } diff --git a/cli/examples/custom-working-copy/main.rs b/cli/examples/custom-working-copy/main.rs index d75d07c374..cb5ae1d6d0 100644 --- a/cli/examples/custom-working-copy/main.rs +++ b/cli/examples/custom-working-copy/main.rs @@ -34,7 +34,7 @@ use jj_lib::working_copy::{ CheckoutError, CheckoutStats, LockedWorkingCopy, ResetError, SnapshotError, SnapshotOptions, WorkingCopy, WorkingCopyFactory, WorkingCopyStateError, }; -use jj_lib::workspace::{default_working_copy_factories, Workspace, WorkspaceInitError}; +use jj_lib::workspace::{WorkingCopyFactories, Workspace, WorkspaceInitError}; #[derive(clap::Parser, Clone, Debug)] enum CustomCommand { @@ -74,13 +74,13 @@ fn run_custom_command( } fn main() -> std::process::ExitCode { - let mut working_copy_factories = default_working_copy_factories(); + let mut working_copy_factories = WorkingCopyFactories::new(); working_copy_factories.insert( ConflictsWorkingCopy::name().to_owned(), Box::new(ConflictsWorkingCopyFactory {}), ); CliRunner::init() - .set_working_copy_factories(working_copy_factories) + .add_working_copy_factories(working_copy_factories) .add_subcommand(run_custom_command) .run() } diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 924413d189..bd1442a72e 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -14,7 +14,7 @@ use core::fmt; use std::borrow::Cow; -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, HashSet}; use std::env::{self, ArgsOs, VarError}; use std::ffi::OsString; use std::fmt::Debug; @@ -49,8 +49,8 @@ use jj_lib::op_store::{OpStoreError, OperationId, WorkspaceId}; use jj_lib::op_walk::OpsetEvaluationError; use jj_lib::operation::Operation; use jj_lib::repo::{ - CheckOutCommitError, EditCommitError, MutableRepo, ReadonlyRepo, Repo, RepoLoader, - StoreFactories, StoreLoadError, + merge_factories_map, CheckOutCommitError, EditCommitError, MutableRepo, ReadonlyRepo, Repo, + RepoLoader, StoreFactories, StoreLoadError, }; use jj_lib::repo_path::{FsPathParseError, RepoPath, RepoPathBuf}; use jj_lib::revset::{ @@ -67,7 +67,8 @@ use jj_lib::working_copy::{ CheckoutStats, LockedWorkingCopy, SnapshotOptions, WorkingCopy, WorkingCopyFactory, }; use jj_lib::workspace::{ - default_working_copy_factories, LockedWorkspace, Workspace, WorkspaceLoadError, WorkspaceLoader, + default_working_copy_factories, LockedWorkspace, WorkingCopyFactories, Workspace, + WorkspaceLoadError, WorkspaceLoader, }; use jj_lib::{dag_walk, file_util, fileset, git, op_heads_store, op_walk, revset}; use once_cell::unsync::OnceCell; @@ -192,11 +193,11 @@ pub struct CommandHelper { global_args: GlobalArgs, settings: UserSettings, layered_configs: LayeredConfigs, - commit_template_extension: Option>, - operation_template_extension: Option>, + commit_template_extensions: Vec>, + operation_template_extensions: Vec>, maybe_workspace_loader: Result, store_factories: StoreFactories, - working_copy_factories: HashMap>, + working_copy_factories: WorkingCopyFactories, } impl CommandHelper { @@ -264,8 +265,10 @@ impl CommandHelper { )?) } - pub fn operation_template_extension(&self) -> Option<&dyn OperationTemplateLanguageExtension> { - self.operation_template_extension.as_deref() + pub fn operation_template_extensions( + &self, + ) -> &Vec> { + &self.operation_template_extensions } pub fn workspace_loader(&self) -> Result<&WorkspaceLoader, CommandError> { @@ -403,7 +406,7 @@ pub struct WorkspaceCommandHelper { user_repo: ReadonlyUserRepo, // TODO: Parsed template can be cached if it doesn't capture 'repo lifetime commit_summary_template_text: String, - commit_template_extension: Option>, + commit_template_extensions: Vec>, revset_aliases_map: RevsetAliasesMap, template_aliases_map: TemplateAliasesMap, may_update_working_copy: bool, @@ -434,7 +437,7 @@ impl WorkspaceCommandHelper { workspace, user_repo: ReadonlyUserRepo::new(repo), commit_summary_template_text, - commit_template_extension: command.commit_template_extension.clone(), + commit_template_extensions: command.commit_template_extensions.clone(), revset_aliases_map, template_aliases_map, may_update_working_copy, @@ -956,7 +959,7 @@ impl WorkspaceCommandHelper { self.workspace_id(), self.revset_parse_context(), self.id_prefix_context()?, - self.commit_template_extension.as_deref(), + &self.commit_template_extensions, )) } @@ -1436,7 +1439,7 @@ impl WorkspaceCommandTransaction<'_> { self.helper.workspace_id(), self.helper.revset_parse_context(), &id_prefix_context, - self.helper.commit_template_extension.as_deref(), + &self.helper.commit_template_extensions, ); let template = self .helper @@ -2502,11 +2505,11 @@ pub fn format_template(ui: &Ui, arg: &C, template: &TemplateRenderer, - store_factories: Option, - working_copy_factories: Option>>, - commit_template_extension: Option>, - operation_template_extension: Option>, + extra_configs: Vec, + store_factories: StoreFactories, + working_copy_factories: WorkingCopyFactories, + commit_template_extensions: Vec>, + operation_template_extensions: Vec>, dispatch_fn: CliDispatchFn, start_hook_fns: Vec, process_global_args_fns: Vec, @@ -2525,11 +2528,11 @@ impl CliRunner { CliRunner { tracing_subscription, app: crate::commands::default_app(), - extra_configs: None, - store_factories: None, - working_copy_factories: None, - commit_template_extension: None, - operation_template_extension: None, + extra_configs: vec![], + store_factories: StoreFactories::default(), + working_copy_factories: default_working_copy_factories(), + commit_template_extensions: vec![], + operation_template_extensions: vec![], dispatch_fn: Box::new(crate::commands::run_command), start_hook_fns: vec![], process_global_args_fns: vec![], @@ -2543,39 +2546,41 @@ impl CliRunner { } /// Adds default configs in addition to the normal defaults. - pub fn set_extra_config(mut self, extra_configs: config::Config) -> Self { - self.extra_configs = Some(extra_configs); + pub fn add_extra_config(mut self, extra_configs: config::Config) -> Self { + self.extra_configs.push(extra_configs); self } - /// Replaces `StoreFactories` to be used. - pub fn set_store_factories(mut self, store_factories: StoreFactories) -> Self { - self.store_factories = Some(store_factories); + /// Adds `StoreFactories` to be used. + pub fn add_store_factories(mut self, store_factories: StoreFactories) -> Self { + self.store_factories.merge(store_factories); self } - /// Replaces working copy factories to be used. - pub fn set_working_copy_factories( + /// Adds working copy factories to be used. + pub fn add_working_copy_factories( mut self, - working_copy_factories: HashMap>, + working_copy_factories: WorkingCopyFactories, ) -> Self { - self.working_copy_factories = Some(working_copy_factories); + merge_factories_map(&mut self.working_copy_factories, working_copy_factories); self } - pub fn set_commit_template_extension( + pub fn add_commit_template_extension( mut self, commit_template_extension: Box, ) -> Self { - self.commit_template_extension = Some(commit_template_extension.into()); + self.commit_template_extensions + .push(commit_template_extension.into()); self } - pub fn set_operation_template_extension( + pub fn add_operation_template_extension( mut self, operation_template_extension: Box, ) -> Self { - self.operation_template_extension = Some(operation_template_extension.into()); + self.operation_template_extensions + .push(operation_template_extension.into()); self } @@ -2687,9 +2692,6 @@ impl CliRunner { } let settings = UserSettings::from_config(config); - let working_copy_factories = self - .working_copy_factories - .unwrap_or_else(default_working_copy_factories); let command_helper = CommandHelper { app: self.app, cwd, @@ -2698,11 +2700,11 @@ impl CliRunner { global_args: args.global_args, settings, layered_configs, - commit_template_extension: self.commit_template_extension, - operation_template_extension: self.operation_template_extension, + commit_template_extensions: self.commit_template_extensions, + operation_template_extensions: self.operation_template_extensions, maybe_workspace_loader, - store_factories: self.store_factories.unwrap_or_default(), - working_copy_factories, + store_factories: self.store_factories, + working_copy_factories: self.working_copy_factories, }; for start_hook_fn in self.start_hook_fns { start_hook_fn(ui, &command_helper)?; @@ -2714,10 +2716,10 @@ impl CliRunner { #[instrument(skip(self))] pub fn run(mut self) -> ExitCode { let mut default_config = crate::config::default_config(); - if let Some(extra_configs) = self.extra_configs.take() { + for extra_config in self.extra_configs.drain(..) { default_config = config::Config::builder() .add_source(default_config) - .add_source(extra_configs) + .add_source(extra_config) .build() .unwrap(); } diff --git a/cli/src/commands/operation.rs b/cli/src/commands/operation.rs index 105008bc1e..a5374b4cd6 100644 --- a/cli/src/commands/operation.rs +++ b/cli/src/commands/operation.rs @@ -164,7 +164,7 @@ fn cmd_op_log( let language = OperationTemplateLanguage::new( repo_loader.op_store().root_operation_id(), current_op_id, - command.operation_template_extension(), + command.operation_template_extensions(), ); let text = match &args.template { Some(value) => value.to_owned(), diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 702a04a486..95861d66e5 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -17,6 +17,7 @@ use std::cmp::max; use std::collections::HashMap; use std::io; use std::rc::Rc; +use std::sync::Arc; use itertools::Itertools as _; use jj_lib::backend::{ChangeId, CommitId}; @@ -71,14 +72,12 @@ impl<'repo> CommitTemplateLanguage<'repo> { workspace_id: &WorkspaceId, revset_parse_context: RevsetParseContext<'repo>, id_prefix_context: &'repo IdPrefixContext, - extension: Option<&dyn CommitTemplateLanguageExtension>, + extensions: &Vec>, ) -> Self { let mut build_fn_table = CommitTemplateBuildFnTable::builtin(); let mut cache_extensions = ExtensionsMap::empty(); - // TODO: Extension methods should be refactored to be plural, to support - // multiple extensions in a dynamic load environment - if let Some(extension) = extension { + for extension in extensions { build_fn_table.merge(extension.build_fn_table()); extension.build_cache_extensions(&mut cache_extensions); } diff --git a/cli/src/operation_templater.rs b/cli/src/operation_templater.rs index 3e9a9a4100..27f00aca8f 100644 --- a/cli/src/operation_templater.rs +++ b/cli/src/operation_templater.rs @@ -15,6 +15,7 @@ use std::any::Any; use std::collections::HashMap; use std::io; +use std::sync::Arc; use itertools::Itertools as _; use jj_lib::extensions_map::ExtensionsMap; @@ -51,13 +52,13 @@ impl OperationTemplateLanguage { pub fn new( root_op_id: &OperationId, current_op_id: Option<&OperationId>, - extension: Option<&dyn OperationTemplateLanguageExtension>, + extensions: &Vec>, ) -> Self { let mut build_fn_table = OperationTemplateBuildFnTable::builtin(); let mut cache_extensions = ExtensionsMap::empty(); - if let Some(extension) = extension { - let ext_table = extension.build_fn_table(); - build_fn_table.merge(ext_table); + + for extension in extensions { + build_fn_table.merge(extension.build_fn_table()); extension.build_cache_extensions(&mut cache_extensions); } diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 422c3f42a5..7431fc5204 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -347,6 +347,15 @@ type IndexStoreFactory = Box Result, BackendLoadError>>; type SubmoduleStoreFactory = Box Box>; +pub fn merge_factories_map(base: &mut HashMap, ext: HashMap) { + for (name, factory) in ext { + let copy = name.clone(); + if base.insert(name, factory).is_some() { + panic!("Conflicting factory definitions for '{copy}' factory"); + } + } +} + pub struct StoreFactories { backend_factories: HashMap, op_store_factories: HashMap, @@ -426,6 +435,25 @@ impl StoreFactories { } } + pub fn merge(&mut self, ext: StoreFactories) { + let StoreFactories { + backend_factories, + op_store_factories, + op_heads_store_factories, + index_store_factories, + submodule_store_factories, + } = ext; + + merge_factories_map(&mut self.backend_factories, backend_factories); + merge_factories_map(&mut self.op_store_factories, op_store_factories); + merge_factories_map(&mut self.op_heads_store_factories, op_heads_store_factories); + merge_factories_map(&mut self.index_store_factories, index_store_factories); + merge_factories_map( + &mut self.submodule_store_factories, + submodule_store_factories, + ); + } + pub fn add_backend(&mut self, name: &str, factory: BackendFactory) { self.backend_factories.insert(name.to_string(), factory); } diff --git a/lib/src/workspace.rs b/lib/src/workspace.rs index 49fe714772..42bcb95f56 100644 --- a/lib/src/workspace.rs +++ b/lib/src/workspace.rs @@ -332,7 +332,7 @@ impl Workspace { user_settings: &UserSettings, workspace_path: &Path, store_factories: &StoreFactories, - working_copy_factories: &HashMap>, + working_copy_factories: &WorkingCopyFactories, ) -> Result { let loader = WorkspaceLoader::init(workspace_path)?; let workspace = loader.load(user_settings, store_factories, working_copy_factories)?; @@ -425,6 +425,8 @@ pub struct WorkspaceLoader { working_copy_state_path: PathBuf, } +pub type WorkingCopyFactories = HashMap>; + impl WorkspaceLoader { pub fn init(workspace_root: &Path) -> Result { let jj_dir = workspace_root.join(".jj"); @@ -468,7 +470,7 @@ impl WorkspaceLoader { &self, user_settings: &UserSettings, store_factories: &StoreFactories, - working_copy_factories: &HashMap>, + working_copy_factories: &WorkingCopyFactories, ) -> Result { let repo_loader = RepoLoader::init(user_settings, &self.repo_dir, store_factories)?; let working_copy = self.load_working_copy(repo_loader.store(), working_copy_factories)?; @@ -478,7 +480,7 @@ impl WorkspaceLoader { pub fn get_working_copy_factory<'a>( &self, - working_copy_factories: &'a HashMap>, + 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( @@ -500,7 +502,7 @@ impl WorkspaceLoader { fn load_working_copy( &self, store: &Arc, - working_copy_factories: &HashMap>, + working_copy_factories: &WorkingCopyFactories, ) -> Result, StoreLoadError> { let working_copy_factory = self.get_working_copy_factory(working_copy_factories)?; Ok(working_copy_factory.load_working_copy( @@ -511,8 +513,8 @@ impl WorkspaceLoader { } } -pub fn default_working_copy_factories() -> HashMap> { - let mut factories: HashMap> = HashMap::new(); +pub fn default_working_copy_factories() -> WorkingCopyFactories { + let mut factories = WorkingCopyFactories::new(); factories.insert( LocalWorkingCopy::name().to_owned(), Box::new(LocalWorkingCopyFactory {}),