cli: Review builder fixes

Diff hunks were not being applied properly due to the line offsets
changing when a hunk was skipped.
This commit is contained in:
cloudhead 2023-12-13 13:29:53 +01:00
parent 99aeae19ea
commit c761d03a5a
No known key found for this signature in database
2 changed files with 51 additions and 17 deletions

View File

@ -23,6 +23,7 @@ use radicle::storage::git::Repository;
use radicle_surf::diff::*; use radicle_surf::diff::*;
use crate::git::unified_diff; use crate::git::unified_diff;
use crate::git::unified_diff::Encode;
use crate::terminal as term; use crate::terminal as term;
/// Help message shown to user. /// Help message shown to user.
@ -177,6 +178,8 @@ impl<'a> ReviewBuilder<'a> {
pub fn new(patch_id: PatchId, nid: NodeId, repo: &'a Repository) -> Self { pub fn new(patch_id: PatchId, nid: NodeId, repo: &'a Repository) -> Self {
Self { Self {
patch_id, patch_id,
// TODO: Validate this leads to correct UX for potentially abandoned drafts on
// past revisions.
refname: git::refs::storage::draft::review(&nid, &patch_id), refname: git::refs::storage::draft::review(&nid, &patch_id),
repo, repo,
hunk: None, hunk: None,
@ -202,7 +205,14 @@ impl<'a> ReviewBuilder<'a> {
let base = repo.find_commit((*revision.base()).into())?; let base = repo.find_commit((*revision.base()).into())?;
let author = repo.signature()?; let author = repo.signature()?;
let patch_id = self.patch_id; let patch_id = self.patch_id;
let review = if let Ok(c) = self.current() { let tree = {
let commit = repo.find_commit(revision.head().into())?;
commit.tree()?
};
let mut stdin = io::stdin().lock();
let mut stderr = io::stderr().lock();
let mut review = if let Ok(c) = self.current() {
term::success!( term::success!(
"Loaded existing review {} for patch {}", "Loaded existing review {} for patch {}",
term::format::secondary(term::format::parens(term::format::oid(c.id()))), term::format::secondary(term::format::parens(term::format::oid(c.id()))),
@ -216,20 +226,15 @@ impl<'a> ReviewBuilder<'a> {
&author, &author,
&format!("Review {patch_id}"), &format!("Review {patch_id}"),
&base.tree()?, &base.tree()?,
// TODO: Verify this is necessary, shouldn't matter.
&[&base], &[&base],
)?; )?;
repo.find_commit(oid)? repo.find_commit(oid)?
}; };
let mut brain = review.tree()?;
let mut writer = unified_diff::Writer::new(io::stdout()).styled(true); let mut writer = unified_diff::Writer::new(io::stdout()).styled(true);
let mut queue = ReviewQueue::default(); // Queue of hunks to review. let mut queue = ReviewQueue::default(); // Queue of hunks to review.
let mut current = None; // File of the current hunk. let mut current = None; // File of the current hunk.
let mut stdin = io::stdin().lock();
let mut stderr = io::stderr().lock();
let commit = repo.find_commit(revision.head().into())?;
let tree = commit.tree()?;
let brain = review.tree()?;
let mut find_opts = git::raw::DiffFindOptions::new(); let mut find_opts = git::raw::DiffFindOptions::new();
find_opts.exact_match_only(true); find_opts.exact_match_only(true);
@ -271,6 +276,7 @@ impl<'a> ReviewBuilder<'a> {
} }
} }
let total = queue.len(); let total = queue.len();
let mut delta: i32 = 0;
while let Some((ix, item)) = queue.next() { while let Some((ix, item)) = queue.next() {
if let Some(hunk) = self.hunk { if let Some(hunk) = self.hunk {
@ -284,33 +290,52 @@ impl<'a> ReviewBuilder<'a> {
if current.map_or(true, |c| c != file) { if current.map_or(true, |c| c != file) {
writer.encode(&unified_diff::FileHeader::from(file))?; writer.encode(&unified_diff::FileHeader::from(file))?;
current = Some(file); current = Some(file);
delta = 0;
} }
if let Some(h) = hunk {
writer.encode(h)?; let header = hunk
} .map(|h| {
let header = unified_diff::HunkHeader::try_from(h)?;
writer.encode(h)?;
Ok::<_, anyhow::Error>(header)
})
.transpose()?;
match self.prompt(&mut stdin, &mut stderr, progress) { match self.prompt(&mut stdin, &mut stderr, progress) {
Some(ReviewAction::Accept) => { Some(ReviewAction::Accept) => {
let mut buf = Vec::new(); let mut buf = Vec::new();
{ {
let mut writer = unified_diff::Writer::new(&mut buf); let mut writer = unified_diff::Writer::new(&mut buf);
writer.encode(&unified_diff::FileHeader::from(file))?; writer.encode(&unified_diff::FileHeader::from(file))?;
if let Some(h) = hunk {
writer.encode(h)?; if let (Some(h), Some(mut header)) = (hunk, header) {
header.old_line_no -= delta as u32;
header.new_line_no -= delta as u32;
let h = Hunk {
header: header.to_unified_string()?.as_bytes().to_owned().into(),
lines: h.lines.clone(),
old: h.old.clone(),
new: h.new.clone(),
};
writer.encode(&h)?;
} }
} }
let diff = git::raw::Diff::from_buffer(&buf)?; let diff = git::raw::Diff::from_buffer(&buf)?;
let mut index = repo.apply_to_tree(&brain, &diff, None)?; let mut index = repo.apply_to_tree(&brain, &diff, None)?;
let brain = index.write_tree_to(repo)?; let brain_oid = index.write_tree_to(repo)?;
let brain = repo.find_tree(brain)?; brain = repo.find_tree(brain_oid)?;
let _oid = let oid =
review.amend(Some(&self.refname), None, None, None, None, Some(&brain))?; review.amend(Some(&self.refname), None, None, None, None, Some(&brain))?;
review = repo.find_commit(oid)?;
} }
Some(ReviewAction::Ignore) => { Some(ReviewAction::Ignore) => {
// Do nothing. Hunk will be reviewable again next time. // Do nothing. Hunk will be reviewable again next time.
if let Some(h) = header {
delta += h.new_size as i32 - h.old_size as i32;
}
} }
Some(ReviewAction::Comment) => { Some(ReviewAction::Comment) => {
eprintln!( eprintln!(

View File

@ -119,6 +119,15 @@ pub struct HunkHeader {
pub text: Vec<u8>, pub text: Vec<u8>,
} }
impl TryFrom<&Hunk<Modification>> for HunkHeader {
type Error = Error;
fn try_from(hunk: &Hunk<Modification>) -> Result<Self, Self::Error> {
let mut r = io::BufReader::new(hunk.header.as_bytes());
Self::decode(&mut r)
}
}
impl HunkHeader { impl HunkHeader {
pub fn old_line_range(&self) -> std::ops::Range<u32> { pub fn old_line_range(&self) -> std::ops::Range<u32> {
let start: u32 = self.old_line_no; let start: u32 = self.old_line_no;