cmd/go/internal/modfetch/codehost: add lockfiles for repos

The lockfile guards calls that may change the repo's filesystem contents.

We don't know how robust VCS implementations are to running
simultaneous commands, and this way we don't need to care: only one
'go' command at a time will modify any given repository.

If we can guarantee that particular VCS implementations are robust
enough across all of the VCS tool versions we support, we may be able
to remove some of this locking to improve parallelism.

Updates #26794

Change-Id: I578524974f5015629239cef43d3793aee2b9075c
Reviewed-on: https://go-review.googlesource.com/c/146381
Run-TryBot: Bryan C. Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>

1 files changed, 40 insertions, 18 deletions

diff --git a/src/cmd/go/internal/modfetch/codehost/git.go b/src/cmd/go/internal/modfetch/codehost/git.go
index bcf8609826..7b3775779b 100644
--- a/src/cmd/go/internal/modfetch/codehost/git.go
+++ b/src/cmd/go/internal/modfetch/codehost/git.go
@@ -17,6 +17,7 @@ import (
 	"sync"
 	"time"
 
+	"cmd/go/internal/lockedfile"
 	"cmd/go/internal/par"
 )
 
@@ -57,22 +58,29 @@ func newGitRepo(remote string, localOK bool) (Repo, error) {
 	r := &gitRepo{remote: remote}
 	if strings.Contains(remote, "://") {
 		// This is a remote path.
-		dir, err := WorkDir(gitWorkDirType, r.remote)
+		var err error
+		r.dir, r.mu.Path, err = WorkDir(gitWorkDirType, r.remote)
 		if err != nil {
 			return nil, err
 		}
-		r.dir = dir
-		if _, err := os.Stat(filepath.Join(dir, "objects")); err != nil {
-			if _, err := Run(dir, "git", "init", "--bare"); err != nil {
-				os.RemoveAll(dir)
+
+		unlock, err := r.mu.Lock()
+		if err != nil {
+			return nil, err
+		}
+		defer unlock()
+
+		if _, err := os.Stat(filepath.Join(r.dir, "objects")); err != nil {
+			if _, err := Run(r.dir, "git", "init", "--bare"); err != nil {
+				os.RemoveAll(r.dir)
 				return nil, err
 			}
 			// We could just say git fetch https://whatever later,
 			// but this lets us say git fetch origin instead, which
 			// is a little nicer. More importantly, using a named remote
 			// avoids a problem with Git LFS. See golang.org/issue/25605.
-			if _, err := Run(dir, "git", "remote", "add", "origin", r.remote); err != nil {
-				os.RemoveAll(dir)
+			if _, err := Run(r.dir, "git", "remote", "add", "origin", r.remote); err != nil {
+				os.RemoveAll(r.dir)
 				return nil, err
 			}
 			r.remote = "origin"
@@ -97,6 +105,7 @@ func newGitRepo(remote string, localOK bool) (Repo, error) {
 			return nil, fmt.Errorf("%s exists but is not a directory", remote)
 		}
 		r.dir = remote
+		r.mu.Path = r.dir + ".lock"
 	}
 	return r, nil
 }
@@ -106,7 +115,8 @@ type gitRepo struct {
 	local  bool
 	dir    string
 
-	mu         sync.Mutex // protects fetchLevel, some git repo state
+	mu lockedfile.Mutex // protects fetchLevel and git repo state
+
 	fetchLevel int
 
 	statCache par.Cache
@@ -304,11 +314,11 @@ func (r *gitRepo) stat(rev string) (*RevInfo, error) {
 	}
 
 	// Protect r.fetchLevel and the "fetch more and more" sequence.
-	// TODO(rsc): Add LockDir and use it for protecting that
-	// sequence, so that multiple processes don't collide in their
-	// git commands.
-	r.mu.Lock()
-	defer r.mu.Unlock()
+	unlock, err := r.mu.Lock()
+	if err != nil {
+		return nil, err
+	}
+	defer unlock()
 
 	// Perhaps r.localTags did not have the ref when we loaded local tags,
 	// but we've since done fetches that pulled down the hash we need
@@ -495,8 +505,11 @@ func (r *gitRepo) ReadFileRevs(revs []string, file string, maxSize int64) (map[s
 
 	// Protect r.fetchLevel and the "fetch more and more" sequence.
 	// See stat method above.
-	r.mu.Lock()
-	defer r.mu.Unlock()
+	unlock, err := r.mu.Lock()
+	if err != nil {
+		return nil, err
+	}
+	defer unlock()
 
 	var refs []string
 	var protoFlag []string
@@ -658,8 +671,11 @@ func (r *gitRepo) RecentTag(rev, prefix string) (tag string, err error) {
 	// There are plausible tags, but we don't know if rev is a descendent of any of them.
 	// Fetch the history to find out.
 
-	r.mu.Lock()
-	defer r.mu.Unlock()
+	unlock, err := r.mu.Lock()
+	if err != nil {
+		return "", err
+	}
+	defer unlock()
 
 	if r.fetchLevel < fetchAll {
 		// Fetch all heads and tags and see if that gives us enough history.
@@ -678,7 +694,7 @@ func (r *gitRepo) RecentTag(rev, prefix string) (tag string, err error) {
 	// unreachable for a reason).
 	//
 	// Try one last time in case some other goroutine fetched rev while we were
-	// waiting on r.mu.
+	// waiting on the lock.
 	describe()
 	return tag, err
 }
@@ -694,6 +710,12 @@ func (r *gitRepo) ReadZip(rev, subdir string, maxSize int64) (zip io.ReadCloser,
 		return nil, "", err
 	}
 
+	unlock, err := r.mu.Lock()
+	if err != nil {
+		return nil, "", err
+	}
+	defer unlock()
+
 	if err := ensureGitAttributes(r.dir); err != nil {
 		return nil, "", err
 	}