refactor: extract showLockFileAtCommit to public git.ShowFileAtCommit+ lockfile.ShowAtCommit#141
Conversation
… + lockfile.ShowAtCommit Move the private showLockFileAtCommit function from synthistory.go into two public, reusable packages: - git.ShowFileAtCommit: generic CLI-based 'git show commit:path' with ErrFileNotFound sentinel for missing files/directories - lockfile.ShowAtCommit: lockfile-specific wrapper combining ShowFileAtCommit with lockfile.Parse Uses CLI git instead of go-git for better performance on large repositories (avoids loading full tree into memory). Simplify readLockFileAtHEAD to pass "HEAD" directly instead of resolving via git rev-parse first.
There was a problem hiding this comment.
Pull request overview
Refactors lockfile history inspection to use CLI git show for reading lock files at specific revisions, extracting the previous private helper into reusable utilities.
Changes:
- Added
git.ShowFileAtCommit()andgit.ErrFileNotFoundsentinel forgit show <rev>:<path>reads. - Added
lockfile.ShowAtCommit()wrapper that reads via git util and parses aComponentLock. - Updated synthetic history code to use the new helpers and to read
HEADdirectly without an extrarev-parse.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/utils/git/git.go | Introduces ErrFileNotFound and ShowFileAtCommit() implemented via CLI git show. |
| internal/utils/git/showfile_test.go | Adds unit tests + an E2E-style test validating stderr parsing behavior. |
| internal/lockfile/show.go | Adds lockfile.ShowAtCommit() wrapper around git.ShowFileAtCommit() + lockfile.Parse(). |
| internal/lockfile/show_test.go | Adds tests for lockfile.ShowAtCommit() success, not-found propagation, and parse/version failures. |
| internal/app/azldev/core/sources/synthistory.go | Switches synthetic history lockfile reads from go-git tree walking to the new CLI-based helpers. |
| var stderr bytes.Buffer | ||
|
|
||
| objectRef := revision + ":" + relPath | ||
| rawCmd := exec.CommandContext(ctx, "git", "-C", repoDir, "show", objectRef) |
There was a problem hiding this comment.
ShowFileAtCommit constructs git show without a -- end-of-options separator. If revision (branch/tag) ever begins with '-' (legal in git) it can be interpreted as a flag, changing behavior unexpectedly. Add -- before the revision:relPath argument to ensure it’s always parsed as an object spec, not options.
| rawCmd := exec.CommandContext(ctx, "git", "-C", repoDir, "show", objectRef) | |
| rawCmd := exec.CommandContext(ctx, "git", "-C", repoDir, "show", "--", objectRef) |
| repoDir string, | ||
| revision string, | ||
| relPath string, | ||
| ) (string, error) { |
There was a problem hiding this comment.
ShowFileAtCommit shells out to git but doesn’t verify it exists via cmdFactory.CommandInSearchPath("git"). Adding an early check lets callers get a clear, deterministic error (and aligns with the repo pattern of checking tool availability through the command factory rather than exec.LookPath).
| ) (string, error) { | |
| ) (string, error) { | |
| if _, err := cmdFactory.CommandInSearchPath("git"); err != nil { | |
| return "", fmt.Errorf("git is not available in PATH:\n%w", err) | |
| } |
| if _, err := exec.LookPath("git"); err != nil { | ||
| t.Skip("git not available") | ||
| } |
There was a problem hiding this comment.
This test uses exec.LookPath("git") to probe tool availability. Elsewhere in the repo we check tool presence through the opctx command factory/context (CommandInSearchPath), e.g. internal/utils/prereqs/prereqs.go:41-66. Consider switching to cmdFactory.CommandInSearchPath("git") (or ctx.CommandInSearchPath) so the check is stub-friendly and consistent with production code.
Revert to go-git for lockfile.ShowAtCommit based on review feedback. The CLI git approach was motivated by git-log path filtering performance (which remains CLI), but single-file reads via go-git tree.File() are adequate — the cost is two tree object lookups per call, not a full tree load. Go-git eliminates stderr parsing fragility (locale dependence, multiple error message variants) and subprocess overhead. - Remove git.ShowFileAtCommit and git.ErrFileNotFound (CLI-based) - Remove showfile_test.go (CLI e2e tests) - Rewrite lockfile.ShowAtCommit to use go-git tree traversal - Rewrite lockfile tests to use in-memory go-git repos - Restore *gogit.Repository params in FindFingerprintChanges and readLockFileAtHEAD
Move the private showLockFileAtCommit function from synthistory.go into two public, reusable packages:
Uses CLI git instead of go-git for better performance on large repositories (avoids loading full tree into memory).
Simplify readLockFileAtHEAD to pass "HEAD" directly instead of resolving via git rev-parse first.