config: refactor and move config set/unset functions to jj-lib

cli/src/command_error.rs

@@ -22,6 +22,7 @@ use std::sync::Arc;
 
 use itertools::Itertools as _;
 use jj_lib::backend::BackendError;
+use jj_lib::config::ConfigFileSaveError;
 use jj_lib::config::ConfigGetError;
 use jj_lib::config::ConfigLoadError;
 use jj_lib::dsl_util::Diagnostics;
@@ -246,6 +247,12 @@ impl From<ConfigEnvError> for CommandError {
     }
 }
 
+impl From<ConfigFileSaveError> for CommandError {
+    fn from(err: ConfigFileSaveError) -> Self {
+        user_error(err)
+    }
+}
+
 impl From<ConfigGetError> for CommandError {
     fn from(err: ConfigGetError) -> Self {
         let hint = match &err {

cli/src/commands/config/mod.rs

@@ -21,6 +21,7 @@ mod unset;
 
 use std::path::Path;
 
+use jj_lib::config::ConfigFile;
 use jj_lib::config::ConfigSource;
 use tracing::instrument;
 
@@ -83,6 +84,18 @@ impl ConfigLevelArgs {
             panic!("No config_level provided")
         }
     }
+
+    fn edit_config_file(&self, config_env: &ConfigEnv) -> Result<ConfigFile, CommandError> {
+        let path = self.new_config_file_path(config_env)?;
+        if path.is_dir() {
+            return Err(user_error(format!(
+                "Can't set config in path {path} (dirs not supported)",
+                path = path.display()
+            )));
+        }
+        let source = self.get_source_kind().unwrap();
+        Ok(ConfigFile::load_or_empty(source, path)?)
+    }
 }
 
 /// Manage config options

cli/src/commands/config/set.rs

@@ -23,11 +23,10 @@ use tracing::instrument;
 use super::ConfigLevelArgs;
 use crate::cli_util::CommandHelper;
 use crate::cli_util::WorkspaceCommandHelper;
-use crate::command_error::user_error;
+use crate::command_error::user_error_with_message;
 use crate::command_error::CommandError;
 use crate::complete;
 use crate::config::parse_toml_value_or_bare_string;
-use crate::config::write_config_value_to_file;
 use crate::ui::Ui;
 
 /// Update config file to set the given option to a given value.
@@ -53,13 +52,7 @@ pub fn cmd_config_set(
     command: &CommandHelper,
     args: &ConfigSetArgs,
 ) -> Result<(), CommandError> {
-    let config_path = args.level.new_config_file_path(command.config_env())?;
-    if config_path.is_dir() {
-        return Err(user_error(format!(
-            "Can't set config in path {path} (dirs not supported)",
-            path = config_path.display()
-        )));
-    }
+    let mut file = args.level.edit_config_file(command.config_env())?;
 
     // TODO(#531): Infer types based on schema (w/ --type arg to override).
     let value = parse_toml_value_or_bare_string(&args.value);
@@ -72,7 +65,10 @@ pub fn cmd_config_set(
         check_wc_author(ui, command, &value, AuthorChange::Email)?;
     };
 
-    write_config_value_to_file(&args.name, value, config_path)
+    file.set_value(&args.name, value)
+        .map_err(|err| user_error_with_message(format!("Failed to set {}", args.name), err))?;
+    file.save()?;
+    Ok(())
 }
 
 /// Returns the commit of the working copy if it exists.

cli/src/commands/config/unset.rs

@@ -19,9 +19,9 @@ use tracing::instrument;
 use super::ConfigLevelArgs;
 use crate::cli_util::CommandHelper;
 use crate::command_error::user_error;
+use crate::command_error::user_error_with_message;
 use crate::command_error::CommandError;
 use crate::complete;
-use crate::config::remove_config_value_from_file;
 use crate::ui::Ui;
 
 /// Update config file to unset the given option.
@@ -39,13 +39,13 @@ pub fn cmd_config_unset(
     command: &CommandHelper,
     args: &ConfigUnsetArgs,
 ) -> Result<(), CommandError> {
-    let config_path = args.level.new_config_file_path(command.config_env())?;
-    if config_path.is_dir() {
-        return Err(user_error(format!(
-            "Can't set config in path {path} (dirs not supported)",
-            path = config_path.display()
-        )));
+    let mut file = args.level.edit_config_file(command.config_env())?;
+    let old_value = file
+        .delete_value(&args.name)
+        .map_err(|err| user_error_with_message(format!("Failed to unset {}", args.name), err))?;
+    if old_value.is_none() {
+        return Err(user_error(format!(r#""{}" doesn't exist"#, args.name)));
     }
-
-    remove_config_value_from_file(&args.name, config_path)
+    file.save()?;
+    Ok(())
 }

cli/src/commands/git/clone.rs

@@ -19,22 +19,21 @@ use std::num::NonZeroU32;
 use std::path::Path;
 use std::path::PathBuf;
 
-use jj_lib::config::ConfigNamePathBuf;
 use jj_lib::git;
 use jj_lib::git::GitFetchError;
 use jj_lib::git::GitFetchStats;
 use jj_lib::repo::Repo;
 use jj_lib::str_util::StringPattern;
 use jj_lib::workspace::Workspace;
 
+use super::write_repository_level_trunk_alias;
 use crate::cli_util::CommandHelper;
 use crate::cli_util::WorkspaceCommandHelper;
 use crate::command_error::cli_error;
 use crate::command_error::user_error;
 use crate::command_error::user_error_with_message;
 use crate::command_error::CommandError;
 use crate::commands::git::maybe_add_gitignore;
-use crate::config::write_config_value_to_file;
 use crate::git_util::get_git_repo;
 use crate::git_util::map_git_error;
 use crate::git_util::print_git_import_stats;
@@ -165,16 +164,11 @@ pub fn cmd_git_clone(
 
     let (mut workspace_command, stats) = clone_result?;
     if let Some(default_branch) = &stats.default_branch {
-        // Set repository level `trunk()` alias to the default remote branch.
-        let config_path = workspace_command.repo_path().join("config.toml");
-        write_config_value_to_file(
-            &ConfigNamePathBuf::from_iter(["revset-aliases", "trunk()"]),
-            format!("{default_branch}@{remote_name}").into(),
-            &config_path,
-        )?;
-        writeln!(
-            ui.status(),
-            "Setting the revset alias \"trunk()\" to \"{default_branch}@{remote_name}\""
+        write_repository_level_trunk_alias(
+            ui,
+            workspace_command.repo_path(),
+            remote_name,
+            default_branch,
         )?;
 
         let default_branch_remote_ref = workspace_command

cli/src/commands/git/init.rs

@@ -17,7 +17,6 @@ use std::path::Path;
 use std::path::PathBuf;
 use std::sync::Arc;
 
-use jj_lib::config::ConfigNamePathBuf;
 use jj_lib::file_util;
 use jj_lib::git;
 use jj_lib::git::parse_git_ref;
@@ -26,6 +25,7 @@ use jj_lib::repo::ReadonlyRepo;
 use jj_lib::repo::Repo;
 use jj_lib::workspace::Workspace;
 
+use super::write_repository_level_trunk_alias;
 use crate::cli_util::print_trackable_remote_bookmarks;
 use crate::cli_util::start_repo_transaction;
 use crate::cli_util::CommandHelper;
@@ -35,7 +35,6 @@ use crate::command_error::user_error_with_hint;
 use crate::command_error::user_error_with_message;
 use crate::command_error::CommandError;
 use crate::commands::git::maybe_add_gitignore;
-use crate::config::write_config_value_to_file;
 use crate::git_util::get_git_repo;
 use crate::git_util::is_colocated_git_workspace;
 use crate::git_util::print_failed_git_export;
@@ -240,20 +239,12 @@ pub fn maybe_set_repository_level_trunk_alias(
     let git_repo = get_git_repo(workspace_command.repo().store())?;
     if let Ok(reference) = git_repo.find_reference("refs/remotes/origin/HEAD") {
         if let Some(reference_name) = reference.symbolic_target() {
-            if let Some(RefName::RemoteBranch {
-                branch: default_branch,
-                ..
-            }) = parse_git_ref(reference_name)
-            {
-                let config_path = workspace_command.repo_path().join("config.toml");
-                write_config_value_to_file(
-                    &ConfigNamePathBuf::from_iter(["revset-aliases", "trunk()"]),
-                    format!("{default_branch}@origin").into(),
-                    &config_path,
-                )?;
-                writeln!(
-                    ui.status(),
-                    "Setting the revset alias \"trunk()\" to \"{default_branch}@origin\"",
+            if let Some(RefName::RemoteBranch { branch, .. }) = parse_git_ref(reference_name) {
+                write_repository_level_trunk_alias(
+                    ui,
+                    workspace_command.repo_path(),
+                    "origin",
+                    &branch,
                 )?;
             }
         };

cli/src/commands/git/mod.rs

@@ -21,7 +21,11 @@ pub mod push;
 pub mod remote;
 pub mod submodule;
 
+use std::path::Path;
+
 use clap::Subcommand;
+use jj_lib::config::ConfigFile;
+use jj_lib::config::ConfigSource;
 
 use self::clone::cmd_git_clone;
 use self::clone::GitCloneArgs;
@@ -102,3 +106,21 @@ fn get_single_remote(git_repo: &git2::Repository) -> Result<Option<String>, Comm
         _ => None,
     })
 }
+
+/// Sets repository level `trunk()` alias to the specified remote branch.
+fn write_repository_level_trunk_alias(
+    ui: &Ui,
+    repo_path: &Path,
+    remote: &str,
+    branch: &str,
+) -> Result<(), CommandError> {
+    let mut file = ConfigFile::load_or_empty(ConfigSource::Repo, repo_path.join("config.toml"))?;
+    file.set_value(["revset-aliases", "trunk()"], format!("{branch}@{remote}"))
+        .expect("initial repo config shouldn't have invalid values");
+    file.save()?;
+    writeln!(
+        ui.status(),
+        r#"Setting the revset alias "trunk()" to "{branch}@{remote}""#,
+    )?;
+    Ok(())
+}

cli/src/config.rs

@@ -33,10 +33,6 @@ use regex::Regex;
 use thiserror::Error;
 use tracing::instrument;
 
-use crate::command_error::user_error;
-use crate::command_error::user_error_with_message;
-use crate::command_error::CommandError;
-
 // TODO(#879): Consider generating entire schema dynamically vs. static file.
 pub const CONFIG_SCHEMA: &str = include_str!("config-schema.json");
 
@@ -426,107 +422,6 @@ pub fn parse_config_args(toml_strs: &[ConfigArg]) -> Result<Vec<ConfigLayer>, Co
         .try_collect()
 }
 
-fn read_config(path: &Path) -> Result<toml_edit::ImDocument<String>, CommandError> {
-    let config_toml = std::fs::read_to_string(path).or_else(|err| {
-        match err.kind() {
-            // If config doesn't exist yet, read as empty and we'll write one.
-            std::io::ErrorKind::NotFound => Ok("".to_string()),
-            _ => Err(user_error_with_message(
-                format!("Failed to read file {path}", path = path.display()),
-                err,
-            )),
-        }
-    })?;
-    config_toml.parse().map_err(|err| {
-        user_error_with_message(
-            format!("Failed to parse file {path}", path = path.display()),
-            err,
-        )
-    })
-}
-
-fn write_config(path: &Path, doc: &toml_edit::DocumentMut) -> Result<(), CommandError> {
-    std::fs::write(path, doc.to_string()).map_err(|err| {
-        user_error_with_message(
-            format!("Failed to write file {path}", path = path.display()),
-            err,
-        )
-    })
-}
-
-pub fn write_config_value_to_file(
-    key: &ConfigNamePathBuf,
-    value: toml_edit::Value,
-    path: &Path,
-) -> Result<(), CommandError> {
-    let mut doc = read_config(path)?.into_mut();
-
-    // Apply config value
-    let mut target_table = doc.as_table_mut();
-    let mut key_parts_iter = key.components();
-    let last_key_part = key_parts_iter.next_back().expect("key must not be empty");
-    for key_part in key_parts_iter {
-        target_table = target_table
-            .entry(key_part)
-            .or_insert_with(|| toml_edit::Item::Table(toml_edit::Table::new()))
-            .as_table_mut()
-            .ok_or_else(|| {
-                user_error(format!(
-                    "Failed to set {key}: would overwrite non-table value with parent table"
-                ))
-            })?;
-    }
-    // Error out if overwriting non-scalar value for key (table or array) with
-    // scalar.
-    match target_table.get(last_key_part) {
-        None | Some(toml_edit::Item::None | toml_edit::Item::Value(_)) => {}
-        Some(toml_edit::Item::Table(_) | toml_edit::Item::ArrayOfTables(_)) => {
-            return Err(user_error(format!(
-                "Failed to set {key}: would overwrite entire table"
-            )));
-        }
-    }
-    target_table[last_key_part] = toml_edit::Item::Value(value);
-
-    write_config(path, &doc)
-}
-
-pub fn remove_config_value_from_file(
-    key: &ConfigNamePathBuf,
-    path: &Path,
-) -> Result<(), CommandError> {
-    let mut doc = read_config(path)?.into_mut();
-
-    // Find target table
-    let mut key_iter = key.components();
-    let last_key = key_iter.next_back().expect("key must not be empty");
-    let target_table = key_iter.try_fold(doc.as_table_mut(), |table, key| {
-        table
-            .get_mut(key)
-            .ok_or_else(|| user_error(format!(r#""{key}" doesn't exist"#)))
-            .and_then(|table| {
-                table
-                    .as_table_mut()
-                    .ok_or_else(|| user_error(format!(r#""{key}" is not a table"#)))
-            })
-    })?;
-
-    // Remove config value
-    match target_table.entry(last_key) {
-        toml_edit::Entry::Occupied(entry) => {
-            if entry.get().is_table() {
-                return Err(user_error(format!("Won't remove table {key}")));
-            }
-            entry.remove();
-        }
-        toml_edit::Entry::Vacant(_) => {
-            return Err(user_error(format!(r#""{key}" doesn't exist"#)));
-        }
-    }
-
-    write_config(path, &doc)
-}
-
 /// Command name and arguments specified by config.
 #[derive(Clone, Debug, Eq, PartialEq, serde::Deserialize)]
 #[serde(untagged)]