Skip to content

Commit

Permalink
settings: own StackedConfig by UserSettings, migrate tests to use con…
Browse files Browse the repository at this point in the history
…fig layer

UserSettings::get_*() will be changed to look up a merged value from
StackedConfig, not from a merged config::Value. This will help migrate away
from the config crate.

Not all tests are ported to ConfigLayer::parse() because it seemed a bit odd
to format!() a TOML document and parse it to build a table of configuration
variables.
  • Loading branch information
yuja committed Nov 30, 2024
1 parent 512d85b commit da01734
Show file tree
Hide file tree
Showing 10 changed files with 133 additions and 85 deletions.
13 changes: 3 additions & 10 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,6 @@ struct CommandHelperData {
global_args: GlobalArgs,
config_env: ConfigEnv,
settings: UserSettings,
stacked_config: StackedConfig,
revset_extensions: Arc<RevsetExtensions>,
commit_template_extensions: Vec<Arc<dyn CommitTemplateLanguageExtension>>,
operation_template_extensions: Vec<Arc<dyn OperationTemplateLanguageExtension>>,
Expand Down Expand Up @@ -323,11 +322,6 @@ impl CommandHelper {
&self.data.settings
}

// TODO: will be moved to UserSettings
pub fn stacked_config(&self) -> &StackedConfig {
&self.data.stacked_config
}

pub fn revset_extensions(&self) -> &Arc<RevsetExtensions> {
&self.data.revset_extensions
}
Expand All @@ -337,7 +331,7 @@ impl CommandHelper {
/// For most commands that depend on a loaded repo, you should use
/// `WorkspaceCommandHelper::template_aliases_map()` instead.
fn load_template_aliases(&self, ui: &Ui) -> Result<TemplateAliasesMap, CommandError> {
load_template_aliases(ui, &self.data.stacked_config)
load_template_aliases(ui, self.settings().stacked_config())
}

/// Parses template of the given language into evaluation tree.
Expand Down Expand Up @@ -724,7 +718,7 @@ impl WorkspaceCommandEnvironment {
#[instrument(skip_all)]
fn new(ui: &Ui, command: &CommandHelper, workspace: &Workspace) -> Result<Self, CommandError> {
let revset_aliases_map =
revset_util::load_revset_aliases(ui, &command.data.stacked_config)?;
revset_util::load_revset_aliases(ui, command.settings().stacked_config())?;
let template_aliases_map = command.load_template_aliases(ui)?;
let path_converter = RepoPathUiConverter::Fs {
cwd: command.cwd().to_owned(),
Expand Down Expand Up @@ -3627,7 +3621,7 @@ impl CliRunner {
}
}

let settings = UserSettings::from_config(config);
let settings = UserSettings::from_config(stacked_config);
let command_helper_data = CommandHelperData {
app: self.app,
cwd,
Expand All @@ -3636,7 +3630,6 @@ impl CliRunner {
global_args: args.global_args,
config_env,
settings,
stacked_config,
revset_extensions: self.revset_extensions.into(),
commit_template_extensions: self.commit_template_extensions,
operation_template_extensions: self.operation_template_extensions,
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/config/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ pub fn cmd_config_list(
let mut formatter = ui.stdout_formatter();
let name_path = args.name.clone().unwrap_or_else(ConfigNamePathBuf::root);
let mut wrote_values = false;
for annotated in resolved_config_values(command.stacked_config(), &name_path)? {
for annotated in resolved_config_values(command.settings().stacked_config(), &name_path)? {
// Remove overridden values.
if annotated.is_overridden && !args.include_overridden {
continue;
Expand Down
24 changes: 15 additions & 9 deletions cli/src/merge_tools/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,15 +359,21 @@ impl MergeEditor {

#[cfg(test)]
mod tests {
use jj_lib::config::ConfigLayer;
use jj_lib::config::ConfigSource;
use jj_lib::config::StackedConfig;

use super::*;

fn config_from_string(text: &str) -> config::Config {
config::Config::builder()
// Load defaults to test the default args lookup
.add_source(crate::config::default_config())
.add_source(config::File::from_str(text, config::FileFormat::Toml))
.build()
.unwrap()
fn config_from_string(text: &str) -> StackedConfig {
let mut config = StackedConfig::empty();
// Load defaults to test the default args lookup
config.add_layer(ConfigLayer::with_data(
ConfigSource::Default,
crate::config::default_config(),
));
config.add_layer(ConfigLayer::parse(ConfigSource::User, text).unwrap());
config
}

#[test]
Expand Down Expand Up @@ -440,7 +446,7 @@ mod tests {
fn test_get_diff_editor_from_settings() {
let get = |text| {
let config = config_from_string(text);
let ui = Ui::with_config(&config).unwrap();
let ui = Ui::with_config(&config.merge()).unwrap();
let settings = UserSettings::from_config(config);
DiffEditor::from_settings(
&ui,
Expand Down Expand Up @@ -657,7 +663,7 @@ mod tests {
fn test_get_merge_editor_from_settings() {
let get = |text| {
let config = config_from_string(text);
let ui = Ui::with_config(&config).unwrap();
let ui = Ui::with_config(&config.merge()).unwrap();
let settings = UserSettings::from_config(config);
MergeEditor::from_settings(&ui, &settings, ConflictMarkerStyle::Diff)
.map(|editor| editor.tool)
Expand Down
3 changes: 2 additions & 1 deletion lib/src/git_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1536,6 +1536,7 @@ mod tests {
use test_case::test_case;

use super::*;
use crate::config::StackedConfig;
use crate::content_hash::blake2b_hash;

#[test_case(false; "legacy tree format")]
Expand Down Expand Up @@ -2220,7 +2221,7 @@ mod tests {
// UserSettings type. testutils returns jj_lib (2)'s UserSettings, whereas
// our UserSettings type comes from jj_lib (1).
fn user_settings() -> UserSettings {
let config = config::Config::default();
let config = StackedConfig::empty();
UserSettings::from_config(config)
}
}
19 changes: 15 additions & 4 deletions lib/src/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,24 @@ use crate::backend::Commit;
use crate::backend::Signature;
use crate::backend::Timestamp;
use crate::config::ConfigError;
use crate::config::StackedConfig;
use crate::conflicts::ConflictMarkerStyle;
use crate::fmt_util::binary_prefix;
use crate::fsmonitor::FsmonitorSettings;
use crate::signing::SignBehavior;

#[derive(Debug, Clone)]
pub struct UserSettings {
// TODO: merged "config" will be replaced by StackedConfig
stacked_config: StackedConfig,
config: config::Config,
timestamp: Option<Timestamp>,
rng: Arc<JJRng>,
}

#[derive(Debug, Clone)]
pub struct RepoSettings {
_config: config::Config,
_stacked_config: StackedConfig,
}

#[derive(Debug, Clone)]
Expand Down Expand Up @@ -135,10 +138,12 @@ fn get_rng_seed_config(config: &config::Config) -> Option<u64> {
}

impl UserSettings {
pub fn from_config(config: config::Config) -> Self {
pub fn from_config(stacked_config: StackedConfig) -> Self {
let config = stacked_config.merge();
let timestamp = get_timestamp_config(&config, "debug.commit-timestamp");
let rng_seed = get_rng_seed_config(&config);
UserSettings {
stacked_config,
config,
timestamp,
rng: Arc::new(JJRng::new(rng_seed)),
Expand All @@ -148,8 +153,10 @@ impl UserSettings {
// TODO: Reconsider UserSettings/RepoSettings abstraction. See
// https://github.com/martinvonz/jj/issues/616#issuecomment-1345170699
pub fn with_repo(&self, _repo_path: &Path) -> Result<RepoSettings, ConfigError> {
let config = self.config.clone();
Ok(RepoSettings { _config: config })
let stacked_config = self.stacked_config.clone();
Ok(RepoSettings {
_stacked_config: stacked_config,
})
}

pub fn get_rng(&self) -> Arc<JJRng> {
Expand Down Expand Up @@ -224,6 +231,10 @@ impl UserSettings {
self.get_bool("ui.allow-init-native").unwrap_or(false)
}

pub fn stacked_config(&self) -> &StackedConfig {
&self.stacked_config
}

// TODO: remove
pub fn raw_config(&self) -> &config::Config {
&self.config
Expand Down
85 changes: 54 additions & 31 deletions lib/tests/test_commit_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,15 @@
// limitations under the License.

use futures::StreamExt as _;
use indoc::indoc;
use itertools::Itertools;
use jj_lib::backend::ChangeId;
use jj_lib::backend::MillisSinceEpoch;
use jj_lib::backend::Signature;
use jj_lib::backend::Timestamp;
use jj_lib::config::ConfigLayer;
use jj_lib::config::ConfigSource;
use jj_lib::config::StackedConfig;
use jj_lib::matchers::EverythingMatcher;
use jj_lib::merged_tree::MergedTree;
use jj_lib::repo::Repo;
Expand Down Expand Up @@ -152,13 +156,17 @@ fn test_rewrite(backend: TestRepoBackend) {
],
);

let config = config::Config::builder()
.set_override("user.name", "Rewrite User")
.unwrap()
.set_override("user.email", "[email protected]")
.unwrap()
.build()
.unwrap();
let mut config = StackedConfig::empty();
config.add_layer(
ConfigLayer::parse(
ConfigSource::User,
indoc! {"
user.name = 'Rewrite User'
user.email = '[email protected]'
"},
)
.unwrap(),
);
let rewrite_settings = UserSettings::from_config(config);
let mut tx = repo.start_transaction(&settings);
let rewritten_commit = tx
Expand Down Expand Up @@ -203,8 +211,7 @@ fn test_rewrite(backend: TestRepoBackend) {
#[test_case(TestRepoBackend::Local ; "local backend")]
#[test_case(TestRepoBackend::Git ; "git backend")]
fn test_rewrite_update_missing_user(backend: TestRepoBackend) {
let missing_user_settings =
UserSettings::from_config(config::Config::builder().build().unwrap());
let missing_user_settings = UserSettings::from_config(StackedConfig::empty());
let test_repo = TestRepo::init_with_backend(backend);
let repo = &test_repo.repo;

Expand All @@ -223,13 +230,17 @@ fn test_rewrite_update_missing_user(backend: TestRepoBackend) {
assert_eq!(initial_commit.committer().name, "");
assert_eq!(initial_commit.committer().email, "");

let config = config::Config::builder()
.set_override("user.name", "Configured User")
.unwrap()
.set_override("user.email", "[email protected]")
.unwrap()
.build()
.unwrap();
let mut config = StackedConfig::empty();
config.add_layer(
ConfigLayer::parse(
ConfigSource::User,
indoc! {"
user.name = 'Configured User'
user.email = '[email protected]'
"},
)
.unwrap(),
);
let settings = UserSettings::from_config(config);
let rewritten_commit = tx
.repo_mut()
Expand Down Expand Up @@ -257,11 +268,15 @@ fn test_rewrite_resets_author_timestamp(backend: TestRepoBackend) {

// Create discardable commit
let initial_timestamp = "2001-02-03T04:05:06+07:00";
let config = testutils::base_config()
.set_override("debug.commit-timestamp", initial_timestamp)
.unwrap()
.build()
.unwrap();
let mut config = testutils::base_user_config();
config.add_layer(ConfigLayer::with_data(
ConfigSource::User,
config::Config::builder()
.set_override("debug.commit-timestamp", initial_timestamp)
.unwrap()
.build()
.unwrap(),
));
let settings = UserSettings::from_config(config);
let mut tx = repo.start_transaction(&settings);
let initial_commit = tx
Expand All @@ -281,11 +296,15 @@ fn test_rewrite_resets_author_timestamp(backend: TestRepoBackend) {

// Rewrite discardable commit to no longer be discardable
let new_timestamp_1 = "2002-03-04T05:06:07+08:00";
let config = testutils::base_config()
.set_override("debug.commit-timestamp", new_timestamp_1)
.unwrap()
.build()
.unwrap();
let mut config = testutils::base_user_config();
config.add_layer(ConfigLayer::with_data(
ConfigSource::User,
config::Config::builder()
.set_override("debug.commit-timestamp", new_timestamp_1)
.unwrap()
.build()
.unwrap(),
));
let settings = UserSettings::from_config(config);
let rewritten_commit_1 = tx
.repo_mut()
Expand All @@ -304,11 +323,15 @@ fn test_rewrite_resets_author_timestamp(backend: TestRepoBackend) {

// Rewrite non-discardable commit
let new_timestamp_2 = "2003-04-05T06:07:08+09:00";
let config = testutils::base_config()
.set_override("debug.commit-timestamp", new_timestamp_2)
.unwrap()
.build()
.unwrap();
let mut config = testutils::base_user_config();
config.add_layer(ConfigLayer::with_data(
ConfigSource::User,
config::Config::builder()
.set_override("debug.commit-timestamp", new_timestamp_2)
.unwrap()
.build()
.unwrap(),
));
let settings = UserSettings::from_config(config);
let rewritten_commit_2 = tx
.repo_mut()
Expand Down
5 changes: 4 additions & 1 deletion lib/tests/test_init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use std::path::Path;
use std::path::PathBuf;

use jj_lib::config::StackedConfig;
use jj_lib::git_backend::GitBackend;
use jj_lib::op_store::WorkspaceId;
use jj_lib::repo::Repo;
Expand Down Expand Up @@ -140,7 +141,9 @@ fn test_init_external_git() {
#[test_case(TestRepoBackend::Git ; "git backend")]
fn test_init_no_config_set(backend: TestRepoBackend) {
// Test that we can create a repo without setting any config
let settings = UserSettings::from_config(config::Config::default());
// TODO: Perhaps, StackedConfig::empty() will be replaced with ::default()
// or something that includes the minimal configuration variables.
let settings = UserSettings::from_config(StackedConfig::empty());
let test_workspace = TestWorkspace::init_with_backend(&settings, backend);
let repo = &test_workspace.repo;
let wc_commit_id = repo
Expand Down
20 changes: 11 additions & 9 deletions lib/tests/test_operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ use std::time::SystemTime;
use assert_matches::assert_matches;
use itertools::Itertools as _;
use jj_lib::backend::CommitId;
use jj_lib::config::ConfigLayer;
use jj_lib::config::ConfigSource;
use jj_lib::object_id::ObjectId;
use jj_lib::op_walk;
use jj_lib::op_walk::OpsetEvaluationError;
Expand Down Expand Up @@ -421,15 +423,15 @@ fn test_reparent_range_bookmarky() {
}

fn stable_op_id_settings() -> UserSettings {
UserSettings::from_config(
testutils::base_config()
.add_source(config::File::from_str(
"debug.operation-timestamp = '2001-02-03T04:05:06+07:00'",
config::FileFormat::Toml,
))
.build()
.unwrap(),
)
let mut config = testutils::base_user_config();
config.add_layer(
ConfigLayer::parse(
ConfigSource::User,
"debug.operation-timestamp = '2001-02-03T04:05:06+07:00'",
)
.unwrap(),
);
UserSettings::from_config(config)
}

#[test]
Expand Down
Loading

0 comments on commit da01734

Please sign in to comment.