cybercyst/jj
1
0
Fork 0
mirror of https://github.com/martinvonz/jj.git synced 2025-05-05 15:32:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

[jj fix]: Support tools that return per-file warnings/errors.

Browse Source
This commit is contained in:
David Rieber 2025-04-01 13:17:35 -07:00
parent 7146a99029
commit 656bee7206
3 changed files with 103 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

70
cli/src/commands/fix.rs
View File

@ -20,13 +20,13 @@ use std::process::Stdio;
use clap_complete::ArgValueCandidates;
use itertools::Itertools as _;
use jj_lib::backend::CommitId;
use jj_lib::backend::FileId;
use jj_lib::fileset;
use jj_lib::fileset::FilesetDiagnostics;
use jj_lib::fileset::FilesetExpression;
use jj_lib::fix::fix_files;
use jj_lib::fix::FileToFix;
use jj_lib::fix::FixError;
use jj_lib::fix::FixResult;
use jj_lib::fix::ParallelFileFixer;
use jj_lib::matchers::Matcher;
use jj_lib::repo_path::RepoPathUiConverter;
@ -179,42 +179,52 @@ fn fix_one_file(
tools_config: &ToolsConfig,
store: &Store,
file_to_fix: &FileToFix,
) -> Result<Option<FileId>, FixError> {
) -> Result<FixResult, FixError> {
let mut matching_tools = tools_config
.tools
.iter()
.filter(|tool_config| tool_config.matcher.matches(&file_to_fix.repo_path))
.peekable();
if matching_tools.peek().is_some() {
// The first matching tool gets its input from the committed file, and any
// subsequent matching tool gets its input from the previous matching tool's
// output.
let mut old_content = vec![];
let mut read = store.read_file(&file_to_fix.repo_path, &file_to_fix.file_id)?;
read.read_to_end(&mut old_content)?;
let new_content = matching_tools.fold(old_content.clone(), |prev_content, tool_config| {
match run_tool(
workspace_root,
&tool_config.command,
file_to_fix,
&prev_content,
) {
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
// name, etc.
Err(_) => prev_content,
}
if matching_tools.peek().is_none() {
return Ok(FixResult {
file_id: Some(file_to_fix.file_id.clone()),
messages: vec![],
});
if new_content != old_content {
// TODO: send futures back over channel
let new_file_id = store
.write_file(&file_to_fix.repo_path, &mut new_content.as_slice())
.block_on()?;
return Ok(Some(new_file_id));
}
}
Ok(None)
// The first matching tool gets its input from the committed file, and any
// subsequent matching tool gets its input from the previous matching tool's
// output.
let mut old_content = vec![];
let mut read = store.read_file(&file_to_fix.repo_path, &file_to_fix.file_id)?;
read.read_to_end(&mut old_content)?;
let new_content = matching_tools.fold(old_content.clone(), |prev_content, tool_config| {
match run_tool(
workspace_root,
&tool_config.command,
file_to_fix,
&prev_content,
) {
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
// name, etc.
Err(_) => prev_content,
}
});
if new_content == old_content {
return Ok(FixResult {
file_id: None,
messages: vec![],
});
}
// TODO: send futures back over channel
let new_file_id = store
.write_file(&file_to_fix.repo_path, &mut new_content.as_slice())
.block_on()?;
Ok(FixResult {
file_id: Some(new_file_id),
messages: vec![],
})
}
/// Runs the `tool_command` to fix the given file content.

72
lib/src/fix.rs
View File

@ -58,6 +58,25 @@ pub struct FileToFix {
pub repo_path: RepoPathBuf,
}
/// The results of applying the [FileFixer] to fix a given file.
#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub struct FixResult {
/// If file content was changed, this is the new FileId.
pub file_id: Option<FileId>,
/// Could be empty, or one value for accumulated stderr output,
/// or multiple values from a more structured source.
pub messages: Vec<FixMessage>,
}
/// A message returned by a file fixing tool.
#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub struct FixMessage {
// Optional fields like arbitrary text, line numbers, etc. that `fix_fn`
// can choose to emit, and callers can choose to display or ignore.
text: Option<String>,
line_number: Option<usize>,
}
/// Error fixing files.
#[derive(Debug, thiserror::Error)]
pub enum FixError {
@ -86,14 +105,11 @@ pub trait FileFixer {
/// Returns a map describing the subset of `files_to_fix` that resulted in
/// changed file content (unchanged files should not be present in the map),
/// pointing to the new FileId for the file.
///
/// TODO: Better error handling so we can tell the user what went wrong with
/// each failed input.
fn fix_files<'a>(
&self,
store: &Store,
files_to_fix: &'a HashSet<FileToFix>,
) -> Result<HashMap<&'a FileToFix, FileId>, FixError>;
) -> Result<HashMap<&'a FileToFix, FixResult>, FixError>;
}
/// Aggregate information about the outcome of the file fixer.
@ -104,6 +120,7 @@ pub struct FixSummary {
/// The number of commits that had files that were passed to the file fixer.
pub num_checked_commits: i32,
/// The number of new commits created due to file content changed by the
/// fixer.
pub num_fixed_commits: i32,
@ -121,7 +138,7 @@ pub struct ParallelFileFixer<T> {
impl<T> ParallelFileFixer<T>
where
T: Fn(&Store, &FileToFix) -> Result<Option<FileId>, FixError> + Sync + Send,
T: Fn(&Store, &FileToFix) -> Result<FixResult, FixError> + Sync + Send,
{
/// Creates a ParallelFileFixer.
pub fn new(fix_fn: T) -> Self {
@ -131,32 +148,27 @@ where
impl<T> FileFixer for ParallelFileFixer<T>
where
T: Fn(&Store, &FileToFix) -> Result<Option<FileId>, FixError> + Sync + Send,
T: Fn(&Store, &FileToFix) -> Result<FixResult, FixError> + Sync + Send,
{
/// Applies `fix_fn()` to the inputs and stores the resulting file content.
fn fix_files<'a>(
&self,
store: &Store,
files_to_fix: &'a HashSet<FileToFix>,
) -> Result<HashMap<&'a FileToFix, FileId>, FixError> {
) -> Result<HashMap<&'a FileToFix, FixResult>, FixError> {
let (updates_tx, updates_rx) = channel();
files_to_fix.into_par_iter().try_for_each_init(
|| updates_tx.clone(),
|updates_tx, file_to_fix| -> Result<(), FixError> {
let result = (self.fix_fn)(store, file_to_fix)?;
match result {
Some(new_file_id) => {
updates_tx.send((file_to_fix, new_file_id)).unwrap();
Ok(())
}
None => Ok(()),
}
let fix_result = (self.fix_fn)(store, file_to_fix)?;
updates_tx.send((file_to_fix, fix_result)).unwrap();
Ok(())
},
)?;
drop(updates_tx);
let mut result = HashMap::new();
while let Ok((file_to_fix, new_file_id)) = updates_rx.recv() {
result.insert(file_to_fix, new_file_id);
while let Ok((file_to_fix, fix_result)) = updates_rx.recv() {
result.insert(file_to_fix, fix_result);
}
Ok(result)
}
@ -269,8 +281,8 @@ pub fn fix_files(
);
// Fix all of the chosen inputs.
let fixed_file_ids = file_fixer.fix_files(repo_mut.store().as_ref(), &unique_files_to_fix)?;
tracing::debug!(?fixed_file_ids, "file fixer fixed these files:");
let fix_results = file_fixer.fix_files(repo_mut.store().as_ref(), &unique_files_to_fix)?;
tracing::debug!(?fix_results, "file fixer fixed these files:");
// Substitute the fixed file IDs into all of the affected commits. Currently,
// fixes cannot delete or rename files, change the executable bit, or modify
@ -291,11 +303,23 @@ pub fn fix_files(
file_id: id.clone(),
repo_path: repo_path.clone(),
};
if let Some(new_id) = fixed_file_ids.get(&file_to_fix) {
return Some(TreeValue::File {
id: new_id.clone(),
executable: *executable,
});
if let Some(fix_result) = fix_results.get(&file_to_fix) {
if !fix_result.messages.is_empty() {
// TODO: Add messages to FixSummary (but put an
// upper bound on this?).
}
match &fix_result.file_id {
Some(new_file_id) => {
return Some(TreeValue::File {
id: new_file_id.clone(),
executable: *executable,
});
}
None => {
// TODO: consider counting files examined but
// unchanged in FixSummary.
}
}
}
}
old_term.clone()

25
lib/tests/test_fix.rs
View File

@ -18,12 +18,12 @@ use std::sync::Arc;
use itertools::Itertools as _;
use jj_lib::backend::CommitId;
use jj_lib::backend::FileId;
use jj_lib::backend::MergedTreeId;
use jj_lib::fix::fix_files;
use jj_lib::fix::FileFixer;
use jj_lib::fix::FileToFix;
use jj_lib::fix::FixError;
use jj_lib::fix::FixResult;
use jj_lib::fix::ParallelFileFixer;
use jj_lib::matchers::EverythingMatcher;
use jj_lib::merged_tree::MergedTree;
@ -53,14 +53,13 @@ impl FileFixer for TestFileFixer {
&self,
store: &Store,
files_to_fix: &'a HashSet<FileToFix>,
) -> Result<HashMap<&'a FileToFix, FileId>, FixError> {
let mut changed_files = HashMap::new();
) -> Result<HashMap<&'a FileToFix, FixResult>, FixError> {
let mut results = HashMap::new();
for file_to_fix in files_to_fix {
if let Some(new_file_id) = fix_file(store, file_to_fix)? {
changed_files.insert(file_to_fix, new_file_id);
}
let result = fix_file(store, file_to_fix)?;
results.insert(file_to_fix, result);
}
Ok(changed_files)
Ok(results)
}
}
@ -75,7 +74,7 @@ fn make_fix_content_error(message: &str) -> FixError {
// Reads the file from store. If the file starts with "fixme", its contents are
// changed to uppercase and the new file id is returned. If the file starts with
// "error", an error is raised. Otherwise returns None.
fn fix_file(store: &Store, file_to_fix: &FileToFix) -> Result<Option<FileId>, FixError> {
fn fix_file(store: &Store, file_to_fix: &FileToFix) -> Result<FixResult, FixError> {
let mut old_content = vec![];
let mut read = store
.read_file(&file_to_fix.repo_path, &file_to_fix.file_id)
@ -88,11 +87,17 @@ fn fix_file(store: &Store, file_to_fix: &FileToFix) -> Result<Option<FileId>, Fi
.write_file(&file_to_fix.repo_path, &mut new_content.as_slice())
.block_on()
.unwrap();
Ok(Some(new_file_id))
Ok(FixResult {
file_id: Some(new_file_id),
messages: vec![],
})
} else if let Some(rest) = old_content.strip_prefix(b"error:") {
Err(make_fix_content_error(std::str::from_utf8(rest).unwrap()))
} else {
Ok(None)
Ok(FixResult {
file_id: None,
messages: vec![],
})
}
}