From b45bada00f2251121fe34f6c552db95310338bf4 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 1 Mar 2022 22:57:28 -0800 Subject: [PATCH] cli: clarify error message when `jj untrack` argument is not ignored As pointed out by @arxanas in #88, the message saying something like "At least 'bin/.DS_Store' was added back ..." is confusing especially when the command you ran was just `jj untrack bin/.DS_Store`. Let's clarify the message by saying exactly how many more files there are, and specialize the message for when there is only one file. Also update the message to say "would be added back" instead of "was added back" since we don't actually change anything if some files would be added back (since 4b91ad408cb3). Should we even list all the files? I'm concerned that such a list could be very long. On the other hand, it can also be annoying to have to run `jj untrack some/dir/` and only be told about single file to add to the ignore patterns every time. --- src/commands.rs | 25 ++++++--- src/ui.rs | 1 + tests/test_untrack_command.rs | 98 +++++++++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 7 deletions(-) create mode 100644 tests/test_untrack_command.rs diff --git a/src/commands.rs b/src/commands.rs index 2cb3ab5eb..0f2c7b304 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -1820,14 +1820,25 @@ fn cmd_untrack( let wc_tree_id = locked_working_copy.write_tree(); if wc_tree_id != new_tree_id { let wc_tree = store.get_tree(&RepoPath::root(), &wc_tree_id)?; - if let Some((path, _value)) = wc_tree.entries_matching(matcher.as_ref()).next() { + let added_back = wc_tree.entries_matching(matcher.as_ref()).collect_vec(); + if !added_back.is_empty() { locked_working_copy.discard(); - let ui_path = ui.format_file_path(workspace_command.workspace_root(), &path); - return Err(CommandError::UserError(format!( - "At least '{}' was added back because it is not ignored. Make sure it's ignored, \ - then try again.", - ui_path - ))); + let path = &added_back[0].0; + let ui_path = ui.format_file_path(workspace_command.workspace_root(), path); + if added_back.len() > 1 { + return Err(CommandError::UserError(format!( + "'{}' and {} other files would be added back because they're not ignored. \ + Make sure they're ignored, then try again.", + ui_path, + added_back.len() - 1 + ))); + } else { + return Err(CommandError::UserError(format!( + "'{}' would be added back because it's not ignored. Make sure it's ignored, \ + then try again.", + ui_path + ))); + } } else { // This means there were some concurrent changes made in the working copy. We // don't want to mix those in, so reset the working copy again. diff --git a/src/ui.rs b/src/ui.rs index 6d214b939..eb4716b61 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -105,6 +105,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(); formatter.add_label(String::from("error"))?; formatter.write_str(text)?; diff --git a/tests/test_untrack_command.rs b/tests/test_untrack_command.rs new file mode 100644 index 000000000..8f185c8f6 --- /dev/null +++ b/tests/test_untrack_command.rs @@ -0,0 +1,98 @@ +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::path::PathBuf; + +use jujutsu::testutils::{get_stdout_string, TestEnvironment}; + +#[test] +fn test_untrack() { + let test_env = TestEnvironment::default(); + test_env + .jj_cmd(test_env.env_root(), &["init", "repo"]) + .assert() + .success(); + let repo_path = test_env.env_root().join("repo"); + + std::fs::write(repo_path.join("file1"), "initial").unwrap(); + std::fs::write(repo_path.join("file1.bak"), "initial").unwrap(); + std::fs::write(repo_path.join("file2.bak"), "initial").unwrap(); + let target_dir = repo_path.join("target"); + std::fs::create_dir(&target_dir).unwrap(); + std::fs::write(target_dir.join("file2"), "initial").unwrap(); + std::fs::write(target_dir.join("file3"), "initial").unwrap(); + + // Run a command so all the files get tracked, then add "*.bak" to the ignore + // patterns + test_env.jj_cmd(&repo_path, &["st"]).assert().success(); + std::fs::write(repo_path.join(".gitignore"), "*.bak\n").unwrap(); + let files_before = + get_stdout_string(&test_env.jj_cmd(&repo_path, &["files"]).assert().success()); + + // Errors out when a specified file is not ignored + let assert = test_env + .jj_cmd(&repo_path, &["untrack", "file1", "file1.bak"]) + .assert() + .failure(); + assert.stdout( + "Error: 'file1' would be added back because it's not ignored. Make sure it's ignored, \ + then try again.\n", + ); + let files_after = + get_stdout_string(&test_env.jj_cmd(&repo_path, &["files"]).assert().success()); + // There should be no changes to the state when there was an error + assert_eq!(files_after, files_before); + + // Can untrack a single file + assert!(files_before.contains("file1.bak\n")); + test_env + .jj_cmd(&repo_path, &["untrack", "file1.bak"]) + .assert() + .success() + .stdout(""); + let files_after = + get_stdout_string(&test_env.jj_cmd(&repo_path, &["files"]).assert().success()); + // The file is no longer tracked + assert!(!files_after.contains("file1.bak")); + // Other files that match the ignore pattern are not untracked + assert!(files_after.contains("file2.bak")); + // The files still exist on disk + assert!(repo_path.join("file1.bak").exists()); + assert!(repo_path.join("file2.bak").exists()); + + // Errors out when multiple specified files are not ignored + let assert = test_env + .jj_cmd(&repo_path, &["untrack", "target"]) + .assert() + .failure(); + assert_eq!( + get_stdout_string(&assert), + format!( + "Error: '{}' and 1 other files would be added back because they're not ignored. Make \ + sure they're ignored, then try again.\n", + PathBuf::from("target").join("file2").display() + ) + ); + + // Can untrack after adding to ignore patterns + std::fs::write(repo_path.join(".gitignore"), ".bak\ntarget/\n").unwrap(); + test_env + .jj_cmd(&repo_path, &["untrack", "target"]) + .assert() + .success() + .stdout(""); + let files_after = + get_stdout_string(&test_env.jj_cmd(&repo_path, &["files"]).assert().success()); + assert!(!files_after.contains("target")); +}