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 <oid>` 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: <img width="200" alt="Screenshot 2026-07-06 at 19 22 57" src="https://github.com/user-attachments/assets/16efc348-5909-4cb0-8589-b0662923df56" /> After: <img width="200" alt="Screenshot 2026-07-06 at 20 20 11" src="https://github.com/user-attachments/assets/3201b264-6e61-45de-a9fc-c585f3b54e09" /> <img width="200" alt="Screenshot 2026-07-06 at 20 37 57" src="https://github.com/user-attachments/assets/182ed4e1-9e90-4e33-9136-7626aba31153" /> --- Release Notes: - Fixed Git Graph showing `0 Changed Files` for commits that update submodules.
This commit is contained in:
Eagl61 authored and GitHub committed
1 parent
08359123ac
commit
ca0b3c92d7
2 files changed
+335
-117
No files matched your search
+128
-44
@@ -100,22 +100,88 @@ async fn get_messages_impl(git: &GitBinary, shas: &[Oid]) -> Result<Vec<String>>
|
||||
.collect::<Vec<_>>())
|
||||
}
|
||||
|
||||
/// Parse the output of `git diff --name-status -z`
|
||||
pub fn parse_git_diff_name_status(content: &str) -> impl Iterator<Item = (&str, StatusCode)> {
|
||||
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<CommitDiffObject<'a>>,
|
||||
pub new_object: Option<CommitDiffObject<'a>>,
|
||||
}
|
||||
|
||||
/// Parses the output of `git diff --raw --no-abbrev -z`.
|
||||
pub(crate) fn parse_git_diff_raw(
|
||||
content: &str,
|
||||
) -> impl Iterator<Item = Result<CommitDiffEntry<'_>>> {
|
||||
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<CommitDiffEntry<'a>> {
|
||||
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::<Vec<_>>();
|
||||
let entries = parse_git_diff_raw(input)
|
||||
.collect::<Result<Vec<_>>>()
|
||||
.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"));
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user