Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: migrate off config.merge() #5015

Merged
merged 7 commits into from
Dec 4, 2024
Merged

config: migrate off config.merge() #5015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cae8d55
config: migrate resolved_config_values() to new getter API
yuja Nov 29, 2024
0a69fb9
config: remove unneeded Result wrapping from resolved_config_values()
yuja Dec 1, 2024
f10e5d7
completion: migrate to UserSettings API
yuja Nov 28, 2024
bb41024
completion: remove redundant rebuild of default config
yuja Nov 28, 2024
32e1a3e
formatter: migrate tests to be using StackedConfig
yuja Nov 28, 2024
2ff8750
cli: migrate remainder of Config::get() to StackedConfig::get()
yuja Nov 28, 2024
aa7d7c8
config: remove merge() function which is used only in tests
yuja Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 17 additions & 20 deletions cli/src/cli_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3078,7 +3078,10 @@ impl ValueParserFactory for RevisionArg {
}
}

fn get_string_or_array(config: &config::Config, key: &str) -> Result<Vec<String>, ConfigError> {
fn get_string_or_array(
config: &StackedConfig,
key: &'static str,
) -> Result<Vec<String>, ConfigError> {
config
.get(key)
.map(|string| vec![string])
Expand All @@ -3087,7 +3090,7 @@ fn get_string_or_array(config: &config::Config, key: &str) -> Result<Vec<String>

fn resolve_default_command(
ui: &Ui,
config: &config::Config,
config: &StackedConfig,
app: &Command,
mut string_args: Vec<String>,
) -> Result<Vec<String>, CommandError> {
Expand Down Expand Up @@ -3130,7 +3133,7 @@ fn resolve_default_command(

fn resolve_aliases(
ui: &Ui,
config: &config::Config,
config: &StackedConfig,
app: &Command,
mut string_args: Vec<String>,
) -> Result<Vec<String>, CommandError> {
Expand Down Expand Up @@ -3240,15 +3243,15 @@ fn handle_early_args(
}
if !args.config_toml.is_empty() {
config.add_layer(parse_config_args(&args.config_toml)?);
ui.reset(&config.merge())?;
ui.reset(config)?;
}
Ok(())
}

fn handle_shell_completion(
ui: &Ui,
app: &Command,
config: &config::Config,
config: &StackedConfig,
cwd: &Path,
) -> Result<(), CommandError> {
let mut args = vec![];
Expand Down Expand Up @@ -3296,7 +3299,7 @@ pub fn expand_args(
ui: &Ui,
app: &Command,
args_os: impl IntoIterator<Item = OsString>,
config: &config::Config,
config: &StackedConfig,
) -> Result<Vec<String>, CommandError> {
let mut string_args: Vec<String> = vec![];
for arg_os in args_os {
Expand Down Expand Up @@ -3509,11 +3512,7 @@ impl CliRunner {
}

#[instrument(skip_all)]
fn run_internal(
self,
ui: &mut Ui,
mut stacked_config: StackedConfig,
) -> Result<(), CommandError> {
fn run_internal(self, ui: &mut Ui, mut config: StackedConfig) -> Result<(), CommandError> {
// `cwd` is canonicalized for consistency with `Workspace::workspace_root()` and
// to easily compute relative paths between them.
let cwd = env::current_dir()
Expand All @@ -3532,12 +3531,11 @@ impl CliRunner {
.workspace_loader_factory
.create(find_workspace_dir(&cwd))
.map_err(|err| map_workspace_load_error(err, None));
config_env.reload_user_config(&mut stacked_config)?;
config_env.reload_user_config(&mut config)?;
if let Ok(loader) = &maybe_cwd_workspace_loader {
config_env.reset_repo_path(loader.repo_path());
config_env.reload_repo_config(&mut stacked_config)?;
config_env.reload_repo_config(&mut config)?;
}
let config = stacked_config.merge();
ui.reset(&config).map_err(|e| {
let user_config_path = config_env.existing_user_config_path();
let repo_config_path = config_env.existing_repo_config_path();
Expand All @@ -3559,9 +3557,9 @@ impl CliRunner {
&self.app,
&self.tracing_subscription,
&string_args,
&mut stacked_config,
&mut config,
)
.map_err(|err| map_clap_cli_error(err, ui, &stacked_config))?;
.map_err(|err| map_clap_cli_error(err, ui, &config))?;
for process_global_args_fn in self.process_global_args_fns {
process_global_args_fn(ui, &matches)?;
}
Expand All @@ -3573,14 +3571,13 @@ impl CliRunner {
.create(&cwd.join(path))
.map_err(|err| map_workspace_load_error(err, Some(path)))?;
config_env.reset_repo_path(loader.repo_path());
config_env.reload_repo_config(&mut stacked_config)?;
config_env.reload_repo_config(&mut config)?;
Ok(loader)
} else {
maybe_cwd_workspace_loader
};

// Apply workspace configs and --config-toml arguments.
let config = stacked_config.merge();
ui.reset(&config)?;

// If -R is specified, check if the expanded arguments differ. Aliases
Expand All @@ -3595,7 +3592,7 @@ impl CliRunner {
}
}

let settings = UserSettings::from_config(stacked_config);
let settings = UserSettings::from_config(config);
let command_helper_data = CommandHelperData {
app: self.app,
cwd,
Expand Down Expand Up @@ -3631,7 +3628,7 @@ impl CliRunner {
.build()
.unwrap();
let stacked_config = config_from_environment(config);
let mut ui = Ui::with_config(&stacked_config.merge())
let mut ui = Ui::with_config(&stacked_config)
.expect("default config should be valid, env vars are stringly typed");
let result = self.run_internal(&mut ui, stacked_config);
let exit_code = handle_command_result(&mut ui, result);
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/config/list.rs
View file
Open in desktop
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.settings().config(), &name_path)? {
for annotated in resolved_config_values(command.settings().config(), &name_path) {
// Remove overridden values.
if annotated.is_overridden && !args.include_overridden {
continue;
Expand Down
47 changes: 21 additions & 26 deletions cli/src/complete.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ use std::io::BufRead;
use clap::builder::StyledStr;
use clap::FromArgMatches as _;
use clap_complete::CompletionCandidate;
use config::Config;
use itertools::Itertools;
use jj_lib::config::ConfigNamePathBuf;
use jj_lib::settings::UserSettings;
use jj_lib::workspace::DefaultWorkspaceLoaderFactory;
use jj_lib::workspace::WorkspaceLoaderFactory as _;

Expand Down Expand Up @@ -101,7 +101,7 @@ pub fn tracked_bookmarks() -> Vec<CompletionCandidate> {
}

pub fn untracked_bookmarks() -> Vec<CompletionCandidate> {
with_jj(|jj, config| {
with_jj(|jj, settings| {
let output = jj
.build()
.arg("bookmark")
Expand All @@ -118,7 +118,7 @@ pub fn untracked_bookmarks() -> Vec<CompletionCandidate> {
.output()
.map_err(user_error)?;

let prefix = config.get::<String>("git.push-bookmark-prefix").ok();
let prefix = settings.get_string("git.push-bookmark-prefix").ok();

Ok(String::from_utf8_lossy(&output.stdout)
.lines()
Expand All @@ -139,7 +139,7 @@ pub fn untracked_bookmarks() -> Vec<CompletionCandidate> {
}

pub fn bookmarks() -> Vec<CompletionCandidate> {
with_jj(|jj, config| {
with_jj(|jj, settings| {
let output = jj
.build()
.arg("bookmark")
Expand All @@ -156,7 +156,7 @@ pub fn bookmarks() -> Vec<CompletionCandidate> {
.map_err(user_error)?;
let stdout = String::from_utf8_lossy(&output.stdout);

let prefix = config.get::<String>("git.push-bookmark-prefix").ok();
let prefix = settings.get_string("git.push-bookmark-prefix").ok();

Ok((&stdout
.lines()
Expand Down Expand Up @@ -204,8 +204,8 @@ pub fn git_remotes() -> Vec<CompletionCandidate> {
}

pub fn aliases() -> Vec<CompletionCandidate> {
with_jj(|_, config| {
Ok(config
with_jj(|_, settings| {
Ok(settings
.get_table("aliases")?
.into_keys()
// This is opinionated, but many people probably have several
Expand All @@ -219,7 +219,7 @@ pub fn aliases() -> Vec<CompletionCandidate> {
}

fn revisions(revisions: Option<&str>) -> Vec<CompletionCandidate> {
with_jj(|jj, config| {
with_jj(|jj, settings| {
// display order
const LOCAL_BOOKMARK_MINE: usize = 0;
const LOCAL_BOOKMARK: usize = 1;
Expand All @@ -232,7 +232,7 @@ fn revisions(revisions: Option<&str>) -> Vec<CompletionCandidate> {

// bookmarks

let prefix = config.get::<String>("git.push-bookmark-prefix").ok();
let prefix = settings.get_string("git.push-bookmark-prefix").ok();

let mut cmd = jj.build();
cmd.arg("bookmark")
Expand Down Expand Up @@ -298,8 +298,8 @@ fn revisions(revisions: Option<&str>) -> Vec<CompletionCandidate> {

let revisions = revisions
.map(String::from)
.or_else(|| config.get_string("revsets.short-prefixes").ok())
.or_else(|| config.get_string("revsets.log").ok())
.or_else(|| settings.get_string("revsets.short-prefixes").ok())
.or_else(|| settings.get_string("revsets.log").ok())
.unwrap_or_default();

let output = jj
Expand Down Expand Up @@ -643,10 +643,10 @@ pub fn interdiff_files(current: &std::ffi::OsStr) -> Vec<CompletionCandidate> {
/// In case of errors, print them and early return an empty vector.
fn with_jj<F>(completion_fn: F) -> Vec<CompletionCandidate>
where
F: FnOnce(JjBuilder, &Config) -> Result<Vec<CompletionCandidate>, CommandError>,
F: FnOnce(JjBuilder, &UserSettings) -> Result<Vec<CompletionCandidate>, CommandError>,
{
get_jj_command()
.and_then(|(jj, config)| completion_fn(jj, &config))
.and_then(|(jj, settings)| completion_fn(jj, &settings))
.unwrap_or_else(|e| {
eprintln!("{}", e.error);
Vec::new()
Expand All @@ -662,7 +662,7 @@ where
/// give completion code access to custom backends. Shelling out was chosen as
/// the preferred method, because it's more maintainable and the performance
/// requirements of completions aren't very high.
fn get_jj_command() -> Result<(JjBuilder, Config), CommandError> {
fn get_jj_command() -> Result<(JjBuilder, UserSettings), CommandError> {
let current_exe = std::env::current_exe().map_err(user_error)?;
let mut cmd_args = Vec::<String>::new();

Expand All @@ -676,23 +676,18 @@ fn get_jj_command() -> Result<(JjBuilder, Config), CommandError> {
// child process. This shouldn't fail, since none of the global args are
// required.
let app = crate::commands::default_app();
let config = config::Config::builder()
.add_source(default_config())
.build()
.expect("default config should be valid");
let mut stacked_config = config_from_environment(config);
let ui = Ui::with_config(&stacked_config.merge()).expect("default config should be valid");
let mut config = config_from_environment(default_config());
let ui = Ui::with_config(&config).expect("default config should be valid");
let cwd = std::env::current_dir()
.and_then(|cwd| cwd.canonicalize())
.map_err(user_error)?;
let mut config_env = ConfigEnv::from_environment()?;
let maybe_cwd_workspace_loader = DefaultWorkspaceLoaderFactory.create(find_workspace_dir(&cwd));
let _ = config_env.reload_user_config(&mut stacked_config);
let _ = config_env.reload_user_config(&mut config);
if let Ok(loader) = &maybe_cwd_workspace_loader {
config_env.reset_repo_path(loader.repo_path());
let _ = config_env.reload_repo_config(&mut stacked_config);
let _ = config_env.reload_repo_config(&mut config);
}
let mut config = stacked_config.merge();
// skip 2 because of the clap_complete prelude: jj -- jj <actual args...>
let args = std::env::args_os().skip(2);
let args = expand_args(&ui, &app, args, &config)?;
Expand All @@ -708,8 +703,7 @@ fn get_jj_command() -> Result<(JjBuilder, Config), CommandError> {
// Try to update repo-specific config on a best-effort basis.
if let Ok(loader) = DefaultWorkspaceLoaderFactory.create(&cwd.join(&repository)) {
config_env.reset_repo_path(loader.repo_path());
let _ = config_env.reload_repo_config(&mut stacked_config);
config = stacked_config.merge();
let _ = config_env.reload_repo_config(&mut config);
}
cmd_args.push("--repository".into());
cmd_args.push(repository);
Expand Down Expand Up @@ -750,8 +744,9 @@ fn get_jj_command() -> Result<(JjBuilder, Config), CommandError> {
cmd: current_exe,
args: cmd_args,
};
let settings = UserSettings::from_config(config);

Ok((builder, config))
Ok((builder, settings))
}

/// A helper struct to allow completion functions to call jj multiple times with
Expand Down
Loading
Loading