-
Notifications
You must be signed in to change notification settings - Fork 219
add model name to blame #1668
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.
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
base: main
Are you sure you want to change the base?
add model name to blame #1668
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,8 @@ | ||
| use crate::auth::CredentialStore; | ||
| use crate::authorship::authorship_log::{HumanRecord, PromptRecord, SessionRecord}; | ||
| use crate::authorship::authorship_log_serialization::AuthorshipLog; | ||
| use crate::authorship::authorship_log_serialization::{AUTHORSHIP_LOG_VERSION, AuthorshipLog}; | ||
| use crate::authorship::working_log::CheckpointKind; | ||
| use crate::error::GitAiError; | ||
| use crate::git::notes_api::read_authorship_v3 as get_reference_as_authorship_log_v3; | ||
| use crate::git::repository::Repository; | ||
| use crate::git::repository::{exec_git, exec_git_stdin}; | ||
| #[cfg(windows)] | ||
|
|
@@ -935,6 +934,43 @@ impl Repository { | |
| Ok(hunks) | ||
| } | ||
|
|
||
| /// Batch-load v3 authorship logs for a set of commits in a handful of git | ||
| /// invocations (one `ls-tree` + batched `cat-file`) instead of one | ||
| /// `git notes show` subprocess per commit. | ||
| /// | ||
| /// Commits without a valid note are recorded as `None` so callers can cache | ||
| /// negative lookups too. Behavior mirrors `read_authorship_v3`: the schema | ||
| /// version is enforced and each log's `base_commit_sha` is aligned to the | ||
| /// commit its note is attached to. | ||
| fn load_authorship_logs_batched( | ||
| &self, | ||
| commit_shas: impl IntoIterator<Item = String>, | ||
| ) -> HashMap<String, Option<AuthorshipLog>> { | ||
| let unique: Vec<String> = commit_shas | ||
| .into_iter() | ||
| .collect::<HashSet<_>>() | ||
| .into_iter() | ||
| .collect(); | ||
| let mut cache: HashMap<String, Option<AuthorshipLog>> = HashMap::new(); | ||
| if unique.is_empty() { | ||
| return cache; | ||
| } | ||
|
|
||
| let notes = crate::git::notes_api::read_notes_batch(self, &unique).unwrap_or_default(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Batched note loading drops HTTP fetch-and-cache step present in old per-commit path The old code called Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| for sha in unique { | ||
| let log = notes.get(&sha).and_then(|content| { | ||
| let mut log = AuthorshipLog::deserialize_from_string(content).ok()?; | ||
| if log.metadata.schema_version != AUTHORSHIP_LOG_VERSION { | ||
| return None; | ||
| } | ||
| log.metadata.base_commit_sha = sha.clone(); | ||
| Some(log) | ||
| }); | ||
| cache.insert(sha, log); | ||
| } | ||
| cache | ||
| } | ||
|
|
||
| /// Post-process blame hunks to populate ai_human_author from authorship logs. | ||
| /// For each hunk, looks up the authorship log for its commit and finds the human_author | ||
| /// from the prompt record that covers lines in the hunk. | ||
|
|
@@ -946,23 +982,21 @@ impl Repository { | |
| file_path: &str, | ||
| options: &GitAiBlameOptions, | ||
| ) -> Result<Vec<BlameHunk>, GitAiError> { | ||
| // Cache authorship logs by commit SHA to avoid repeated lookups | ||
| let mut commit_authorship_cache: HashMap<String, Option<AuthorshipLog>> = HashMap::new(); | ||
| // Batch-load authorship logs for every commit in one shot to avoid one | ||
| // `git notes show` subprocess per commit. | ||
| let commit_authorship_cache = | ||
| self.load_authorship_logs_batched(hunks.iter().map(|h| h.commit_sha.clone())); | ||
| // Cache for foreign prompts to avoid repeated grepping | ||
| let mut foreign_prompts_cache: HashMap<String, Option<PromptRecord>> = HashMap::new(); | ||
|
|
||
| let mut result_hunks: Vec<BlameHunk> = Vec::new(); | ||
|
|
||
| for hunk in hunks { | ||
| // Get or fetch the authorship log for this commit | ||
| let authorship_log = if let Some(cached) = commit_authorship_cache.get(&hunk.commit_sha) | ||
| { | ||
| cached.clone() | ||
| } else { | ||
| let authorship = get_reference_as_authorship_log_v3(self, &hunk.commit_sha).ok(); | ||
| commit_authorship_cache.insert(hunk.commit_sha.clone(), authorship.clone()); | ||
| authorship | ||
| }; | ||
| // Look up the pre-loaded authorship log for this commit. | ||
| let authorship_log = commit_authorship_cache | ||
| .get(&hunk.commit_sha) | ||
| .cloned() | ||
| .flatten(); | ||
|
|
||
| // If we have an authorship log, look up human_author for each line | ||
| if let Some(ref authorship_log) = authorship_log { | ||
|
|
@@ -1040,6 +1074,18 @@ impl Repository { | |
| } | ||
| } | ||
|
|
||
| fn format_agent_author(tool: &str, model: &str) -> String { | ||
| let model = model.trim(); | ||
| if model.is_empty() || model.eq_ignore_ascii_case("unknown") { | ||
| tool.to_string() | ||
| } else { | ||
| // Strip a redundant "claude-" prefix (e.g. "claude-sonnet-4-6" -> "sonnet-4-6") | ||
| // to keep the label compact. | ||
| let model = model.strip_prefix("claude-").unwrap_or(model); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 format_agent_author strips 'claude-' prefix unconditionally from model strings The Was this helpful? React with 👍 or 👎 to provide feedback. |
||
| format!("{} {}", tool, model) | ||
| } | ||
| } | ||
|
|
||
| #[allow(clippy::type_complexity)] | ||
| fn overlay_ai_authorship( | ||
| repo: &Repository, | ||
|
|
@@ -1068,24 +1114,22 @@ fn overlay_ai_authorship( | |
| let mut commits_with_notes: std::collections::HashSet<String> = | ||
| std::collections::HashSet::new(); | ||
|
|
||
| // Group hunks by commit SHA to avoid repeated lookups | ||
| let mut commit_authorship_cache: HashMap<String, Option<AuthorshipLog>> = HashMap::new(); | ||
| // Batch-load authorship logs for every commit in one shot to avoid one | ||
| // `git notes show` subprocess per commit. | ||
| let commit_authorship_cache = | ||
| repo.load_authorship_logs_batched(blame_hunks.iter().map(|h| h.commit_sha.clone())); | ||
| // Simulated authorship logs for agent commits without notes. We keep these separate | ||
| // from commit_authorship_cache so a single agent commit can be handled across multiple | ||
| // blame hunks without being limited to the first hunk's line range. | ||
| let mut simulated_authorship_logs: HashMap<String, AuthorshipLog> = HashMap::new(); | ||
| // Cache for foreign prompts to avoid repeated grepping | ||
| let mut foreign_prompts_cache: HashMap<String, Option<PromptRecord>> = HashMap::new(); | ||
| for hunk in blame_hunks { | ||
| // Check if we've already looked up this commit's authorship | ||
| let authorship_log = if let Some(cached) = commit_authorship_cache.get(&hunk.commit_sha) { | ||
| cached.clone() | ||
| } else { | ||
| // Try to get authorship log for this commit | ||
| let authorship = get_reference_as_authorship_log_v3(repo, &hunk.commit_sha).ok(); | ||
| commit_authorship_cache.insert(hunk.commit_sha.clone(), authorship.clone()); | ||
| authorship | ||
| }; | ||
| // Look up the pre-loaded authorship log for this commit. | ||
| let authorship_log = commit_authorship_cache | ||
| .get(&hunk.commit_sha) | ||
| .cloned() | ||
| .flatten(); | ||
|
|
||
| // If we have AI authorship data, look up the author for lines in this hunk | ||
| if let Some(ref authorship_log) = authorship_log { | ||
|
|
@@ -1131,8 +1175,13 @@ fn overlay_ai_authorship( | |
| if options.use_prompt_hashes_as_names { | ||
| line_authors.insert(current_line_num, prompt_hash.clone()); | ||
| } else { | ||
| line_authors | ||
| .insert(current_line_num, prompt_record.agent_id.tool.clone()); | ||
| line_authors.insert( | ||
| current_line_num, | ||
| format_agent_author( | ||
| &prompt_record.agent_id.tool, | ||
| &prompt_record.agent_id.model, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| prompt_records.insert(prompt_hash, prompt_record.clone()); | ||
|
|
@@ -1724,7 +1773,11 @@ fn output_default_format( | |
| } else if options.show_prompt && prompt_records.contains_key(author) { | ||
| let prompt = &prompt_records[author]; | ||
| let short_hash = &author[..7.min(author.len())]; | ||
| format!("{} [{}]", prompt.agent_id.tool, short_hash) | ||
| format!( | ||
| "{} [{}]", | ||
| format_agent_author(&prompt.agent_id.tool, &prompt.agent_id.model), | ||
| short_hash | ||
| ) | ||
| } else if options.show_email { | ||
| format!("{} <{}>", author, &hunk.author_email) | ||
| } else { | ||
|
|
@@ -2236,3 +2289,32 @@ fn parse_line_range(range_str: &str) -> Option<(u32, u32)> { | |
|
|
||
| None | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_format_agent_author_with_model() { | ||
| // A leading "claude-" is stripped to keep the label compact. | ||
| assert_eq!( | ||
| format_agent_author("claude", "claude-sonnet-4-6"), | ||
| "claude sonnet-4-6" | ||
| ); | ||
| // Non-claude models are shown verbatim. | ||
| assert_eq!(format_agent_author("cursor", "gpt-4"), "cursor gpt-4"); | ||
| // Only a leading "claude-" is stripped, not occurrences elsewhere. | ||
| assert_eq!( | ||
| format_agent_author("amp", "anthropic/claude-opus"), | ||
| "amp anthropic/claude-opus" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_format_agent_author_omits_unknown_or_empty_model() { | ||
| assert_eq!(format_agent_author("mock_ai", "unknown"), "mock_ai"); | ||
| assert_eq!(format_agent_author("mock_ai", "UNKNOWN"), "mock_ai"); | ||
| assert_eq!(format_agent_author("claude", ""), "claude"); | ||
| assert_eq!(format_agent_author("claude", " "), "claude"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| --- | ||
| source: tests/initial_attributions.rs | ||
| source: tests/integration/initial_attributions.rs | ||
| assertion_line: 295 | ||
| expression: normalized | ||
| --- | ||
| "COMMIT_SHA (tool1 TIMESTAMP 1) line 1\nCOMMIT_SHA (tool1 TIMESTAMP 2) line 2\nCOMMIT_SHA (tool1 TIMESTAMP 3) line 3\nCOMMIT_SHA (mock_ai TIMESTAMP 4) line 4\nCOMMIT_SHA (tool2 TIMESTAMP 5) line 5\nCOMMIT_SHA (mock_ai TIMESTAMP 6) line 6\nCOMMIT_SHA (mock_ai TIMESTAMP 7) line 7\n" | ||
| "COMMIT_SHA (tool1 model1 TIMESTAMP 1) line 1\nCOMMIT_SHA (tool1 model1 TIMESTAMP 2) line 2\nCOMMIT_SHA (tool1 model1 TIMESTAMP 3) line 3\nCOMMIT_SHA (mock_ai TIMESTAMP 4) line 4\nCOMMIT_SHA (tool2 model2 TIMESTAMP 5) line 5\nCOMMIT_SHA (mock_ai TIMESTAMP 6) line 6\nCOMMIT_SHA (mock_ai TIMESTAMP 7) line 7\n" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| --- | ||
| source: tests/initial_attributions.rs | ||
| assertion_line: 274 | ||
| source: tests/integration/initial_attributions.rs | ||
| assertion_line: 295 | ||
| expression: normalized | ||
| --- | ||
| "COMMIT_SHA (tool1 TIMESTAMP 1) line 1\nCOMMIT_SHA (tool1 TIMESTAMP 2) line 2\nCOMMIT_SHA (tool1 TIMESTAMP 3) line 3\nCOMMIT_SHA (mock_ai TIMESTAMP 4) line 4\nCOMMIT_SHA (tool2 TIMESTAMP 5) line 5\nCOMMIT_SHA (mock_ai TIMESTAMP 6) line 6\nCOMMIT_SHA (mock_ai TIMESTAMP 7) line 7\n" | ||
| "COMMIT_SHA (tool1 model1 TIMESTAMP 1) line 1\nCOMMIT_SHA (tool1 model1 TIMESTAMP 2) line 2\nCOMMIT_SHA (tool1 model1 TIMESTAMP 3) line 3\nCOMMIT_SHA (mock_ai TIMESTAMP 4) line 4\nCOMMIT_SHA (tool2 model2 TIMESTAMP 5) line 5\nCOMMIT_SHA (mock_ai TIMESTAMP 6) line 6\nCOMMIT_SHA (mock_ai TIMESTAMP 7) line 7\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Model name missing from blame output when --show-prompt flag is used
The agent model name is omitted from the rendered blame line (
format!("{} [{}]", prompt.agent_id.tool, short_hash)atsrc/commands/blame.rs:1832) despite the width calculation including it, so--show-promptoutput shows only the tool name with excess padding instead of the intended "tool model [hash]" format.Impact: Users running
git-ai blame --show-promptsee the agent name without the model, contradicting the PR's goal of displaying both.Width calculation was updated but the parallel rendering site was not
The
output_default_formatfunction has two nearly-identicalshow_promptbranches:src/commands/blame.rs:1776-1780(correctly updated):src/commands/blame.rs:1830-1832(not updated):The width loop at line 1778 computes
max_author_widthusing the fullformat_agent_authoroutput (e.g. "cursor sonnet-4-6 [abc1234]"), but the rendering loop at line 1832 produces only "cursor [abc1234]". This results in (a) model info missing from--show-promptoutput and (b) extra whitespace padding in the author column.(Refers to line 1832)
Was this helpful? React with 👍 or 👎 to provide feedback.