cmd/go/internal/modfetch/codehost: work around an apparent bug in 'git fetch --unshallow'

When 'git fetch' is passed the '--unshallow' flag, it assumes that the
local and remote refs are equal.¹ However, we were fetching an
expanded set of refs explicitly in the same command, violating that
assumption.

Now we first expand the set of refs, then unshallow the repo in a
separate fetch. Empirically, this seems to work, whereas the opposite
order does not.

¹https://github.com/git/git/blob/4c86140027f4a0d2caaa3ab4bd8bfc5ce3c11c8a/transport.c#L1303-L1309

Fixes #34266

Change-Id: Ie97eb7c1223f944003a1e31d0ec9e69aad0efc0d
Reviewed-on: https://go-review.googlesource.com/c/go/+/196961
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

src/cmd/go/internal/modfetch/codehost/git.go

@@ -265,13 +265,6 @@ func (r *gitRepo) findRef(hash string) (ref string, ok bool) {
 	return "", false
 }
 
-func unshallow(gitDir string) []string {
-	if _, err := os.Stat(filepath.Join(gitDir, "shallow")); err == nil {
-		return []string{"--unshallow"}
-	}
-	return []string{}
-}
-
 // minHashDigits is the minimum number of digits to require
 // before accepting a hex digit sequence as potentially identifying
 // a specific commit in a git repo. (Of course, users can always
@@ -421,29 +414,27 @@ func (r *gitRepo) stat(rev string) (*RevInfo, error) {
 // fetchRefsLocked requires that r.mu remain locked for the duration of the call.
 func (r *gitRepo) fetchRefsLocked() error {
 	if r.fetchLevel < fetchAll {
-		if err := r.fetchUnshallow("refs/heads/*:refs/heads/*", "refs/tags/*:refs/tags/*"); err != nil {
+		// NOTE: To work around a bug affecting Git clients up to at least 2.23.0
+		// (2019-08-16), we must first expand the set of local refs, and only then
+		// unshallow the repository as a separate fetch operation. (See
+		// golang.org/issue/34266 and
+		// https://github.com/git/git/blob/4c86140027f4a0d2caaa3ab4bd8bfc5ce3c11c8a/transport.c#L1303-L1309.)
+
+		if _, err := Run(r.dir, "git", "fetch", "-f", r.remote, "refs/heads/*:refs/heads/*", "refs/tags/*:refs/tags/*"); err != nil {
 			return err
 		}
+
+		if _, err := os.Stat(filepath.Join(r.dir, "shallow")); err == nil {
+			if _, err := Run(r.dir, "git", "fetch", "--unshallow", "-f", r.remote); err != nil {
+				return err
+			}
+		}
+
 		r.fetchLevel = fetchAll
 	}
 	return nil
 }
 
-func (r *gitRepo) fetchUnshallow(refSpecs ...string) error {
-	// To work around a protocol version 2 bug that breaks --unshallow,
-	// add -c protocol.version=0.
-	// TODO(rsc): The bug is believed to be server-side, meaning only
-	// on Google's Git servers. Once the servers are fixed, drop the
-	// protocol.version=0. See Google-internal bug b/110495752.
-	var protoFlag []string
-	unshallowFlag := unshallow(r.dir)
-	if len(unshallowFlag) > 0 {
-		protoFlag = []string{"-c", "protocol.version=0"}
-	}
-	_, err := Run(r.dir, "git", protoFlag, "fetch", unshallowFlag, "-f", r.remote, refSpecs)
-	return err
-}
-
 // statLocal returns a RevInfo describing rev in the local git repository.
 // It uses version as info.Version.
 func (r *gitRepo) statLocal(version, rev string) (*RevInfo, error) {
@@ -563,39 +554,10 @@ func (r *gitRepo) ReadFileRevs(revs []string, file string, maxSize int64) (map[s
 	}
 	defer unlock()
 
-	var refs []string
-	var protoFlag []string
-	var unshallowFlag []string
-	for _, tag := range redo {
-		refs = append(refs, "refs/tags/"+tag+":refs/tags/"+tag)
-	}
-	if len(refs) > 1 {
-		unshallowFlag = unshallow(r.dir)
-		if len(unshallowFlag) > 0 {
-			// To work around a protocol version 2 bug that breaks --unshallow,
-			// add -c protocol.version=0.
-			// TODO(rsc): The bug is believed to be server-side, meaning only
-			// on Google's Git servers. Once the servers are fixed, drop the
-			// protocol.version=0. See Google-internal bug b/110495752.
-			protoFlag = []string{"-c", "protocol.version=0"}
-		}
-	}
-	if _, err := Run(r.dir, "git", protoFlag, "fetch", unshallowFlag, "-f", r.remote, refs); err != nil {
+	if err := r.fetchRefsLocked(); err != nil {
 		return nil, err
 	}
 
-	// TODO(bcmills): after the 1.11 freeze, replace the block above with:
-	//	if r.fetchLevel <= fetchSome {
-	//		r.fetchLevel = fetchSome
-	//		var refs []string
-	//		for _, tag := range redo {
-	//			refs = append(refs, "refs/tags/"+tag+":refs/tags/"+tag)
-	//		}
-	//		if _, err := Run(r.dir, "git", "fetch", "--update-shallow", "-f", r.remote, refs); err != nil {
-	//			return nil, err
-	//		}
-	//	}
-
 	if _, err := r.readFileRevs(redo, file, files); err != nil {
 		return nil, err
 	}

src/cmd/go/testdata/script/mod_get_direct.txt

+# Regression test for golang.org/issue/34092: with an empty module cache,
+# 'GOPROXY=direct go get golang.org/x/tools/gopls@master' did not correctly
+# resolve the pseudo-version for its dependency on golang.org/x/tools.
+
+[short] skip
+[!net] skip
+[!exec:git] skip
+
+env GO111MODULE=on
+env GOPROXY=direct
+env GOSUMDB=off
+
+go list -m cloud.google.com/go@master
+! stdout 'v0.0.0-'
+
+-- go.mod --
+module example.com
+
+go 1.14
+-- go.sum --