refactor(vcs): remove mux and use interface for other packages (#1859)

* refactor(vcs): remove context passing mutex from VCS interface

* simplify devel upgrade gather

* update vcs upgrade tests

* remove unused mock

aur_install.go

@@ -163,10 +163,7 @@ func (installer *Installer) installAURPackages(ctx context.Context,
 	deps, exps := make([]string, 0, aurDepNames.Cardinality()), make([]string, 0, aurExpNames.Cardinality())
 	pkgArchives := make([]string, 0, len(exps)+len(deps))
 
-	var (
-		mux sync.Mutex
-		wg  sync.WaitGroup
-	)
+	var wg sync.WaitGroup
 
 	for _, name := range all {
 		base := nameToBase[name]
@@ -207,7 +204,10 @@ func (installer *Installer) installAURPackages(ctx context.Context,
 
 		srcinfo := srcinfos[base]
 		wg.Add(1)
-		go installer.vcsStore.Update(ctx, name, srcinfo.Source, &mux, &wg)
+		go func(name string) {
+			installer.vcsStore.Update(ctx, name, srcinfo.Source)
+			wg.Done()
+		}(name)
 	}
 
 	wg.Wait()

install.go

@@ -794,10 +794,7 @@ func buildInstallPkgbuilds(
 		}
 		text.Debugln("deps:", deps, "exp:", exp, "pkgArchives:", pkgArchives)
 
-		var (
-			mux sync.Mutex
-			wg  sync.WaitGroup
-		)
+		var wg sync.WaitGroup
 
 		for _, pkg := range base {
 			if srcinfo == nil {
@@ -808,7 +805,10 @@ func buildInstallPkgbuilds(
 			wg.Add(1)
 
 			text.Debugln("checking vcs store for:", pkg.Name)
-			go config.Runtime.VCSStore.Update(ctx, pkg.Name, srcinfo.Source, &mux, &wg)
+			go func(name string) {
+				config.Runtime.VCSStore.Update(ctx, name, srcinfo.Source)
+				wg.Done()
+			}(pkg.Name)
 		}
 
 		wg.Wait()

pkg/upgrade/sources.go

@@ -2,7 +2,6 @@ package upgrade
 
 import (
 	"context"
-	"sync"
 
 	"github.com/leonelquinteros/gotext"
 
@@ -14,49 +13,26 @@ import (
 
 func UpDevel(
 	ctx context.Context,
-	remote []db.IPackage,
+	remote []db.IPackage, // should be a map
 	aurdata map[string]*query.Pkg,
-	localCache *vcs.InfoStore,
+	localCache vcs.Store,
 ) UpSlice {
 	toUpdate := make([]db.IPackage, 0, len(aurdata))
 	toRemove := make([]string, 0)
 
-	var (
-		mux1, mux2 sync.Mutex
-		wg         sync.WaitGroup
-	)
-
-	checkUpdate := func(pkgName string, e vcs.OriginInfoByURL) {
-		defer wg.Done()
-
-		if localCache.NeedsUpdate(ctx, e) {
-			if _, ok := aurdata[pkgName]; ok {
-				for _, pkg := range remote {
-					if pkg.Name() == pkgName {
-						mux1.Lock()
-						toUpdate = append(toUpdate, pkg)
-						mux1.Unlock()
-
-						return
-					}
+	for _, pkgName := range localCache.ToUpgrade(ctx) {
+		if _, ok := aurdata[pkgName]; ok {
+			for _, pkg := range remote {
+				if pkg.Name() == pkgName {
+					toUpdate = append(toUpdate, pkg)
 				}
 			}
-
-			mux2.Lock()
+		} else {
 			toRemove = append(toRemove, pkgName)
-			mux2.Unlock()
 		}
 	}
 
-	for pkgName, e := range localCache.OriginsByPackage {
-		wg.Add(1)
-
-		go checkUpdate(pkgName, e)
-	}
-
-	wg.Wait()
-
-	toUpgrade := UpSlice{Up: make([]Upgrade, 0), Repos: []string{"devel"}}
+	toUpgrade := UpSlice{Up: make([]Upgrade, 0, len(toUpdate)), Repos: []string{"devel"}}
 
 	for _, pkg := range toUpdate {
 		if pkg.ShouldIgnore() {

pkg/upgrade/sources_test.go

@@ -2,9 +2,6 @@ package upgrade
 
 import (
 	"context"
-	"fmt"
-	"os/exec"
-	"strconv"
 	"testing"
 	"time"
 
@@ -14,7 +11,6 @@ import (
 	alpm "github.com/Jguer/go-alpm/v2"
 
 	"github.com/Jguer/yay/v11/pkg/db/mock"
-	"github.com/Jguer/yay/v11/pkg/settings/exe"
 	"github.com/Jguer/yay/v11/pkg/vcs"
 )
 
@@ -76,42 +72,13 @@ func Test_upAUR(t *testing.T) {
 	}
 }
 
-type MockRunner struct {
-	Returned []string
-	Index    int
-	t        *testing.T
-}
-
-func (r *MockRunner) Show(cmd *exec.Cmd) error {
-	return nil
-}
-
-func (r *MockRunner) Capture(cmd *exec.Cmd) (stdout, stderr string, err error) {
-	i, _ := strconv.Atoi(cmd.Args[len(cmd.Args)-1])
-	if i >= len(r.Returned) {
-		fmt.Println(r.Returned)
-		fmt.Println(cmd.Args)
-		fmt.Println(i)
-	}
-	stdout = r.Returned[i]
-	assert.Contains(r.t, cmd.Args, "ls-remote")
-	return stdout, stderr, err
-}
-
 func Test_upDevel(t *testing.T) {
 	t.Parallel()
-	returnValue := []string{
-		"7f4c277ce7149665d1c79b76ca8fbb832a65a03b	HEAD",
-		"7f4c277ce7149665d1c79b76ca8fbb832a65a03b	HEAD",
-		"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa	HEAD",
-		"cccccccccccccccccccccccccccccccccccccccc	HEAD",
-		"991c5b4146fd27f4aacf4e3111258a848934aaa1	HEAD",
-	}
 
 	type args struct {
 		remote  []alpm.IPackage
 		aurdata map[string]*aur.Pkg
-		cached  vcs.InfoStore
+		cached  vcs.Store
 	}
 	tests := []struct {
 		name     string
@@ -122,13 +89,7 @@ func Test_upDevel(t *testing.T) {
 		{
 			name: "No Updates",
 			args: args{
-				cached: vcs.InfoStore{
-					CmdBuilder: &exe.CmdBuilder{
-						Runner: &MockRunner{
-							Returned: returnValue,
-						},
-					},
-				},
+				cached: &vcs.Mock{},
 				remote: []alpm.IPackage{
 					&mock.Package{PName: "hello", PVersion: "2.0.0"},
 					&mock.Package{PName: "local_pkg", PVersion: "1.1.0"},
@@ -145,47 +106,8 @@ func Test_upDevel(t *testing.T) {
 			name:     "Simple Update",
 			finalLen: 3,
 			args: args{
-				cached: vcs.InfoStore{
-					CmdBuilder: &exe.CmdBuilder{
-						Runner: &MockRunner{
-							Returned: returnValue,
-						},
-					},
-					OriginsByPackage: map[string]vcs.OriginInfoByURL{
-						"hello": {
-							"github.com/Jguer/z.git": vcs.OriginInfo{
-								Protocols: []string{"https"},
-								Branch:    "0",
-								SHA:       "991c5b4146fd27f4aacf4e3111258a848934aaa1",
-							},
-						},
-						"hello-non-existent": {
-							"github.com/Jguer/y.git": vcs.OriginInfo{
-								Protocols: []string{"https"},
-								Branch:    "0",
-								SHA:       "991c5b4146fd27f4aacf4e3111258a848934aaa1",
-							},
-						},
-						"hello2": {
-							"github.com/Jguer/a.git": vcs.OriginInfo{
-								Protocols: []string{"https"},
-								Branch:    "1",
-								SHA:       "7f4c277ce7149665d1c79b76ca8fbb832a65a03b",
-							},
-						},
-						"hello4": {
-							"github.com/Jguer/b.git": vcs.OriginInfo{
-								Protocols: []string{"https"},
-								Branch:    "2",
-								SHA:       "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
-							},
-							"github.com/Jguer/c.git": vcs.OriginInfo{
-								Protocols: []string{"https"},
-								Branch:    "3",
-								SHA:       "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb",
-							},
-						},
-					},
+				cached: &vcs.Mock{
+					ToUpgradeReturn: []string{"hello", "hello4"},
 				},
 				remote: []alpm.IPackage{
 					&mock.Package{PName: "hello", PVersion: "2.0.0"},
@@ -219,22 +141,7 @@ func Test_upDevel(t *testing.T) {
 			name:     "No update returned",
 			finalLen: 1,
 			args: args{
-				cached: vcs.InfoStore{
-					CmdBuilder: &exe.CmdBuilder{
-						Runner: &MockRunner{
-							Returned: returnValue,
-						},
-					},
-					OriginsByPackage: map[string]vcs.OriginInfoByURL{
-						"hello": {
-							"github.com/Jguer/d.git": vcs.OriginInfo{
-								Protocols: []string{"https"},
-								Branch:    "4",
-								SHA:       "991c5b4146fd27f4aacf4e3111258a848934aaa1",
-							},
-						},
-					},
-				},
+				cached:  &vcs.Mock{ToUpgradeReturn: []string{}},
 				remote:  []alpm.IPackage{&mock.Package{PName: "hello", PVersion: "2.0.0"}},
 				aurdata: map[string]*aur.Pkg{"hello": {Version: "2.0.0", Name: "hello"}},
 			},
@@ -244,21 +151,8 @@ func Test_upDevel(t *testing.T) {
 			name:     "No update returned - ignored",
 			finalLen: 1,
 			args: args{
-				cached: vcs.InfoStore{
-					CmdBuilder: &exe.CmdBuilder{
-						Runner: &MockRunner{
-							Returned: returnValue,
-						},
-					},
-					OriginsByPackage: map[string]vcs.OriginInfoByURL{
-						"hello": {
-							"github.com/Jguer/e.git": vcs.OriginInfo{
-								Protocols: []string{"https"},
-								Branch:    "3",
-								SHA:       "991c5b4146fd27f4aacf4e3111258a848934aaa1",
-							},
-						},
-					},
+				cached: &vcs.Mock{
+					ToUpgradeReturn: []string{"hello"},
 				},
 				remote:  []alpm.IPackage{&mock.Package{PName: "hello", PVersion: "2.0.0", PShouldIgnore: true}},
 				aurdata: map[string]*aur.Pkg{"hello": {Version: "2.0.0", Name: "hello"}},
@@ -270,10 +164,8 @@ func Test_upDevel(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			tt.args.cached.CmdBuilder.(*exe.CmdBuilder).Runner.(*MockRunner).t = t
-			got := UpDevel(context.TODO(), tt.args.remote, tt.args.aurdata, &tt.args.cached)
+			got := UpDevel(context.TODO(), tt.args.remote, tt.args.aurdata, tt.args.cached)
 			assert.ElementsMatch(t, tt.want.Up, got.Up)
-			assert.Equal(t, tt.finalLen, len(tt.args.cached.OriginsByPackage))
 		})
 	}
 }

pkg/vcs/mock.go

@@ -2,15 +2,20 @@ package vcs
 
 import (
 	"context"
-	"sync"
 
 	gosrc "github.com/Morganamilo/go-srcinfo"
 )
 
-type Mock struct{}
+type Mock struct {
+	OriginsByPackage map[string]OriginInfoByURL
+	ToUpgradeReturn  []string
+}
 
-func (m *Mock) Update(ctx context.Context, pkgName string, sources []gosrc.ArchString, mux sync.Locker, wg *sync.WaitGroup) {
-	wg.Done()
+func (m *Mock) ToUpgrade(ctx context.Context) []string {
+	return m.ToUpgradeReturn
+}
+
+func (m *Mock) Update(ctx context.Context, pkgName string, sources []gosrc.ArchString) {
 }
 
 func (m *Mock) Save() error {

pkg/vcs/vcs.go

@@ -18,11 +18,15 @@ import (
 )
 
 type Store interface {
-	Update(ctx context.Context, pkgName string,
-		sources []gosrc.ArchString, mux sync.Locker, wg *sync.WaitGroup,
-	)
+	// ToUpgrade returns a list of packages that need to be updated.
+	ToUpgrade(ctx context.Context) []string
+	// Update updates the VCS info of a package.
+	Update(ctx context.Context, pkgName string, sources []gosrc.ArchString)
+	// Save saves the VCS info to disk.
 	Save() error
+	// RemovePackage removes the VCS info of a package.
 	RemovePackage(pkgs []string)
+	// Load loads the VCS info from disk.
 	Load() error
 }
 
@@ -32,6 +36,7 @@ type InfoStore struct {
 	OriginsByPackage map[string]OriginInfoByURL
 	FilePath         string
 	CmdBuilder       exe.GitCmdBuilder
+	mux              sync.Mutex
 }
 
 // OriginInfoByURL stores the OriginInfo of each origin URL provided.
@@ -58,6 +63,7 @@ func NewInfoStore(filePath string, cmdBuilder exe.GitCmdBuilder) *InfoStore {
 		CmdBuilder:       cmdBuilder,
 		FilePath:         filePath,
 		OriginsByPackage: map[string]OriginInfoByURL{},
+		mux:              sync.Mutex{},
 	}
 
 	return infoStore
@@ -99,11 +105,8 @@ func (v *InfoStore) getCommit(ctx context.Context, url, branch string, protocols
 	return ""
 }
 
-func (v *InfoStore) Update(ctx context.Context, pkgName string,
-	sources []gosrc.ArchString, mux sync.Locker, wg *sync.WaitGroup,
-) {
-	defer wg.Done()
-
+func (v *InfoStore) Update(ctx context.Context, pkgName string, sources []gosrc.ArchString) {
+	var wg sync.WaitGroup
 	info := make(OriginInfoByURL)
 	checkSource := func(source gosrc.ArchString) {
 		defer wg.Done()
@@ -118,7 +121,7 @@ func (v *InfoStore) Update(ctx context.Context, pkgName string,
 			return
 		}
 
-		mux.Lock()
+		v.mux.Lock()
 		info[url] = OriginInfo{
 			protocols,
 			branch,
@@ -132,7 +135,7 @@ func (v *InfoStore) Update(ctx context.Context, pkgName string,
 		if err := v.Save(); err != nil {
 			fmt.Fprintln(os.Stderr, err)
 		}
-		mux.Unlock()
+		v.mux.Unlock()
 	}
 
 	for _, source := range sources {
@@ -140,6 +143,8 @@ func (v *InfoStore) Update(ctx context.Context, pkgName string,
 
 		go checkSource(source)
 	}
+
+	wg.Wait()
 }
 
 // parseSource returns the git url, default branch and protocols it supports.
@@ -193,7 +198,18 @@ func parseSource(source string) (url, branch string, protocols []string) {
 	return url, branch, protocols
 }
 
-func (v *InfoStore) NeedsUpdate(ctx context.Context, infos OriginInfoByURL) bool {
+func (v *InfoStore) ToUpgrade(ctx context.Context) []string {
+	pkgs := make([]string, 0, len(v.OriginsByPackage))
+	for pkgName, infos := range v.OriginsByPackage {
+		if v.needsUpdate(ctx, infos) {
+			pkgs = append(pkgs, pkgName)
+		}
+	}
+
+	return pkgs
+}
+
+func (v *InfoStore) needsUpdate(ctx context.Context, infos OriginInfoByURL) bool {
 	// used to signal we have gone through all sources and found nothing
 	finished := make(chan struct{})
 	alive := 0
@@ -279,7 +295,7 @@ func (v *InfoStore) RemovePackage(pkgs []string) {
 }
 
 // LoadStore reads a json file and populates a InfoStore structure.
-func (v InfoStore) Load() error {
+func (v *InfoStore) Load() error {
 	vfile, err := os.Open(v.FilePath)
 	if !os.IsNotExist(err) && err != nil {
 		return fmt.Errorf("failed to open vcs file '%s': %s", v.FilePath, err)

pkg/vcs/vcs_test.go

@@ -7,7 +7,6 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
-	"sync"
 	"testing"
 
 	gosrc "github.com/Morganamilo/go-srcinfo"
@@ -102,6 +101,138 @@ func (r *MockRunner) Capture(cmd *exec.Cmd) (stdout, stderr string, err error) {
 	return stdout, stderr, err
 }
 
+func TestInfoStoreToUpgrade(t *testing.T) {
+	t.Parallel()
+	type fields struct {
+		CmdBuilder *exe.CmdBuilder
+	}
+	type args struct {
+		infos OriginInfoByURL
+	}
+	tests := []struct {
+		name   string
+		fields fields
+		args   args
+		want   []string
+	}{
+		{
+			name: "simple-has_update",
+			args: args{infos: OriginInfoByURL{
+				"github.com/Jguer/z.git": OriginInfo{
+					Protocols: []string{"https"},
+					Branch:    "0",
+					SHA:       "991c5b4146fd27f4aacf4e3111258a848934aaa1",
+				},
+			}}, fields: fields{
+				CmdBuilder: &exe.CmdBuilder{GitBin: "git", GitFlags: []string{""}, Runner: &MockRunner{
+					Returned: []string{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa	HEAD"},
+				}},
+			},
+			want: []string{"yay"},
+		},
+		{
+			name: "double-has_update",
+			args: args{infos: OriginInfoByURL{
+				"github.com/Jguer/z.git": OriginInfo{
+					Protocols: []string{"https"},
+					Branch:    "0",
+					SHA:       "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+				},
+				"github.com/Jguer/a.git": OriginInfo{
+					Protocols: []string{"https"},
+					Branch:    "0",
+					SHA:       "991c5b4146fd27f4aacf4e3111258a848934aaa1",
+				},
+			}}, fields: fields{
+				CmdBuilder: &exe.CmdBuilder{GitBin: "git", GitFlags: []string{""}, Runner: &MockRunner{
+					Returned: []string{
+						"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa	HEAD",
+						"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa	HEAD",
+					},
+				}},
+			},
+			want: []string{"yay"},
+		},
+		{
+			name: "simple-no_update",
+			args: args{infos: OriginInfoByURL{
+				"github.com/Jguer/z.git": OriginInfo{
+					Protocols: []string{"https"},
+					Branch:    "0",
+					SHA:       "991c5b4146fd27f4aacf4e3111258a848934aaa1",
+				},
+			}}, fields: fields{
+				CmdBuilder: &exe.CmdBuilder{GitBin: "git", GitFlags: []string{""}, Runner: &MockRunner{
+					Returned: []string{"991c5b4146fd27f4aacf4e3111258a848934aaa1	HEAD"},
+				}},
+			},
+			want: []string{},
+		},
+		{
+			name: "simple-no_split",
+			args: args{infos: OriginInfoByURL{
+				"github.com/Jguer/z.git": OriginInfo{
+					Protocols: []string{"https"},
+					Branch:    "0",
+					SHA:       "991c5b4146fd27f4aacf4e3111258a848934aaa1",
+				},
+			}}, fields: fields{
+				CmdBuilder: &exe.CmdBuilder{GitBin: "git", GitFlags: []string{""}, Runner: &MockRunner{
+					Returned: []string{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"},
+				}},
+			},
+			want: []string{},
+		},
+		{
+			name: "simple-error",
+			args: args{infos: OriginInfoByURL{
+				"github.com/Jguer/z.git": OriginInfo{
+					Protocols: []string{"https"},
+					Branch:    "0",
+					SHA:       "991c5b4146fd27f4aacf4e3111258a848934aaa1",
+				},
+			}}, fields: fields{
+				CmdBuilder: &exe.CmdBuilder{
+					GitBin: "git", GitFlags: []string{""},
+					Runner: &MockRunner{
+						Returned: []string{"error"},
+					},
+				},
+			},
+			want: []string{},
+		},
+		{
+			name: "simple-no protocol",
+			args: args{infos: OriginInfoByURL{
+				"github.com/Jguer/z.git": OriginInfo{
+					Protocols: []string{},
+					Branch:    "0",
+					SHA:       "991c5b4146fd27f4aacf4e3111258a848934aaa1",
+				},
+			}}, fields: fields{
+				CmdBuilder: &exe.CmdBuilder{GitBin: "git", GitFlags: []string{""}, Runner: &MockRunner{
+					Returned: []string{"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"},
+				}},
+			},
+			want: []string{},
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			v := &InfoStore{
+				CmdBuilder: tt.fields.CmdBuilder,
+				OriginsByPackage: map[string]OriginInfoByURL{
+					"yay": tt.args.infos,
+				},
+			}
+			got := v.ToUpgrade(context.Background())
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
 func TestInfoStore_NeedsUpdate(t *testing.T) {
 	t.Parallel()
 	type fields struct {
@@ -225,7 +356,7 @@ func TestInfoStore_NeedsUpdate(t *testing.T) {
 			v := &InfoStore{
 				CmdBuilder: tt.fields.CmdBuilder,
 			}
-			got := v.NeedsUpdate(context.TODO(), tt.args.infos)
+			got := v.needsUpdate(context.TODO(), tt.args.infos)
 			assert.Equal(t, tt.want, got)
 		})
 	}
@@ -275,11 +406,8 @@ func TestInfoStore_Update(t *testing.T) {
 				FilePath:         filePath,
 				CmdBuilder:       tt.fields.CmdBuilder,
 			}
-			var mux sync.Mutex
-			var wg sync.WaitGroup
-			wg.Add(1)
-			v.Update(context.TODO(), tt.args.pkgName, tt.args.sources, &mux, &wg)
-			wg.Wait()
+
+			v.Update(context.TODO(), tt.args.pkgName, tt.args.sources)
 			assert.Len(t, tt.fields.OriginsByPackage, 1)
 
 			marshalledinfo, err := json.MarshalIndent(tt.fields.OriginsByPackage, "", "\t")

vcs.go

@@ -19,11 +19,6 @@ import (
 
 // createDevelDB forces yay to create a DB of the existing development packages.
 func createDevelDB(ctx context.Context, config *settings.Configuration, dbExecutor db.Executor) error {
-	var (
-		mux sync.Mutex
-		wg  sync.WaitGroup
-	)
-
 	remoteNames := dbExecutor.InstalledRemotePackageNames()
 	info, err := query.AURInfoPrint(ctx, config.Runtime.AURClient, remoteNames, config.RequestSplitN)
 	if err != nil {
@@ -61,11 +56,15 @@ func createDevelDB(ctx context.Context, config *settings.Configuration, dbExecut
 		return err
 	}
 
+	var wg sync.WaitGroup
 	for i := range srcinfos {
 		for iP := range srcinfos[i].Packages {
 			wg.Add(1)
 
-			go config.Runtime.VCSStore.Update(ctx, srcinfos[i].Packages[iP].Pkgname, srcinfos[i].Source, &mux, &wg)
+			go func(i string, iP int) {
+				config.Runtime.VCSStore.Update(ctx, srcinfos[i].Packages[iP].Pkgname, srcinfos[i].Source)
+				wg.Done()
+			}(i, iP)
 		}
 	}