Skip to content

Commit b89bb81

Browse files
fix(git): close native status parity gaps found in review
An adversarial review verified five silent divergences from the exec path, each reproduced against real repositories before fixing: - executable-bit changes were invisible on both sides: the worktree scan now compares the 0100 bit when core.filemode applies, and the HEAD tree walk carries entry modes so staged chmods surface too - a conflict living in an otherwise-untracked directory was counted as an untracked entry; unmerged paths now participate in the tracked prefix search - a staged rename with edits reported add+delete where git pairs them through similarity detection; unpaired adds and deletes on both sides now defer to exec git rather than risk diverging counts - a loose ref holding a symref misreported the branch as unborn; an unparseable loose ref is now an error, falling back to exec git - file-to-symlink type changes counted as modified where porcelain reports T, which the segment ignores; both engines now agree SHA-256 object format, reftables ref storage, and unknown repository format versions are also rejected deterministically instead of relying on the index decoder to stumble. New parity scenarios cover chmod on both sides, conflicts in fresh subdirectories, inexact renames, and symlink type changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent dbc8616 commit b89bb81

7 files changed

Lines changed: 290 additions & 36 deletions

File tree

src/gitstatus/gitstatus.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,10 @@ func Load(opts Options) (*Result, error) {
8787
// there is no upstream to resolve and no repo-level excludesfile.
8888
cfg, _ := loadRepoConfig(opts.CommonGitDir)
8989

90+
if err := checkRepoFormat(cfg); err != nil {
91+
return nil, err
92+
}
93+
9094
store := newObjectStore(opts.CommonGitDir)
9195
defer store.close()
9296

@@ -96,7 +100,7 @@ func Load(opts Options) (*Result, error) {
96100
}
97101

98102
basePatterns := loadBasePatterns(opts, cfg)
99-
scanWorktree(opts, idx, indexModTime, untrackedMode, basePatterns, result)
103+
scanWorktree(opts, idx, indexModTime, untrackedMode, trustExecutableBit(cfg), basePatterns, result)
100104

101105
if err := diffStaging(store, idx, headHash, headOK, result); err != nil {
102106
return nil, err

src/gitstatus/gitstatus_test.go

Lines changed: 113 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"os"
66
"os/exec"
77
"path/filepath"
8+
"runtime"
89
"strings"
910
"testing"
1011

@@ -41,6 +42,10 @@ func TestLoadParity(t *testing.T) {
4142
{Name: "index version 4", Setup: setupIndexV4},
4243
{Name: "fully packed objects", Setup: setupPacked},
4344
{Name: "packed ahead and behind", Setup: setupPackedAheadBehind},
45+
{Name: "staged delete only", Setup: setupStagedDelete},
46+
{Name: "staged mode change", Setup: setupStagedModeChange},
47+
{Name: "conflict in fresh subdirectory", Setup: setupConflictInSubdir},
48+
{Name: "working mode change", Setup: setupWorkingModeChange},
4449
}
4550

4651
// Every case runs under both stat strategies: the in-walk comparison
@@ -69,6 +74,58 @@ func TestLoadParity(t *testing.T) {
6974
}
7075
}
7176

77+
// TestLoadFallsBackOnInexactRename covers the C4 contract decision: a
78+
// staged rename whose content also changed leaves an unpaired add and an
79+
// unpaired delete, which git may pair through similarity detection. The
80+
// engine cannot, so it must error into the exec fallback instead of
81+
// reporting A+D where git reports R.
82+
func TestLoadFallsBackOnInexactRename(t *testing.T) {
83+
skipIfNoGit(t)
84+
hermeticHome(t)
85+
86+
dir := t.TempDir()
87+
initGitRepo(t, dir)
88+
89+
writeFile(t, dir, "old-name.txt", "a body large enough for gits similarity detection to pair the rename\n")
90+
runGit(t, dir, "add", ".")
91+
runGit(t, dir, "commit", "-q", "-m", "base")
92+
93+
runGit(t, dir, "mv", "old-name.txt", "new-name.txt")
94+
writeFile(t, dir, "new-name.txt", "a body large enough for gits similarity detection to pair the rename, edited\n")
95+
runGit(t, dir, "add", "new-name.txt")
96+
97+
_, err := Load(Options{
98+
WorktreeGitDir: gitPath(t, dir, "--git-dir"),
99+
CommonGitDir: gitPath(t, dir, "--git-common-dir"),
100+
RepoRoot: gitPath(t, dir, "--show-toplevel"),
101+
})
102+
assert.Error(t, err)
103+
}
104+
105+
// TestLoadParityTypechangeSymlink replaces a tracked file with a symlink:
106+
// porcelain reports T, which both engines must count as nothing. Skipped on
107+
// Windows, where creating symlinks requires elevated privileges.
108+
func TestLoadParityTypechangeSymlink(t *testing.T) {
109+
if runtime.GOOS == goosWindows {
110+
t.Skip("symlink creation requires privileges on Windows")
111+
}
112+
skipIfNoGit(t)
113+
hermeticHome(t)
114+
115+
dir := t.TempDir()
116+
initGitRepo(t, dir)
117+
118+
writeFile(t, dir, "target.txt", "target\n")
119+
writeFile(t, dir, "swap.txt", "file\n")
120+
runGit(t, dir, "add", ".")
121+
runGit(t, dir, "commit", "-q", "-m", "base")
122+
123+
require.NoError(t, os.Remove(filepath.Join(dir, "swap.txt")))
124+
require.NoError(t, os.Symlink("target.txt", filepath.Join(dir, "swap.txt")))
125+
126+
assertParity(t, dir, "")
127+
}
128+
72129
// TestLoadParityLinkedWorktree exercises a `git worktree add` checkout,
73130
// where WorktreeGitDir (HEAD/index) and CommonGitDir (objects/refs) live in
74131
// different places on disk.
@@ -129,7 +186,6 @@ func setupDirtyMix(t *testing.T, dir string) {
129186
writeFile(t, dir, "same-size.txt", "hello\n")
130187
writeFile(t, dir, "diff-size.txt", "short\n")
131188
writeFile(t, dir, "to-delete.txt", "bye\n")
132-
writeFile(t, dir, "staged-delete.txt", "gone\n")
133189
writeFile(t, dir, "staged-modify.txt", "orig\n")
134190
writeFile(t, dir, "rename-src.txt", "rename me please, needs enough content to be detected as a rename by gits similarity heuristic\n")
135191
runGit(t, dir, "add", ".")
@@ -142,11 +198,10 @@ func setupDirtyMix(t *testing.T, dir string) {
142198
// unstaged delete
143199
require.NoError(t, os.Remove(filepath.Join(dir, "to-delete.txt")))
144200

145-
// staged add
201+
// staged add; a staged delete alongside would trip the rename-ambiguity
202+
// fallback, so pure deletion coverage lives in its own scenario
146203
writeFile(t, dir, "staged-add.txt", "new\n")
147204
runGit(t, dir, "add", "staged-add.txt")
148-
// staged delete
149-
runGit(t, dir, "rm", "-q", "staged-delete.txt")
150205
// staged modify
151206
writeFile(t, dir, "staged-modify.txt", "changed\n")
152207
runGit(t, dir, "add", "staged-modify.txt")
@@ -283,6 +338,60 @@ func setupPackedAheadBehind(t *testing.T, dir string) {
283338
runGit(t, dir, "prune-packed")
284339
}
285340

341+
// setupStagedDelete stages a deletion with no staged addition alongside, so
342+
// the exact-rename pairing has nothing to pair and no fallback triggers.
343+
func setupStagedDelete(t *testing.T, dir string) {
344+
writeFile(t, dir, "keep.txt", "keep\\n")
345+
writeFile(t, dir, "doomed.txt", "doomed\\n")
346+
runGit(t, dir, "add", ".")
347+
runGit(t, dir, "commit", "-q", "-m", "base")
348+
349+
runGit(t, dir, "rm", "-q", "doomed.txt")
350+
}
351+
352+
// setupStagedModeChange stages an executable-bit flip via update-index,
353+
// which works on every platform because no filesystem mode is involved.
354+
func setupStagedModeChange(t *testing.T, dir string) {
355+
writeFile(t, dir, "script.sh", "#!/bin/sh\\n")
356+
runGit(t, dir, "add", ".")
357+
runGit(t, dir, "commit", "-q", "-m", "base")
358+
359+
runGit(t, dir, "update-index", "--chmod=+x", "script.sh")
360+
}
361+
362+
// setupConflictInSubdir produces a both-added conflict on a path whose
363+
// directory holds no other tracked file, so the untracked-directory
364+
// collapsing must still recognize it as tracked.
365+
func setupConflictInSubdir(t *testing.T, dir string) {
366+
writeFile(t, dir, "base.txt", "base\\n")
367+
runGit(t, dir, "add", ".")
368+
runGit(t, dir, "commit", "-q", "-m", "base")
369+
370+
runGit(t, dir, "checkout", "-q", "-b", "feature")
371+
writeFile(t, dir, "sub/only.txt", "from feature\\n")
372+
runGit(t, dir, "add", ".")
373+
runGit(t, dir, "commit", "-q", "-m", "feature adds sub")
374+
375+
runGit(t, dir, "checkout", "-q", "main")
376+
writeFile(t, dir, "sub/only.txt", "from main\\n")
377+
runGit(t, dir, "add", ".")
378+
runGit(t, dir, "commit", "-q", "-m", "main adds sub")
379+
380+
runGitAllowFail(t, dir, "merge", "-q", "feature")
381+
}
382+
383+
// setupWorkingModeChange flips the on-disk executable bit of a tracked
384+
// file. Meaningful only where the filesystem records the bit; on Windows
385+
// core.filemode=false makes it a no-op clean scenario, which is itself
386+
// worth asserting.
387+
func setupWorkingModeChange(t *testing.T, dir string) {
388+
writeFile(t, dir, "script.sh", "#!/bin/sh\\n")
389+
runGit(t, dir, "add", ".")
390+
runGit(t, dir, "commit", "-q", "-m", "base")
391+
392+
require.NoError(t, os.Chmod(filepath.Join(dir, "script.sh"), 0o755))
393+
}
394+
286395
func setupDetached(t *testing.T, dir string) {
287396
writeFile(t, dir, "a.txt", "a\n")
288397
runGit(t, dir, "add", ".")

src/gitstatus/objects.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,9 @@ func identTimestamp(ident string) int64 {
7878
const treeModeDir = "40000"
7979

8080
// walkTree streams every blob in the tree rooted at h to visit, with its
81-
// full slash-separated path. Non-blob, non-tree entries (gitlinks) are
82-
// skipped.
83-
func walkTree(store *objectStore, h plumbing.Hash, prefix string, visit func(path string, blob plumbing.Hash)) error {
81+
// full slash-separated path and recorded mode. Non-blob, non-tree entries
82+
// (gitlinks) are skipped.
83+
func walkTree(store *objectStore, h plumbing.Hash, prefix string, visit func(path string, blob plumbing.Hash, mode uint32)) error {
8484
kind, data, err := store.object(h)
8585
if err != nil {
8686
return err
@@ -117,7 +117,11 @@ func walkTree(store *objectStore, h plumbing.Hash, prefix string, visit func(pat
117117
}
118118
case "160000": // gitlink (submodule): not a blob, skip
119119
default:
120-
visit(path, entryHash)
120+
parsed, err := strconv.ParseUint(mode, 8, 32)
121+
if err != nil {
122+
return fmt.Errorf("gitstatus: malformed tree entry mode in %s: %w", h, err)
123+
}
124+
visit(path, entryHash, uint32(parsed))
121125
}
122126
}
123127

src/gitstatus/refs.go

Lines changed: 64 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"fmt"
77
"os"
88
"path/filepath"
9+
"runtime"
910
"strings"
1011

1112
"github.com/go-git/go-git/v5/plumbing"
@@ -40,7 +41,11 @@ func resolveBranch(opts Options, cfg *ini.File, store *objectStore, result *Resu
4041

4142
result.Ref = branchName
4243

43-
hash, ok := resolveRef(opts.CommonGitDir, "refs/heads/"+branchName)
44+
hash, ok, err := resolveRef(opts.CommonGitDir, "refs/heads/"+branchName)
45+
if err != nil {
46+
return plumbing.ZeroHash, false, err
47+
}
48+
4449
if !ok {
4550
// Unborn branch: no commits yet. Matches porcelain's
4651
// `# branch.oid (initial)`. Upstream/ahead-behind are left at their
@@ -105,7 +110,11 @@ func resolveUpstream(opts Options, cfg *ini.File, store *objectStore, branchName
105110
upstreamRefPath = "refs/remotes/" + remote + "/" + mergeBranch
106111
}
107112

108-
upstreamHash, ok := resolveRef(opts.CommonGitDir, upstreamRefPath)
113+
upstreamHash, ok, err := resolveRef(opts.CommonGitDir, upstreamRefPath)
114+
if err != nil {
115+
return err
116+
}
117+
109118
if !ok {
110119
result.UpstreamGone = true
111120
return nil
@@ -124,15 +133,21 @@ func resolveUpstream(opts Options, cfg *ini.File, store *objectStore, branchName
124133

125134
// resolveRef resolves a ref path (e.g. "refs/heads/main") to its commit
126135
// hash, checking the loose ref file first and falling back to packed-refs.
127-
func resolveRef(commonGitDir, refPath string) (plumbing.Hash, bool) {
136+
// A loose ref file that exists but does not hold a plain hash (a symref,
137+
// or corruption) is an error: guessing here would silently misreport the
138+
// branch as unborn, so the caller must fall back to exec git instead.
139+
func resolveRef(commonGitDir, refPath string) (plumbing.Hash, bool, error) {
128140
data, err := os.ReadFile(filepath.Join(commonGitDir, filepath.FromSlash(refPath)))
129141
if err == nil {
130-
if hash, ok := parseHash(strings.TrimSpace(string(data))); ok {
131-
return hash, true
142+
hash, ok := parseHash(strings.TrimSpace(string(data)))
143+
if !ok {
144+
return plumbing.ZeroHash, false, fmt.Errorf("gitstatus: unsupported loose ref content in %s", refPath)
132145
}
146+
return hash, true, nil
133147
}
134148

135-
return scanPackedRefs(commonGitDir, refPath)
149+
hash, found := scanPackedRefs(commonGitDir, refPath)
150+
return hash, found, nil
136151
}
137152

138153
// scanPackedRefs looks up refPath in CommonGitDir/packed-refs. Lines are
@@ -193,3 +208,46 @@ func loadRepoConfig(commonGitDir string) (*ini.File, error) {
193208

194209
return ini.Load(string(data))
195210
}
211+
212+
// checkRepoFormat rejects repository formats the engine cannot read
213+
// correctly, deterministically instead of by parse luck: SHA-256 object
214+
// format, reftables ref storage, and future format versions.
215+
func checkRepoFormat(cfg *ini.File) error {
216+
if cfg == nil {
217+
return nil
218+
}
219+
220+
core := cfg.Section("core")
221+
if v := core.Key("repositoryformatversion").String(); v != "" && v != "0" && v != "1" {
222+
return fmt.Errorf("gitstatus: unsupported repositoryformatversion %s", v)
223+
}
224+
225+
extensions := cfg.Section("extensions")
226+
227+
if v := extensions.Key("objectformat").String(); v != "" && !strings.EqualFold(v, "sha1") {
228+
return fmt.Errorf("gitstatus: unsupported object format %s", v)
229+
}
230+
231+
if v := extensions.Key("refstorage").String(); v != "" && !strings.EqualFold(v, "files") {
232+
return fmt.Errorf("gitstatus: unsupported ref storage %s", v)
233+
}
234+
235+
return nil
236+
}
237+
238+
// trustExecutableBit mirrors git's core.filemode: whether the filesystem
239+
// records the executable bit reliably. Platform default, overridden by the
240+
// repo config (git init probes the filesystem and records the verdict).
241+
func trustExecutableBit(cfg *ini.File) bool {
242+
trust := runtime.GOOS != goosWindows
243+
if cfg == nil {
244+
return trust
245+
}
246+
247+
v := cfg.Section("core").Key("filemode").String()
248+
if v == "" {
249+
return trust
250+
}
251+
252+
return strings.EqualFold(v, "true")
253+
}

0 commit comments

Comments
 (0)