cli: make "jj config edit" always open file, not directory

I don't think the new behavior is strictly better, but it's more consistent
with the other "jj config" commands so we can remove the special case for
"jj config edit".

CHANGELOG.md

@@ -24,6 +24,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   documentation](docs/config.md#dotted-style-headings-and-inline-tables) for
   details.
 
+* `jj config edit --user` now opens a file even if `$JJ_CONFIG` points to a
+  directory. If there are multiple config files, the command will fail.
+
 * The deprecated `[alias]` config section is no longer respected. Move command
   aliases to the `[aliases]` section.
 

cli/src/commands/config/edit.rs

@@ -36,6 +36,9 @@ pub fn cmd_config_edit(
     command: &CommandHelper,
     args: &ConfigEditArgs,
 ) -> Result<(), CommandError> {
-    let config_path = args.level.new_config_file_path(command.config_env())?;
-    run_ui_editor(command.settings(), config_path)
+    let file = args.level.edit_config_file(command)?;
+    if !file.path().exists() {
+        file.save()?;
+    }
+    run_ui_editor(command.settings(), file.path())
 }

cli/src/commands/config/mod.rs

@@ -67,25 +67,6 @@ impl ConfigLevelArgs {
         }
     }
 
-    fn new_config_file_path<'a>(
-        &self,
-        config_env: &'a ConfigEnv,
-    ) -> Result<&'a Path, CommandError> {
-        if self.user {
-            // TODO(#531): Special-case for editors that can't handle viewing
-            // directories?
-            config_env
-                .new_user_config_path()?
-                .ok_or_else(|| user_error("No user config path found to edit"))
-        } else if self.repo {
-            config_env
-                .repo_config_path()
-                .ok_or_else(|| user_error("No repo config path found to edit"))
-        } else {
-            panic!("No config_level provided")
-        }
-    }
-
     fn config_path<'a>(&self, config_env: &'a ConfigEnv) -> Result<&'a Path, CommandError> {
         if self.user {
             config_env

cli/src/config.rs

@@ -52,8 +52,6 @@ pub fn parse_toml_value_or_bare_string(value_str: &str) -> toml_edit::Value {
 pub enum ConfigEnvError {
     #[error("Both {0} and {1} exist. Please consolidate your configs in one of them.")]
     AmbiguousSource(PathBuf, PathBuf),
-    #[error(transparent)]
-    CreateFile(std::io::Error),
 }
 
 /// Configuration variable with its source information.
@@ -164,18 +162,6 @@ fn create_dir_all(path: &Path) -> std::io::Result<()> {
     dir.create(path)
 }
 
-fn create_config_file(path: &Path) -> std::io::Result<std::fs::File> {
-    if let Some(parent) = path.parent() {
-        create_dir_all(parent)?;
-    }
-    // TODO: Use File::create_new once stabilized.
-    std::fs::OpenOptions::new()
-        .read(true)
-        .write(true)
-        .create_new(true)
-        .open(path)
-}
-
 // The struct exists so that we can mock certain global values in unit tests.
 #[derive(Clone, Default, Debug)]
 struct UnresolvedConfigEnv {
@@ -246,27 +232,6 @@ impl ConfigEnv {
         }
     }
 
-    /// Returns a path to the user-specific config file.
-    ///
-    /// If no config file is found, tries to guess a reasonable new location for
-    /// it. If a path to a new config file is returned, the parent directory may
-    /// be created as a result of this call.
-    pub fn new_user_config_path(&self) -> Result<Option<&Path>, ConfigEnvError> {
-        match &self.user_config_path {
-            ConfigPath::Existing(path) => Ok(Some(path)),
-            ConfigPath::New(path) => {
-                // TODO: Maybe we shouldn't create new file here. Not all
-                // callers need an empty file. "jj config set" doesn't have
-                // to create an empty file to be overwritten. Since it's unclear
-                // who and when to update ConfigPath::New(_) to ::Existing(_),
-                // it's probably better to not cache the path existence.
-                create_config_file(path).map_err(ConfigEnvError::CreateFile)?;
-                Ok(Some(path))
-            }
-            ConfigPath::Unavailable => Ok(None),
-        }
-    }
-
     /// Returns user configuration files for modification. Instantiates one if
     /// `config` has no user configuration layers.
     ///
@@ -1116,19 +1081,10 @@ mod tests {
             let env = self
                 .resolve(tmp.path())
                 .map_err(|e| anyhow!("new_config_path: {e}"))?;
-            let got = env
-                .new_user_config_path()
-                .map_err(|e| anyhow!("new_config_path: {e}"))?;
+            let got = env.user_config_path();
             if got != want.as_deref() {
                 return Err(anyhow!("new_config_path: got {got:?}, want {want:?}"));
             }
-            if let Some(path) = got {
-                if !Path::new(&path).is_file() {
-                    return Err(anyhow!(
-                        "new_config_path returned {path:?} which is not a file"
-                    ));
-                }
-            }
             Ok(())
         }
     }

cli/tests/test_config_command.rs

@@ -784,32 +784,52 @@ fn test_config_edit_user() {
     let mut test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
     let repo_path = test_env.env_root().join("repo");
+    // Remove one of the config file to disambiguate
+    std::fs::remove_file(test_env.last_config_file_path()).unwrap();
     let edit_script = test_env.set_up_fake_editor();
 
     std::fs::write(edit_script, "dump-path path").unwrap();
     test_env.jj_cmd_ok(&repo_path, &["config", "edit", "--user"]);
 
     let edited_path =
         PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap());
-    assert_eq!(edited_path, dunce::simplified(test_env.config_path()));
+    assert_eq!(
+        edited_path,
+        dunce::simplified(&test_env.last_config_file_path())
+    );
+}
+
+#[test]
+fn test_config_edit_user_new_file() {
+    let mut test_env = TestEnvironment::default();
+    let user_config_path = test_env.config_path().join("config").join("file.toml");
+    test_env.set_up_fake_editor(); // set $EDITOR, but added configuration is ignored
+    test_env.set_config_path(user_config_path.clone());
+    assert!(!user_config_path.exists());
+
+    test_env.jj_cmd_ok(test_env.env_root(), &["config", "edit", "--user"]);
+    assert!(
+        user_config_path.exists(),
+        "new file and directory should be created"
+    );
 }
 
 #[test]
 fn test_config_edit_repo() {
     let mut test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
     let repo_path = test_env.env_root().join("repo");
+    let repo_config_path = repo_path.join(PathBuf::from_iter([".jj", "repo", "config.toml"]));
     let edit_script = test_env.set_up_fake_editor();
+    assert!(!repo_config_path.exists());
 
     std::fs::write(edit_script, "dump-path path").unwrap();
     test_env.jj_cmd_ok(&repo_path, &["config", "edit", "--repo"]);
 
     let edited_path =
         PathBuf::from(std::fs::read_to_string(test_env.env_root().join("path")).unwrap());
-    assert_eq!(
-        edited_path,
-        dunce::simplified(&repo_path.join(".jj/repo/config.toml"))
-    );
+    assert_eq!(edited_path, dunce::simplified(&repo_config_path));
+    assert!(repo_config_path.exists(), "new file should be created");
 }
 
 #[test]