diff --git a/appview/pages/templates/repo/pulls/fragments/pullStepReview.html b/appview/pages/templates/repo/pulls/fragments/pullStepReview.html index d28cf761..c6b55ace 100644 --- a/appview/pages/templates/repo/pulls/fragments/pullStepReview.html +++ b/appview/pages/templates/repo/pulls/fragments/pullStepReview.html @@ -9,6 +9,12 @@ {{ end }} {{ else }} + {{ if .Comparison.BinaryOmitted }} +
+ {{ i "triangle-alert" "w-4 h-4 flex-shrink-0 mt-0.5" }} + Some binary files are too large to carry in this patch. The pull request will open, but merging it will leave those files untouched. +
+ {{ end }} {{ $commits := .Comparison.FormatPatch }} {{ if $commits }}
diff --git a/appview/pulls/create.go b/appview/pulls/create.go index 2416c561..3aeb36f2 100644 --- a/appview/pulls/create.go +++ b/appview/pulls/create.go @@ -68,6 +68,10 @@ func (s *Pulls) handleBranchBasedPull( return } + if comparison.BinaryOmitted { + l.Warn("knot left binary payloads out of the compare, so this patch won't apply cleanly", "knot", repo.Knot, "repo", repo.RepoIdentifier()) + } + sourceRev := comparison.Rev2 patch := comparison.FormatPatchRaw combined := comparison.CombinedPatchRaw @@ -180,6 +184,10 @@ func (s *Pulls) handleForkBasedPull(w http.ResponseWriter, r *http.Request, repo return } + if comparison.BinaryOmitted { + l.Warn("knot left binary payloads out of the compare, so this patch won't apply cleanly", "knot", fork.Knot, "repo", fork.RepoIdentifier(), "hidden_ref", hiddenRef) + } + sourceRev := comparison.Rev2 patch := comparison.FormatPatchRaw combined := comparison.CombinedPatchRaw diff --git a/knot2/crates/knot-git/src/base85.rs b/knot2/crates/knot-git/src/base85.rs new file mode 100644 index 00000000..218b377d --- /dev/null +++ b/knot2/crates/knot-git/src/base85.rs @@ -0,0 +1,154 @@ +use std::sync::LazyLock; + +const MARKERS: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"; + +const BYTES_PER_LINE: usize = MARKERS.len(); + +const ALPHABET: &[u8] = + b"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz!#$%&()*+-;<=>?@^_`{|}~"; + +static DIGITS: LazyLock<[Option; 256]> = LazyLock::new(|| { + std::array::from_fn(|byte| { + ALPHABET + .iter() + .position(|&candidate| candidate as usize == byte) + .map(|digit| digit as u8) + }) +}); + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct Malformed; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct LineLength(u8); + +impl LineLength { + fn of(bytes: usize) -> Option { + bytes + .checked_sub(1) + .filter(|index| *index < MARKERS.len()) + .map(|index| Self(index as u8)) + } + + fn from_marker(marker: u8) -> Option { + MARKERS + .iter() + .position(|&candidate| candidate == marker) + .map(|index| Self(index as u8)) + } + + fn marker(self) -> char { + MARKERS[self.0 as usize] as char + } + + fn get(self) -> usize { + self.0 as usize + 1 + } +} + +pub(crate) fn encode(packed: &[u8], out: &mut String) { + out.reserve(encoded_len(packed.len() as u64) as usize); + packed.chunks(BYTES_PER_LINE).for_each(|chunk| { + let length = LineLength::of(chunk.len()) + .expect("chunking by the line width yields 1..=BYTES_PER_LINE bytes"); + out.push(length.marker()); + chunk.chunks(4).for_each(|group| { + let word = group + .iter() + .fold(0u32, |acc, &byte| (acc << 8) | byte as u32) + << (8 * (4 - group.len())); + (0..5).rev().for_each(|power| { + let digit = (word / 85u32.pow(power)) % 85; + out.push(ALPHABET[digit as usize] as char); + }); + }); + out.push('\n'); + }); +} + +pub(crate) fn encoded_len(packed: u64) -> u64 { + packed.div_ceil(4) * 5 + packed.div_ceil(BYTES_PER_LINE as u64) * 2 +} + +pub(crate) fn decode_line(line: &str, out: &mut Vec) -> Result<(), Malformed> { + let (marker, data) = line.as_bytes().split_first().ok_or(Malformed)?; + let length = LineLength::from_marker(*marker).ok_or(Malformed)?; + if data.len() != length.get().div_ceil(4) * 5 { + return Err(Malformed); + } + data.chunks(5).enumerate().try_for_each(|(group, chunk)| { + let word = chunk + .iter() + .try_fold(0u64, |acc, &character| { + DIGITS[character as usize].map(|digit| acc * 85 + digit as u64) + }) + .filter(|&word| word <= u32::MAX as u64) + .ok_or(Malformed)?; + let take = (length.get() - group * 4).min(4); + out.extend_from_slice(&(word as u32).to_be_bytes()[..take]); + Ok(()) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn encoding_round_trips_at_every_payload_length() { + (0..=260usize).for_each(|len| { + let bytes: Vec = (0..len) + .map(|index| (index as u8).wrapping_mul(37) ^ 0x5a) + .collect(); + let mut encoded = String::new(); + encode(&bytes, &mut encoded); + assert_eq!( + encoded_len(len as u64), + encoded.len() as u64, + "encoded length of a {len} byte payload" + ); + let decoded = encoded.lines().fold(Vec::new(), |mut out, line| { + decode_line(line, &mut out).unwrap(); + out + }); + assert_eq!(decoded, bytes, "round trip of a {len} byte payload"); + }); + } + + #[test] + fn line_markers_map_both_ways() { + (1..=BYTES_PER_LINE).for_each(|len| { + let length = LineLength::of(len).unwrap(); + assert_eq!( + LineLength::from_marker(length.marker() as u8), + Some(length), + "marker for a line of {len} bytes" + ); + }); + assert_eq!(LineLength::of(0), None); + assert_eq!(LineLength::of(BYTES_PER_LINE + 1), None); + assert_eq!(LineLength::of(1).unwrap().marker(), 'A'); + assert_eq!(LineLength::of(26).unwrap().marker(), 'Z'); + assert_eq!(LineLength::of(27).unwrap().marker(), 'a'); + assert_eq!(LineLength::of(BYTES_PER_LINE).unwrap().marker(), 'z'); + let mut encoded = String::new(); + encode(&[0u8; BYTES_PER_LINE + 1], &mut encoded); + assert_eq!( + encoded.lines().map(|line| &line[..1]).collect::>(), + vec!["z", "A"], + "a payload past the line width gets a second marker" + ); + } + + #[test] + fn the_marker_sets_the_line_length_and_anything_else_is_malformed() { + let mut out = Vec::new(); + decode_line("D00000", &mut out).unwrap(); + decode_line("B00000", &mut out).unwrap(); + assert_eq!(out, vec![0, 0, 0, 0, 0, 0]); + assert_eq!(decode_line("D0000", &mut Vec::new()), Err(Malformed)); + assert_eq!(decode_line("D0\"000", &mut Vec::new()), Err(Malformed)); + assert_eq!(decode_line("?00000", &mut Vec::new()), Err(Malformed)); + assert_eq!(decode_line("", &mut Vec::new()), Err(Malformed)); + } +} diff --git a/knot2/crates/knot-git/src/lib.rs b/knot2/crates/knot-git/src/lib.rs index c27746a9..83fbd753 100644 --- a/knot2/crates/knot-git/src/lib.rs +++ b/knot2/crates/knot-git/src/lib.rs @@ -1,4 +1,5 @@ mod archive; +mod base85; mod bitmap; mod error; #[cfg(feature = "instrument")] @@ -22,8 +23,8 @@ pub use objects::{ ShallowPlan, Tree, TreeDepth, TreeEntry, Wants, }; pub use patch::{ - FilePatch, Hunk, HunkLine, LineCount, LineNumber, LineOp, MAX_DIFF_BLOB_BYTES, PatchRange, - PatchStatus, + BinaryBudget, BinaryDiff, BinarySizes, FilePatch, Hunk, HunkLine, LineCount, LineNumber, + LineOp, MAX_DIFF_BLOB_BYTES, PatchBody, PatchRange, PatchStatus, }; pub use patch_apply::{ ApplyError, ApplyOutcome, Conflict, ConflictReason, NewCommit, PatchApplier, StagedAction, @@ -31,7 +32,7 @@ pub use patch_apply::{ }; pub use patch_parse::{ FileIntent, MailPatch, ParsedFile, PatchParseError, PatchPayload, is_format_patch, - parse_mailbox, parse_mailbox_bounded, parse_patch, parse_patch_bounded, + parse_mailbox, parse_mailbox_bounded, parse_patch, parse_patch_bounded, quote_path, }; pub use reads::{ AnnotatedTag, BranchInfo, BranchTip, LastCommit, LogLimit, LogSkip, PathEntry, SizedEntry, diff --git a/knot2/crates/knot-git/src/patch.rs b/knot2/crates/knot-git/src/patch.rs index a039e7d2..e9f7be9a 100644 --- a/knot2/crates/knot-git/src/patch.rs +++ b/knot2/crates/knot-git/src/patch.rs @@ -1,15 +1,22 @@ use std::convert::Infallible; +use std::fmt::Write as _; +use std::io::Write; use std::ops::ControlFlow; +use flate2::Compression; +use flate2::write::ZlibEncoder; use gix::diff::blob::unified_diff::{ConsumeHunk, ContextSize, DiffLineKind, HunkHeader}; use gix::diff::blob::{Algorithm, Diff, InternedInput, UnifiedDiff}; use knot_types::{ChangedFiles, ChangedFilesBudget, Listing, Oid, RepoPath}; +use crate::base85; use crate::error::{GitError, backend}; use crate::objects::EntryKind; use crate::repo::Repo; const BINARY_SNIFF_BYTES: usize = 8000; +const BLOCK_HEADER_MAX: usize = "literal 18446744073709551615\n".len(); +const BINARY_PATCH_HEADER: &str = "GIT binary patch\n"; pub const MAX_DIFF_BLOB_BYTES: u64 = 25 * 1024 * 1024; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -74,6 +81,113 @@ pub enum PatchStatus { Modified, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct BinarySizes { + pub old: u64, + pub new: u64, +} + +impl BinarySizes { + fn of(old: &[u8], new: &[u8]) -> Self { + Self { + old: old.len() as u64, + new: new.len() as u64, + } + } + + fn wire_bound(self) -> u64 { + let block = |inflated: u64| { + let deflated = inflated + .saturating_add(inflated.div_ceil(8)) + .saturating_add(inflated.div_ceil(64)) + .saturating_add(11); + base85::encoded_len(deflated).saturating_add(BLOCK_HEADER_MAX as u64 + 1) + }; + block(self.old) + .saturating_add(block(self.new)) + .saturating_add(BINARY_PATCH_HEADER.len() as u64) + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BinaryBudget { + Omit, + Spend { remaining: u64, omitted: bool }, +} + +impl BinaryBudget { + pub fn new(bytes: u64) -> Self { + Self::Spend { + remaining: bytes, + omitted: false, + } + } + + pub fn omitted(self) -> bool { + matches!(self, Self::Spend { omitted: true, .. }) + } + + fn admit(&mut self, sizes: BinarySizes) -> bool { + match self { + Self::Omit => false, + Self::Spend { remaining, omitted } => match remaining.checked_sub(sizes.wire_bound()) { + Some(rest) => { + *remaining = rest; + true + } + None => { + *omitted = true; + false + } + }, + } + } +} + +fn literal_block(content: &[u8], out: &mut String) -> Result<(), GitError> { + let mut encoder = ZlibEncoder::new(Vec::new(), Compression::fast()); + encoder.write_all(content).map_err(backend)?; + writeln!(out, "literal {}", content.len()).expect("formatting into a String never fails"); + base85::encode(&encoder.finish().map_err(backend)?, out); + out.push('\n'); + Ok(()) +} + +fn encode_binary(old: &[u8], new: &[u8]) -> Result { + let mut text = String::from(BINARY_PATCH_HEADER); + literal_block(new, &mut text)?; + literal_block(old, &mut text)?; + Ok(BinaryDiff::Encoded { + sizes: BinarySizes::of(old, new), + text, + }) +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum BinaryDiff { + Encoded { sizes: BinarySizes, text: String }, + Omitted(BinarySizes), + Unchanged(u64), +} + +impl BinaryDiff { + pub fn sizes(&self) -> BinarySizes { + match self { + Self::Encoded { sizes, .. } | Self::Omitted(sizes) => *sizes, + Self::Unchanged(bytes) => BinarySizes { + old: *bytes, + new: *bytes, + }, + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum PatchBody { + Text(Vec), + Binary(BinaryDiff), +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct FilePatch { pub status: PatchStatus, @@ -82,8 +196,20 @@ pub struct FilePatch { pub new_oid: Oid, pub old_kind: Option, pub new_kind: Option, - pub is_binary: bool, - pub hunks: Vec, + pub body: PatchBody, +} + +impl FilePatch { + pub fn is_binary(&self) -> bool { + matches!(self.body, PatchBody::Binary(_)) + } + + pub fn hunks(&self) -> &[Hunk] { + match &self.body { + PatchBody::Text(hunks) => hunks, + PatchBody::Binary(_) => &[], + } + } } fn is_binary(content: &[u8]) -> bool { @@ -169,28 +295,45 @@ impl Side { } } +fn subproject_line(oid: Oid) -> Vec { + format!("Subproject commit {}\n", oid.to_hex()).into_bytes() +} + impl Repo { fn patch_content(&self, side: &Side) -> Result, GitError> { match side { Side::Absent => Ok(Vec::new()), Side::Present { oid, kind } => match kind { - EntryKind::Commit => { - Ok(format!("Subproject commit {}\n", oid.to_hex()).into_bytes()) - } + EntryKind::Commit => Ok(subproject_line(*oid)), EntryKind::Tree => Ok(Vec::new()), _ => self.read_blob(*oid), }, } } - fn side_within_diff_budget(&self, side: &Side) -> Result { - match side { + fn sides_past_diff_budget( + &self, + old: &Side, + new: &Side, + ) -> Result, GitError> { + let size = |side: &Side| match side { + Side::Absent + | Side::Present { + kind: EntryKind::Tree, + .. + } => Ok(None), Side::Present { oid, - kind: EntryKind::Blob | EntryKind::BlobExecutable | EntryKind::Link, - } => Ok(self.blob_size(*oid)? <= MAX_DIFF_BLOB_BYTES), - _ => Ok(true), - } + kind: EntryKind::Commit, + } => Ok(Some(subproject_line(*oid).len() as u64)), + Side::Present { oid, .. } => self.blob_size(*oid).map(Some), + }; + let (old, new) = (size(old)?, size(new)?); + let past = |bytes: Option| bytes.is_some_and(|bytes| bytes > MAX_DIFF_BLOB_BYTES); + Ok((past(old) || past(new)).then(|| BinarySizes { + old: old.unwrap_or(0), + new: new.unwrap_or(0), + })) } fn file_patch( @@ -199,20 +342,31 @@ impl Repo { path: RepoPath, old: Side, new: Side, + budget: &mut BinaryBudget, ) -> Result { - let within_budget = - self.side_within_diff_budget(&old)? && self.side_within_diff_budget(&new)?; - let (binary, hunks) = match within_budget { - false => (true, Vec::new()), - true => { + let same_content = matches!( + (&old, &new), + (Side::Present { oid: before, .. }, Side::Present { oid: after, .. }) + if before == after + ); + let body = match self.sides_past_diff_budget(&old, &new)? { + Some(sizes) => PatchBody::Binary(BinaryDiff::Omitted(sizes)), + None => { let old_content = self.patch_content(&old)?; let new_content = self.patch_content(&new)?; - let binary = is_binary(&old_content) || is_binary(&new_content); - let hunks = match binary { - true => Vec::new(), - false => text_hunks(&old_content, &new_content)?, - }; - (binary, hunks) + match is_binary(&old_content) || is_binary(&new_content) { + true => { + let sizes = BinarySizes::of(&old_content, &new_content); + PatchBody::Binary(match same_content { + true => BinaryDiff::Unchanged(sizes.new), + false => match budget.admit(sizes) { + true => encode_binary(&old_content, &new_content)?, + false => BinaryDiff::Omitted(sizes), + }, + }) + } + false => PatchBody::Text(text_hunks(&old_content, &new_content)?), + } } }; Ok(FilePatch { @@ -222,8 +376,7 @@ impl Repo { new_oid: new.oid(self.object_format().null_oid()), old_kind: old.kind(), new_kind: new.kind(), - is_binary: binary, - hunks, + body, }) } @@ -296,7 +449,11 @@ impl Repo { } } - pub fn commit_patches(&self, range: PatchRange) -> Result, GitError> { + pub fn commit_patches( + &self, + range: PatchRange, + budget: &mut BinaryBudget, + ) -> Result, GitError> { let (old_tree, new_tree) = self.diff_trees(range)?; let mut sides: Vec<(PatchStatus, String, Side, Side)> = Vec::new(); old_tree @@ -383,8 +540,56 @@ impl Repo { .map(|(status, path, old, new)| { let path = RepoPath::new(path).map_err(|error| GitError::Decode(error.to_string()))?; - self.file_patch(status, path, old, new) + self.file_patch(status, path, old, new, budget) }) .collect() } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_wire_bound_covers_every_byte_the_patch_writes() { + let payload = |len: usize, fill: fn(usize) -> u8| (0..len).map(fill).collect::>(); + [0usize, 1, 3, 4, 51, 52, 53, 1000, 65_536] + .into_iter() + .flat_map(|len| { + let noise = payload(len, |index| (index as u8).wrapping_mul(37) ^ 0x5a); + [(payload(len, |_| 0), noise.clone()), (noise, Vec::new())] + }) + .for_each(|(old, new)| { + let BinaryDiff::Encoded { sizes, text } = encode_binary(&old, &new).unwrap() else { + panic!("encode_binary returns Encoded for every payload"); + }; + assert!( + text.len() as u64 <= sizes.wire_bound(), + "payload of {} bytes: expected at most {}, wrote {}", + old.len().max(new.len()), + sizes.wire_bound(), + text.len() + ); + }); + } + + #[test] + fn the_budget_reports_the_first_payload_it_refuses() { + let sizes = BinarySizes { old: 0, new: 4096 }; + let mut budget = BinaryBudget::new(sizes.wire_bound()); + assert!(budget.admit(sizes)); + assert!( + !budget.omitted(), + "omitted is false while admit returns true" + ); + assert!(!budget.admit(sizes)); + assert!(budget.omitted(), "omitted is true once admit returns false"); + + let mut omit = BinaryBudget::Omit; + assert!(!omit.admit(sizes)); + assert!( + !omit.omitted(), + "omitted is false under Omit, where admit always returns false" + ); + } +} diff --git a/knot2/crates/knot-git/src/patch_parse.rs b/knot2/crates/knot-git/src/patch_parse.rs index de202c2a..93614e97 100644 --- a/knot2/crates/knot-git/src/patch_parse.rs +++ b/knot2/crates/knot-git/src/patch_parse.rs @@ -1,9 +1,9 @@ use std::io::Read; -use std::sync::LazyLock; use base64::Engine; use knot_types::{AuthorName, Email, Oid}; +use crate::base85; use crate::objects::{CommitChangeId, EntryKind}; use crate::patch::{Hunk, HunkLine, LineCount, LineNumber, LineOp, MAX_DIFF_BLOB_BYTES}; @@ -200,6 +200,35 @@ fn unquote(raw: &str) -> Result { } } +const PRINTABLE_ASCII: std::ops::Range = 0x20..0x7f; + +fn needs_quoting(byte: u8) -> bool { + !PRINTABLE_ASCII.contains(&byte) || matches!(byte, b'"' | b'\\') +} + +pub fn quote_path(path: &str) -> String { + match path.bytes().any(needs_quoting) { + false => path.to_string(), + true => { + let mut quoted = path.bytes().fold(String::from("\""), |mut out, byte| { + match byte { + b'\n' => out.push_str("\\n"), + b'\t' => out.push_str("\\t"), + b'"' => out.push_str("\\\""), + b'\\' => out.push_str("\\\\"), + other if !PRINTABLE_ASCII.contains(&other) => { + out.push_str(&format!("\\{other:03o}")) + } + other => out.push(other as char), + } + out + }); + quoted.push('"'); + quoted + } + } +} + fn strip_level(path: &str) -> String { path.split_once('/') .map(|(_, rest)| rest.to_string()) @@ -224,15 +253,23 @@ fn diff_paths(rest: &str) -> Option<(String, String)> { let (new, _) = take_path_token(after.strip_prefix(' ')?)?; Some((strip_level(&old), strip_level(&new))) } - false => { - let split = rest.rfind(" b/")?; - let old = rest.get(..split)?.strip_prefix("a/")?; - let new = rest.get(split + 3..)?; - Some((old.to_string(), new.to_string())) - } + false => unquoted_diff_paths(rest), } } +fn unquoted_diff_paths(rest: &str) -> Option<(String, String)> { + let split_at = |at: usize| { + let old = rest.get(..at)?.strip_prefix("a/")?; + let new = rest.get(at + " b/".len()..)?; + Some((old, new)) + }; + rest.match_indices(" b/") + .filter_map(|(at, _)| split_at(at)) + .find(|(old, new)| old == new) + .or_else(|| split_at(rest.rfind(" b/")?)) + .map(|(old, new)| (old.to_string(), new.to_string())) +} + fn take_path_token(rest: &str) -> Option<(String, &str)> { match rest.strip_prefix('"') { Some(inner) => { @@ -248,7 +285,7 @@ fn take_path_token(rest: &str) -> Option<(String, &str)> { } fn full_oid(hex: &str) -> Option { - (hex.len() == 40).then(|| Oid::from_hex(hex).ok()).flatten() + Oid::from_hex(hex).ok() } fn parse_hunk_header(line: &str) -> Option<(LineNumber, LineCount, LineNumber, LineCount)> { @@ -344,46 +381,6 @@ fn parse_hunks(cursor: &mut Cursor<'_>, budget: &mut Budget) -> Result .collect() } -static BASE85: LazyLock<[i16; 256]> = LazyLock::new(|| { - const ALPHABET: &[u8] = - b"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz!#$%&()*+-;<=>?@^_`{|}~"; - std::array::from_fn(|byte| { - ALPHABET - .iter() - .position(|&c| c as usize == byte) - .map(|digit| digit as i16) - .unwrap_or(-1) - }) -}); - -fn decode_base85_line(line: &str, out: &mut Vec) -> Result<(), PatchParseError> { - let bad = || malformed("bad base85 line in binary patch"); - let (len_char, data) = line.as_bytes().split_first().ok_or_else(bad)?; - let line_len = match len_char { - b'A'..=b'Z' => (len_char - b'A' + 1) as usize, - b'a'..=b'z' => (len_char - b'a' + 27) as usize, - _ => return Err(bad()), - }; - if data.len() != line_len.div_ceil(4) * 5 { - return Err(bad()); - } - data.chunks(5) - .enumerate() - .try_for_each(|(group, chunk)| -> Result<(), PatchParseError> { - let acc = chunk - .iter() - .try_fold(0u64, |acc, &c| { - let digit = BASE85[c as usize]; - (digit >= 0).then(|| acc * 85 + digit as u64) - }) - .filter(|&acc| acc <= u32::MAX as u64) - .ok_or_else(bad)?; - let take = (line_len - group * 4).min(4); - out.extend_from_slice(&(acc as u32).to_be_bytes()[..take]); - Ok(()) - }) -} - fn parse_binary_block( cursor: &mut Cursor<'_>, budget: &mut Budget, @@ -414,7 +411,10 @@ fn parse_binary_block( .is_some_and(|line| !line.is_empty()) .then(|| cursor.next().expect("peeked line is present")) }) - .try_for_each(|line| decode_base85_line(line, &mut packed))?; + .try_for_each(|line| { + base85::decode_line(line, &mut packed) + .map_err(|_| malformed("invalid base85 line in binary patch")) + })?; cursor.next(); let mut inflated: Vec = Vec::new(); flate2::read::ZlibDecoder::new(packed.as_slice()) @@ -970,6 +970,99 @@ mod tests { assert!(unquote("\"a/broken").is_err()); } + #[test] + fn a_plain_path_stays_bare_and_a_quoted_one_unquotes_back() { + [ + "a/reef.txt", + "a/~tilde", + "a/{brace}", + "a/sp ace.txt", + "a/b/ b/c", + ] + .into_iter() + .for_each(|plain| { + assert_eq!(quote_path(plain), plain, "git leaves {plain} bare too"); + }); + [ + "a/quote\".txt", + "a/back\\slash", + "a/tab\there", + "a/new\nline", + "a/é", + "a/\u{7f}del", + ] + .into_iter() + .for_each(|awkward| { + let quoted = quote_path(awkward); + assert!(quoted.starts_with('"'), "{awkward} must come out quoted"); + assert_eq!( + unquote("ed).unwrap(), + awkward, + "{awkward} must unquote back to itself" + ); + }); + } + + #[test] + fn diff_paths_splits_a_header_whose_path_holds_the_separator() { + [ + ("a/b/ b/c.bin b/b/ b/c.bin", "b/ b/c.bin", "b/ b/c.bin"), + ( + "\"a/quote\\\".bin\" \"b/quote\\\".bin\"", + "quote\".bin", + "quote\".bin", + ), + ( + "a/old name.txt b/new name.txt", + "old name.txt", + "new name.txt", + ), + ] + .into_iter() + .for_each(|(header, old, new)| { + assert_eq!( + diff_paths(header), + Some((old.to_string(), new.to_string())), + "the a-side and b-side must agree on the split: {header}" + ); + }); + } + + #[test] + fn a_binary_file_named_around_the_separator_keeps_its_path() { + let patch = concat!( + "diff --git a/b/ b/c.bin b/b/ b/c.bin\n", + "index 1111111111111111111111111111111111111111..2222222222222222222222222222222222222222 100644\n", + "GIT binary patch\n", + "literal 4\n", + "LcmZRms;UA20^k8}\n", + "\n", + "literal 5\n", + "Mcmb RepoPath { RepoPath::new(path).unwrap() } +fn patches_of( + bare: &Repo, + base: Option, + head: Oid, + budget: &mut BinaryBudget, +) -> Vec { + bare.commit_patches(knot_git::PatchRange { base, head }, budget) + .unwrap() +} + +fn named<'a>(patches: &'a [FilePatch], path: &str) -> &'a FilePatch { + patches + .iter() + .find(|patch| patch.path.as_str() == path) + .unwrap() +} + +fn binary_diff(patch: &FilePatch) -> &BinaryDiff { + match &patch.body { + PatchBody::Binary(diff) => diff, + PatchBody::Text(_) => panic!("expected a binary patch for {}, found text", patch.path), + } +} + mod common; use common::{commit_file, contains, git_ok as git}; @@ -289,19 +316,14 @@ fn typed_reads_over_a_rich_repo() { git(work, &["log", "-1", "--format=%H", "--", "src/lib.rs"]) ); - let patches = bare - .commit_patches(knot_git::PatchRange { - base: Some(parent), - head, - }) - .unwrap(); + let patches = patches_of(&bare, Some(parent), head, &mut BinaryBudget::Omit); assert_eq!(patches.len(), 1); let patch = &patches[0]; assert_eq!(patch.path.as_str(), "src/lib.rs"); assert_eq!(patch.status, knot_git::PatchStatus::Modified); - assert!(!patch.is_binary); - assert_eq!(patch.hunks.len(), 1); - let hunk = &patch.hunks[0]; + assert!(!patch.is_binary()); + assert_eq!(patch.hunks().len(), 1); + let hunk = &patch.hunks()[0]; assert_eq!( ( hunk.old_start.get(), @@ -321,20 +343,15 @@ fn typed_reads_over_a_rich_repo() { vec!["pub fn nel() {}\n", "pub fn teq() {}\n"] ); - let initial = bare - .commit_patches(knot_git::PatchRange { - base: None, - head: root, - }) - .unwrap(); + let initial = patches_of(&bare, None, root, &mut BinaryBudget::Omit); assert_eq!(initial.len(), 1); assert_eq!(initial[0].status, knot_git::PatchStatus::Added); assert_eq!( - initial[0].hunks[0].old_start.get(), + initial[0].hunks()[0].old_start.get(), 0, "added file hunk starts at -0,0" ); - assert_eq!(initial[0].hunks[0].old_lines.get(), 0); + assert_eq!(initial[0].hunks()[0].old_lines.get(), 0); let tag_commit = bare.peel_to_commit(tag_object).unwrap(); assert_eq!( @@ -595,23 +612,29 @@ fn extended_history_reads() { let bare = layout.open(&did).unwrap(); let head = Oid::from_hex(&git(work, &["rev-parse", "HEAD"])).unwrap(); let parent = Oid::from_hex(&git(work, &["rev-parse", "HEAD~1"])).unwrap(); - let patches = bare - .commit_patches(knot_git::PatchRange { - base: Some(parent), - head, - }) - .unwrap(); - let binary = patches - .iter() - .find(|patch| patch.path.as_str() == "blob.bin") - .unwrap(); - assert!(binary.is_binary); - assert!(binary.hunks.is_empty()); - let noeol = patches - .iter() - .find(|patch| patch.path.as_str() == "noeol.txt") + let patches = patches_of(&bare, Some(parent), head, &mut BinaryBudget::new(1024)); + let blob = binary_diff(named(&patches, "blob.bin")); + assert!( + matches!(blob, BinaryDiff::Encoded { .. }), + "a blob under the budget is encoded" + ); + let mut thin = BinaryBudget::new(5); + let thinned = patches_of(&bare, Some(parent), head, &mut thin); + let starved = binary_diff(named(&thinned, "blob.bin")); + assert!( + matches!(starved, BinaryDiff::Omitted(_)), + "a blob past the budget is omitted" + ); + assert!(thin.omitted(), "omitted is true once a blob is left out"); + assert_eq!( + [blob.sizes(), starved.sizes()].map(|sizes| (sizes.old, sizes.new)), + [(0, 6); 2], + "a new file has an empty pre-image, encoded or omitted" + ); + let last = named(&patches, "noeol.txt").hunks()[0] + .lines + .last() .unwrap(); - let last = noeol.hunks[0].lines.last().unwrap(); assert_eq!(last.text, b"no newline at end".to_vec()); } @@ -764,19 +787,19 @@ fn an_oversized_blob_diffs_as_binary_without_loading_it() { let bare = layout.open(&did).unwrap(); let head = Oid::from_hex(&git(&clone, &["rev-parse", "HEAD"])).unwrap(); let parent = Oid::from_hex(&git(&clone, &["rev-parse", "HEAD~1"])).unwrap(); - let patches = bare - .commit_patches(knot_git::PatchRange { - base: Some(parent), - head, - }) - .unwrap(); - let huge = patches - .iter() - .find(|patch| patch.path.as_str() == "huge.txt") - .unwrap(); + let patches = patches_of(&bare, Some(parent), head, &mut BinaryBudget::new(u64::MAX)); + let huge = named(&patches, "huge.txt"); assert!( - huge.is_binary, + huge.is_binary() && huge.hunks().is_empty(), "blob past diff budget falls back to binary instead of being loaded" ); - assert!(huge.hunks.is_empty()); + assert!( + matches!(binary_diff(huge), BinaryDiff::Omitted(_)), + "a blob nobody read has no bytes to encode, whatever the budget allows" + ); + assert_eq!( + binary_diff(huge).sizes().new, + knot_git::MAX_DIFF_BLOB_BYTES + 1, + "the size comes from the blob header the knot did read" + ); } diff --git a/knot2/crates/knot-xrpc/src/lib.rs b/knot2/crates/knot-xrpc/src/lib.rs index 71bdd7b3..70c78581 100644 --- a/knot2/crates/knot-xrpc/src/lib.rs +++ b/knot2/crates/knot-xrpc/src/lib.rs @@ -129,6 +129,17 @@ impl Default for ByteLimits { } } +const BINARY_RESPONSE_SHARE: u64 = 4; +const BINARY_WIRE_COPIES: u64 = 3; + +impl ByteLimits { + pub fn binary_patch(self) -> knot_git::BinaryBudget { + knot_git::BinaryBudget::new( + self.response.get() as u64 / BINARY_RESPONSE_SHARE / BINARY_WIRE_COPIES, + ) + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Budgets { pub tree_last_commit: TreeReadBudget, diff --git a/knot2/crates/knot-xrpc/src/patchtext.rs b/knot2/crates/knot-xrpc/src/patchtext.rs index 7fb94285..dfbf3216 100644 --- a/knot2/crates/knot-xrpc/src/patchtext.rs +++ b/knot2/crates/knot-xrpc/src/patchtext.rs @@ -1,9 +1,16 @@ -use knot_git::{Commit, FilePatch, Hunk, LineCount, LineNumber, LineOp, PatchStatus}; +use knot_git::{ + BinaryDiff, Commit, EntryKind, FilePatch, Hunk, LineCount, LineNumber, LineOp, PatchBody, + PatchStatus, quote_path, +}; use crate::wire::{entry_mode_octal, fold_subject, message_body, rfc2822}; const GRAPH_WIDTH: usize = 60; +fn entry_mode(kind: Option) -> String { + kind.map(entry_mode_octal).unwrap_or_default() +} + fn span(start: LineNumber, lines: LineCount) -> String { match lines.get() { 1 => format!("{}", start.get()), @@ -31,66 +38,58 @@ fn render_hunk(out: &mut String, hunk: &Hunk) { } fn render_file(out: &mut String, patch: &FilePatch) { - let (a, b) = (&patch.path, &patch.path); - out.push_str(&format!("diff --git a/{a} b/{b}\n")); + let old_side = quote_path(&format!("a/{}", patch.path)); + let new_side = quote_path(&format!("b/{}", patch.path)); + let index = format!( + "index {}..{}", + patch.old_oid.to_hex(), + patch.new_oid.to_hex() + ); + out.push_str(&format!("diff --git {old_side} {new_side}\n")); match patch.status { - PatchStatus::Added => { - let mode = patch.new_kind.map(entry_mode_octal).unwrap_or_default(); - out.push_str(&format!("new file mode {mode}\n")); - out.push_str(&format!( - "index {}..{}\n", - patch.old_oid.to_hex(), - patch.new_oid.to_hex() - )); + PatchStatus::Added => out.push_str(&format!( + "new file mode {}\n{index}\n", + entry_mode(patch.new_kind) + )), + PatchStatus::Deleted => out.push_str(&format!( + "deleted file mode {}\n{index}\n", + entry_mode(patch.old_kind) + )), + PatchStatus::Modified if patch.old_kind == patch.new_kind => { + out.push_str(&format!("{index} {}\n", entry_mode(patch.old_kind))) } - PatchStatus::Deleted => { - let mode = patch.old_kind.map(entry_mode_octal).unwrap_or_default(); - out.push_str(&format!("deleted file mode {mode}\n")); + PatchStatus::Modified => { out.push_str(&format!( - "index {}..{}\n", - patch.old_oid.to_hex(), - patch.new_oid.to_hex() + "old mode {}\nnew mode {}\n", + entry_mode(patch.old_kind), + entry_mode(patch.new_kind) )); - } - PatchStatus::Modified => { - if patch.old_kind == patch.new_kind { - let mode = patch.old_kind.map(entry_mode_octal).unwrap_or_default(); - out.push_str(&format!( - "index {}..{} {mode}\n", - patch.old_oid.to_hex(), - patch.new_oid.to_hex() - )); - } else { - let old = patch.old_kind.map(entry_mode_octal).unwrap_or_default(); - let new = patch.new_kind.map(entry_mode_octal).unwrap_or_default(); - out.push_str(&format!("old mode {old}\nnew mode {new}\n")); - out.push_str(&format!( - "index {}..{}\n", - patch.old_oid.to_hex(), - patch.new_oid.to_hex() - )); + match patch.old_oid == patch.new_oid { + true => {} + false => out.push_str(&format!("{index}\n")), } } } let old_label = match patch.status { - PatchStatus::Added => "/dev/null".to_string(), - _ => format!("a/{a}"), + PatchStatus::Added => "/dev/null", + _ => old_side.as_str(), }; let new_label = match patch.status { - PatchStatus::Deleted => "/dev/null".to_string(), - _ => format!("b/{b}"), + PatchStatus::Deleted => "/dev/null", + _ => new_side.as_str(), }; - if patch.is_binary { - out.push_str(&format!( + match &patch.body { + PatchBody::Binary(BinaryDiff::Encoded { text, .. }) => out.push_str(text), + PatchBody::Binary(BinaryDiff::Omitted(_)) => out.push_str(&format!( "Binary files {old_label} and {new_label} differ\n" - )); - return; - } - if patch.hunks.is_empty() { - return; + )), + PatchBody::Binary(BinaryDiff::Unchanged(_)) => {} + PatchBody::Text(hunks) if hunks.is_empty() => {} + PatchBody::Text(hunks) => { + out.push_str(&format!("--- {old_label}\n+++ {new_label}\n")); + hunks.iter().for_each(|hunk| render_hunk(out, hunk)); + } } - out.push_str(&format!("--- {old_label}\n+++ {new_label}\n")); - patch.hunks.iter().for_each(|hunk| render_hunk(out, hunk)); } pub(crate) fn render_patches(patches: &[FilePatch]) -> String { @@ -101,7 +100,7 @@ pub(crate) fn render_patches(patches: &[FilePatch]) -> String { } fn stat_counts(patch: &FilePatch) -> (usize, usize) { - patch.hunks.iter().fold((0, 0), |(added, deleted), hunk| { + patch.hunks().iter().fold((0, 0), |(added, deleted), hunk| { ( added + hunk.added().get() as usize, deleted + hunk.deleted().get() as usize, @@ -120,25 +119,27 @@ fn graph(added: usize, deleted: usize) -> String { } fn diffstat(patches: &[FilePatch]) -> String { - let width = patches + let named: Vec<(String, &FilePatch)> = patches .iter() - .map(|patch| patch.path.as_str().len()) - .max() - .unwrap_or(0); - let rows: String = patches + .map(|patch| (quote_path(patch.path.as_str()), patch)) + .collect(); + let width = named.iter().map(|(name, _)| name.len()).max().unwrap_or(0); + let rows: String = named .iter() - .map(|patch| { - if patch.is_binary { - format!(" {: format!(" {name: { + let sizes = binary.sizes(); format!( - " {: {} bytes\n", + sizes.old, sizes.new ) } + PatchBody::Text(_) => { + let (added, deleted) = stat_counts(patch); + let total = added + deleted; + format!(" {name: String { )); } summary.push('\n'); - let created: String = patches + let modes: String = named .iter() - .filter(|patch| patch.status == PatchStatus::Added) - .map(|patch| { - format!( - " create mode {} {}\n", - patch.new_kind.map(entry_mode_octal).unwrap_or_default(), - patch.path - ) - }) - .collect(); - let deleted_rows: String = patches - .iter() - .filter(|patch| patch.status == PatchStatus::Deleted) - .map(|patch| { - format!( - " delete mode {} {}\n", - patch.old_kind.map(entry_mode_octal).unwrap_or_default(), - patch.path - ) + .filter_map(|(name, patch)| { + let (old, new) = (entry_mode(patch.old_kind), entry_mode(patch.new_kind)); + match patch.status { + PatchStatus::Added => Some(format!(" create mode {new} {name}\n")), + PatchStatus::Deleted => Some(format!(" delete mode {old} {name}\n")), + PatchStatus::Modified if patch.old_kind != patch.new_kind => { + Some(format!(" mode change {old} => {new} {name}\n")) + } + PatchStatus::Modified => None, + } }) .collect(); - format!("{rows}{summary}{created}{deleted_rows}") + format!("{rows}{summary}{modes}") } pub(crate) fn render_format_patch(commit: &Commit, patches: &[FilePatch]) -> String { diff --git a/knot2/crates/knot-xrpc/src/reads.rs b/knot2/crates/knot-xrpc/src/reads.rs index e5451142..78a24d58 100644 --- a/knot2/crates/knot-xrpc/src/reads.rs +++ b/knot2/crates/knot-xrpc/src/reads.rs @@ -13,8 +13,8 @@ use tower_http::services::ServeFile; use knot_cobs::RepoRef; use knot_git::{ - ArchiveFormat, Commit, CommitRange, EntryKind, Layout, LogLimit, LogSkip, Repo, SizedEntry, - is_public_ref, screens_reserved, + ArchiveFormat, BinaryBudget, Commit, CommitRange, EntryKind, Layout, LogLimit, LogSkip, Repo, + SizedEntry, is_public_ref, screens_reserved, }; use knot_index::{Coverage, Resolved}; use knot_runtime::{Clock, HttpTransport}; @@ -901,10 +901,13 @@ pub(crate) async fn repo_diff( let repo = open(&layout, &did)?; let target = commit_for(&repo, ¶ms.refspec)?; let commit = repo.find_commit(target)?; - let patches = repo.commit_patches(knot_git::PatchRange { - base: commit.parents.first().copied(), - head: target, - })?; + let patches = repo.commit_patches( + knot_git::PatchRange { + base: commit.parents.first().copied(), + head: target, + }, + &mut BinaryBudget::Omit, + )?; json( DiffOut { refspec: params.refspec.as_str().to_string(), @@ -939,6 +942,8 @@ struct CompareOut { combined_patch: Option>, #[serde(skip_serializing_if = "Option::is_none")] combined_patch_raw: Option, + #[serde(skip_serializing_if = "Option::is_none")] + binary_omitted: Option, } fn format_patch_entry( @@ -1009,6 +1014,8 @@ pub(crate) async fn repo_compare( } let layout = state.layout.clone(); let limit = state.byte_limits.response.get(); + let mut series_binary = state.byte_limits.binary_patch(); + let mut combined_binary = state.byte_limits.binary_patch(); run_blocking(move || { let repo = open(&layout, &did)?; let resolve = |rev: &str| { @@ -1069,10 +1076,13 @@ pub(crate) async fn repo_compare( let entries: Vec<(FormatPatchWire, String)> = commits .iter() .map(|commit| { - repo.commit_patches(knot_git::PatchRange { - base: commit.parents.first().copied(), - head: commit.id, - }) + repo.commit_patches( + knot_git::PatchRange { + base: commit.parents.first().copied(), + head: commit.id, + }, + &mut series_binary, + ) .map(|patches| { let raw = render_format_patch(commit, &patches); (format_patch_entry(commit, &patches, &raw), raw) @@ -1080,14 +1090,20 @@ pub(crate) async fn repo_compare( }) .collect::, _>>() .map_err(compare_error)?; - let patch_raw: String = entries.iter().map(|(_, raw)| format!("{raw}\n")).collect(); + let patch_raw: String = entries + .iter() + .flat_map(|(_, raw)| [raw.as_str(), "\n"]) + .collect(); let merge_base = repo.merge_base(base, head).ok().flatten(); - let (combined_patch, combined_patch_raw) = match (entries.len() >= 2, merge_base) { + let (combined_patch, combined_patch_raw) = match (commits.len() >= 2, merge_base) { (true, Some(merge_base)) => repo - .commit_patches(knot_git::PatchRange { - base: Some(merge_base), - head, - }) + .commit_patches( + knot_git::PatchRange { + base: Some(merge_base), + head, + }, + &mut combined_binary, + ) .ok() .map(|patches| { ( @@ -1107,6 +1123,8 @@ pub(crate) async fn repo_compare( patch_raw, combined_patch, combined_patch_raw, + binary_omitted: (series_binary.omitted() || combined_binary.omitted()) + .then_some(true), }, limit, ) diff --git a/knot2/crates/knot-xrpc/src/tests.rs b/knot2/crates/knot-xrpc/src/tests.rs index f0b42b24..dd367f48 100644 --- a/knot2/crates/knot-xrpc/src/tests.rs +++ b/knot2/crates/knot-xrpc/src/tests.rs @@ -3408,4 +3408,24 @@ mod legacy_admin_route { "past the burst the knot sheds the guess flood before it reaches the secret comparison, got {statuses:?}" ); } + + #[test] + fn every_wire_copy_of_an_embedded_payload_fits_a_quarter_of_the_response() { + let limits = crate::ByteLimits::default(); + assert_eq!(limits.response.get(), 5 * 1024 * 1024); + let response = limits.response.get() as u64; + assert_eq!( + limits.binary_patch(), + knot_git::BinaryBudget::new(response / 4 / 3), + "a compare serialises the series twice and the combined patch once" + ); + let per_pass = match limits.binary_patch() { + knot_git::BinaryBudget::Spend { remaining, .. } => remaining, + knot_git::BinaryBudget::Omit => panic!("binary_patch spends, it doesn't omit"), + }; + assert!( + per_pass * 3 <= response / 4, + "three copies of {per_pass} bytes must stay inside a quarter of {response}" + ); + } } diff --git a/knot2/crates/knot-xrpc/src/wire.rs b/knot2/crates/knot-xrpc/src/wire.rs index 87620638..1b3156dd 100644 --- a/knot2/crates/knot-xrpc/src/wire.rs +++ b/knot2/crates/knot-xrpc/src/wire.rs @@ -394,7 +394,7 @@ pub(crate) struct DiffWire { impl DiffWire { pub fn of(patch: &FilePatch) -> Self { let fragments: Vec = - patch.hunks.iter().map(TextFragmentWire::of).collect(); + patch.hunks().iter().map(TextFragmentWire::of).collect(); Self { name: DiffNameWire { old: match patch.status { @@ -407,7 +407,7 @@ impl DiffWire { }, }, text_fragments: (!fragments.is_empty()).then_some(fragments), - is_binary: patch.is_binary, + is_binary: patch.is_binary(), is_new: patch.status == PatchStatus::Added, is_delete: patch.status == PatchStatus::Deleted, is_copy: false, @@ -435,12 +435,12 @@ pub(crate) fn nice_diff(commit: &Commit, patches: &[FilePatch]) -> NiceDiffWire let stat = DiffStatWire { insertions: patches .iter() - .flat_map(|patch| patch.hunks.iter()) + .flat_map(|patch| patch.hunks().iter()) .map(|hunk| hunk.added().get() as i64) .sum(), deletions: patches .iter() - .flat_map(|patch| patch.hunks.iter()) + .flat_map(|patch| patch.hunks().iter()) .map(|hunk| hunk.deleted().get() as i64) .sum(), files_changed: patches.len() as i64, @@ -558,7 +558,7 @@ pub(crate) struct FileWire { impl FileWire { pub fn of(patch: &FilePatch) -> Self { let fragments: Vec = - patch.hunks.iter().map(TextFragmentWire::of).collect(); + patch.hunks().iter().map(TextFragmentWire::of).collect(); let same_mode = patch.old_kind.is_some() && patch.old_kind == patch.new_kind; Self { old_name: match patch.status { @@ -589,7 +589,7 @@ impl FileWire { new_oid_prefix: patch.new_oid.to_hex(), score: 0, text_fragments: (!fragments.is_empty()).then_some(fragments), - is_binary: patch.is_binary, + is_binary: patch.is_binary(), binary_fragment: None, reverse_binary_fragment: None, } diff --git a/knot2/crates/knot-xrpc/tests/common/mod.rs b/knot2/crates/knot-xrpc/tests/common/mod.rs index e34abf7b..f14c396e 100644 --- a/knot2/crates/knot-xrpc/tests/common/mod.rs +++ b/knot2/crates/knot-xrpc/tests/common/mod.rs @@ -1,6 +1,7 @@ #![allow(dead_code)] use std::collections::BTreeSet; +use std::os::unix::fs::PermissionsExt; use std::path::Path; use std::sync::Arc; @@ -448,25 +449,13 @@ pub fn commit_file(work: &Path, file: &str, contents: &[u8], message: &str, when } pub fn seeded(world: &World, rkey: &str) -> (RepoDid, tempfile::TempDir) { - seeded_with_format(world, rkey, ObjectFormat::SHA1) -} - -pub fn seeded_with_format( - world: &World, - rkey: &str, - object_format: ObjectFormat, -) -> (RepoDid, tempfile::TempDir) { let did = RepoDid::new(format!("did:plc:{rkey}fixture")).unwrap(); - world.layout.create(&did).unwrap(); + let object_format = world.layout.create(&did).unwrap().object_format(); world.register(&did, rkey); let bare = world.layout.repo_path(&did).unwrap(); let work_dir = tempfile::tempdir().unwrap(); let work = work_dir.path(); - let init = match object_format == ObjectFormat::SHA256 { - true => vec!["init", "-q", "--object-format=sha256", "-b", "main"], - false => vec!["init", "-q", "-b", "main"], - }; - sh_git(work, &init); + sh_git(work, &init_args(object_format)); commit_file( work, "README.md", @@ -508,9 +497,16 @@ pub fn seeded_with_format( (did, work_dir) } +fn init_args(object_format: ObjectFormat) -> Vec<&'static str> { + match object_format == ObjectFormat::SHA256 { + true => vec!["init", "-q", "--object-format=sha256", "-b", "main"], + false => vec!["init", "-q", "-b", "main"], + } +} + pub fn empty_repo(world: &World, rkey: &str) -> (RepoDid, String, tempfile::TempDir) { let did = RepoDid::new(format!("did:plc:{rkey}fixture")).unwrap(); - world.layout.create(&did).unwrap(); + let object_format = world.layout.create(&did).unwrap().object_format(); world.register(&did, rkey); let bare = world .layout @@ -520,39 +516,75 @@ pub fn empty_repo(world: &World, rkey: &str) -> (RepoDid, String, tempfile::Temp .unwrap() .to_string(); let work_dir = tempfile::tempdir().unwrap(); - sh_git(work_dir.path(), &["init", "-q", "-b", "main"]); + sh_git(work_dir.path(), &init_args(object_format)); (did, bare, work_dir) } +fn shell_bytes(seed: u8) -> Vec { + let stream = std::iter::successors(Some(seed as u32), |state| { + Some(state.wrapping_mul(1_664_525).wrapping_add(1_013_904_223)) + }) + .map(|state| (state >> 16) as u8); + std::iter::once(0).chain(stream).take(4096).collect() +} + +fn at(clock: &str) -> String { + format!("2026-06-01T{clock}+02:00") +} + pub fn seeded_feature_branch(world: &World, rkey: &str) -> (RepoDid, Oid, Oid) { let (did, bare, work_dir) = empty_repo(world, rkey); let work = work_dir.path(); - commit_file( - work, - "reef.txt", - b"one\ntwo\n", - "base", - "2026-06-01T12:30:00+02:00", - ); + let commits = |list: Vec<(&str, Vec, &str, &str)>| { + list.into_iter().for_each(|(file, bytes, message, minute)| { + commit_file(work, file, &bytes, message, &at(minute)) + }); + }; + commits(vec![ + ("reef.txt", b"one\ntwo\n".into(), "base", "12:30:00"), + ("shell.bin", shell_bytes(0), "add shell", "12:30:10"), + ("anchor.bin", shell_bytes(211), "add anchor", "12:30:20"), + ("hull.bin", shell_bytes(159), "add hull", "12:30:30"), + ]); sh_git(work, &["checkout", "-q", "-b", "feature"]); - commit_file( + std::fs::set_permissions( + work.join("hull.bin"), + std::fs::Permissions::from_mode(0o755), + ) + .unwrap(); + sh_git_at(work, &at("12:30:40"), &["add", "-A"]); + sh_git_at( work, - "reef.txt", - b"one\nTWO\n", - "capitalize two\n\nbecause waves", - "2026-06-01T12:31:00+02:00", + &at("12:30:40"), + &["commit", "-q", "-m", "make hull runnable"], ); - commit_file( + commits(vec![ + ( + "reef.txt", + b"one\nTWO\n".into(), + "capitalize two\n\nbecause waves", + "12:31:00", + ), + ("kelp.txt", b"frond\n".into(), "add kelp", "12:31:10"), + ("shell.bin", shell_bytes(97), "reshape shell", "12:31:20"), + ("pearl.bin", vec![0, 255, 12, 0, 9], "add pearl", "12:31:30"), + ( + "deep water.bin", + shell_bytes(43), + "add deep water", + "12:31:40", + ), + ]); + std::fs::remove_file(work.join("anchor.bin")).unwrap(); + sh_git_at(work, &at("12:32:00"), &["add", "-A"]); + sh_git_at( work, - "kelp.txt", - b"frond\n", - "add kelp", - "2026-06-01T12:32:00+02:00", + &at("12:32:00"), + &["commit", "-q", "-m", "drop anchor"], ); sh_git(work, &["push", "-q", &bare, "main", "feature"]); - let main = Oid::from_hex(&sh_git(work, &["rev-parse", "main"])).unwrap(); - let feature = Oid::from_hex(&sh_git(work, &["rev-parse", "feature"])).unwrap(); - (did, main, feature) + let tip = |name: &str| Oid::from_hex(&sh_git(work, &["rev-parse", name])).unwrap(); + (did, tip("main"), tip("feature")) } pub async fn get_with_headers( diff --git a/knot2/crates/knot-xrpc/tests/reads.rs b/knot2/crates/knot-xrpc/tests/reads.rs index fbf529a1..01da15e9 100644 --- a/knot2/crates/knot-xrpc/tests/reads.rs +++ b/knot2/crates/knot-xrpc/tests/reads.rs @@ -10,14 +10,13 @@ use http::{HeaderMap, StatusCode, header}; use tokio_tungstenite::tungstenite; use knot_events::{EventCursor, GitRefUpdate}; -use knot_types::{AccountDid, ObjectFormat, Oid, OwnerDid, RepoDid}; +use knot_types::{AccountDid, Oid, OwnerDid, RepoDid}; use knot_xrpc::{ArchiveLimit, ResponseLimit}; use common::{ OWNER, World, archive_full, assert_immutable_round_trip, assert_post_rejected, assert_warming, commit_file, empty_repo, get, get_error, get_json, get_with_headers, git_run, post_authed, - post_json, ref_names, repo_dids, seeded, seeded_feature_branch, seeded_with_format, sh_git, - sh_git_at, + post_json, ref_names, repo_dids, seeded, seeded_feature_branch, sh_git, sh_git_at, }; #[tokio::test] @@ -790,7 +789,7 @@ async fn archive_link_advertises_the_prefix_it_served() { #[tokio::test] async fn archive_serves_a_sha256_repo_with_a_stable_etag() { let world = World::sha256(); - let (did, _work) = seeded_with_format(&world, "nautilus", ObjectFormat::SHA256); + let (did, _work) = seeded(&world, "nautilus"); let (status, headers, full) = get( &world, @@ -1588,51 +1587,100 @@ async fn languages_omit_files_with_zero_size() { #[tokio::test] async fn a_compare_patch_round_trips_through_merge_check() { - let world = World::new(); - let (_did, main_sha, feature_sha) = seeded_feature_branch(&world, "periwinkle"); - let registered = RepoDid::new("did:plc:periwinklefixture").unwrap(); + compare_round_trip(World::new()).await; +} + +#[tokio::test] +async fn a_sha256_compare_patch_round_trips_through_merge_check() { + compare_round_trip(World::sha256()).await; +} + +async fn compare_round_trip(world: World) { + let (did, main_sha, feature_sha) = seeded_feature_branch(&world, "periwinkle"); let compared = get_json( &world, - &format!( - "/xrpc/sh.tangled.repo.compare?repo={registered}&rev1={main_sha}&rev2={feature_sha}" - ), + &format!("/xrpc/sh.tangled.repo.compare?repo={did}&rev1={main_sha}&rev2={feature_sha}"), ) .await; let patch = compared["patch"].as_str().unwrap(); - - let (status, check) = post_json( - &world, - "/xrpc/sh.tangled.repo.mergeCheck", - serde_json::json!({ - "repo": registered, - "branch": "main", - "patch": patch, - }), - ) - .await; - assert_eq!(status, StatusCode::OK); + let combined = compared["combined_patch_raw"].as_str().unwrap(); + + let checks = stream::iter([("main", patch), ("main", combined), ("feature", patch)]) + .then(|(branch, candidate)| { + post_json( + &world, + "/xrpc/sh.tangled.repo.mergeCheck", + serde_json::json!({ + "repo": did, + "branch": branch, + "patch": candidate, + }), + ) + }) + .collect::>() + .await; assert_eq!( - check["is_conflicted"], - serde_json::Value::Bool(false), - "knot's own compare output must pass its own merge check: {check}" + checks + .iter() + .map(|(status, check)| (*status, check["is_conflicted"].clone())) + .collect::>(), + [false, false, true] + .map(|conflicted| (StatusCode::OK, serde_json::Value::Bool(conflicted))) + .to_vec(), + "main takes both patches and feature already has them: {checks:?}" + ); + + [ + "GIT binary patch", + " shell.bin | Bin 4096 -> 4096 bytes\n", + "deleted file mode 100644\n", + " delete mode 100644 anchor.bin\n", + "diff --git a/deep water.bin b/deep water.bin\n", + "old mode 100644\nnew mode 100755\n", + " mode change 100644 => 100755 hull.bin\n", + " hull.bin | Bin\n", + ] + .into_iter() + .for_each(|needle| { + assert!( + patch.contains(needle), + "the format patch is missing {needle:?}: {patch}" + ) + }); + assert!( + combined.contains("GIT binary patch"), + "the combined patch is missing its binary payloads: {combined}" + ); + assert!( + !patch.contains("new mode 100755\nindex "), + "a mode change leaves both sides at the same oid, so git prints no index line: {patch}" + ); + assert_eq!( + compared.get("binary_omitted"), + None, + "binary_omitted is absent when every payload was embedded: {compared}" ); - let (status, stale) = post_json( - &world, - "/xrpc/sh.tangled.repo.mergeCheck", - serde_json::json!({ - "repo": registered, - "branch": "feature", - "patch": patch, - }), - ) - .await; - assert_eq!(status, StatusCode::OK); + let applied = tempfile::tempdir().unwrap(); + let bare = world.layout.repo_path(&did).unwrap(); + sh_git( + applied.path(), + &["clone", "-q", bare.to_str().unwrap(), "."], + ); + sh_git(applied.path(), &["checkout", "-q", "main"]); + std::fs::write(applied.path().join("knot.patch"), patch).unwrap(); + sh_git(applied.path(), &["am", "knot.patch"]); assert_eq!( - stale["is_conflicted"], - serde_json::Value::Bool(true), - "re-applying an already-landed patch must conflict: {stale}" + sh_git(applied.path(), &["rev-parse", "HEAD^{tree}"]), + sh_git(applied.path(), &["rev-parse", "origin/feature^{tree}"]), + "git am of the knot's own patch rebuilds the tree the branch already has" + ); + assert!( + !applied.path().join("anchor.bin").exists() + && applied.path().join("deep water.bin").exists() + && sh_git(applied.path(), &["ls-files", "-s", "hull.bin"]).starts_with("100755 "), + "git am dropped anchor.bin, wrote deep water.bin and kept the exec bit on hull.bin" ); } @@ -2047,7 +2095,7 @@ async fn bad_post_bodies_are_invalid_request() { } #[tokio::test] -async fn merge_applies_a_plain_patch_under_the_supplied_author() { +async fn merge_applies_a_patch_under_the_supplied_author() { let world = World::new(); let (_did, main_sha, feature_sha) = seeded_feature_branch(&world, "mussel"); let registered = RepoDid::new("did:plc:musselfixture").unwrap(); diff --git a/types/repo.go b/types/repo.go index f10b7eae..58bd999e 100644 --- a/types/repo.go +++ b/types/repo.go @@ -39,6 +39,7 @@ type RepoFormatPatchResponse struct { FormatPatchRaw string `json:"patch,omitempty"` CombinedPatch []*gitdiff.File `json:"combined_patch,omitempty"` CombinedPatchRaw string `json:"combined_patch_raw,omitempty"` + BinaryOmitted bool `json:"binary_omitted,omitempty"` } type TagReference struct {