Skip to content

Commit 82523a4

Browse files
committed
fix(engine): exclude enola's output dir from git dirtiness
gitInfo set Dirty on any `git status --porcelain` output, and --porcelain lists untracked paths. The snapshot creates <repo>/.enola itself, before the receipt reads git state, so any repo that does not gitignore it recorded dirty:true for a fully committed checkout — and from the second snapshot on, the directory pre-exists the run. That wrong baseline disabled the staleness check's VCS arm, which can only fire when the recorded state was clean, leaving those repos with no git-drift signal at all. The status query now excludes the configured output dir via a pathspec, in gitInfo itself so the snapshot-time capture and the live check share one definition of dirty. Splitting them would record clean against a live dirty and fire on every snapshot. Fact output is unchanged; no cacheVersion bump.
1 parent fe82cb3 commit 82523a4

5 files changed

Lines changed: 144 additions & 5 deletions

File tree

internal/engine/engine.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,7 @@ func (e *Engine) GenerateSnapshot(ctx context.Context, repoPath string, appendMo
427427

428428
EnolaVersion: version.Version,
429429
SnapshotID: computeSnapshotID(factsBuf.Bytes(), version.Version, configHash),
430-
Git: gitInfo(absRepo),
430+
Git: gitInfo(absRepo, e.cfg.Output.Dir),
431431
ConfigHash: configHash,
432432

433433
FilesSeen: len(files),

internal/engine/freshness.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,11 @@ func (e *Engine) Staleness(maxAge time.Duration, now time.Time) Staleness {
7070
if r.Path == "" || r.Git == nil {
7171
continue // non-git or unknown: covered by the age signal only
7272
}
73-
cur := gitInfo(r.Path)
73+
// Same outputDir exclusion as the snapshot-time capture in receipt.go. The two
74+
// MUST agree: recording "clean" while reading the live tree as dirty (because
75+
// enola's own output dir is untracked) would make the arm below fire on every
76+
// single snapshot.
77+
cur := gitInfo(r.Path, e.cfg.Output.Dir)
7478
if cur == nil {
7579
continue
7680
}

internal/engine/global_receipt.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ func (e *Engine) repoEntries(b *snapshotBundle) []facts.GraphRepoEntry {
158158
entries = append(entries, facts.GraphRepoEntry{
159159
Label: label,
160160
Path: abs,
161-
Git: gitInfo(abs),
161+
Git: gitInfo(abs, e.cfg.Output.Dir),
162162
FactCount: b.store.CountByRepo(label),
163163
SourceBytes: e.repoSourceBytes(b, abs),
164164
})

internal/engine/receipt.go

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,18 @@ func computeConfigHash(cfg *config.Config) string {
7676
// gitInfo captures the repository's VCS state. It returns nil when repoPath is
7777
// not a git working tree or git is unavailable, so a non-git or sandboxed run
7878
// degrades to no git section rather than erroring.
79-
func gitInfo(repoPath string) *facts.GitInfo {
79+
//
80+
// outputDir is the engine's output directory (cfg.Output.Dir), excluded from the
81+
// dirtiness check. enola writes its own artifacts there and --porcelain lists
82+
// untracked paths, so without the exclusion any repo that does not gitignore that
83+
// directory records dirty:true from its first snapshot onward — the cache save
84+
// creates it before this function runs, and it pre-exists every later run. That
85+
// wrong baseline silently disables the git-drift half of the staleness check, whose
86+
// "newly dirty" arm can only fire when the recorded state was clean (see
87+
// freshness.go). The walker already excludes the same directory from analysis, so
88+
// keeping it out here makes the flag describe the SOURCE rather than enola's own
89+
// output. Pass "" to check the whole tree.
90+
func gitInfo(repoPath, outputDir string) *facts.GitInfo {
8091
commit, err := runGit(repoPath, "rev-parse", "HEAD")
8192
if err != nil || commit == "" {
8293
return nil
@@ -86,7 +97,14 @@ func gitInfo(repoPath string) *facts.GitInfo {
8697
info.Ref = ref
8798
}
8899
// --porcelain prints one line per changed/untracked path; any output => dirty.
89-
if status, err := runGit(repoPath, "status", "--porcelain"); err == nil && strings.TrimSpace(status) != "" {
100+
// The ':(exclude)' pathspec drops our own output dir; '.' is the positive
101+
// pathspec it subtracts from. An outputDir configured outside the repo simply
102+
// matches nothing, which is harmless.
103+
statusArgs := []string{"status", "--porcelain"}
104+
if outputDir != "" {
105+
statusArgs = append(statusArgs, "--", ".", ":(exclude)"+outputDir)
106+
}
107+
if status, err := runGit(repoPath, statusArgs...); err == nil && strings.TrimSpace(status) != "" {
90108
info.Dirty = true
91109
}
92110
return info

internal/engine/restore_test.go

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,123 @@ func writeForeignGlobalReceipt(t *testing.T, home, foreignRepoPath, generatedAt
201201
}
202202
}
203203

204+
// TestSnapshotGit_OwnOutputDirIsNotTreeDirt is the regression for enola recording its
205+
// OWN artifacts as working-tree dirt. gitInfo decides dirtiness from any
206+
// `git status --porcelain` output, and --porcelain lists untracked paths — so the
207+
// .enola/ directory the snapshot itself creates (extractor-cache save, engine.go:328,
208+
// which runs BEFORE the receipt reads git state) made a pristine committed repo record
209+
// dirty: true.
210+
//
211+
// The repo here deliberately does NOT gitignore the output dir, which is the case that
212+
// regresses. The second snapshot matters as much as the first: by then .enola/ already
213+
// exists before the run begins, so call ordering is no longer the operative cause and
214+
// only excluding the directory fixes it.
215+
func TestSnapshotGit_OwnOutputDirIsNotTreeDirt(t *testing.T) {
216+
if _, err := exec.LookPath("git"); err != nil {
217+
t.Skip("git not available")
218+
}
219+
repo := t.TempDir()
220+
writeFile(t, filepath.Join(repo, "go.mod"), "module dirtmod\n\ngo 1.21\n")
221+
writeFile(t, filepath.Join(repo, "pkg", "a", "a.go"), "package a\n\nfunc A() {}\n")
222+
initGitRepo(t, repo)
223+
224+
cfg := config.Default()
225+
eng, err := engine.New(cfg)
226+
if err != nil {
227+
t.Fatal(err)
228+
}
229+
eng.RegisterExtractor(goextractor.New())
230+
231+
for _, run := range []string{"first", "second"} {
232+
snap, err := eng.GenerateSnapshot(context.Background(), repo, false)
233+
if err != nil {
234+
t.Fatalf("%s snapshot: %v", run, err)
235+
}
236+
if err := eng.WriteArtifacts(repo); err != nil {
237+
t.Fatalf("%s WriteArtifacts: %v", run, err)
238+
}
239+
if snap.Meta.Git == nil {
240+
t.Fatalf("%s run: no git info recorded", run)
241+
}
242+
if snap.Meta.Git.Dirty {
243+
t.Errorf("%s run: committed-clean repo recorded dirty:true — enola's own %s/ counted as tree dirt",
244+
run, cfg.Output.Dir)
245+
}
246+
}
247+
}
248+
249+
// TestSnapshotGit_RealChangeStillDirty guards the fix above against over-suppressing:
250+
// excluding the output dir must not blind the flag to actual uncommitted source changes.
251+
func TestSnapshotGit_RealChangeStillDirty(t *testing.T) {
252+
if _, err := exec.LookPath("git"); err != nil {
253+
t.Skip("git not available")
254+
}
255+
repo := t.TempDir()
256+
writeFile(t, filepath.Join(repo, "go.mod"), "module dirtmod2\n\ngo 1.21\n")
257+
writeFile(t, filepath.Join(repo, "pkg", "a", "a.go"), "package a\n\nfunc A() {}\n")
258+
initGitRepo(t, repo)
259+
260+
// A real, uncommitted modification to a TRACKED file.
261+
writeFile(t, filepath.Join(repo, "pkg", "a", "a.go"), "package a\n\nfunc A() {}\nfunc B() {}\n")
262+
263+
cfg := config.Default()
264+
eng, err := engine.New(cfg)
265+
if err != nil {
266+
t.Fatal(err)
267+
}
268+
eng.RegisterExtractor(goextractor.New())
269+
270+
snap, err := eng.GenerateSnapshot(context.Background(), repo, false)
271+
if err != nil {
272+
t.Fatal(err)
273+
}
274+
if snap.Meta.Git == nil {
275+
t.Fatal("no git info recorded")
276+
}
277+
if !snap.Meta.Git.Dirty {
278+
t.Error("a modified tracked file must still record dirty:true")
279+
}
280+
}
281+
282+
// TestStaleness_DetectsEditAfterCleanSnapshotWithoutGitignore is the payoff. A repo that
283+
// does not gitignore the output dir previously recorded dirty:true at snapshot time,
284+
// which killed the `!r.Git.Dirty && cur.Dirty` arm for its whole life — so a later edit
285+
// produced no staleness signal at all. With the baseline recorded correctly, the arm is
286+
// live again.
287+
func TestStaleness_DetectsEditAfterCleanSnapshotWithoutGitignore(t *testing.T) {
288+
if _, err := exec.LookPath("git"); err != nil {
289+
t.Skip("git not available")
290+
}
291+
t.Setenv("HOME", t.TempDir())
292+
293+
repo := t.TempDir()
294+
writeFile(t, filepath.Join(repo, "go.mod"), "module dirtmod3\n\ngo 1.21\n")
295+
writeFile(t, filepath.Join(repo, "pkg", "a", "a.go"), "package a\n\nfunc A() {}\n")
296+
initGitRepo(t, repo)
297+
298+
cfg := config.Default()
299+
eng, err := engine.New(cfg)
300+
if err != nil {
301+
t.Fatal(err)
302+
}
303+
eng.RegisterExtractor(goextractor.New())
304+
if _, err := eng.GenerateSnapshot(context.Background(), repo, false); err != nil {
305+
t.Fatal(err)
306+
}
307+
308+
// Edit a tracked file after the snapshot, leaving it uncommitted.
309+
writeFile(t, filepath.Join(repo, "pkg", "a", "a.go"), "package a\n\nfunc A() {}\nfunc Added() {}\n")
310+
311+
now := time.Now()
312+
st := eng.Staleness(24*time.Hour, now)
313+
if st.TooOld {
314+
t.Fatalf("snapshot is seconds old, TooOld should be false (Age=%s)", st.Age)
315+
}
316+
if len(st.Changed) != 1 || st.Changed[0].Reason != "uncommitted changes" {
317+
t.Errorf("got Changed=%+v, want one \"uncommitted changes\" for the edited repo", st.Changed)
318+
}
319+
}
320+
204321
// initGitRepo creates a git repo with one commit and returns its HEAD.
205322
func initGitRepo(t *testing.T, repo string) string {
206323
t.Helper()

0 commit comments

Comments
 (0)