]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: use a nil *Origin to represent "uncheckable"
authorBryan C. Mills <bcmills@google.com>
Thu, 16 Nov 2023 22:03:06 +0000 (17:03 -0500)
committerGopher Robot <gobot@golang.org>
Fri, 17 Nov 2023 22:43:50 +0000 (22:43 +0000)
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 <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
src/cmd/go/internal/modfetch/codehost/codehost.go
src/cmd/go/internal/modfetch/coderepo.go
src/cmd/go/internal/modload/build.go
src/cmd/go/internal/modload/list.go
src/cmd/go/internal/modload/query.go
src/cmd/go/testdata/script/mod_list_issue61423.txt

index 6ef9d298d4d0fe2ab6345dac976ce6e833e8b35b..69a3c57e26697552c7783ff0570f83a48c889060 100644 (file)
@@ -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
index 4f10f1f5ddce7613833159a69ffdbaeb3549fda7..7d83ac6971d2ad25c93f74db4011dded2180afb5 100644 (file)
@@ -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
                                }
index 4244a767a7ddfa5c436d35919f693c1f97ac3aec..5cf1487c3ede31302b89b65d7a08fe460a0b91de 100644 (file)
@@ -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 = ""
index e8872ba4b84c74397a8191a91befe3bedb8d9c73..ef93c25121acc8ebbf58ff8676a9a93a4c3146c7 100644 (file)
@@ -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
index 21e4db87fefe93956a4b94cda43c55ea6a046dcd..895c6c003274ce61ad1e0df0fa880ab193337e77 100644 (file)
@@ -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,
index b788d70c85b7393229c7047dcfa8154f032bcfd4..2888391f6d63025aedca5c4ac3e570c16e3bdf46 100644 (file)
@@ -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/issue61415.git@v0.0.0-20231114180000-08a4fa6bb9c0
+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/issue61415.git@v0.0.0-20231114180000-08a4fa6bb9c0
 stdout '"Version": "v0.0.0-20231114180000-08a4fa6bb9c0"'
 stdout '"Origin":'
 stdout '"VCS": "git"'