cli: don't crash if $EDITOR fails

CHANGELOG.md

@@ -26,6 +26,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * `jj untrack` now requires at least one path (allowing no arguments was a UX
   bug).
 
+* You now get a proper error message instead of a crash when `$EDITOR` doesn't
+  exist or exits with an error.
+
 ## [0.4.0] - 2022-04-02
 
 ### Breaking changes

src/commands.rs

@@ -2951,7 +2951,7 @@ fn cmd_obslog(ui: &mut Ui, command: &CommandHelper, args: &ObslogArgs) -> Result
     Ok(())
 }
 
-fn edit_description(repo: &ReadonlyRepo, description: &str) -> String {
+fn edit_description(repo: &ReadonlyRepo, description: &str) -> Result<String, CommandError> {
     let random: u32 = rand::random();
     let description_file_path = repo.repo_path().join(format!("description-{}", random));
     {
@@ -2975,8 +2975,12 @@ fn edit_description(repo: &ReadonlyRepo, description: &str) -> String {
         .args(editor_args)
         .arg(&description_file_path)
         .status()
-        .expect("failed to run editor");
+        .map_err(|_| CommandError::UserError(format!("Failed to run editor '{editor}'")))?;
-    assert!(exit_status.success(), "failed to run editor");
+    if !exit_status.success() {
+        return Err(CommandError::UserError(format!(
+            "Editor '{editor}' exited with an error"
+        )));
+    }
 
     let mut description_file = OpenOptions::new()
         .read(true)
@@ -2996,7 +3000,7 @@ fn edit_description(repo: &ReadonlyRepo, description: &str) -> String {
     while matches!(lines.last(), Some(&"\n") | Some(&"\r\n")) {
         lines.pop().unwrap();
     }
-    lines.join("")
+    Ok(lines.join(""))
 }
 
 fn cmd_describe(
@@ -3016,7 +3020,7 @@ fn cmd_describe(
     } else if let Some(message) = &args.message {
         description = message.to_owned()
     } else {
-        description = edit_description(repo, commit.description());
+        description = edit_description(repo, commit.description())?;
     }
     if description == *commit.description() {
         ui.write("Nothing changed.\n")?;
@@ -3054,9 +3058,9 @@ fn cmd_close(ui: &mut Ui, command: &CommandHelper, args: &CloseArgs) -> Result<(
     let description = if let Some(message) = &args.message {
         message.to_string()
     } else if commit.description().is_empty() {
-        edit_description(repo, "\n\nJJ: Enter commit description.\n")
+        edit_description(repo, "\n\nJJ: Enter commit description.\n")?
     } else if args.edit {
-        edit_description(repo, commit.description())
+        edit_description(repo, commit.description())?
     } else {
         commit.description().to_string()
     };
@@ -3496,7 +3500,7 @@ any changes, then the operation will be aborted.
             repo,
             &("JJ: Enter commit description for the first part.\n".to_string()
                 + commit.description()),
-        );
+        )?;
         let first_commit = CommitBuilder::for_rewrite_from(ui.settings(), repo.store(), &commit)
             .set_tree(tree_id)
             .set_description(first_description)
@@ -3505,7 +3509,7 @@ any changes, then the operation will be aborted.
             repo,
             &("JJ: Enter commit description for the second part.\n".to_string()
                 + commit.description()),
-        );
+        )?;
         let second_commit = CommitBuilder::for_rewrite_from(ui.settings(), repo.store(), &commit)
             .set_parents(vec![first_commit.id().clone()])
             .set_tree(commit.tree().id().clone())
@@ -3566,7 +3570,7 @@ fn cmd_merge(ui: &mut Ui, command: &CommandHelper, args: &MergeArgs) -> Result<(
         edit_description(
             repo,
             "\n\nJJ: Enter commit description for the merge commit.\n",
-        )
+        )?
     };
     let merged_tree = merge_commit_trees(repo.as_repo_ref(), &commits);
     let mut tx = workspace_command.start_transaction("merge commits");

tests/test_describe_command.rs

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use crate::common::TestEnvironment;
+use crate::common::{get_stderr_string, TestEnvironment};
 
 pub mod common;
 
@@ -42,8 +42,26 @@ fn test_describe() {
 ");
 
     // Lines in editor starting with "JJ: " are ignored
-    std::fs::write(&edit_script, "write\nJJ: ignored\ndescription among comment\nJJ: ignored").unwrap();
+    std::fs::write(
+        &edit_script,
+        "write\nJJ: ignored\ndescription among comment\nJJ: ignored",
+    )
+    .unwrap();
     let stdout = test_env.jj_cmd_success(&repo_path, &["describe"]);
     insta::assert_snapshot!(stdout, @"Working copy now at: 95664f6316ae description among comment
 ");
+
+    // Fails if the editor fails
+    std::fs::write(&edit_script, "fail").unwrap();
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["describe"]);
+    assert!(stderr.contains("exited with an error"));
+
+    // Fails if the editor doesn't exist
+    std::fs::write(&edit_script, "").unwrap();
+    let assert = test_env
+        .jj_cmd(&repo_path, &["describe"])
+        .env("EDITOR", "this-editor-does-not-exist")
+        .assert()
+        .failure();
+    assert!(get_stderr_string(&assert).contains("Failed to run"));
 }