Skip to content

Commit a27296e

Browse files
committed
test: fail loudly if EvalSymlinks errors instead of vacuous pass
Address review: discarded EvalSymlinks errors could yield empty paths and a misleading assertion result on restricted filesystems.
1 parent fbfc2c9 commit a27296e

2 files changed

Lines changed: 19 additions & 4 deletions

File tree

cmd/opencodereview/shared.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,9 @@ func resolveWorkingDir(input string, requireGit bool) (string, bool, error) {
108108
// requireGit is true only for the review path; scan (requireGit=false) keeps
109109
// the CWD so its `git ls-files` walk stays scoped to the subdirectory.
110110
if isGit && requireGit {
111+
// runGitCmdStdout captures stdout only so git stderr notices can't
112+
// pollute the resolved path. absPath is overridden only on success;
113+
// on any rev-parse failure it falls back to the caller-provided dir.
111114
if top, topErr := runGitCmdStdout(absPath, "rev-parse", "--show-toplevel"); topErr == nil {
112115
if t := strings.TrimSpace(string(top)); t != "" {
113116
absPath = t

cmd/opencodereview/shared_test.go

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,10 @@ func TestResolveWorkingDir_MonorepoSubdir(t *testing.T) {
136136

137137
// macOS /var -> /private/var symlink means t.TempDir() differs from the
138138
// canonicalized toplevel git returns; compare via EvalSymlinks.
139-
wantRoot, _ := filepath.EvalSymlinks(root)
139+
wantRoot, err := filepath.EvalSymlinks(root)
140+
if err != nil {
141+
t.Fatalf("EvalSymlinks(%q): %v", root, err)
142+
}
140143

141144
// review path: hoisted to the git top-level.
142145
got, isGit, err := resolveWorkingDir(sub, true)
@@ -146,7 +149,10 @@ func TestResolveWorkingDir_MonorepoSubdir(t *testing.T) {
146149
if !isGit {
147150
t.Error("expected isGit=true for a git subdirectory")
148151
}
149-
gotResolved, _ := filepath.EvalSymlinks(got)
152+
gotResolved, err := filepath.EvalSymlinks(got)
153+
if err != nil {
154+
t.Fatalf("EvalSymlinks(%q): %v", got, err)
155+
}
150156
if gotResolved != wantRoot {
151157
t.Errorf("review RepoDir = %q, want git top-level %q", gotResolved, wantRoot)
152158
}
@@ -156,8 +162,14 @@ func TestResolveWorkingDir_MonorepoSubdir(t *testing.T) {
156162
if err != nil {
157163
t.Fatalf("resolveWorkingDir(sub, false) error: %v", err)
158164
}
159-
gotScanResolved, _ := filepath.EvalSymlinks(gotScan)
160-
wantSub, _ := filepath.EvalSymlinks(sub)
165+
gotScanResolved, err := filepath.EvalSymlinks(gotScan)
166+
if err != nil {
167+
t.Fatalf("EvalSymlinks(%q): %v", gotScan, err)
168+
}
169+
wantSub, err := filepath.EvalSymlinks(sub)
170+
if err != nil {
171+
t.Fatalf("EvalSymlinks(%q): %v", sub, err)
172+
}
161173
if gotScanResolved != wantSub {
162174
t.Errorf("scan RepoDir = %q, want subdir %q (must stay scoped)", gotScanResolved, wantSub)
163175
}

0 commit comments

Comments
 (0)