cli: let "config set"/"unset" continue if file to edit can be disambi…

…guated

If the user config path was an empty directory, these commands would fail with
EISDIR instead of "can't set config in path (dirs not supported)". I think this
can be changed later to create new "$dir/config.toml" file. Maybe we can also
change the default path to ~/.config/jj directory, and load all *.toml files
from there?

cli/src/commands/config/mod.rs

@@ -21,6 +21,7 @@ mod unset;
 
 use std::path::Path;
 
+use itertools::Itertools as _;
 use jj_lib::config::ConfigFile;
 use jj_lib::config::ConfigSource;
 use tracing::instrument;
@@ -99,16 +100,35 @@ impl ConfigLevelArgs {
         }
     }
 
-    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()
-            )));
+    fn edit_config_file(&self, command: &CommandHelper) -> Result<ConfigFile, CommandError> {
+        let config_env = command.config_env();
+        let config = command.settings().config();
+        let pick_one = |mut files: Vec<ConfigFile>, not_found_error: &str| {
+            if files.len() > 1 {
+                // TODO: prompt or pick the last?
+                return Err(user_error(format!(
+                    "Cannot determine config file to edit:\n{}",
+                    files
+                        .iter()
+                        .map(|file| format!("  {}", file.path().display()))
+                        .join("\n")
+                )));
+            }
+            files.pop().ok_or_else(|| user_error(not_found_error))
+        };
+        if self.user {
+            pick_one(
+                config_env.user_config_files(config)?,
+                "No user config path found to edit",
+            )
+        } else if self.repo {
+            pick_one(
+                config_env.repo_config_files(config)?,
+                "No repo config path found to edit",
+            )
+        } else {
+            panic!("No config_level provided")
         }
-        let source = self.get_source_kind().unwrap();
-        Ok(ConfigFile::load_or_empty(source, path)?)
     }
 }
 

cli/src/commands/config/set.rs

@@ -52,7 +52,7 @@ pub fn cmd_config_set(
     command: &CommandHelper,
     args: &ConfigSetArgs,
 ) -> Result<(), CommandError> {
-    let mut file = args.level.edit_config_file(command.config_env())?;
+    let mut file = args.level.edit_config_file(command)?;
 
     // TODO(#531): Infer types based on schema (w/ --type arg to override).
     let value = parse_toml_value_or_bare_string(&args.value);

cli/src/commands/config/unset.rs

@@ -39,7 +39,7 @@ pub fn cmd_config_unset(
     command: &CommandHelper,
     args: &ConfigUnsetArgs,
 ) -> Result<(), CommandError> {
-    let mut file = args.level.edit_config_file(command.config_env())?;
+    let mut file = args.level.edit_config_file(command)?;
     let old_value = file
         .delete_value(&args.name)
         .map_err(|err| user_error_with_message(format!("Failed to unset {}", args.name), err))?;

cli/src/config.rs

@@ -22,6 +22,7 @@ use std::path::PathBuf;
 use std::process::Command;
 
 use itertools::Itertools;
+use jj_lib::config::ConfigFile;
 use jj_lib::config::ConfigLayer;
 use jj_lib::config::ConfigLoadError;
 use jj_lib::config::ConfigNamePathBuf;
@@ -266,6 +267,34 @@ impl ConfigEnv {
         }
     }
 
+    /// Returns user configuration files for modification. Instantiates one if
+    /// `config` has no user configuration layers.
+    ///
+    /// The parent directory for the new file may be created by this function.
+    /// If the user configuration path is unknown, this function returns an
+    /// empty `Vec`.
+    pub fn user_config_files(
+        &self,
+        config: &StackedConfig,
+    ) -> Result<Vec<ConfigFile>, ConfigLoadError> {
+        config_files_for(config, ConfigSource::User, || self.new_user_config_file())
+    }
+
+    fn new_user_config_file(&self) -> Result<Option<ConfigFile>, ConfigLoadError> {
+        self.user_config_path()
+            .map(|path| {
+                // No need to propagate io::Error here. If the directory
+                // couldn't be created, file.save() would fail later.
+                if let Some(dir) = path.parent() {
+                    create_dir_all(dir).ok();
+                }
+                // The path doesn't usually exist, but we shouldn't overwrite it
+                // with an empty config if it did exist.
+                ConfigFile::load_or_empty(ConfigSource::User, path)
+            })
+            .transpose()
+    }
+
     /// Loads user-specific config files into the given `config`. The old
     /// user-config layers will be replaced if any.
     #[instrument]
@@ -300,6 +329,27 @@ impl ConfigEnv {
         }
     }
 
+    /// Returns repo configuration files for modification. Instantiates one if
+    /// `config` has no repo configuration layers.
+    ///
+    /// If the repo path is unknown, this function returns an empty `Vec`. Since
+    /// the repo config path cannot be a directory, the returned `Vec` should
+    /// have at most one config file.
+    pub fn repo_config_files(
+        &self,
+        config: &StackedConfig,
+    ) -> Result<Vec<ConfigFile>, ConfigLoadError> {
+        config_files_for(config, ConfigSource::Repo, || self.new_repo_config_file())
+    }
+
+    fn new_repo_config_file(&self) -> Result<Option<ConfigFile>, ConfigLoadError> {
+        self.repo_config_path()
+            // The path doesn't usually exist, but we shouldn't overwrite it
+            // with an empty config if it did exist.
+            .map(|path| ConfigFile::load_or_empty(ConfigSource::Repo, path))
+            .transpose()
+    }
+
     /// Loads repo-specific config file into the given `config`. The old
     /// repo-config layer will be replaced if any.
     #[instrument]
@@ -312,6 +362,22 @@ impl ConfigEnv {
     }
 }
 
+fn config_files_for(
+    config: &StackedConfig,
+    source: ConfigSource,
+    new_file: impl FnOnce() -> Result<Option<ConfigFile>, ConfigLoadError>,
+) -> Result<Vec<ConfigFile>, ConfigLoadError> {
+    let mut files = config
+        .layers_for(source)
+        .iter()
+        .filter_map(|layer| ConfigFile::from_layer(layer.clone()).ok())
+        .collect_vec();
+    if files.is_empty() {
+        files.extend(new_file()?);
+    }
+    Ok(files)
+}
+
 /// Initializes stacked config with the given `default_layers` and infallible
 /// sources.
 ///

cli/tests/common/mod.rs

@@ -248,6 +248,12 @@ impl TestEnvironment {
         &self.config_path
     }
 
+    pub fn last_config_file_path(&self) -> PathBuf {
+        let config_file_number = self.config_file_number.borrow();
+        self.config_path
+            .join(format!("config{config_file_number:04}.toml"))
+    }
+
     pub fn set_config_path(&mut self, config_path: PathBuf) {
         self.config_path = config_path;
     }

cli/tests/test_config_command.rs

@@ -201,6 +201,7 @@ bar
 fn test_config_list_layer() {
     let mut test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
+    // Test with fresh new config file
     let user_config_path = test_env.config_path().join("config.toml");
     test_env.set_config_path(user_config_path.clone());
     let repo_path = test_env.env_root().join("repo");
@@ -473,7 +474,7 @@ fn test_config_set_bad_opts() {
 fn test_config_set_for_user() {
     let mut test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
-    // Point to a config file since `config set` can't handle directories.
+    // Test with fresh new config file
     let user_config_path = test_env.config_path().join("config.toml");
     test_env.set_config_path(user_config_path.clone());
     let repo_path = test_env.env_root().join("repo");
@@ -503,6 +504,36 @@ fn test_config_set_for_user() {
     "###);
 }
 
+#[test]
+fn test_config_set_for_user_directory() {
+    let test_env = TestEnvironment::default();
+
+    test_env.jj_cmd_ok(
+        test_env.env_root(),
+        &["config", "set", "--user", "test-key", "test-val"],
+    );
+    insta::assert_snapshot!(
+        std::fs::read_to_string(test_env.last_config_file_path()).unwrap(),
+        @r#"
+    test-key = "test-val"
+
+    [template-aliases]
+    'format_time_range(time_range)' = 'time_range.start() ++ " - " ++ time_range.end()'
+    "#);
+
+    // Add one more config file to the directory
+    test_env.add_config("");
+    let stderr = test_env.jj_cmd_failure(
+        test_env.env_root(),
+        &["config", "set", "--user", "test-key", "test-val"],
+    );
+    insta::assert_snapshot!(stderr, @r"
+    Error: Cannot determine config file to edit:
+      $TEST_ENV/config/config0001.toml
+      $TEST_ENV/config/config0002.toml
+    ");
+}
+
 #[test]
 fn test_config_set_for_repo() {
     let test_env = TestEnvironment::default();
@@ -537,6 +568,7 @@ fn test_config_set_for_repo() {
 fn test_config_set_toml_types() {
     let mut test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
+    // Test with fresh new config file
     let user_config_path = test_env.config_path().join("config.toml");
     test_env.set_config_path(user_config_path.clone());
     let repo_path = test_env.env_root().join("repo");
@@ -563,10 +595,8 @@ fn test_config_set_toml_types() {
 
 #[test]
 fn test_config_set_type_mismatch() {
-    let mut test_env = TestEnvironment::default();
+    let test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
-    let user_config_path = test_env.config_path().join("config.toml");
-    test_env.set_config_path(user_config_path);
     let repo_path = test_env.env_root().join("repo");
 
     test_env.jj_cmd_ok(
@@ -603,10 +633,8 @@ fn test_config_set_type_mismatch() {
 
 #[test]
 fn test_config_set_nontable_parent() {
-    let mut test_env = TestEnvironment::default();
+    let test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
-    let user_config_path = test_env.config_path().join("config.toml");
-    test_env.set_config_path(user_config_path);
     let repo_path = test_env.env_root().join("repo");
 
     test_env.jj_cmd_ok(
@@ -625,11 +653,8 @@ fn test_config_set_nontable_parent() {
 
 #[test]
 fn test_config_unset_non_existent_key() {
-    let mut test_env = TestEnvironment::default();
+    let test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
-    // Point to a config file since `config set` can't handle directories.
-    let user_config_path = test_env.config_path().join("config.toml");
-    test_env.set_config_path(user_config_path);
     let repo_path = test_env.env_root().join("repo");
 
     let stderr = test_env.jj_cmd_failure(&repo_path, &["config", "unset", "--user", "nonexistent"]);
@@ -638,11 +663,8 @@ fn test_config_unset_non_existent_key() {
 
 #[test]
 fn test_config_unset_inline_table_key() {
-    let mut test_env = TestEnvironment::default();
+    let test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
-    // Point to a config file since `config set` can't handle directories.
-    let user_config_path = test_env.config_path().join("config.toml");
-    test_env.set_config_path(user_config_path.clone());
     let repo_path = test_env.env_root().join("repo");
 
     test_env.jj_cmd_ok(
@@ -660,7 +682,7 @@ fn test_config_unset_inline_table_key() {
 #[test]
 fn test_config_unset_table_like() {
     let mut test_env = TestEnvironment::default();
-    // Point to a config file since `config unset` can't handle directories.
+    // Test with fresh new config file
     let user_config_path = test_env.config_path().join("config.toml");
     test_env.set_config_path(user_config_path.clone());
 
@@ -700,7 +722,7 @@ fn test_config_unset_table_like() {
 fn test_config_unset_for_user() {
     let mut test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
-    // Point to a config file since `config set` can't handle directories.
+    // Test with fresh new config file
     let user_config_path = test_env.config_path().join("config.toml");
     test_env.set_config_path(user_config_path.clone());
     let repo_path = test_env.env_root().join("repo");
@@ -991,10 +1013,8 @@ fn test_config_path_syntax() {
 
 #[test]
 fn test_config_show_paths() {
-    let mut test_env = TestEnvironment::default();
+    let test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
-    let user_config_path = test_env.config_path().join("config.toml");
-    test_env.set_config_path(user_config_path.clone());
     let repo_path = test_env.env_root().join("repo");
 
     test_env.jj_cmd_ok(
@@ -1006,7 +1026,7 @@ fn test_config_show_paths() {
     Config error: Invalid type or value for ui.paginate
     Caused by: unknown variant `:builtin`, expected `never` or `auto`
 
-    Hint: Check the config file: $TEST_ENV/config/config.toml
+    Hint: Check the config file: $TEST_ENV/config/config0001.toml
     For help, see https://martinvonz.github.io/jj/latest/config/.
     ");
 }
@@ -1039,9 +1059,7 @@ fn test_config_author_change_warning() {
 
 #[test]
 fn test_config_author_change_warning_root_env() {
-    let mut test_env = TestEnvironment::default();
-    let user_config_path = test_env.config_path().join("config.toml");
-    test_env.set_config_path(user_config_path);
+    let test_env = TestEnvironment::default();
     test_env.jj_cmd_success(
         test_env.env_root(),
         &["config", "set", "--user", "user.email", "'Foo'"],

lib/src/config.rs

@@ -503,6 +503,17 @@ impl ConfigFile {
         Ok(ConfigFile { layer })
     }
 
+    /// Wraps file-based [`ConfigLayer`] for modification. Returns `Err(layer)`
+    /// if the source `path` is unknown.
+    #[allow(clippy::result_large_err)] // Ok-variant is as large as Err anyway
+    pub fn from_layer(layer: ConfigLayer) -> Result<Self, ConfigLayer> {
+        if layer.path.is_some() {
+            Ok(ConfigFile { layer })
+        } else {
+            Err(layer)
+        }
+    }
+
     /// Writes serialized data to the source file.
     pub fn save(&self) -> Result<(), ConfigFileSaveError> {
         fs::write(self.path(), self.layer.data.to_string())
@@ -656,6 +667,11 @@ impl StackedConfig {
         &self.layers
     }
 
+    /// Layers of the specified `source`.
+    pub fn layers_for(&self, source: ConfigSource) -> &[ConfigLayer] {
+        &self.layers[self.layer_range(source)]
+    }
+
     /// Looks up value of the specified type `T` from all layers, merges sub
     /// fields as needed.
     pub fn get<'de, T: Deserialize<'de>>(