From 13b6dfdc59d3668afb57380f486c70ab86f4a366 Mon Sep 17 00:00:00 2001 From: Matt Stark Date: Fri, 15 Nov 2024 14:26:59 +1100 Subject: [PATCH] cli: Make invalid fix tools fail gracefully. Fixes #4866 --- cli/src/commands/fix.rs | 19 ++++++++++++++++++- cli/tests/test_fix_command.rs | 27 +++++++++++++++++++-------- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/cli/src/commands/fix.rs b/cli/src/commands/fix.rs index 3677074a44..21e53b6ce7 100644 --- a/cli/src/commands/fix.rs +++ b/cli/src/commands/fix.rs @@ -15,6 +15,7 @@ use std::collections::HashMap; use std::collections::HashSet; use std::io::Write; +use std::path::Path; use std::process::Stdio; use std::sync::mpsc::channel; @@ -181,6 +182,17 @@ pub(crate) fn cmd_fix( .try_collect()?; let mut unique_tool_inputs: HashSet = HashSet::new(); let mut commit_paths: HashMap> = HashMap::new(); + + // See https://github.com/martinvonz/jj/issues/4866 + // TLDR is that a fix tool should not read from a file, and one which does will + // squash all your changes together. To prevent a tool from reading a file, + // we make the relative paths we provide invalid by changing the working + // directory. + let working_dir = tempfile::Builder::new() + .prefix("jj-fix-") + .tempdir() + .unwrap(); + for commit in commits.iter().rev() { let mut paths: HashSet = HashSet::new(); @@ -237,6 +249,7 @@ pub(crate) fn cmd_fix( tx.repo().store().as_ref(), &tools_config, &unique_tool_inputs, + working_dir.path(), )?; // Substitute the fixed file IDs into all of the affected commits. Currently, @@ -324,6 +337,7 @@ fn fix_file_ids<'a>( store: &Store, tools_config: &ToolsConfig, tool_inputs: &'a HashSet, + working_dir: &Path, ) -> Result, CommandError> { let (updates_tx, updates_rx) = channel(); // TODO: Switch to futures, or document the decision not to. We don't need @@ -345,7 +359,8 @@ fn fix_file_ids<'a>( read.read_to_end(&mut old_content)?; let new_content = matching_tools.fold(old_content.clone(), |prev_content, tool_config| { - match run_tool(&tool_config.command, tool_input, &prev_content) { + match run_tool(&tool_config.command, tool_input, &prev_content, working_dir) + { Ok(next_content) => next_content, // TODO: Because the stderr is passed through, this isn't always failing // silently, but it should do something better will the exit code, tool @@ -384,6 +399,7 @@ fn run_tool( tool_command: &CommandNameAndArgs, tool_input: &ToolInput, old_content: &[u8], + working_dir: &Path, ) -> Result, ()> { // TODO: Pipe stderr so we can tell the user which commit, file, and tool it is // associated with. @@ -391,6 +407,7 @@ fn run_tool( vars.insert("path", tool_input.repo_path.as_internal_file_string()); let mut child = tool_command .to_command_with_variables(&vars) + .current_dir(working_dir) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .spawn() diff --git a/cli/tests/test_fix_command.rs b/cli/tests/test_fix_command.rs index 8a963d4f65..4bc5c48138 100644 --- a/cli/tests/test_fix_command.rs +++ b/cli/tests/test_fix_command.rs @@ -734,11 +734,16 @@ fn test_fix_cyclic() { #[test] fn test_deduplication() { + let logfile = tempfile::Builder::new() + .prefix("jj-fix-log") + .tempfile() + .unwrap(); + let logfile_path_str = logfile.path().as_os_str().to_str().unwrap(); // Append all fixed content to a log file. This assumes we're running the tool // in the root directory of the repo, which is worth reconsidering if we // establish a contract regarding cwd. let (test_env, repo_path, redact) = - init_with_fake_formatter(&["--uppercase", "--tee", "$path-fixlog"]); + init_with_fake_formatter(&["--uppercase", "--tee", logfile_path_str]); // There are at least two interesting cases: the content is repeated immediately // in the child commit, or later in another descendant. @@ -756,11 +761,11 @@ fn test_deduplication() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "a"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr.replace(logfile_path_str, "$logfile")), @r###" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--uppercase", "--tee", "$path-fixlog"] + command = [, "--uppercase", "--tee", "$logfile"] patterns = ["all()"] Fixed 4 commits of 4 checked. @@ -780,7 +785,7 @@ fn test_deduplication() { // Each new content string only appears once in the log, because all the other // inputs (like file name) were identical, and so the results were re-used. We // sort the log because the order of execution inside `jj fix` is undefined. - insta::assert_snapshot!(sorted_lines(repo_path.join("file-fixlog")), @"BAR\nFOO\n"); + insta::assert_snapshot!(sorted_lines(logfile.path().to_path_buf()), @"BAR\nFOO\n"); } fn sorted_lines(path: PathBuf) -> String { @@ -795,18 +800,24 @@ fn sorted_lines(path: PathBuf) -> String { #[test] fn test_executed_but_nothing_changed() { + let logfile = tempfile::Builder::new() + .prefix("jj-fix-log") + .tempfile() + .unwrap(); + let logfile_path_str = logfile.path().as_os_str().to_str().unwrap(); + // Show that the tool ran by causing a side effect with --tee, and test that we // do the right thing when the tool's output is exactly equal to its input. - let (test_env, repo_path, redact) = init_with_fake_formatter(&["--tee", "$path-copy"]); + let (test_env, repo_path, redact) = init_with_fake_formatter(&["--tee", logfile_path_str]); std::fs::write(repo_path.join("file"), "content\n").unwrap(); let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["fix", "-s", "@"]); insta::assert_snapshot!(stdout, @""); - insta::assert_snapshot!(redact(&stderr), @r###" + insta::assert_snapshot!(redact(&stderr.replace(logfile_path_str, "$logfile")), @r###" Warning: The `fix.tool-command` config option is deprecated and will be removed in a future version. Hint: Replace it with the following: [fix.tools.legacy-tool-command] - command = [, "--tee", "$path-copy"] + command = [, "--tee", "$logfile"] patterns = ["all()"] Fixed 0 commits of 1 checked. @@ -814,7 +825,7 @@ fn test_executed_but_nothing_changed() { "###); let content = test_env.jj_cmd_success(&repo_path, &["file", "show", "file", "-r", "@"]); insta::assert_snapshot!(content, @"content\n"); - let copy_content = std::fs::read_to_string(repo_path.join("file-copy").as_os_str()).unwrap(); + let copy_content = std::fs::read_to_string(logfile.path()).unwrap(); insta::assert_snapshot!(copy_content, @"content\n"); }