Skip to content

Handle all Git notes fanout depths #1908

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
svarlamov wants to merge 1 commit into
base: main
Choose a base branch
from
+251 −41
Open

Handle all Git notes fanout depths #1908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 113 additions & 41 deletions src/git/refs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,47 @@ pub fn notes_path_for_object(oid: &str) -> String {
}
}

fn normalize_note_path(path: &mut String) -> Option<usize> {
// 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<u8>, 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)
Expand Down Expand Up @@ -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::<Vec<_>>();
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)]
Expand All @@ -262,6 +285,34 @@ struct LsTreeNoteEntry {
path: String,
}

fn record_matching_note_entries(
entries: Vec<LsTreeNoteEntry>,
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,
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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";
Expand Down
138 changes: 138 additions & 0 deletions tests/integration/refs_unit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <test@test.com> 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<String> {
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::<Vec<_>>()
.join("/")
}

#[test]
fn test_notes_add_and_show_authorship_note() {
let (repo, gitai_repo) = repo_with_handle();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down
Loading