Skip to content

Commit 56ad62e

Browse files
fix(brew): use typed key to prevent formula/cask name collision in retry filter
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
1 parent 2a28a95 commit 56ad62e

2 files changed

Lines changed: 56 additions & 20 deletions

File tree

internal/brew/brew.go

Lines changed: 43 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -199,10 +199,10 @@ type installResult struct {
199199
errMsg string
200200
}
201201

202-
func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) error {
202+
func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) (installedFormulae []string, installedCasks []string, err error) {
203203
total := len(cliPkgs) + len(caskPkgs)
204204
if total == 0 {
205-
return nil
205+
return nil, nil, nil
206206
}
207207

208208
if dryRun {
@@ -213,24 +213,28 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) error {
213213
for _, p := range caskPkgs {
214214
fmt.Printf(" brew install --cask %s\n", p)
215215
}
216-
return nil
216+
return nil, nil, nil
217217
}
218218

219-
installedFormulae, installedCasks, err := GetInstalledPackages()
220-
if err != nil {
221-
return fmt.Errorf("failed to check installed packages: %w", err)
219+
alreadyFormulae, alreadyCasks, checkErr := GetInstalledPackages()
220+
if checkErr != nil {
221+
return nil, nil, fmt.Errorf("failed to check installed packages: %w", checkErr)
222222
}
223223

224224
var newCli []string
225225
for _, p := range cliPkgs {
226-
if !installedFormulae[p] {
226+
if !alreadyFormulae[p] {
227227
newCli = append(newCli, p)
228+
} else {
229+
installedFormulae = append(installedFormulae, p)
228230
}
229231
}
230232
var newCask []string
231233
for _, p := range caskPkgs {
232-
if !installedCasks[p] {
234+
if !alreadyCasks[p] {
233235
newCask = append(newCask, p)
236+
} else {
237+
installedCasks = append(installedCasks, p)
234238
}
235239
}
236240

@@ -242,11 +246,11 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) error {
242246

243247
if len(newCli)+len(newCask) == 0 {
244248
ui.Success("All packages already installed!")
245-
return nil
249+
return installedFormulae, installedCasks, nil
246250
}
247251

248-
if err := PreInstallChecks(len(newCli) + len(newCask)); err != nil {
249-
return err
252+
if preErr := PreInstallChecks(len(newCli) + len(newCask)); preErr != nil {
253+
return installedFormulae, installedCasks, preErr
250254
}
251255

252256
progress := ui.NewStickyProgress(len(newCli) + len(newCask))
@@ -257,6 +261,15 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) error {
257261

258262
if len(newCli) > 0 {
259263
failed := runParallelInstallWithProgress(newCli, progress)
264+
failedSet := make(map[string]bool, len(failed))
265+
for _, f := range failed {
266+
failedSet[f.name] = true
267+
}
268+
for _, p := range newCli {
269+
if !failedSet[p] {
270+
installedFormulae = append(installedFormulae, p)
271+
}
272+
}
260273
allFailed = append(allFailed, failed...)
261274
}
262275

@@ -271,10 +284,9 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) error {
271284
duration := ui.FormatDuration(elapsed)
272285
if errMsg == "" {
273286
progress.PrintLine(" %s %s", ui.Green("✔ "+pkg), ui.Cyan("("+duration+")"))
287+
installedCasks = append(installedCasks, pkg)
274288
} else {
275289
progress.PrintLine(" %s %s", ui.Red("✗ "+pkg+" ("+errMsg+")"), ui.Cyan("("+duration+")"))
276-
}
277-
if errMsg != "" {
278290
allFailed = append(allFailed, failedJob{
279291
installJob: installJob{name: pkg, isCask: true},
280292
errMsg: errMsg,
@@ -288,8 +300,6 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) error {
288300
if len(allFailed) > 0 {
289301
fmt.Printf("\nRetrying %d failed packages...\n", len(allFailed))
290302

291-
retriedSuccessfully := make(map[string]bool)
292-
293303
for _, f := range allFailed {
294304
var errMsg string
295305
if f.isCask {
@@ -299,15 +309,30 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) error {
299309
}
300310
if errMsg == "" {
301311
fmt.Printf(" ✔ %s (retry succeeded)\n", f.name)
302-
retriedSuccessfully[f.name] = true
312+
if f.isCask {
313+
installedCasks = append(installedCasks, f.name)
314+
} else {
315+
installedFormulae = append(installedFormulae, f.name)
316+
}
303317
} else {
304318
fmt.Printf(" ✗ %s (still failed)\n", f.name)
305319
}
306320
}
307321

322+
type pkgKey struct {
323+
name string
324+
isCask bool
325+
}
326+
succeeded := make(map[pkgKey]bool)
327+
for _, p := range installedFormulae {
328+
succeeded[pkgKey{p, false}] = true
329+
}
330+
for _, p := range installedCasks {
331+
succeeded[pkgKey{p, true}] = true
332+
}
308333
var stillFailed []failedJob
309334
for _, f := range allFailed {
310-
if !retriedSuccessfully[f.name] {
335+
if !succeeded[pkgKey{f.name, f.isCask}] {
311336
stillFailed = append(stillFailed, f)
312337
}
313338
}
@@ -316,7 +341,7 @@ func InstallWithProgress(cliPkgs, caskPkgs []string, dryRun bool) error {
316341

317342
handleFailedJobs(allFailed)
318343

319-
return nil
344+
return installedFormulae, installedCasks, nil
320345
}
321346

322347
func handleFailedJobs(failed []failedJob) {

internal/brew/brew_test.go

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,13 +155,24 @@ func TestInstallTaps_DryRun(t *testing.T) {
155155
}
156156

157157
func TestInstallWithProgress_EmptyPackages(t *testing.T) {
158-
err := InstallWithProgress([]string{}, []string{}, false)
158+
formulae, casks, err := InstallWithProgress([]string{}, []string{}, false)
159159
assert.NoError(t, err)
160+
assert.Empty(t, formulae)
161+
assert.Empty(t, casks)
160162
}
161163

162164
func TestInstallWithProgress_DryRun(t *testing.T) {
163-
err := InstallWithProgress([]string{"git", "curl"}, []string{"firefox"}, true)
165+
formulae, casks, err := InstallWithProgress([]string{"git", "curl"}, []string{"firefox"}, true)
164166
assert.NoError(t, err)
167+
assert.Empty(t, formulae)
168+
assert.Empty(t, casks)
169+
}
170+
171+
func TestInstallWithProgress_DryRunReturnsNoInstalledPackages(t *testing.T) {
172+
formulae, casks, err := InstallWithProgress([]string{"ripgrep", "fd"}, []string{"visual-studio-code"}, true)
173+
assert.NoError(t, err)
174+
assert.Empty(t, formulae, "dry-run should not report packages as installed")
175+
assert.Empty(t, casks, "dry-run should not report casks as installed")
165176
}
166177

167178
func TestUpdate_DryRun(t *testing.T) {

0 commit comments

Comments
 (0)