cli: respect $VISUAL, overriding $EDITOR

With this patch, the order is this:

`$JJ_EDITOR` environement variable
`ui.editor` config
`$VISUAL` environement variable
`$EDITOR` environement variable
`pico`

That matches git, except that git falls back to an editor determined
at compile time (usually `vi`) instead of using `pico`.

CHANGELOG.md

@@ -45,9 +45,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * The `$JJ_CONFIG` environment variable can now point to a directory. If it
   does, all files in the directory will be read, in alphabetical order.
 
-* You can now override the `$EDITOR` environment variable by setting the
+* The `$VISUAL` environment is now respected and overrides `$EDITOR`. The new
-  `ui.editor` config. There is also a new `$JJ_EDITOR` environment variable,
+  `ui.editor` config has higher priority than both of them. There is also a new
-  which has even higher priority than the config.
+  `$JJ_EDITOR` environment variable, which has even higher priority than the
+  config.
 
 * You can now use `-` and `+` in revset symbols. You used to have to quote
   branch names like `my-feature` in nested quotes (outer layer for your shell)

src/config.rs

@@ -53,7 +53,9 @@ fn config_path() -> Result<Option<PathBuf>, ConfigError> {
 /// Environment variables that should be overridden by config values
 fn env_base() -> config::Config {
     let mut builder = config::Config::builder();
-    if let Ok(value) = env::var("EDITOR") {
+    if let Ok(value) = env::var("VISUAL") {
+        builder = builder.set_override("ui.editor", value).unwrap();
+    } else if let Ok(value) = env::var("EDITOR") {
         builder = builder.set_override("ui.editor", value).unwrap();
     }
     builder.build().unwrap()

tests/test_describe_command.rs

@@ -69,15 +69,23 @@ fn test_describe() {
         .failure();
     assert!(get_stderr_string(&assert).contains("Failed to run"));
 
-    // `ui.editor` config overrides `$EDITOR`
+    // `$VISUAL` overrides `$EDITOR`
-    std::fs::write(&edit_script, "").unwrap();
+    let assert = test_env
+        .jj_cmd(&repo_path, &["describe"])
+        .env("VISUAL", "bad-editor-from-visual-env")
+        .env("EDITOR", "bad-editor-from-editor-env")
+        .assert()
+        .failure();
+    assert!(get_stderr_string(&assert).contains("bad-editor-from-visual-env"));
+
+    // `ui.editor` config overrides `$VISUAL`
     test_env.add_config(
         br#"[ui]
     editor = "bad-editor-from-config""#,
     );
     let assert = test_env
         .jj_cmd(&repo_path, &["describe"])
-        .env("EDITOR", "bad-editor-from-env")
+        .env("VISUAL", "bad-editor-from-visual-env")
         .assert()
         .failure();
     assert!(get_stderr_string(&assert).contains("bad-editor-from-config"));
@@ -85,8 +93,8 @@ fn test_describe() {
     // `$JJ_EDITOR` overrides `ui.editor` config
     let assert = test_env
         .jj_cmd(&repo_path, &["describe"])
-        .env("JJ_EDITOR", "bad-jj-editor-from-env")
+        .env("JJ_EDITOR", "bad-jj-editor-from-jj-editor-env")
         .assert()
         .failure();
-    assert!(get_stderr_string(&assert).contains("bad-jj-editor-from-env"));
+    assert!(get_stderr_string(&assert).contains("bad-jj-editor-from-jj-editor-env"));
 }