From bc3c2db828177ce2090370ee01b8d9969a65eded Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 6 Apr 2022 23:25:01 -0700 Subject: [PATCH] cli: print errors to stderr --- CHANGELOG.md | 4 ++++ src/ui.rs | 22 +++++++++++++++------- tests/common/mod.rs | 10 ++++++---- tests/test_edit_command.rs | 4 ++-- tests/test_git_push.rs | 8 ++++---- tests/test_global_opts.rs | 12 ++++++------ tests/test_move_command.rs | 4 ++-- tests/test_print_command.rs | 8 ++++---- tests/test_restore_command.rs | 4 ++-- tests/test_squash_command.rs | 4 ++-- tests/test_ui.rs | 30 +++++++++++++++++++++++++++--- tests/test_untrack_command.rs | 8 ++++---- 12 files changed, 78 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1c75659a..aec5031e4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/ui.rs b/src/ui.rs index bb20fed08..f32e61aca 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -30,7 +30,8 @@ use crate::templater::TemplateFormatter; pub struct Ui<'a> { cwd: PathBuf, color: bool, - formatter: Mutex>, + stdout_formatter: Mutex>, + stderr_formatter: Mutex>, settings: UserSettings, } @@ -65,14 +66,17 @@ impl<'stdout> Ui<'stdout> { pub fn new( cwd: PathBuf, stdout: Box, + stderr: Box, 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 = Box::new(io::stdout()); + let stderr: Box = 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> { - self.formatter.lock().unwrap() + self.stdout_formatter.lock().unwrap() + } + + pub fn stderr_formatter(&self) -> MutexGuard> { + 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()?; diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 9e8fabe57..a5bf3843d 100644 --- a/tests/common/mod.rs +++ b/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() +} diff --git a/tests/test_edit_command.rs b/tests/test_edit_command.rs index e9f24e04a..25da66e75 100644 --- a/tests/test_edit_command.rs +++ b/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###" diff --git a/tests/test_git_push.rs b/tests/test_git_push.rs index 5f222d13b..fab2ee9fb 100644 --- a/tests/test_git_push.rs +++ b/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 "); } diff --git a/tests/test_global_opts.rs b/tests/test_global_opts.rs index 952bed471..72d362a09 100644 --- a/tests/test_global_opts.rs +++ b/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 "); } diff --git a/tests/test_move_command.rs b/tests/test_move_command.rs index e0fcb785f..720010111 100644 --- a/tests/test_move_command.rs +++ b/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###" diff --git a/tests/test_print_command.rs b/tests/test_print_command.rs index 5171308e8..9201adff4 100644 --- a/tests/test_print_command.rs +++ b/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 diff --git a/tests/test_restore_command.rs b/tests/test_restore_command.rs index 575b2c6fc..15c2fa0ca 100644 --- a/tests/test_restore_command.rs +++ b/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###" diff --git a/tests/test_squash_command.rs b/tests/test_squash_command.rs index 86f912bd1..d0e264826 100644 --- a/tests/test_squash_command.rs +++ b/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 diff --git a/tests/test_ui.rs b/tests/test_ui.rs index 2ee941276..51a615754 100644 --- a/tests/test_ui.rs +++ b/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, ""), diff --git a/tests/test_untrack_command.rs b/tests/test_untrack_command.rs index ee77df258..474312c16 100644 --- a/tests/test_untrack_command.rs +++ b/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",