cmd/go: use a nil *Origin to represent "uncheckable"

Previously, we used the presence of individual origin fields
to decide whether an Origin could be checked for staleness,
with a nil Origin representing “use whatever you have”.

However, that turns out to be fairly bug-prone: if we forget
to populate an Origin somewhere, we end up with an incomplete
check instead of a non-reusable origin (see #61415, #61423).

As of CL 543155, the reusability check for a given query
now depends on what is needed by the query more than what
is populated in the origin. With that in place, we can simplify
the handling of the Origin struct by using a nil pointer
to represent inconsistent or unavailable origin data, and
otherwise always reporting whatever origin information we have
regardless of whether we expect it to be reused.

Updates #61415.
Updates #61423.

Change-Id: I97c51063d6c2afa394a05bf304a80c72c08f82cf
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/543216
Auto-Submit: Bryan Mills <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Michael Matloob <[email protected]>

Author: Bryan C. Mills

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

@@ -122,21 +122,6 @@ type Origin struct {
 	RepoSum string `json:",omitempty"`
 }
 
-// Checkable reports whether the Origin contains anything that can be checked.
-// If not, the Origin is purely informational and should fail a CheckReuse call.
-func (o *Origin) Checkable() bool {
-	return o != nil && (o.TagSum != "" || o.Ref != "" || o.Hash != "" || o.RepoSum != "")
-}
-
-// ClearCheckable clears the Origin enough to make Checkable return false.
-func (o *Origin) ClearCheckable() {
-	o.TagSum = ""
-	o.TagPrefix = ""
-	o.Ref = ""
-	o.Hash = ""
-	o.RepoSum = ""
-}
-
 // A Tags describes the available tags in a code repository.
 type Tags struct {
 	Origin *Origin

src/cmd/go/internal/modfetch/coderepo.go

@@ -362,7 +362,7 @@ func (r *codeRepo) convert(ctx context.Context, info *codehost.RevInfo, statVers
 			}
 			tags, tagsErr := r.code.Tags(ctx, prefix)
 			if tagsErr != nil {
-				origin.ClearCheckable()
+				revInfo.Origin = nil
 				if err == nil {
 					err = tagsErr
 				}

src/cmd/go/internal/modload/build.go

@@ -161,53 +161,53 @@ func addUpdate(ctx context.Context, m *modinfo.ModulePublic) {
 	}
 }
 
-// mergeOrigin merges two origins,
+// mergeOrigin returns the union of data from two origins,
 // returning either a new origin or one of its unmodified arguments.
-// If the two origins conflict, mergeOrigin returns a non-specific one
-// that will not pass CheckReuse.
-// If m1 or m2 is nil, the other is returned unmodified.
-// But if m1 or m2 is non-nil and uncheckable, the result is also uncheckable,
-// to preserve uncheckability.
+// If the two origins conflict including if either is nil,
+// mergeOrigin returns nil.
 func mergeOrigin(m1, m2 *codehost.Origin) *codehost.Origin {
-	if m1 == nil {
-		return m2
-	}
-	if m2 == nil {
-		return m1
-	}
-	if !m1.Checkable() {
-		return m1
-	}
-	if !m2.Checkable() {
-		return m2
+	if m1 == nil || m2 == nil {
+		return nil
 	}
 
-	merged := new(codehost.Origin)
-	*merged = *m1 // Clone to avoid overwriting fields in cached results.
+	if m2.VCS != m1.VCS ||
+		m2.URL != m1.URL ||
+		m2.Subdir != m1.Subdir {
+		return nil
+	}
 
+	merged := *m1
+	if m2.Hash != "" {
+		if m1.Hash != "" && m1.Hash != m2.Hash {
+			return nil
+		}
+		merged.Hash = m2.Hash
+	}
 	if m2.TagSum != "" {
 		if m1.TagSum != "" && (m1.TagSum != m2.TagSum || m1.TagPrefix != m2.TagPrefix) {
-			merged.ClearCheckable()
-			return merged
+			return nil
 		}
 		merged.TagSum = m2.TagSum
 		merged.TagPrefix = m2.TagPrefix
 	}
-	if m2.Hash != "" {
-		if m1.Hash != "" && m1.Hash != m2.Hash {
-			merged.ClearCheckable()
-			return merged
-		}
-		merged.Hash = m2.Hash
-	}
 	if m2.Ref != "" {
 		if m1.Ref != "" && m1.Ref != m2.Ref {
-			merged.ClearCheckable()
-			return merged
+			return nil
 		}
 		merged.Ref = m2.Ref
 	}
-	return merged
+
+	switch {
+	case merged == *m1:
+		return m1
+	case merged == *m2:
+		return m2
+	default:
+		// Clone the result to avoid an alloc for merged
+		// if the result is equal to one of the arguments.
+		clone := merged
+		return &clone
+	}
 }
 
 // addVersions fills in m.Versions with the list of known versions.
@@ -331,7 +331,7 @@ func moduleInfo(ctx context.Context, rs *Requirements, m module.Version, mode Li
 		mod := module.Version{Path: m.Path, Version: m.Version}
 
 		if m.Version != "" {
-			if old := reuse[mod]; old != nil && old.Origin.Checkable() {
+			if old := reuse[mod]; old != nil {
 				if err := checkReuse(ctx, mod, old.Origin); err == nil {
 					*m = *old
 					m.Query = ""

src/cmd/go/internal/modload/list.go

@@ -57,8 +57,7 @@ func ListModules(ctx context.Context, args []string, mode ListMode, reuseFile st
 				}
 				return nil, fmt.Errorf("parsing %s: %v", reuseFile, err)
 			}
-			if m.Origin == nil || !m.Origin.Checkable() {
-				// Nothing to check to validate reuse.
+			if m.Origin == nil {
 				continue
 			}
 			m.Reuse = true

src/cmd/go/internal/modload/query.go

@@ -111,8 +111,8 @@ func checkReuse(ctx context.Context, m module.Version, old *codehost.Origin) err
 }
 
 func checkReuseRepo(ctx context.Context, repo versionRepo, path, query string, origin *codehost.Origin) error {
-	if !origin.Checkable() {
-		return errors.New("Origin is not checkable")
+	if origin == nil {
+		return errors.New("nil Origin")
 	}
 
 	// Ensure that the Origin actually includes enough fields to resolve the query.
@@ -138,6 +138,9 @@ func checkReuseRepo(ctx context.Context, repo versionRepo, path, query string, o
 		// If the version did not successfully resolve, the origin may indicate
 		// a TagSum and/or RepoSum instead of a Hash, in which case we still need
 		// to check those to ensure that the error is still applicable.
+		if origin.Hash == "" && origin.Ref == "" && origin.TagSum == "" {
+			return errors.New("no Origin information to check")
+		}
 
 	case IsRevisionQuery(path, query):
 		// This query may refer to a branch, non-version tag, or commit ID.
@@ -225,7 +228,7 @@ func queryProxy(ctx context.Context, proxy, path, query, current string, allowed
 		return nil, err
 	}
 
-	if old := reuse[module.Version{Path: path, Version: query}]; old != nil && old.Origin.Checkable() {
+	if old := reuse[module.Version{Path: path, Version: query}]; old != nil {
 		if err := checkReuseRepo(ctx, repo, path, query, old.Origin); err == nil {
 			info := &modfetch.RevInfo{
 				Version: old.Version,

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

@@ -51,10 +51,20 @@ env GOMODCACHE=$WORK/modcache2
 # it is only going to have Git origin information about the one
 # commit — not the other tags that would go into resolving
 # the underlying version list.
+# 'go list' should not emit the partial information,
+# since it isn't enough to reconstruct the result.
 
 go list -m -json vcs-test.golang.org/git/issue61415.git@latest
 cp stdout proxy-latest.json
 stdout '"Version": "v0.0.0-20231114180000-08a4fa6bb9c0"'
+! stdout '"Origin":'
+
+# However, if we list a specific, stable version, we should get
+# whatever origin metadata the proxy has for the version.
+
+go list -m -json vcs-test.golang.org/git/[email protected]
+cp stdout proxy-version.json
+stdout '"Version": "v0.0.0-20231114180000-08a4fa6bb9c0"'
 stdout '"Origin":'
 stdout '"VCS": "git"'
 stdout '"Hash": "08a4fa6bb9c04ffba03b26ae427b0d6335d90a2a"'
@@ -63,7 +73,8 @@ stdout '"Hash": "08a4fa6bb9c04ffba03b26ae427b0d6335d90a2a"'
 
 # The -reuse flag has no effect with a proxy, since the proxy can serve
 # metadata about a given module version cheaply anyway.
-go list -reuse=proxy-latest.json -m -json vcs-test.golang.org/git/issue61415.git@latest
+
+go list -reuse=proxy-version.json -m -json vcs-test.golang.org/git/[email protected]
 stdout '"Version": "v0.0.0-20231114180000-08a4fa6bb9c0"'
 stdout '"Origin":'
 stdout '"VCS": "git"'