Skip to content

Commit fa4d9b8

Browse files
author
Bryan C. Mills
committed
cmd/go/internal/modfetch: do not short-circuit canonical versions
Since at least CL 121857, the conversion logic in (*modfetch).codeRepo.Stat has had a short-circuit to use the version requested by the caller if it successfully resolves and is already canonical. However, we should not use that version if it refers to a branch instead of a tag, because branches (unlike tags) usually do not refer to a single, stable release: a branch named "v1.0.0" may be for the development of the v1.0.0 release, or for the development of patches based on v1.0.0, but only one commit (perhaps at the end of that branch — but possibly not even written yet!) can be that specific version. We already have some logic to prefer tags that are semver-equivalent to the version requested by the caller. That more general case suffices for exact equality too — so we can eliminate the special-case, fixing the bug and (happily!) also somewhat simplifying the code. Fixes #35671 Updates #41512 Change-Id: I2fd290190b8a99a580deec7e26d15659b58a50b0 Reviewed-on: https://go-review.googlesource.com/c/go/+/378400 Trust: Bryan Mills <[email protected]> Run-TryBot: Bryan Mills <[email protected]> Reviewed-by: Russ Cox <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
1 parent b004470 commit fa4d9b8

3 files changed

Lines changed: 277 additions & 250 deletions

File tree

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

Lines changed: 105 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -298,16 +298,13 @@ func (r *codeRepo) Latest() (*RevInfo, error) {
298298
// If statVers is a valid module version, it is used for the Version field.
299299
// Otherwise, the Version is derived from the passed-in info and recent tags.
300300
func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, error) {
301-
info2 := &RevInfo{
302-
Name: info.Name,
303-
Short: info.Short,
304-
Time: info.Time,
305-
}
306-
307301
// If this is a plain tag (no dir/ prefix)
308302
// and the module path is unversioned,
309303
// and if the underlying file tree has no go.mod,
310304
// then allow using the tag with a +incompatible suffix.
305+
//
306+
// (If the version is +incompatible, then the go.mod file must not exist:
307+
// +incompatible is not an ongoing opt-out from semantic import versioning.)
311308
var canUseIncompatible func() bool
312309
canUseIncompatible = func() bool {
313310
var ok bool
@@ -321,19 +318,12 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e
321318
return ok
322319
}
323320

324-
invalidf := func(format string, args ...any) error {
325-
return &module.ModuleError{
326-
Path: r.modPath,
327-
Err: &module.InvalidVersionError{
328-
Version: info2.Version,
329-
Err: fmt.Errorf(format, args...),
330-
},
331-
}
332-
}
333-
334-
// checkGoMod verifies that the go.mod file for the module exists or does not
335-
// exist as required by info2.Version and the module path represented by r.
336-
checkGoMod := func() (*RevInfo, error) {
321+
// checkCanonical verifies that the canonical version v is compatible with the
322+
// module path represented by r, adding a "+incompatible" suffix if needed.
323+
//
324+
// If statVers is also canonical, checkCanonical also verifies that v is
325+
// either statVers or statVers with the added "+incompatible" suffix.
326+
checkCanonical := func(v string) (*RevInfo, error) {
337327
// If r.codeDir is non-empty, then the go.mod file must exist: the module
338328
// author — not the module consumer, — gets to decide how to carve up the repo
339329
// into modules.
@@ -344,73 +334,91 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e
344334
// r.findDir verifies both of these conditions. Execute it now so that
345335
// r.Stat will correctly return a notExistError if the go.mod location or
346336
// declared module path doesn't match.
347-
_, _, _, err := r.findDir(info2.Version)
337+
_, _, _, err := r.findDir(v)
348338
if err != nil {
349339
// TODO: It would be nice to return an error like "not a module".
350340
// Right now we return "missing go.mod", which is a little confusing.
351341
return nil, &module.ModuleError{
352342
Path: r.modPath,
353343
Err: &module.InvalidVersionError{
354-
Version: info2.Version,
344+
Version: v,
355345
Err: notExistError{err: err},
356346
},
357347
}
358348
}
359349

360-
// If the version is +incompatible, then the go.mod file must not exist:
361-
// +incompatible is not an ongoing opt-out from semantic import versioning.
362-
if strings.HasSuffix(info2.Version, "+incompatible") {
363-
if !canUseIncompatible() {
350+
invalidf := func(format string, args ...any) error {
351+
return &module.ModuleError{
352+
Path: r.modPath,
353+
Err: &module.InvalidVersionError{
354+
Version: v,
355+
Err: fmt.Errorf(format, args...),
356+
},
357+
}
358+
}
359+
360+
// Add the +incompatible suffix if needed or requested explicitly, and
361+
// verify that its presence or absence is appropriate for this version
362+
// (which depends on whether it has an explicit go.mod file).
363+
364+
if v == strings.TrimSuffix(statVers, "+incompatible") {
365+
v = statVers
366+
}
367+
base := strings.TrimSuffix(v, "+incompatible")
368+
var errIncompatible error
369+
if !module.MatchPathMajor(base, r.pathMajor) {
370+
if canUseIncompatible() {
371+
v = base + "+incompatible"
372+
} else {
364373
if r.pathMajor != "" {
365-
return nil, invalidf("+incompatible suffix not allowed: module path includes a major version suffix, so major version must match")
374+
errIncompatible = invalidf("module path includes a major version suffix, so major version must match")
366375
} else {
367-
return nil, invalidf("+incompatible suffix not allowed: module contains a go.mod file, so semantic import versioning is required")
376+
errIncompatible = invalidf("module contains a go.mod file, so module path must match major version (%q)", path.Join(r.pathPrefix, semver.Major(v)))
368377
}
369378
}
379+
} else if strings.HasSuffix(v, "+incompatible") {
380+
errIncompatible = invalidf("+incompatible suffix not allowed: major version %s is compatible", semver.Major(v))
381+
}
370382

371-
if err := module.CheckPathMajor(strings.TrimSuffix(info2.Version, "+incompatible"), r.pathMajor); err == nil {
372-
return nil, invalidf("+incompatible suffix not allowed: major version %s is compatible", semver.Major(info2.Version))
383+
if statVers != "" && statVers == module.CanonicalVersion(statVers) {
384+
// Since the caller-requested version is canonical, it would be very
385+
// confusing to resolve it to anything but itself, possibly with a
386+
// "+incompatible" suffix. Error out explicitly.
387+
if statBase := strings.TrimSuffix(statVers, "+incompatible"); statBase != base {
388+
return nil, &module.ModuleError{
389+
Path: r.modPath,
390+
Err: &module.InvalidVersionError{
391+
Version: statVers,
392+
Err: fmt.Errorf("resolves to version %v (%s is not a tag)", v, statBase),
393+
},
394+
}
373395
}
374396
}
375397

376-
return info2, nil
398+
if errIncompatible != nil {
399+
return nil, errIncompatible
400+
}
401+
402+
return &RevInfo{
403+
Name: info.Name,
404+
Short: info.Short,
405+
Time: info.Time,
406+
Version: v,
407+
}, nil
377408
}
378409

379410
// Determine version.
380-
//
381-
// If statVers is canonical, then the original call was repo.Stat(statVers).
382-
// Since the version is canonical, we must not resolve it to anything but
383-
// itself, possibly with a '+incompatible' annotation: we do not need to do
384-
// the work required to look for an arbitrary pseudo-version.
385-
if statVers != "" && statVers == module.CanonicalVersion(statVers) {
386-
info2.Version = statVers
387-
388-
if module.IsPseudoVersion(info2.Version) {
389-
if err := r.validatePseudoVersion(info, info2.Version); err != nil {
390-
return nil, err
391-
}
392-
return checkGoMod()
393-
}
394411

395-
if err := module.CheckPathMajor(info2.Version, r.pathMajor); err != nil {
396-
if canUseIncompatible() {
397-
info2.Version += "+incompatible"
398-
return checkGoMod()
399-
} else {
400-
if vErr, ok := err.(*module.InvalidVersionError); ok {
401-
// We're going to describe why the version is invalid in more detail,
402-
// so strip out the existing “invalid version” wrapper.
403-
err = vErr.Err
404-
}
405-
return nil, invalidf("module contains a go.mod file, so major version must be compatible: %v", err)
406-
}
412+
if module.IsPseudoVersion(statVers) {
413+
if err := r.validatePseudoVersion(info, statVers); err != nil {
414+
return nil, err
407415
}
408-
409-
return checkGoMod()
416+
return checkCanonical(statVers)
410417
}
411418

412-
// statVers is empty or non-canonical, so we need to resolve it to a canonical
413-
// version or pseudo-version.
419+
// statVers is not a pseudo-version, so we need to either resolve it to a
420+
// canonical version or verify that it is already a canonical tag
421+
// (not a branch).
414422

415423
// Derive or verify a version from a code repo tag.
416424
// Tag must have a prefix matching codeDir.
@@ -441,71 +449,62 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e
441449
if v == "" || !strings.HasPrefix(trimmed, v) {
442450
return "", false // Invalid or incomplete version (just vX or vX.Y).
443451
}
444-
if isRetracted(v) {
445-
return "", false
446-
}
447452
if v == trimmed {
448453
tagIsCanonical = true
449454
}
450-
451-
if err := module.CheckPathMajor(v, r.pathMajor); err != nil {
452-
if canUseIncompatible() {
453-
return v + "+incompatible", tagIsCanonical
454-
}
455-
return "", false
456-
}
457-
458455
return v, tagIsCanonical
459456
}
460457

461458
// If the VCS gave us a valid version, use that.
462459
if v, tagIsCanonical := tagToVersion(info.Version); tagIsCanonical {
463-
info2.Version = v
464-
return checkGoMod()
460+
if info, err := checkCanonical(v); err == nil {
461+
return info, err
462+
}
465463
}
466464

467465
// Look through the tags on the revision for either a usable canonical version
468466
// or an appropriate base for a pseudo-version.
469-
var pseudoBase string
467+
var (
468+
highestCanonical string
469+
pseudoBase string
470+
)
470471
for _, pathTag := range info.Tags {
471472
v, tagIsCanonical := tagToVersion(pathTag)
472-
if tagIsCanonical {
473-
if statVers != "" && semver.Compare(v, statVers) == 0 {
474-
// The user requested a non-canonical version, but the tag for the
475-
// canonical equivalent refers to the same revision. Use it.
476-
info2.Version = v
477-
return checkGoMod()
473+
if statVers != "" && semver.Compare(v, statVers) == 0 {
474+
// The tag is equivalent to the version requested by the user.
475+
if tagIsCanonical {
476+
// This tag is the canonical form of the requested version,
477+
// not some other form with extra build metadata.
478+
// Use this tag so that the resolved version will match exactly.
479+
// (If it isn't actually allowed, we'll error out in checkCanonical.)
480+
return checkCanonical(v)
478481
} else {
479-
// Save the highest canonical tag for the revision. If we don't find a
480-
// better match, we'll use it as the canonical version.
482+
// The user explicitly requested something equivalent to this tag. We
483+
// can't use the version from the tag directly: since the tag is not
484+
// canonical, it could be ambiguous. For example, tags v0.0.1+a and
485+
// v0.0.1+b might both exist and refer to different revisions.
481486
//
482-
// NOTE: Do not replace this with semver.Max. Despite the name,
483-
// semver.Max *also* canonicalizes its arguments, which uses
484-
// semver.Canonical instead of module.CanonicalVersion and thereby
485-
// strips our "+incompatible" suffix.
486-
if semver.Compare(info2.Version, v) < 0 {
487-
info2.Version = v
488-
}
487+
// The tag is otherwise valid for the module, so we can at least use it as
488+
// the base of an unambiguous pseudo-version.
489+
//
490+
// If multiple tags match, tagToVersion will canonicalize them to the same
491+
// base version.
492+
pseudoBase = v
493+
}
494+
}
495+
// Save the highest non-retracted canonical tag for the revision.
496+
// If we don't find a better match, we'll use it as the canonical version.
497+
if tagIsCanonical && semver.Compare(highestCanonical, v) < 0 && !isRetracted(v) {
498+
if module.MatchPathMajor(v, r.pathMajor) || canUseIncompatible() {
499+
highestCanonical = v
489500
}
490-
} else if v != "" && semver.Compare(v, statVers) == 0 {
491-
// The user explicitly requested something equivalent to this tag. We
492-
// can't use the version from the tag directly: since the tag is not
493-
// canonical, it could be ambiguous. For example, tags v0.0.1+a and
494-
// v0.0.1+b might both exist and refer to different revisions.
495-
//
496-
// The tag is otherwise valid for the module, so we can at least use it as
497-
// the base of an unambiguous pseudo-version.
498-
//
499-
// If multiple tags match, tagToVersion will canonicalize them to the same
500-
// base version.
501-
pseudoBase = v
502501
}
503502
}
504503

505-
// If we found any canonical tag for the revision, return it.
504+
// If we found a valid canonical tag for the revision, return it.
506505
// Even if we found a good pseudo-version base, a canonical version is better.
507-
if info2.Version != "" {
508-
return checkGoMod()
506+
if highestCanonical != "" {
507+
return checkCanonical(highestCanonical)
509508
}
510509

511510
// Find the highest tagged version in the revision's history, subject to
@@ -528,11 +527,10 @@ func (r *codeRepo) convert(info *codehost.RevInfo, statVers string) (*RevInfo, e
528527
tag, _ = r.code.RecentTag(info.Name, tagPrefix, allowedMajor("v0"))
529528
}
530529
}
531-
pseudoBase, _ = tagToVersion(tag) // empty if the tag is invalid
530+
pseudoBase, _ = tagToVersion(tag)
532531
}
533532

534-
info2.Version = module.PseudoVersion(r.pseudoMajor, pseudoBase, info.Time, info.Short)
535-
return checkGoMod()
533+
return checkCanonical(module.PseudoVersion(r.pseudoMajor, pseudoBase, info.Time, info.Short))
536534
}
537535

538536
// validatePseudoVersion checks that version has a major version compatible with
@@ -556,10 +554,6 @@ func (r *codeRepo) validatePseudoVersion(info *codehost.RevInfo, version string)
556554
}
557555
}()
558556

559-
if err := module.CheckPathMajor(version, r.pathMajor); err != nil {
560-
return err
561-
}
562-
563557
rev, err := module.PseudoVersionRev(version)
564558
if err != nil {
565559
return err

0 commit comments

Comments
 (0)