From 45ca3c80e8ab7bfef73aceb32b04885946c02c32 Mon Sep 17 00:00:00 2001
From: Blake Mizerany <blake.mizerany@gmail.com>
Date: Thu, 4 Apr 2024 11:36:10 -0700
Subject: [PATCH] x/build/blob: more tests for ParseRef

Also, remove superfluous checks in Valid for invalid parts that are
present, which is impossible to have since Parts ensures all parts are
valid, and ParseRef ensures the ref is valid.
---
 x/build/blob/ref.go      | 27 +++++++--------------------
 x/build/blob/ref_test.go | 34 ++++++++++++++++++++++------------
 2 files changed, 29 insertions(+), 32 deletions(-)

diff --git a/x/build/blob/ref.go b/x/build/blob/ref.go
index 361b44a3..116c5896 100644
--- a/x/build/blob/ref.go
+++ b/x/build/blob/ref.go
@@ -230,7 +230,8 @@ func Parts(s string) iter.Seq2[PartKind, string] {
 
 		yieldValid := func(kind PartKind, part string) bool {
 			if !isValidPart(part) {
-				return yield(Invalid, "")
+				yield(Invalid, "")
+				return false
 			}
 			return yield(kind, part)
 		}
@@ -303,26 +304,12 @@ func Complete(s string) bool {
 	return ParseRef(s).Complete()
 }
 
+// Valid returns true if the ref has a valid name. To know if a ref is
+// "complete", use Complete.
 func (r Ref) Valid() bool {
-	// Name is required
-	if !isValidPart(r.name) {
-		return false
-	}
-
-	// Optional parts must be valid if present
-	if r.domain != "" && !isValidPart(r.domain) {
-		return false
-	}
-	if r.namespace != "" && !isValidPart(r.namespace) {
-		return false
-	}
-	if r.tag != "" && !isValidPart(r.tag) {
-		return false
-	}
-	if r.build != "" && !isValidPart(r.build) {
-		return false
-	}
-	return true
+	// Parts ensures we only have valid parts, so no need to validate
+	// them here, only check if we have a name or not.
+	return r.name != ""
 }
 
 // isValidPart returns true if given part is valid ascii [a-zA-Z0-9_\.-]
diff --git a/x/build/blob/ref_test.go b/x/build/blob/ref_test.go
index 4e7a2fb2..5e931878 100644
--- a/x/build/blob/ref_test.go
+++ b/x/build/blob/ref_test.go
@@ -33,7 +33,11 @@ var testRefs = map[string]Ref{
 	":/0":                    {},
 	"+0/00000":               {},
 	"0+.\xf2\x80\xf6\x9d00000\xe5\x99\xe6\xd900\xd90\xa60\x91\xdc0\xff\xbf\x99\xe800\xb9\xdc\xd6\xc300\x970\xfb\xfd0\xe0\x8a\xe1\xad\xd40\x9700\xa80\x980\xdd0000\xb00\x91000\xfe0\x89\x9b\x90\x93\x9f0\xe60\xf7\x84\xb0\x87\xa5\xff0\xa000\x9a\x85\xf6\x85\xfe\xa9\xf9\xe9\xde00\xf4\xe0\x8f\x81\xad\xde00\xd700\xaa\xe000000\xb1\xee0\x91": {},
-	"0//0": {},
+	"0//0":                        {},
+	"m+^^^":                       {},
+	"file:///etc/passwd":          {},
+	"file:///etc/passwd:latest":   {},
+	"file:///etc/passwd:latest+u": {},
 }
 
 func TestRefParts(t *testing.T) {
@@ -44,19 +48,25 @@ func TestRefParts(t *testing.T) {
 	}
 }
 
-func TestParseRef(t *testing.T) {
+func TestParseRefWithAndWithoutPrefixes(t *testing.T) {
 	for s, want := range testRefs {
-		t.Run(s, func(t *testing.T) {
-			got := ParseRef(s)
-			if got != want {
-				t.Errorf("ParseRef(%q) = %q; want %q", s, got, want)
-			}
+		for _, prefix := range []string{"", "https://", "http://"} {
+			// We should get the same results with or without the
+			// http(s) prefixes
+			s := prefix + s
 
-			// test round-trip
-			if ParseRef(got.String()) != got {
-				t.Errorf("String() = %s; want %s", got.String(), s)
-			}
-		})
+			t.Run(s, func(t *testing.T) {
+				got := ParseRef(s)
+				if got != want {
+					t.Errorf("ParseRef(%q) = %q; want %q", s, got, want)
+				}
+
+				// test round-trip
+				if ParseRef(got.String()) != got {
+					t.Errorf("String() = %s; want %s", got.String(), s)
+				}
+			})
+		}
 	}
 }