From b747f600231a32fbdd11e883dcd41523bd9f1714 Mon Sep 17 00:00:00 2001 From: Sasha Varlamov Date: Sat, 18 Jul 2026 18:13:04 +0000 Subject: [PATCH] Handle every Git notes fanout depth (#1517) --- src/git/refs.rs | 154 ++++++++++++++++++++++++--------- tests/integration/refs_unit.rs | 138 +++++++++++++++++++++++++++++ 2 files changed, 251 insertions(+), 41 deletions(-) diff --git a/src/git/refs.rs b/src/git/refs.rs index 32b31d5b17..71b34e5c82 100644 --- a/src/git/refs.rs +++ b/src/git/refs.rs @@ -32,6 +32,47 @@ pub fn notes_path_for_object(oid: &str) -> String { } } +fn normalize_note_path(path: &mut String) -> Option { + // Notes fanout components are always two hex characters. Collapse a valid + // path in place so reads need one lookup entry per object instead of one + // entry for every possible fanout depth. + let mut component_len = 0; + let mut fanout_depth = 0; + let mut valid = true; + path.retain(|character| { + if character == '/' { + valid &= component_len == 2; + component_len = 0; + fanout_depth += 1; + false + } else { + component_len += character.len_utf8(); + true + } + }); + + (valid && component_len > 0).then_some(fanout_depth) +} + +fn append_note_deletions(script: &mut Vec, oid: &str) { + // Write directly into the fast-import buffer; constructing every path as a + // separate String would add 20 allocations for SHA-1 and 32 for SHA-256. + let oid = oid.as_bytes(); + script.extend_from_slice(b"D "); + script.extend_from_slice(oid); + script.push(b'\n'); + + for prefix_end in (2..oid.len()).step_by(2) { + script.extend_from_slice(b"D "); + for component_start in (0..prefix_end).step_by(2) { + script.extend_from_slice(&oid[component_start..component_start + 2]); + script.push(b'/'); + } + script.extend_from_slice(&oid[prefix_end..]); + script.push(b'\n'); + } +} + #[doc(hidden)] pub fn flat_note_pathspec_for_commit(commit_sha: &str) -> String { flat_note_pathspec_for_ref(AI_AUTHORSHIP_FULL_REF, commit_sha) @@ -206,53 +247,35 @@ pub fn note_blob_oids_for_commits_from_ref( return Ok(HashMap::new()); } - let mut path_to_commit = HashMap::new(); - let mut fanout_prefixes = HashSet::new(); + // Borrow the requested IDs and keep a single slot per unique commit. The + // returned tree paths are normalized in place before probing this map. + let mut notes_by_commit = HashMap::with_capacity(commit_shas.len()); + let mut fanout_prefixes = HashSet::with_capacity(commit_shas.len().min(256)); for commit_sha in commit_shas { - let flat_path = commit_sha.clone(); - path_to_commit.insert(flat_path, commit_sha.clone()); - - let fanout_path = notes_path_for_object(commit_sha); - path_to_commit.insert(fanout_path, commit_sha.clone()); + notes_by_commit.insert(commit_sha.as_str(), None); if commit_sha.len() > 2 { fanout_prefixes.insert(commit_sha[..2].to_string()); } } - let mut result = HashMap::new(); let Some(root_entries) = ls_tree_note_entries(repo, notes_ref, false, &[])? else { return Ok(HashMap::new()); }; - for entry in root_entries { - if let Some(commit_sha) = path_to_commit.get(&entry.path) { - if entry.object_type != "blob" { - return Err(GitAiError::Generic(format!( - "authorship note path {} in {} is {}, expected blob", - entry.path, notes_ref, entry.object_type - ))); - } - result.entry(commit_sha.clone()).or_insert(entry.oid); - } - } + record_matching_note_entries(root_entries, notes_ref, &mut notes_by_commit)?; let mut prefixes = fanout_prefixes.into_iter().collect::>(); prefixes.sort(); if let Some(entries) = ls_tree_note_entries(repo, notes_ref, true, &prefixes)? { - for entry in entries { - if let Some(commit_sha) = path_to_commit.get(&entry.path) { - if entry.object_type != "blob" { - return Err(GitAiError::Generic(format!( - "authorship note path {} in {} is {}, expected blob", - entry.path, notes_ref, entry.object_type - ))); - } - result.entry(commit_sha.clone()).or_insert(entry.oid); - } - } + record_matching_note_entries(entries, notes_ref, &mut notes_by_commit)?; } - Ok(result) + Ok(notes_by_commit + .into_iter() + .filter_map(|(commit_sha, note)| { + note.map(|(_preference, blob_oid)| (commit_sha.to_string(), blob_oid)) + }) + .collect()) } #[derive(Debug)] @@ -262,6 +285,34 @@ struct LsTreeNoteEntry { path: String, } +fn record_matching_note_entries( + entries: Vec, + notes_ref: &str, + notes_by_commit: &mut HashMap<&str, Option<(usize, String)>>, +) -> Result<(), GitAiError> { + for mut entry in entries { + let Some(preference) = normalize_note_path(&mut entry.path) else { + continue; + }; + let Some(current_note) = notes_by_commit.get_mut(entry.path.as_str()) else { + continue; + }; + if entry.object_type != "blob" { + return Err(GitAiError::Generic(format!( + "authorship note path {} in {} is {}, expected blob", + entry.path, notes_ref, entry.object_type + ))); + } + if current_note + .as_ref() + .is_none_or(|(current_preference, _)| preference < *current_preference) + { + *current_note = Some((preference, entry.oid)); + } + } + Ok(()) +} + fn ls_tree_note_entries( repo: &Repository, notes_ref: &str, @@ -422,11 +473,7 @@ pub(in crate::git) fn notes_add_batch( for (idx, (commit_sha, _note_content)) in deduped_entries.iter().enumerate() { let fanout_path = notes_path_for_object(commit_sha); - let flat_path = commit_sha.clone(); - if flat_path != fanout_path { - script.extend_from_slice(format!("D {}\n", flat_path).as_bytes()); - } - script.extend_from_slice(format!("D {}\n", fanout_path).as_bytes()); + append_note_deletions(&mut script, commit_sha); script.extend_from_slice(format!("M 100644 :{} {}\n", idx + 1, fanout_path).as_bytes()); } script.extend_from_slice(b"\n"); @@ -489,11 +536,7 @@ pub(in crate::git) fn notes_add_blob_batch( for (commit_sha, blob_oid) in &deduped_entries { let fanout_path = notes_path_for_object(commit_sha); - let flat_path = commit_sha.clone(); - if flat_path != fanout_path { - script.extend_from_slice(format!("D {}\n", flat_path).as_bytes()); - } - script.extend_from_slice(format!("D {}\n", fanout_path).as_bytes()); + append_note_deletions(&mut script, commit_sha); script.extend_from_slice(format!("M 100644 {} {}\n", blob_oid, fanout_path).as_bytes()); } script.extend_from_slice(b"\n"); @@ -1081,6 +1124,35 @@ mod tests { ); } + #[test] + fn test_append_note_deletions_includes_every_fanout_depth() { + let mut short_script = Vec::new(); + append_note_deletions(&mut short_script, "a"); + append_note_deletions(&mut short_script, "ab"); + assert_eq!(String::from_utf8(short_script).unwrap(), "D a\nD ab\n"); + + let mut script = Vec::new(); + append_note_deletions(&mut script, "abcdef"); + assert_eq!( + String::from_utf8(script).unwrap(), + "D abcdef\nD ab/cdef\nD ab/cd/ef\n" + ); + } + + #[test] + fn test_normalize_note_path_in_place() { + for (path, expected_depth) in [("abcdef", 0), ("ab/cdef", 1), ("ab/cd/ef", 2)] { + let mut path = path.to_string(); + assert_eq!(normalize_note_path(&mut path), Some(expected_depth)); + assert_eq!(path, "abcdef"); + } + + for invalid_path in ["", "/abcdef", "a/bcdef", "ab/", "ab//cdef"] { + let mut path = invalid_path.to_string(); + assert_eq!(normalize_note_path(&mut path), None); + } + } + #[test] fn test_flat_note_pathspec_for_commit() { let sha = "abcdef1234567890abcdef1234567890abcdef12"; diff --git a/tests/integration/refs_unit.rs b/tests/integration/refs_unit.rs index 50826ed8a6..558b14b0bc 100644 --- a/tests/integration/refs_unit.rs +++ b/tests/integration/refs_unit.rs @@ -49,6 +49,45 @@ fn git_stdin_stdout( .to_string() } +fn install_note_at_paths( + repo: &git_ai::git::repository::Repository, + paths: &[String], + content: &str, +) { + let mut stream = format!( + "blob\nmark :1\ndata {}\n{}\ncommit refs/notes/ai\ncommitter Test 1000000000 +0000\ndata 0\ndeleteall\n", + content.len(), + content + ); + for path in paths { + stream.push_str(&format!("M 100644 :1 {path}\n")); + } + stream.push_str("\ndone\n"); + + git_stdin_stdout( + repo, + &["fast-import", "--quiet", "--done"], + stream.as_bytes(), + ); +} + +fn note_tree_paths(repo: &TestRepo) -> Vec { + repo.git_og(&["ls-tree", "-r", "--name-only", "refs/notes/ai"]) + .expect("list authorship note paths") + .lines() + .map(str::to_string) + .collect() +} + +fn deepest_note_path(commit_sha: &str) -> String { + commit_sha + .as_bytes() + .chunks(2) + .map(|chunk| std::str::from_utf8(chunk).unwrap()) + .collect::>() + .join("/") +} + #[test] fn test_notes_add_and_show_authorship_note() { let (repo, gitai_repo) = repo_with_handle(); @@ -103,6 +142,47 @@ fn test_notes_add_batch_writes_multiple_notes() { assert!(note_b.contains("\"note\":\"b\"")); } +#[test] +fn test_notes_add_batch_replaces_notes_at_every_legacy_fanout_depth() { + let (repo, gitai_repo) = repo_with_handle(); + + fs::write(repo.path().join("legacy.txt"), "legacy\n").unwrap(); + repo.git_og(&["add", "."]).expect("add legacy file"); + repo.git_og(&["commit", "-m", "Legacy note target"]) + .expect("commit legacy note target"); + let commit_sha = head_sha(&repo); + let canonical_path = format!("{}/{}", &commit_sha[..2], &commit_sha[2..]); + let legacy_paths = vec![ + commit_sha.clone(), + canonical_path.clone(), + format!( + "{}/{}/{}", + &commit_sha[..2], + &commit_sha[2..4], + &commit_sha[4..] + ), + deepest_note_path(&commit_sha), + ]; + install_note_at_paths(&gitai_repo, &legacy_paths, "{}"); + assert_eq!(note_tree_paths(&repo).len(), legacy_paths.len()); + assert!( + read_authorship_v3(&gitai_repo, &commit_sha).is_err(), + "duplicate legacy note paths must reproduce the deserialization failure" + ); + + let mut replacement = AuthorshipLog::new(); + replacement.metadata.base_commit_sha = commit_sha.clone(); + let replacement = replacement + .serialize_to_string() + .expect("serialize replacement note"); + notes_add_batch(&gitai_repo, &[(commit_sha.clone(), replacement)]) + .expect("replace mixed-fanout note"); + + assert_eq!(note_tree_paths(&repo), vec![canonical_path]); + let parsed = read_authorship_v3(&gitai_repo, &commit_sha).expect("parse replacement note"); + assert_eq!(parsed.metadata.base_commit_sha, commit_sha); +} + #[test] fn test_notes_add_blob_batch_reuses_existing_note_blob() { let (repo, gitai_repo) = repo_with_handle(); @@ -138,6 +218,64 @@ fn test_notes_add_blob_batch_reuses_existing_note_blob() { assert_eq!(parsed_note_b.metadata.base_commit_sha, commit_b); } +#[test] +fn test_notes_add_blob_batch_replaces_notes_at_every_legacy_fanout_depth() { + let (repo, gitai_repo) = repo_with_handle(); + + fs::write(repo.path().join("legacy-blob.txt"), "legacy\n").unwrap(); + repo.git_og(&["add", "."]).expect("add legacy blob file"); + repo.git_og(&["commit", "-m", "Legacy blob note target"]) + .expect("commit legacy blob note target"); + let commit_sha = head_sha(&repo); + let canonical_path = format!("{}/{}", &commit_sha[..2], &commit_sha[2..]); + let legacy_paths = vec![ + commit_sha.clone(), + format!( + "{}/{}/{}", + &commit_sha[..2], + &commit_sha[2..4], + &commit_sha[4..] + ), + deepest_note_path(&commit_sha), + ]; + install_note_at_paths(&gitai_repo, &legacy_paths, "stale"); + assert_eq!(note_tree_paths(&repo).len(), legacy_paths.len()); + + let replacement_blob = git_stdin_stdout( + &gitai_repo, + &["hash-object", "-w", "--stdin"], + b"replacement", + ); + notes_add_blob_batch(&gitai_repo, &[(commit_sha.clone(), replacement_blob)]) + .expect("replace mixed-fanout blob note"); + + assert_eq!(note_tree_paths(&repo), vec![canonical_path]); + assert_eq!( + read_note(&gitai_repo, &commit_sha).as_deref(), + Some("replacement") + ); +} + +#[test] +fn test_read_notes_batch_finds_deeply_fanned_out_note() { + let (repo, gitai_repo) = repo_with_handle(); + + fs::write(repo.path().join("deep-note.txt"), "deep\n").unwrap(); + repo.git_og(&["add", "."]).expect("add deep note file"); + repo.git_og(&["commit", "-m", "Deep note target"]) + .expect("commit deep note target"); + let commit_sha = head_sha(&repo); + let deepest_path = deepest_note_path(&commit_sha); + install_note_at_paths(&gitai_repo, &[deepest_path], "deeply fanned out"); + + let notes = notes_api::read_notes_batch(&gitai_repo, std::slice::from_ref(&commit_sha)) + .expect("batch-read deeply fanned-out note"); + assert_eq!( + notes.get(&commit_sha).map(String::as_str), + Some("deeply fanned out") + ); +} + #[test] fn test_copy_missing_notes_for_commits_from_ref_copies_only_requested_commits() { let (repo, gitai_repo) = repo_with_handle();