From ca0b3c92d7f3f92d32b98a18eec2df89a1c4e4a3 Mon Sep 17 00:00:00 2001 From: Eagl61 <17625015+Eagl61@users.noreply.github.com> Date: Sun, 12 Jul 2026 23:19:11 +0200 Subject: [PATCH] git: Show submodule changes in commit diffs (#60479) ## Objective Fix Git Graph showing `0 Changed Files` for commits that modify submodules. This happened because submodule entries are gitlinks rather than regular blobs, so the existing commit diff loading path could not resolve them correctly. I ran into this issue previously, and it was confusing because the commit clearly contained changes, but the UI did not reflect them. If a commit included many file changes plus a submodule update, the submodule change was effectively hidden and the UI could still show `0 Changed Files` for that entry. Since submodules are not used in every repository, the bug is easy to miss and may go unnoticed for a long time. ## Solution - Switched commit diff loading to use Git's raw diff output so submodule entries are represented explicitly. - Added parsing for gitlink entries alongside regular file changes. - Render submodule changes as `Subproject commit ` so they show up in commit diffs instead of being dropped. ## Testing - Added a repository test covering commit diffs that include submodule changes. - Ran the `git` crate test suite. - Ran formatting and lint checks. - Verified the Git UI crate still builds. - Ran and tested it locally on MacOS. Reviewer notes: - The change is limited to commit diff loading and should not affect normal blob diffs. - Submodule changes now appear as explicit entries instead of being counted as zero files. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed- industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable ## Showcase Before: Screenshot 2026-07-06 at 19 22 57 After: Screenshot 2026-07-06 at 20 20 11 Screenshot 2026-07-06 at 20 37 57 --- Release Notes: - Fixed Git Graph showing `0 Changed Files` for commits that update submodules. --- crates/git/src/commit.rs | 172 +++++++++++++++------ crates/git/src/repository.rs | 280 ++++++++++++++++++++++++++--------- 2 files changed, 335 insertions(+), 117 deletions(-) diff --git a/crates/git/src/commit.rs b/crates/git/src/commit.rs index 326c741ad1..62bf8b4a10 100644 --- a/crates/git/src/commit.rs +++ b/crates/git/src/commit.rs @@ -100,22 +100,88 @@ async fn get_messages_impl(git: &GitBinary, shas: &[Oid]) -> Result> .collect::>()) } -/// Parse the output of `git diff --name-status -z` -pub fn parse_git_diff_name_status(content: &str) -> impl Iterator { +pub(crate) const GITLINK_MODE: &str = "160000"; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum CommitDiffObjectKind { + Blob, + Gitlink, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) struct CommitDiffObject<'a> { + pub oid: &'a str, + pub kind: CommitDiffObjectKind, +} + +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct CommitDiffEntry<'a> { + pub path: &'a str, + pub status: StatusCode, + pub old_object: Option>, + pub new_object: Option>, +} + +/// Parses the output of `git diff --raw --no-abbrev -z`. +pub(crate) fn parse_git_diff_raw( + content: &str, +) -> impl Iterator>> { let mut parts = content.split('\0'); std::iter::from_fn(move || { - loop { - let status_str = parts.next()?; - let path = parts.next()?; - let status = match status_str { - "M" => StatusCode::Modified, - "T" => StatusCode::TypeChanged, - "A" => StatusCode::Added, - "D" => StatusCode::Deleted, - _ => continue, - }; - return Some((path, status)); + let metadata = parts.next()?; + if metadata.is_empty() { + return None; } + + let path = match parts.next() { + Some(path) => path, + None => return Some(Err(anyhow::anyhow!("raw diff is missing the path"))), + }; + Some(parse_git_diff_raw_entry(metadata, path)) + }) +} + +fn parse_git_diff_raw_entry<'a>(metadata: &'a str, path: &'a str) -> Result> { + let mut fields = metadata + .strip_prefix(':') + .context("raw diff metadata is missing its ':' prefix")? + .split_ascii_whitespace(); + let old_mode = fields.next().context("raw diff is missing the old mode")?; + let new_mode = fields.next().context("raw diff is missing the new mode")?; + let old_oid = fields + .next() + .context("raw diff is missing the old object ID")?; + let new_oid = fields + .next() + .context("raw diff is missing the new object ID")?; + let status = match fields.next() { + Some("M") => StatusCode::Modified, + Some("T") => StatusCode::TypeChanged, + Some("A") => StatusCode::Added, + Some("D") => StatusCode::Deleted, + Some(status) => anyhow::bail!("unsupported raw diff status {status}"), + None => anyhow::bail!("raw diff is missing the status"), + }; + + Ok(CommitDiffEntry { + path, + status, + old_object: (!old_oid.bytes().all(|byte| byte == b'0')).then(|| CommitDiffObject { + oid: old_oid, + kind: if old_mode == GITLINK_MODE { + CommitDiffObjectKind::Gitlink + } else { + CommitDiffObjectKind::Blob + }, + }), + new_object: (!new_oid.bytes().all(|byte| byte == b'0')).then(|| CommitDiffObject { + oid: new_oid, + kind: if new_mode == GITLINK_MODE { + CommitDiffObjectKind::Gitlink + } else { + CommitDiffObjectKind::Blob + }, + }), }) } @@ -125,41 +191,59 @@ mod tests { use super::*; #[test] - fn test_parse_git_diff_name_status() { + fn test_parse_git_diff_raw() { let input = concat!( - "M\x00Cargo.lock\x00", - "T\x00LICENSE-GPL\x00", - "M\x00crates/project/Cargo.toml\x00", - "M\x00crates/project/src/buffer_store.rs\x00", - "D\x00crates/project/src/git.rs\x00", - "A\x00crates/project/src/git_store.rs\x00", - "A\x00crates/project/src/git_store/git_traversal.rs\x00", - "M\x00crates/project/src/project.rs\x00", - "M\x00crates/project/src/worktree_store.rs\x00", - "M\x00crates/project_panel/src/project_panel.rs\x00", + ":100644 100644 1111111111111111111111111111111111111111 2222222222222222222222222222222222222222 M\x00file.txt\x00", + ":160000 160000 3333333333333333333333333333333333333333 4444444444444444444444444444444444444444 M\x00modules/example\x00", + ":000000 100644 0000000000000000000000000000000000000000 5555555555555555555555555555555555555555 A\x00added.txt\x00", + ":160000 000000 6666666666666666666666666666666666666666 0000000000000000000000000000000000000000 D\x00deleted-module\x00", + ":100644 160000 7777777777777777777777777777777777777777 8888888888888888888888888888888888888888 T\x00type-change\x00", ); - let output = parse_git_diff_name_status(input).collect::>(); + let entries = parse_git_diff_raw(input) + .collect::>>() + .unwrap(); + let [file, gitlink, added, deleted, type_change] = entries.as_slice() else { + panic!("expected five raw diff entries"); + }; + + assert_eq!(file.path, "file.txt"); + assert_eq!(file.status, StatusCode::Modified); assert_eq!( - output, - &[ - ("Cargo.lock", StatusCode::Modified), - ("LICENSE-GPL", StatusCode::TypeChanged), - ("crates/project/Cargo.toml", StatusCode::Modified), - ("crates/project/src/buffer_store.rs", StatusCode::Modified), - ("crates/project/src/git.rs", StatusCode::Deleted), - ("crates/project/src/git_store.rs", StatusCode::Added), - ( - "crates/project/src/git_store/git_traversal.rs", - StatusCode::Added, - ), - ("crates/project/src/project.rs", StatusCode::Modified), - ("crates/project/src/worktree_store.rs", StatusCode::Modified), - ( - "crates/project_panel/src/project_panel.rs", - StatusCode::Modified - ), - ] + file.new_object.map(|object| object.kind), + Some(CommitDiffObjectKind::Blob) + ); + assert_eq!(gitlink.path, "modules/example"); + assert_eq!(gitlink.status, StatusCode::Modified); + assert_eq!( + gitlink.old_object.map(|object| object.kind), + Some(CommitDiffObjectKind::Gitlink) + ); + assert_eq!( + gitlink.new_object.map(|object| object.kind), + Some(CommitDiffObjectKind::Gitlink) + ); + assert!(added.old_object.is_none()); + assert_eq!(added.status, StatusCode::Added); + assert!(deleted.new_object.is_none()); + assert_eq!(deleted.status, StatusCode::Deleted); + assert_eq!(type_change.status, StatusCode::TypeChanged); + assert_eq!( + type_change.old_object.map(|object| object.kind), + Some(CommitDiffObjectKind::Blob) + ); + assert_eq!( + type_change.new_object.map(|object| object.kind), + Some(CommitDiffObjectKind::Gitlink) ); } + + #[test] + fn test_parse_git_diff_raw_rejects_malformed_metadata() { + let error = parse_git_diff_raw(":100644\x00file.txt\x00") + .next() + .expect("expected a raw diff entry") + .expect_err("expected malformed metadata to fail"); + assert!(error.to_string().contains("new mode")); + } } diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 7bea8283d7..94d3fac855 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -1,6 +1,6 @@ -use crate::commit::parse_git_diff_name_status; +use crate::commit::{CommitDiffObject, CommitDiffObjectKind, parse_git_diff_raw}; use crate::stash::GitStash; -use crate::status::{DiffTreeType, GitStatus, StatusCode, TreeDiff}; +use crate::status::{DiffTreeType, GitStatus, TreeDiff}; use crate::{Oid, RunHook, SHORT_SHA_LENGTH}; use anyhow::{Context as _, Result, anyhow, bail}; use async_channel::Sender; @@ -572,6 +572,57 @@ pub fn is_binary_content(content: &[u8]) -> bool { content[..check_len].contains(&0) } +struct LoadedCommitObject { + text: String, + is_binary: bool, +} + +async fn read_commit_blob( + stdout: &mut R, + info_line: &mut String, + newline: &mut [u8; 1], +) -> Result { + info_line.clear(); + stdout.read_line(info_line).await?; + + let len = info_line + .trim_end() + .parse() + .with_context(|| format!("invalid object size output from cat-file {info_line}"))?; + + let mut bytes = vec![0; len]; + stdout.read_exact(&mut bytes).await?; + stdout.read_exact(newline).await?; + + let is_binary = is_binary_content(&bytes); + Ok(LoadedCommitObject { + text: if is_binary { + String::new() + } else { + String::from_utf8_lossy(&bytes).to_string() + }, + is_binary, + }) +} + +async fn load_commit_object( + object: Option>, + stdout: &mut R, + info_line: &mut String, + newline: &mut [u8; 1], +) -> Result> { + match object { + Some(object) if object.kind == CommitDiffObjectKind::Gitlink => { + Ok(Some(LoadedCommitObject { + text: format!("Subproject commit {}\n", object.oid), + is_binary: false, + })) + } + Some(_) => Ok(Some(read_commit_blob(stdout, info_line, newline).await?)), + None => Ok(None), + } +} + #[derive(Debug, Clone, Hash, PartialEq, Eq)] pub struct Remote { pub name: SharedString, @@ -1421,7 +1472,8 @@ impl GitRepository for RealGitRepository { "--format=", "-z", "--no-renames", - "--name-status", + "--raw", + "--no-abbrev", "--first-parent", ]) .arg(&commit) @@ -1431,10 +1483,14 @@ impl GitRepository for RealGitRepository { .output() .await .context("starting git show process")?; + anyhow::ensure!( + show_output.status.success(), + "git show failed: {}", + String::from_utf8_lossy(&show_output.stderr) + ); let show_stdout = String::from_utf8_lossy(&show_output.stdout); - let changes = parse_git_diff_name_status(&show_stdout); - let parent_sha = format!("{}^", commit); + let changes = parse_git_diff_raw(&show_stdout); let mut cat_file_process = git .build_command(&["cat-file", "--batch=%(objectsize)"]) @@ -1445,86 +1501,63 @@ impl GitRepository for RealGitRepository { .context("starting git cat-file process")?; let mut files = Vec::::new(); - let mut stdin = BufWriter::with_capacity(512, cat_file_process.stdin.take().unwrap()); - let mut stdout = BufReader::new(cat_file_process.stdout.take().unwrap()); + let stdin = cat_file_process + .stdin + .take() + .context("git cat-file process has no stdin")?; + let stdout = cat_file_process + .stdout + .take() + .context("git cat-file process has no stdout")?; + let mut stdin = BufWriter::with_capacity(512, stdin); + let mut stdout = BufReader::new(stdout); let mut info_line = String::new(); let mut newline = [b'\0']; - for (path, status_code) in changes { + for change in changes { + let change = change?; + let path = change.path; // git-show outputs `/`-delimited paths even on Windows. let Some(rel_path) = RelPath::unix(path).log_err() else { continue; }; - match status_code { - StatusCode::Modified | StatusCode::TypeChanged => { - stdin.write_all(commit.as_bytes()).await?; - stdin.write_all(b":").await?; - stdin.write_all(path.as_bytes()).await?; - stdin.write_all(b"\n").await?; - stdin.write_all(parent_sha.as_bytes()).await?; - stdin.write_all(b":").await?; - stdin.write_all(path.as_bytes()).await?; + let objects = [change.new_object, change.old_object]; + let mut has_blobs = false; + for object in objects.iter().flatten() { + if object.kind == CommitDiffObjectKind::Blob { + stdin.write_all(object.oid.as_bytes()).await?; stdin.write_all(b"\n").await?; + has_blobs = true; } - StatusCode::Added => { - stdin.write_all(commit.as_bytes()).await?; - stdin.write_all(b":").await?; - stdin.write_all(path.as_bytes()).await?; - stdin.write_all(b"\n").await?; - } - StatusCode::Deleted => { - stdin.write_all(parent_sha.as_bytes()).await?; - stdin.write_all(b":").await?; - stdin.write_all(path.as_bytes()).await?; - stdin.write_all(b"\n").await?; - } - _ => continue, } - stdin.flush().await?; - - info_line.clear(); - stdout.read_line(&mut info_line).await?; - - let len = info_line.trim_end().parse().with_context(|| { - format!("invalid object size output from cat-file {info_line}") - })?; - let mut text_bytes = vec![0; len]; - stdout.read_exact(&mut text_bytes).await?; - stdout.read_exact(&mut newline).await?; - - let mut old_text = None; - let mut new_text = None; - let mut is_binary = is_binary_content(&text_bytes); - let text = if is_binary { - String::new() - } else { - String::from_utf8_lossy(&text_bytes).to_string() - }; - - match status_code { - StatusCode::Modified | StatusCode::TypeChanged => { - info_line.clear(); - stdout.read_line(&mut info_line).await?; - let len = info_line.trim_end().parse().with_context(|| { - format!("invalid object size output from cat-file {}", info_line) - })?; - let mut parent_bytes = vec![0; len]; - stdout.read_exact(&mut parent_bytes).await?; - stdout.read_exact(&mut newline).await?; - is_binary = is_binary || is_binary_content(&parent_bytes); - if is_binary { - old_text = Some(String::new()); - new_text = Some(String::new()); - } else { - old_text = Some(String::from_utf8_lossy(&parent_bytes).to_string()); - new_text = Some(text); - } - } - StatusCode::Added => new_text = Some(text), - StatusCode::Deleted => old_text = Some(text), - _ => continue, + if has_blobs { + stdin.flush().await?; } + let [new_object, old_object] = objects; + let new_object = + load_commit_object(new_object, &mut stdout, &mut info_line, &mut newline) + .await?; + let old_object = + load_commit_object(old_object, &mut stdout, &mut info_line, &mut newline) + .await?; + let is_binary = new_object.as_ref().is_some_and(|object| object.is_binary) + || old_object.as_ref().is_some_and(|object| object.is_binary); + let new_text = new_object.map(|object| { + if is_binary { + String::new() + } else { + object.text + } + }); + let old_text = old_object.map(|object| { + if is_binary { + String::new() + } else { + object.text + } + }); + files.push(CommitFile { path: RepoPath(Arc::from(rel_path)), old_text, @@ -4133,6 +4166,107 @@ mod tests { assert_eq!(file.status(), CommitFileStatus::Modified); } + #[gpui::test] + async fn test_load_commit_with_gitlink_changes(cx: &mut TestAppContext) { + const FIRST_SUBMODULE_COMMIT: &str = "1111111111111111111111111111111111111111"; + const SECOND_SUBMODULE_COMMIT: &str = "2222222222222222222222222222222222222222"; + + disable_git_global_config(); + cx.executor().allow_parking(); + + let repo_dir = tempfile::tempdir().expect("failed to create temporary repository"); + git_init_repo(repo_dir.path()); + fs::write(repo_dir.path().join("README.md"), "parent repository\n") + .expect("failed to write regular file"); + git_command(repo_dir.path(), ["add", "README.md"]); + git_command( + repo_dir.path(), + [ + "update-index", + "--add", + "--cacheinfo", + crate::commit::GITLINK_MODE, + FIRST_SUBMODULE_COMMIT, + "modules/example", + ], + ); + git_command(repo_dir.path(), ["commit", "-m", "add submodule"]); + + let repository = RealGitRepository::new( + &repo_dir.path().join(".git"), + None, + Some("git".into()), + cx.executor(), + ) + .expect("failed to open repository"); + + let commit_diff = repository + .load_commit("HEAD".to_string(), cx.to_async()) + .await + .expect("failed to load commit that adds a gitlink"); + assert_eq!(commit_diff.files.len(), 2); + let gitlink = commit_diff + .files + .iter() + .find(|file| file.path.as_unix_str() == "modules/example") + .expect("gitlink should be present alongside the regular file"); + assert_eq!(gitlink.status(), CommitFileStatus::Added); + assert_eq!(gitlink.old_text, None); + assert_eq!( + gitlink.new_text.as_deref(), + Some("Subproject commit 1111111111111111111111111111111111111111\n") + ); + assert!(!gitlink.is_binary); + + git_command( + repo_dir.path(), + [ + "update-index", + "--cacheinfo", + crate::commit::GITLINK_MODE, + SECOND_SUBMODULE_COMMIT, + "modules/example", + ], + ); + git_command(repo_dir.path(), ["commit", "-m", "update submodule"]); + + let commit_diff = repository + .load_commit("HEAD".to_string(), cx.to_async()) + .await + .expect("failed to load commit that updates a gitlink"); + let [gitlink] = commit_diff.files.as_slice() else { + panic!("expected one updated gitlink"); + }; + assert_eq!(gitlink.status(), CommitFileStatus::Modified); + assert_eq!( + gitlink.old_text.as_deref(), + Some("Subproject commit 1111111111111111111111111111111111111111\n") + ); + assert_eq!( + gitlink.new_text.as_deref(), + Some("Subproject commit 2222222222222222222222222222222222222222\n") + ); + assert!(!gitlink.is_binary); + + git_command(repo_dir.path(), ["rm", "--cached", "modules/example"]); + git_command(repo_dir.path(), ["commit", "-m", "remove submodule"]); + + let commit_diff = repository + .load_commit("HEAD".to_string(), cx.to_async()) + .await + .expect("failed to load commit that deletes a gitlink"); + let [gitlink] = commit_diff.files.as_slice() else { + panic!("expected one deleted gitlink"); + }; + assert_eq!(gitlink.status(), CommitFileStatus::Deleted); + assert_eq!( + gitlink.old_text.as_deref(), + Some("Subproject commit 2222222222222222222222222222222222222222\n") + ); + assert_eq!(gitlink.new_text, None); + assert!(!gitlink.is_binary); + } + #[gpui::test] async fn test_check_access(cx: &mut TestAppContext) { disable_git_global_config();