Browse Source

x/model: make Digest just hold a string

This is so much easier to reason about and allows Name to contain only
parts, instead of a special field for the digest.

It also removes the allocs in Digest.String().
Blake Mizerany 1 year ago
parent
commit
06c21f00eb
4 changed files with 28 additions and 42 deletions
  1. 8 12
      x/model/digest.go
  2. 3 3
      x/model/digest_test.go
  3. 11 21
      x/model/name.go
  4. 6 6
      x/model/name_test.go

+ 8 - 12
x/model/digest.go

@@ -15,21 +15,17 @@ import (
 //
 // It is comparable with other Digests and can be used as a map key.
 type Digest struct {
-	typ    string
-	digest string
+	s string
 }
 
-func (d Digest) Type() string   { return d.typ }
-func (d Digest) Digest() string { return d.digest }
-func (d Digest) IsValid() bool  { return d != Digest{} }
-
-func (d Digest) String() string {
-	if !d.IsValid() {
-		return ""
-	}
-	return fmt.Sprintf("%s-%s", d.typ, d.digest)
+func (d Digest) Type() string {
+	typ, _, _ := strings.Cut(d.s, "-")
+	return typ
 }
 
+func (d Digest) IsValid() bool  { return d.s != "" }
+func (d Digest) String() string { return d.s }
+
 func (d Digest) MarshalText() ([]byte, error) {
 	return []byte(d.String()), nil
 }
@@ -75,7 +71,7 @@ func (d Digest) Value() (driver.Value, error) {
 func ParseDigest(s string) Digest {
 	typ, digest, ok := strings.Cut(s, "-")
 	if ok && isValidDigestType(typ) && isValidHex(digest) {
-		return Digest{typ: typ, digest: digest}
+		return Digest{s: s}
 	}
 	return Digest{}
 }

+ 3 - 3
x/model/digest_test.go

@@ -16,9 +16,9 @@ import "testing"
 
 var testDigests = map[string]Digest{
 	"":                 {},
-	"sha256-1234":      {typ: "sha256", digest: "1234"},
-	"sha256-5678":      {typ: "sha256", digest: "5678"},
-	"blake2-9abc":      {typ: "blake2", digest: "9abc"},
+	"sha256-1234":      {s: "sha256-1234"},
+	"sha256-5678":      {s: "sha256-5678"},
+	"blake2-9abc":      {s: "blake2-9abc"},
 	"-1234":            {},
 	"sha256-":          {},
 	"sha256-1234-5678": {},

+ 11 - 21
x/model/name.go

@@ -91,9 +91,8 @@ func (k PartKind) String() string {
 //
 // To make a Name by filling in missing parts from another Name, use [Fill].
 type Name struct {
-	_      structs.Incomparable
-	parts  [5]string // host, namespace, model, tag, build
-	digest Digest    // digest is a special part
+	_     structs.Incomparable
+	parts [6]string // host, namespace, model, tag, build
 
 	// TODO(bmizerany): track offsets and hold s (raw string) here? We
 	// could pack the offests all into a single uint64 since the first
@@ -140,12 +139,8 @@ func ParseName(s string) Name {
 		if kind == PartInvalid {
 			return Name{}
 		}
-		if kind == PartDigest {
-			r.digest = ParseDigest(part)
-			if !r.digest.IsValid() {
-				return Name{}
-			}
-			continue
+		if kind == PartDigest && !ParseDigest(part).IsValid() {
+			return Name{}
 		}
 		r.parts[kind] = part
 	}
@@ -173,7 +168,7 @@ func (r Name) WithBuild(build string) Name {
 }
 
 func (r Name) WithDigest(digest Digest) Name {
-	r.digest = digest
+	r.parts[PartDigest] = digest.String()
 	return r
 }
 
@@ -244,7 +239,8 @@ var seps = [...]string{
 	PartNamespace: "/",
 	PartModel:     ":",
 	PartTag:       "+",
-	PartBuild:     "",
+	PartBuild:     "@",
+	PartDigest:    "",
 }
 
 // WriteTo implements io.WriterTo. It writes the fullest possible display
@@ -263,16 +259,12 @@ func (r Name) writeTo(w io.StringWriter) {
 		if r.parts[i] == "" {
 			continue
 		}
-		if partsWritten > 0 {
+		if partsWritten > 0 || i == int(PartDigest) {
 			w.WriteString(seps[i-1])
 		}
 		w.WriteString(r.parts[i])
 		partsWritten++
 	}
-	if r.IsResolved() {
-		w.WriteString("@")
-		w.WriteString(r.digest.String())
-	}
 }
 
 var builderPool = sync.Pool{
@@ -306,9 +298,6 @@ func (r Name) GoString() string {
 	for i := range r.parts {
 		r.parts[i] = cmp.Or(r.parts[i], "?")
 	}
-	if !r.IsResolved() {
-		r.digest = Digest{"?", "?"}
-	}
 	return r.String()
 }
 
@@ -399,7 +388,7 @@ func (r Name) IsCompleteNoBuild() bool {
 // It is possible to have a valid Name, or a complete Name that is not
 // resolved.
 func (r Name) IsResolved() bool {
-	return r.digest.IsValid()
+	return r.Digest().IsValid()
 }
 
 // Digest returns the digest part of the Name, if any.
@@ -407,7 +396,8 @@ func (r Name) IsResolved() bool {
 // If Digest returns a non-empty string, then [Name.IsResolved] will return
 // true, and digest is considered valid.
 func (r Name) Digest() Digest {
-	return r.digest
+	// This was already validated by ParseName, so we can just return it.
+	return Digest{r.parts[PartDigest]}
 }
 
 // EqualFold reports whether r and o are equivalent model names, ignoring

+ 6 - 6
x/model/name_test.go

@@ -23,7 +23,7 @@ func fieldsFromName(p Name) fields {
 		model:     p.parts[PartModel],
 		tag:       p.parts[PartTag],
 		build:     p.parts[PartBuild],
-		digest:    p.digest.String(),
+		digest:    p.parts[PartDigest],
 	}
 }
 
@@ -103,7 +103,7 @@ var testNames = map[string]fields{
 
 func TestNameParts(t *testing.T) {
 	var p Name
-	if w, g := int(PartBuild+1), len(p.Parts()); w != g {
+	if w, g := int(PartDigest+1), len(p.Parts()); w != g {
 		t.Errorf("Parts() = %d; want %d", g, w)
 	}
 }
@@ -235,7 +235,7 @@ func TestNameDisplay(t *testing.T) {
 			wantLong:     "library/mistral:latest",
 			wantComplete: "example.com/library/mistral:latest",
 			wantModel:    "mistral",
-			wantGoString: "example.com/library/mistral:latest+Q4_0@?-?",
+			wantGoString: "example.com/library/mistral:latest+Q4_0@?",
 		},
 		{
 			name:         "Short Name",
@@ -244,7 +244,7 @@ func TestNameDisplay(t *testing.T) {
 			wantLong:     "mistral:latest",
 			wantComplete: "mistral:latest",
 			wantModel:    "mistral",
-			wantGoString: "?/?/mistral:latest+?@?-?",
+			wantGoString: "?/?/mistral:latest+?@?",
 		},
 		{
 			name:         "Long Name",
@@ -253,7 +253,7 @@ func TestNameDisplay(t *testing.T) {
 			wantLong:     "library/mistral:latest",
 			wantComplete: "library/mistral:latest",
 			wantModel:    "mistral",
-			wantGoString: "?/library/mistral:latest+?@?-?",
+			wantGoString: "?/library/mistral:latest+?@?",
 		},
 		{
 			name:         "Case Preserved",
@@ -262,7 +262,7 @@ func TestNameDisplay(t *testing.T) {
 			wantLong:     "Library/Mistral:Latest",
 			wantComplete: "Library/Mistral:Latest",
 			wantModel:    "Mistral",
-			wantGoString: "?/Library/Mistral:Latest+?@?-?",
+			wantGoString: "?/Library/Mistral:Latest+?@?",
 		},
 		{
 			name:         "With digest",