Skip to content

Commit

Permalink
cli: Make invalid fix tools fail gracefully.
Browse files Browse the repository at this point in the history
Fixes #4866
  • Loading branch information
matts1 committed Nov 15, 2024
1 parent 68b72ad commit 078ff05
Showing 1 changed file with 18 additions and 1 deletion.
19 changes: 18 additions & 1 deletion cli/src/commands/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -181,6 +182,17 @@ pub(crate) fn cmd_fix(
.try_collect()?;
let mut unique_tool_inputs: HashSet<ToolInput> = HashSet::new();
let mut commit_paths: HashMap<CommitId, HashSet<RepoPathBuf>> = 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<RepoPathBuf> = HashSet::new();

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -324,6 +337,7 @@ fn fix_file_ids<'a>(
store: &Store,
tools_config: &ToolsConfig,
tool_inputs: &'a HashSet<ToolInput>,
working_dir: &Path,
) -> Result<HashMap<&'a ToolInput, FileId>, CommandError> {
let (updates_tx, updates_rx) = channel();
// TODO: Switch to futures, or document the decision not to. We don't need
Expand All @@ -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
Expand Down Expand Up @@ -384,13 +399,15 @@ fn run_tool(
tool_command: &CommandNameAndArgs,
tool_input: &ToolInput,
old_content: &[u8],
working_dir: &Path,
) -> Result<Vec<u8>, ()> {
// TODO: Pipe stderr so we can tell the user which commit, file, and tool it is
// associated with.
let mut vars: HashMap<&str, &str> = HashMap::new();
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()
Expand Down

0 comments on commit 078ff05

Please sign in to comment.