Skip to content

Commit 2ffedfa

Browse files
andrewandrew
committed
Fix smaller issues across the codebase
- Fix GIT_PKGS_DIRECT env var parsing to check for true/1/yes instead of any non-empty value - Return parse errors from ecosystems client instead of swallowing them - Avoid mutating caller's slice in OSV BatchQuery - Add mutex protection around both diffCache writes in PrefetchDiffs - Fix prepared statement leak in NewWriter when a later Prepare fails - Use cmd.OutOrStdout() in completions instead of os.Stdout - Thread stdout/stderr writers through RunManagerCommand(s) - Simplify hook uninstall line removal to avoid fragile skipNext mechanism
1 parent b676ce9 commit 2ffedfa

File tree

14 files changed

+189
-37
lines changed

14 files changed

+189
-37
lines changed

‎cmd/add.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ func runAdd(cmd *cobra.Command, args []string) error {
121121
ctx, cancel := context.WithTimeout(context.Background(), timeout)
122122
defer cancel()
123123

124-
if err := RunManagerCommands(ctx, dir, mgr.Name, "add", input); err != nil {
124+
if err := RunManagerCommands(ctx, dir, mgr.Name, "add", input, cmd.OutOrStdout(), cmd.ErrOrStderr()); err != nil {
125125
return fmt.Errorf("add failed: %w", err)
126126
}
127127

‎cmd/completions.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package cmd
22

33
import (
44
"fmt"
5-
"os"
65

76
"github.com/spf13/cobra"
87
)
@@ -48,15 +47,16 @@ PowerShell:
4847
ValidArgs: []string{"bash", "zsh", "fish", "powershell"},
4948
Args: cobra.MatchAll(cobra.ExactArgs(1), cobra.OnlyValidArgs),
5049
RunE: func(cmd *cobra.Command, args []string) error {
50+
out := cmd.OutOrStdout()
5151
switch args[0] {
5252
case "bash":
53-
return cmd.Root().GenBashCompletionV2(os.Stdout, true)
53+
return cmd.Root().GenBashCompletionV2(out, true)
5454
case "zsh":
55-
return cmd.Root().GenZshCompletion(os.Stdout)
55+
return cmd.Root().GenZshCompletion(out)
5656
case "fish":
57-
return cmd.Root().GenFishCompletion(os.Stdout, true)
57+
return cmd.Root().GenFishCompletion(out, true)
5858
case "powershell":
59-
return cmd.Root().GenPowerShellCompletionWithDesc(os.Stdout)
59+
return cmd.Root().GenPowerShellCompletionWithDesc(out)
6060
default:
6161
return fmt.Errorf("unknown shell: %s", args[0])
6262
}

‎cmd/hooks.go‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -131,17 +131,10 @@ func doUninstallHooks(cmd *cobra.Command, hooksDir string) error {
131131
// It's appended to another hook - remove our lines
132132
lines := strings.Split(string(content), "\n")
133133
var newLines []string
134-
skipNext := false
135134
for _, line := range lines {
136-
if skipNext {
137-
skipNext = false
138-
continue
139-
}
140-
if line == "# git-pkgs hook" || strings.Contains(line, "git-pkgs post-commit") {
141-
skipNext = true
142-
continue
143-
}
144-
if strings.Contains(line, "git pkgs reindex") {
135+
if line == "# git-pkgs hook" ||
136+
strings.Contains(line, "git-pkgs post-commit") ||
137+
strings.Contains(line, "git pkgs reindex") {
145138
continue
146139
}
147140
newLines = append(newLines, line)

‎cmd/hooks_test.go‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
package cmd
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
9+
"github.com/spf13/cobra"
10+
)
11+
12+
func TestDoUninstallHooks_AppendedLines(t *testing.T) {
13+
// Simulate a hook file where git-pkgs lines were appended to an existing hook
14+
existingContent := `#!/bin/bash
15+
echo "my custom hook"
16+
do_something
17+
18+
# git-pkgs hook
19+
git pkgs reindex --quiet 2>/dev/null || true
20+
`
21+
22+
dir := t.TempDir()
23+
hooksDir := filepath.Join(dir, "hooks")
24+
if err := os.MkdirAll(hooksDir, 0755); err != nil {
25+
t.Fatal(err)
26+
}
27+
28+
hookPath := filepath.Join(hooksDir, "post-commit")
29+
if err := os.WriteFile(hookPath, []byte(existingContent), 0755); err != nil {
30+
t.Fatal(err)
31+
}
32+
33+
// Save original hookNames and override for test
34+
origHookNames := hookNames
35+
hookNames = []string{"post-commit"}
36+
defer func() { hookNames = origHookNames }()
37+
38+
rootCmd := newTestCommand()
39+
if err := doUninstallHooks(rootCmd, hooksDir); err != nil {
40+
t.Fatalf("unexpected error: %v", err)
41+
}
42+
43+
content, err := os.ReadFile(hookPath)
44+
if err != nil {
45+
t.Fatalf("reading hook: %v", err)
46+
}
47+
48+
result := string(content)
49+
if strings.Contains(result, "git-pkgs") {
50+
t.Errorf("expected git-pkgs lines to be removed, got:\n%s", result)
51+
}
52+
if strings.Contains(result, "git pkgs reindex") {
53+
t.Errorf("expected reindex line to be removed, got:\n%s", result)
54+
}
55+
if !strings.Contains(result, "my custom hook") {
56+
t.Error("expected original hook content to be preserved")
57+
}
58+
if !strings.Contains(result, "do_something") {
59+
t.Error("expected original hook content to be preserved")
60+
}
61+
}
62+
63+
func TestDoUninstallHooks_BlankLineBetweenMarkers(t *testing.T) {
64+
// Edge case: blank line between comment and command should still remove both
65+
existingContent := `#!/bin/bash
66+
echo "my custom hook"
67+
68+
# git-pkgs hook
69+
70+
git pkgs reindex --quiet 2>/dev/null || true
71+
`
72+
73+
dir := t.TempDir()
74+
hooksDir := filepath.Join(dir, "hooks")
75+
if err := os.MkdirAll(hooksDir, 0755); err != nil {
76+
t.Fatal(err)
77+
}
78+
79+
hookPath := filepath.Join(hooksDir, "post-commit")
80+
if err := os.WriteFile(hookPath, []byte(existingContent), 0755); err != nil {
81+
t.Fatal(err)
82+
}
83+
84+
origHookNames := hookNames
85+
hookNames = []string{"post-commit"}
86+
defer func() { hookNames = origHookNames }()
87+
88+
rootCmd := newTestCommand()
89+
if err := doUninstallHooks(rootCmd, hooksDir); err != nil {
90+
t.Fatalf("unexpected error: %v", err)
91+
}
92+
93+
content, err := os.ReadFile(hookPath)
94+
if err != nil {
95+
t.Fatalf("reading hook: %v", err)
96+
}
97+
98+
result := string(content)
99+
if strings.Contains(result, "git-pkgs") {
100+
t.Errorf("expected git-pkgs lines to be removed, got:\n%s", result)
101+
}
102+
if strings.Contains(result, "git pkgs reindex") {
103+
t.Errorf("expected reindex line to be removed, got:\n%s", result)
104+
}
105+
}
106+
107+
func newTestCommand() *cobra.Command {
108+
return &cobra.Command{Use: "test"}
109+
}

‎cmd/install.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ func runInstall(cmd *cobra.Command, args []string) error {
109109
}
110110
}
111111

112-
if err := RunManagerCommands(ctx, dir, mgr.Name, "install", input); err != nil {
112+
if err := RunManagerCommands(ctx, dir, mgr.Name, "install", input, cmd.OutOrStdout(), cmd.ErrOrStderr()); err != nil {
113113
return fmt.Errorf("%s install failed: %w", mgr.Name, err)
114114
}
115115
}

‎cmd/managers.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -428,7 +428,7 @@ func managerNames(detected []DetectedManager) string {
428428
}
429429

430430
// RunManagerCommand builds and executes a package manager command
431-
func RunManagerCommand(ctx context.Context, dir, managerName, operation string, input managers.CommandInput) error {
431+
func RunManagerCommand(ctx context.Context, dir, managerName, operation string, input managers.CommandInput, stdout, stderr io.Writer) error {
432432
translator, err := getTranslator()
433433
if err != nil {
434434
return err
@@ -445,10 +445,10 @@ func RunManagerCommand(ctx context.Context, dir, managerName, operation string,
445445
}
446446

447447
if result.Stdout != "" {
448-
_, _ = os.Stdout.WriteString(result.Stdout)
448+
_, _ = io.WriteString(stdout, result.Stdout)
449449
}
450450
if result.Stderr != "" {
451-
_, _ = os.Stderr.WriteString(result.Stderr)
451+
_, _ = io.WriteString(stderr, result.Stderr)
452452
}
453453

454454
if result.ExitCode != 0 {
@@ -459,7 +459,7 @@ func RunManagerCommand(ctx context.Context, dir, managerName, operation string,
459459
}
460460

461461
// RunManagerCommands builds and executes multiple package manager commands (for chained operations)
462-
func RunManagerCommands(ctx context.Context, dir, managerName, operation string, input managers.CommandInput) error {
462+
func RunManagerCommands(ctx context.Context, dir, managerName, operation string, input managers.CommandInput, stdout, stderr io.Writer) error {
463463
translator, err := getTranslator()
464464
if err != nil {
465465
return err
@@ -477,10 +477,10 @@ func RunManagerCommands(ctx context.Context, dir, managerName, operation string,
477477
}
478478

479479
if result.Stdout != "" {
480-
_, _ = os.Stdout.WriteString(result.Stdout)
480+
_, _ = io.WriteString(stdout, result.Stdout)
481481
}
482482
if result.Stderr != "" {
483-
_, _ = os.Stderr.WriteString(result.Stderr)
483+
_, _ = io.WriteString(stderr, result.Stderr)
484484
}
485485

486486
if result.ExitCode != 0 {

‎cmd/remove.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ func runRemove(cmd *cobra.Command, args []string) error {
107107
ctx, cancel := context.WithTimeout(context.Background(), timeout)
108108
defer cancel()
109109

110-
if err := RunManagerCommands(ctx, dir, mgr.Name, "remove", input); err != nil {
110+
if err := RunManagerCommands(ctx, dir, mgr.Name, "remove", input, cmd.OutOrStdout(), cmd.ErrOrStderr()); err != nil {
111111
return fmt.Errorf("remove failed: %w", err)
112112
}
113113

‎cmd/update.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ func runUpdate(cmd *cobra.Command, args []string) error {
118118
ctx, cancel := context.WithTimeout(context.Background(), timeout)
119119
defer cancel()
120120

121-
if err := RunManagerCommands(ctx, dir, mgr.Name, "update", input); err != nil {
121+
if err := RunManagerCommands(ctx, dir, mgr.Name, "update", input, cmd.OutOrStdout(), cmd.ErrOrStderr()); err != nil {
122122
return fmt.Errorf("update failed: %w", err)
123123
}
124124

‎internal/analyzer/analyzer.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,9 @@ func (a *Analyzer) PrefetchDiffs(commits []*object.Commit, numWorkers int) {
123123
if strings.HasPrefix(line, "COMMIT:") {
124124
// Save previous commit
125125
if currentSHA != "" && currentDiff != nil {
126+
a.diffMu.Lock()
126127
a.diffCache[currentSHA] = currentDiff
128+
a.diffMu.Unlock()
127129
}
128130
currentSHA = line[7:] // Remove "COMMIT:" prefix
129131
currentDiff = &cachedDiff{}
@@ -181,7 +183,9 @@ func (a *Analyzer) PrefetchDiffs(commits []*object.Commit, numWorkers int) {
181183

182184
// Don't forget the last commit
183185
if currentSHA != "" && currentDiff != nil {
186+
a.diffMu.Lock()
184187
a.diffCache[currentSHA] = currentDiff
188+
a.diffMu.Unlock()
185189
}
186190
}
187191

‎internal/database/database_test.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,32 @@ func TestStoreSnapshotWithDuplicates(t *testing.T) {
372372
}
373373
}
374374

375+
func TestNewWriterClose(t *testing.T) {
376+
tmpDir := t.TempDir()
377+
dbPath := filepath.Join(tmpDir, "pkgs.sqlite3")
378+
379+
db, err := database.Create(dbPath)
380+
if err != nil {
381+
t.Fatalf("failed to create database: %v", err)
382+
}
383+
defer func() { _ = db.Close() }()
384+
385+
writer, err := database.NewWriter(db)
386+
if err != nil {
387+
t.Fatalf("failed to create writer: %v", err)
388+
}
389+
390+
// Close should not error
391+
if err := writer.Close(); err != nil {
392+
t.Errorf("unexpected error closing writer: %v", err)
393+
}
394+
395+
// Close again should not panic (statements are nil-safe)
396+
if err := writer.Close(); err != nil {
397+
t.Errorf("unexpected error on second close: %v", err)
398+
}
399+
}
400+
375401
func TestSchemaIndexes(t *testing.T) {
376402
tmpDir := t.TempDir()
377403
dbPath := filepath.Join(tmpDir, "pkgs.sqlite3")

0 commit comments

Comments
 (0)