cli: print errors to stderr

CHANGELOG.md

@@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * The new `jj print` command prints the contents of a file in a revision.
 
+### Fixed bugs
+
+* Errors are now printed to stderr (they used to be printed to stdout).
+
 ## [0.4.0] - 2022-04-02
 
 ### Breaking changes

src/ui.rs

@@ -30,7 +30,8 @@ use crate::templater::TemplateFormatter;
 pub struct Ui<'a> {
     cwd: PathBuf,
     color: bool,
-    formatter: Mutex<Box<dyn Formatter + 'a>>,
+    stdout_formatter: Mutex<Box<dyn Formatter + 'a>>,
+    stderr_formatter: Mutex<Box<dyn Formatter + 'a>>,
     settings: UserSettings,
 }
 
@@ -65,14 +66,17 @@ impl<'stdout> Ui<'stdout> {
     pub fn new(
         cwd: PathBuf,
         stdout: Box<dyn Write + 'stdout>,
+        stderr: Box<dyn Write + 'stdout>,
         color: bool,
         settings: UserSettings,
     ) -> Ui<'stdout> {
-        let formatter = Mutex::new(new_formatter(&settings, color, stdout));
+        let stdout_formatter = Mutex::new(new_formatter(&settings, color, stdout));
+        let stderr_formatter = Mutex::new(new_formatter(&settings, color, stderr));
         Ui {
             cwd,
             color,
-            formatter,
+            stdout_formatter,
+            stderr_formatter,
             settings,
         }
     }
@@ -80,8 +84,9 @@ impl<'stdout> Ui<'stdout> {
     pub fn for_terminal(settings: UserSettings) -> Ui<'static> {
         let cwd = std::env::current_dir().unwrap();
         let stdout: Box<dyn Write + 'static> = Box::new(io::stdout());
+        let stderr: Box<dyn Write + 'static> = Box::new(io::stderr());
         let color = use_color(&settings);
-        Ui::new(cwd, stdout, color, settings)
+        Ui::new(cwd, stdout, stderr, color, settings)
     }
 
     pub fn cwd(&self) -> &Path {
@@ -100,7 +105,11 @@ impl<'stdout> Ui<'stdout> {
     }
 
     pub fn stdout_formatter(&self) -> MutexGuard<Box<dyn Formatter + 'stdout>> {
-        self.formatter.lock().unwrap()
+        self.stdout_formatter.lock().unwrap()
+    }
+
+    pub fn stderr_formatter(&self) -> MutexGuard<Box<dyn Formatter + 'stdout>> {
+        self.stderr_formatter.lock().unwrap()
     }
 
     pub fn write(&mut self, text: &str) -> io::Result<()> {
@@ -112,8 +121,7 @@ impl<'stdout> Ui<'stdout> {
     }
 
     pub fn write_error(&mut self, text: &str) -> io::Result<()> {
-        // TODO: We should print the error to stderr
-        let mut formatter = self.stdout_formatter();
+        let mut formatter = self.stderr_formatter();
         formatter.add_label(String::from("error"))?;
         formatter.write_str(text)?;
         formatter.remove_label()?;

tests/common/mod.rs

@@ -77,11 +77,9 @@ impl TestEnvironment {
     }
 
     /// Run a `jj` command, check that it was successful, and return its stdout
-    // TODO: We should return the stderr instead (or maybe in addition), once we've
-    // fixed errors to go to stderr
     pub fn jj_cmd_failure(&self, current_dir: &Path, args: &[&str]) -> String {
-        let assert = self.jj_cmd(current_dir, args).assert().failure().stderr("");
-        get_stdout_string(&assert)
+        let assert = self.jj_cmd(current_dir, args).assert().failure().stdout("");
+        get_stderr_string(&assert)
     }
 
     pub fn env_root(&self) -> &Path {
@@ -136,3 +134,7 @@ impl TestEnvironment {
 pub fn get_stdout_string(assert: &assert_cmd::assert::Assert) -> String {
     String::from_utf8(assert.get_output().stdout.clone()).unwrap()
 }
+
+pub fn get_stderr_string(assert: &assert_cmd::assert::Assert) -> String {
+    String::from_utf8(assert.get_output().stderr.clone()).unwrap()
+}

tests/test_edit_command.rs

@@ -48,8 +48,8 @@ fn test_edit() {
 
     // Nothing happens if the diff-editor exits with an error
     std::fs::write(&edit_script, "rm file2\0fail").unwrap();
-    let stdout = test_env.jj_cmd_failure(&repo_path, &["edit"]);
-    insta::assert_snapshot!(stdout, @"Error: Failed to edit diff: The diff tool exited with a non-zero code
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["edit"]);
+    insta::assert_snapshot!(stderr, @"Error: Failed to edit diff: The diff tool exited with a non-zero code
 ");
     let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
     insta::assert_snapshot!(stdout, @r###"

tests/test_git_push.rs

@@ -43,9 +43,9 @@ fn test_git_push() {
     "###);
 
     // When pushing a specific branch, won't push it if it points to an open commit
-    let stdout =
+    let stderr =
         test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--branch", "my-branch"]);
-    insta::assert_snapshot!(stdout, @"Error: Won't push open commit
+    insta::assert_snapshot!(stderr, @"Error: Won't push open commit
 ");
 
     // Try pushing a conflict
@@ -57,7 +57,7 @@ fn test_git_push() {
     test_env.jj_cmd_success(&workspace_root, &["rebase", "-d", "@--"]);
     test_env.jj_cmd_success(&workspace_root, &["branch", "my-branch"]);
     test_env.jj_cmd_success(&workspace_root, &["close", "-m", "third"]);
-    let stdout = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
-    insta::assert_snapshot!(stdout, @"Error: Won't push commit 28b5642cb786 since it has conflicts
+    let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push"]);
+    insta::assert_snapshot!(stderr, @"Error: Won't push commit 28b5642cb786 since it has conflicts
 ");
 }

tests/test_global_opts.rs

@@ -50,16 +50,16 @@ fn test_no_commit_working_copy() {
 #[test]
 fn test_repo_arg_with_init() {
     let test_env = TestEnvironment::default();
-    let stdout = test_env.jj_cmd_failure(test_env.env_root(), &["init", "-R=.", "repo"]);
-    insta::assert_snapshot!(stdout, @"Error: '--repository' cannot be used with 'init'
+    let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["init", "-R=.", "repo"]);
+    insta::assert_snapshot!(stderr, @"Error: '--repository' cannot be used with 'init'
 ");
 }
 
 #[test]
 fn test_repo_arg_with_git_clone() {
     let test_env = TestEnvironment::default();
-    let stdout = test_env.jj_cmd_failure(test_env.env_root(), &["git", "clone", "-R=.", "remote"]);
-    insta::assert_snapshot!(stdout, @"Error: '--repository' cannot be used with 'git clone'
+    let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["git", "clone", "-R=.", "remote"]);
+    insta::assert_snapshot!(stderr, @"Error: '--repository' cannot be used with 'git clone'
 ");
 }
 
@@ -100,8 +100,8 @@ fn test_invalid_config() {
         "[section]key = value-missing-quotes",
     )
     .unwrap();
-    let stdout = test_env.jj_cmd_failure(test_env.env_root(), &["init", "repo"]);
-    insta::assert_snapshot!(stdout, @"Invalid config: expected newline, found an identifier at line 1 column 10 in config.toml
+    let stderr = test_env.jj_cmd_failure(test_env.env_root(), &["init", "repo"]);
+    insta::assert_snapshot!(stderr, @"Invalid config: expected newline, found an identifier at line 1 column 10 in config.toml
 ");
 }
 

tests/test_move_command.rs

@@ -70,8 +70,8 @@ fn test_move() {
     // Doesn't do anything without arguments
     // TODO: We should make this error more helpful (saying that --from and/or --to
     // are required)
-    let stdout = test_env.jj_cmd_failure(&repo_path, &["move"]);
-    insta::assert_snapshot!(stdout, @"Error: Source and destination cannot be the same.
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["move"]);
+    insta::assert_snapshot!(stderr, @"Error: Source and destination cannot be the same.
 ");
     let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-T", template]);
     insta::assert_snapshot!(stdout, @r###"

tests/test_print_command.rs

@@ -42,11 +42,11 @@ fn test_print() {
     let stdout = test_env.jj_cmd_success(&repo_path, &["print", subdir_file]);
     insta::assert_snapshot!(stdout, @"c
 ");
-    let stdout = test_env.jj_cmd_failure(&repo_path, &["print", "non-existent"]);
-    insta::assert_snapshot!(stdout, @"Error: No such path
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["print", "non-existent"]);
+    insta::assert_snapshot!(stderr, @"Error: No such path
 ");
-    let stdout = test_env.jj_cmd_failure(&repo_path, &["print", "dir"]);
-    insta::assert_snapshot!(stdout, @"Error: Path exists but is not a file
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["print", "dir"]);
+    insta::assert_snapshot!(stderr, @"Error: Path exists but is not a file
 ");
 
     // Can print a conflict

tests/test_restore_command.rs

@@ -114,8 +114,8 @@ fn test_restore_interactive() {
 
     // Nothing happens if the diff-editor exits with an error
     std::fs::write(&edit_script, "rm file2\0fail").unwrap();
-    let stdout = test_env.jj_cmd_failure(&repo_path, &["restore", "-i"]);
-    insta::assert_snapshot!(stdout, @"Error: Failed to edit diff: The diff tool exited with a non-zero code
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["restore", "-i"]);
+    insta::assert_snapshot!(stderr, @"Error: Failed to edit diff: The diff tool exited with a non-zero code
 ");
     let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]);
     insta::assert_snapshot!(stdout, @r###"

tests/test_squash_command.rs

@@ -94,8 +94,8 @@ fn test_squash() {
     o 90aeefd03044 a
     o 000000000000 
     "###);
-    let stdout = test_env.jj_cmd_failure(&repo_path, &["squash", "-r", "e"]);
-    insta::assert_snapshot!(stdout, @"Error: Cannot squash merge commits
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["squash", "-r", "e"]);
+    insta::assert_snapshot!(stderr, @"Error: Cannot squash merge commits
 ");
 
     // Can squash into a merge commit

tests/test_ui.rs

@@ -24,8 +24,16 @@ fn test_parse_file_path_wc_in_cwd() {
     let cwd_path = temp_dir.path().join("repo");
     let wc_path = cwd_path.clone();
     let mut unused_stdout_buf = vec![];
+    let mut unused_stderr_buf = vec![];
     let unused_stdout = Box::new(Cursor::new(&mut unused_stdout_buf));
-    let ui = Ui::new(cwd_path, unused_stdout, false, user_settings());
+    let unused_stderr = Box::new(Cursor::new(&mut unused_stderr_buf));
+    let ui = Ui::new(
+        cwd_path,
+        unused_stdout,
+        unused_stderr,
+        false,
+        user_settings(),
+    );
 
     assert_eq!(ui.parse_file_path(&wc_path, ""), Ok(RepoPath::root()));
     assert_eq!(ui.parse_file_path(&wc_path, "."), Ok(RepoPath::root()));
@@ -58,8 +66,16 @@ fn test_parse_file_path_wc_in_cwd_parent() {
     let cwd_path = temp_dir.path().join("dir");
     let wc_path = cwd_path.parent().unwrap().to_path_buf();
     let mut unused_stdout_buf = vec![];
+    let mut unused_stderr_buf = vec![];
     let unused_stdout = Box::new(Cursor::new(&mut unused_stdout_buf));
-    let ui = Ui::new(cwd_path, unused_stdout, false, user_settings());
+    let unused_stderr = Box::new(Cursor::new(&mut unused_stderr_buf));
+    let ui = Ui::new(
+        cwd_path,
+        unused_stdout,
+        unused_stderr,
+        false,
+        user_settings(),
+    );
 
     assert_eq!(
         ui.parse_file_path(&wc_path, ""),
@@ -94,8 +110,16 @@ fn test_parse_file_path_wc_in_cwd_child() {
     let cwd_path = temp_dir.path().join("cwd");
     let wc_path = cwd_path.join("repo");
     let mut unused_stdout_buf = vec![];
+    let mut unused_stderr_buf = vec![];
     let unused_stdout = Box::new(Cursor::new(&mut unused_stdout_buf));
-    let ui = Ui::new(cwd_path, unused_stdout, false, user_settings());
+    let unused_stderr = Box::new(Cursor::new(&mut unused_stderr_buf));
+    let ui = Ui::new(
+        cwd_path,
+        unused_stdout,
+        unused_stderr,
+        false,
+        user_settings(),
+    );
 
     assert_eq!(
         ui.parse_file_path(&wc_path, ""),

tests/test_untrack_command.rs

@@ -39,8 +39,8 @@ fn test_untrack() {
     let files_before = test_env.jj_cmd_success(&repo_path, &["files"]);
 
     // Errors out when a specified file is not ignored
-    let stdout = test_env.jj_cmd_failure(&repo_path, &["untrack", "file1", "file1.bak"]);
-    insta::assert_snapshot!(stdout, @"Error: 'file1' would be added back because it's not ignored. Make sure it's ignored, \
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["untrack", "file1", "file1.bak"]);
+    insta::assert_snapshot!(stderr, @"Error: 'file1' would be added back because it's not ignored. Make sure it's ignored, \
          then try again.\n");
     let files_after = test_env.jj_cmd_success(&repo_path, &["files"]);
     // There should be no changes to the state when there was an error
@@ -60,9 +60,9 @@ fn test_untrack() {
     assert!(repo_path.join("file2.bak").exists());
 
     // Errors out when multiple specified files are not ignored
-    let stdout = test_env.jj_cmd_failure(&repo_path, &["untrack", "target"]);
+    let stderr = test_env.jj_cmd_failure(&repo_path, &["untrack", "target"]);
     assert_eq!(
-        stdout,
+        stderr,
         format!(
             "Error: '{}' and 1 other files would be added back because they're not ignored. Make \
              sure they're ignored, then try again.\n",