Repository navigation
Show real diffs for a staged addition instead of the whole file (issue #370) - #758
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation correctly addresses the reported behavior and includes focused regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes staged additions so staged and subsequent unstaged changes render as actual diffs.
Changes:
- Distinguishes untracked files from staged additions.
- Uses
diff-indexanddiff-filesfor staged additions. - Adds regression tests for all affected paths.
| File | Description |
|---|---|
Resources/html/views/commit/commit.js |
Shows whole contents only for untracked files. |
Classes/git/PBGitIndex.m |
Generates proper staged and unstaged diffs for additions. |
GitXTests/PBGitIndexDiffCacheTests.m |
Covers untracked and staged-addition behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
goneng
left a comment
goneng
left a comment
There was a problem hiding this comment.
Thanks @pcambra, the staged and unstaged sides of a new file now read as real diffs, and the unstaged side of a staged addition also stops failing when the file was deleted from disk afterwards. One thing worth handling before this lands, see the inline comment.
| // Only a file git does not know at all is shown whole. Once an addition is | ||
| // staged, each side has a real diff: the index against the parent tree and | ||
| // the working file against the index | ||
| if (file.status == 0 && !file.hasStagedChanges) // Untracked file? |
There was a problem hiding this comment.
With this, a staged addition goes through displayDiff, which also turns on line selection.
Unstaging only some of its lines then fails:
stageLines keeps the diff header, which for an addition carries
new file mode 100644 and --- /dev/null, and
git rejects a partial reverse of a creation patch:
error: new file n.txt depends on old contents
Steps:
- create
n.txtwith lines a, b, c, d, git addit,- select the staged entry,
- select only the
+bline and unstage it.
The line stays staged.
Before this PR the staged entry had no hunk or line controls, so this path was not reachable.
The hunk-level Unstage button is fine: it removes the whole entry from the index and the file becomes untracked again.
A possible fix:
when the diff is an addition, rewrite the header for line operations as a modification of the index entry,
i.e. drop new file mode and use --- a/<path> / +++ b/<path> with old start 1.
This applies cleanly with git apply --unidiff-zero --cached --reverse and
unstages just the selected line:
diff --git a/n.txt b/n.txt
--- a/n.txt
+++ b/n.txt
@@ -1,3 +1,4 @@
a
+b
c
d
|
Thank you for the feedback, not my turf :). Pushed 5bf518c trying to implement your suggestion: for a line operation on an addition, Verification: |
|
@pcambra @hannesa2 LGTM, with one comment: the new branch in The check is
In both, unstaging some of the lines now goes through Either of these would line them up:
The first is simpler, since the rewrite is correct for the other two cases as well. |
|
Pushed the comment rewording. On run 1750 the failures were in PBGitWorktreeManagementTests (-waitForWorktrees timed out, then a nil reached an array literal); the diff-cache tests passed. Could someone approve a re-run when convenient? |
|
@pcambra , please squash the chance to a single commit - it keeps the repo more coherent. Thanks for your contribution! |
|
done! |
Fixes #370
Symptom
Stage a new file, edit it again: both the staged and the unstaged entry show the raw on-disk file with no
+/-lines. The two sides are indistinguishable from a new file until the file is committed.Cause
NEWonly means the path is absent from the parent tree, but two places treated everyNEWfile as untracked:Resources/html/views/commit/commit.js:showFileChangesshort-circuits toshowNewFilefor anystatus == 0, andshowNewFilereads the working file whole regardless of which list was clicked.PBGitIndex readDiffForFile:staged:contextLines:: the staged side of aNEWfile is answered withgit show :0:<path>(a bare blob dump) and the unstaged side by reading the working file from disk.Fix
Only a file without an index entry (
status == NEW && !hasStagedChanges) is shown whole. A staged addition is diffed withdiff-index --cachedagainst the parent tree (the empty tree in an unborn repository), and its later edits withdiff-filesagainst the index, the same as any tracked file.PBChangedFilealready exposeshasStagedChangesto the web view (isKeyExcludedFromWebScriptreturnsNO), so the JS condition needs no bridging change.Tests
Three new cases in
PBGitIndexDiffCacheTests, using its existing stub harness:diff-filesfor its unstaged sidediff-index --cached HEADfor its staged sidemake unit-testis green locally on arm64.Manual check
New file,
git add, edit again, open the repo in the built app: the staged entry shows the whole file as+lines, the unstaged entry only the later edit. On 1.6 both show unmarked text.Notes
Same approach as @hbmartin's fork commit hbmartin/gitx@767dbf7, which never reached a PR; rebuilt against current master since upstream renders through the WebKit view and
diffForFile:is now split into the cachedreadDiffForFile:. The screenshot comparison does not run on fork PRs; if any baseline shows a staged new file it will need updating.