Skip to content

digestset: cleanup/refactor, and touch-up GoDoc #120

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thaJeztah wants to merge 11 commits into
base: master
Choose a base branch
from
Open

digestset: cleanup/refactor, and touch-up GoDoc #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
0fdc85c
digestset: simplify short digest matching
thaJeztah Aug 6, 2026
08b4c62
digestset: remove redundant digestEntries type
thaJeztah Aug 6, 2026
cacba5a
digestset: document, and use zero values for Set
thaJeztah Aug 6, 2026
1541123
digestset: rename hex var for clarity
thaJeztah Aug 6, 2026
6cd09bb
digestset: inline searchFuncs
thaJeztah Aug 6, 2026
1ae6a04
digestset: simplify entries loop
thaJeztah Aug 6, 2026
5ac39d0
digestset: inline checkShortMatch
thaJeztah Aug 6, 2026
f91b86f
digestset: remove redundant lookup search
thaJeztah Aug 6, 2026
0826871
digestset: Remove: remove intermediate digestEntry struct
thaJeztah Aug 6, 2026
df95c11
digestset: simplify short-code generation
thaJeztah Aug 6, 2026
f4ac93b
digestset: touch-up some GoDoc
thaJeztah Aug 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 91 additions & 127 deletions digestset/set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,104 +35,88 @@ var (
ErrDigestAmbiguous = errors.New("ambiguous digest string")
)

// Set is used to hold a unique set of digests which
// may be easily referenced by easily referenced by a string
// representation of the digest as well as short representation.
// The uniqueness of the short representation is based on other
// digests in the set. If digests are omitted from this set,
// collisions in a larger set may not be detected, therefore it
// is important to always do short representation lookups on
// the complete set of digests. To mitigate collisions, an
// appropriately long short code should be used.
// Set holds a unique set of digests that may be referenced by their full or
// shortened string representation.
//
// The uniqueness of a shortened representation depends on the other digests
// in the set. If digests are omitted, collisions in a larger set may not be
// detected. Short representation lookups should therefore always use the
// complete set of digests and an appropriately long short code.
//
// The zero value of Set is ready for use.
type Set struct {
mutex sync.RWMutex
entries digestEntries
entries []*digestEntry
}

// NewSet creates an empty set of digests
// which may have digests added.
//
// In most cases, new([Set]) (or just declaring a [Set] variable) is
// sufficient to initialize a [Set].
func NewSet() *Set {
return &Set{
entries: digestEntries{},
}
}

// checkShortMatch checks whether two digests match as either whole
// values or short values. This function does not test equality,
// rather whether the second value could match against the first
// value.
func checkShortMatch(alg digest.Algorithm, hex, shortAlg, shortHex string) bool {
if len(hex) == len(shortHex) {
if hex != shortHex {
return false
}
if len(shortAlg) > 0 && string(alg) != shortAlg {
return false
}
} else if !strings.HasPrefix(hex, shortHex) {
return false
} else if len(shortAlg) > 0 && string(alg) != shortAlg {
return false
}
return true
return &Set{}
}

// Lookup looks for a digest matching the given string representation.
// If no digests could be found ErrDigestNotFound will be returned
// with an empty digest value. If multiple matches are found
// ErrDigestAmbiguous will be returned with an empty digest value.
// Lookup looks for a digest matching d.
//
// d may be a full digest, such as "sha256:abcdef...", an encoded-value prefix,
// such as "abcdef", or an algorithm-qualified encoded-value prefix, such as
// "sha256:abcdef". An exact full-digest match takes precedence over other
// prefix matches.
//
// If no digest matches, Lookup returns [ErrDigestNotFound]. If multiple prefix
// matches are found, Lookup returns [ErrDigestAmbiguous].
func (dst *Set) Lookup(d string) (digest.Digest, error) {
dst.mutex.RLock()
defer dst.mutex.RUnlock()
if len(dst.entries) == 0 {
return "", ErrDigestNotFound
}
var (
searchFunc func(int) bool
alg digest.Algorithm
hex string
alg digest.Algorithm
hexPrefix string
)
dgst, err := digest.Parse(d)
if errors.Is(err, digest.ErrDigestInvalidFormat) {
hex = d
searchFunc = func(i int) bool {
return dst.entries[i].val >= d
}
if dgst, err := digest.Parse(d); errors.Is(err, digest.ErrDigestInvalidFormat) {
// An input without a valid algorithm separator is treated as an
// unqualified encoded-value prefix.
hexPrefix = d
} else {
hex = dgst.Encoded()
// digest.Parse still returns the parsed algorithm and encoded value for
// qualified short digests, together with digest.ErrDigestInvalidLength.
hexPrefix = dgst.Encoded()
Comment on lines +80 to +87

@sudo-bmitch sudo-bmitch Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels fragile to me. Particularly if digest.Parse throws an unexpected error in the future that doesn't populate dgst with a value containing a colon, we could trigger a panic. Instead of parsing the string, could we make the condition if ind := strings.Index(d, ":"); ind >= 0?

Suggested change
if dgst, err := digest.Parse(d); errors.Is(err, digest.ErrDigestInvalidFormat) {
// An input without a valid algorithm separator is treated as an
// unqualified encoded-value prefix.
hexPrefix = d
} else {
hex = dgst.Encoded()
// digest.Parse still returns the parsed algorithm and encoded value for
// qualified short digests, together with digest.ErrDigestInvalidLength.
hexPrefix = dgst.Encoded()
if i := strings.Index(d, ":"); i >= 0 {
alg = digest.Algorithm(d[:i])
hasPrefix = d[i+1:]
} else {
hasPrefix = d
}

(Note GitHub doesn't let me select the next two lines in the review)

@AkihiroSuda AkihiroSuda Aug 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah PTAL

@thaJeztah thaJeztah Aug 25, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to refresh my memory a bit, and ask my bot to double check, but I don't think this can currently panic;
Parse will return a ErrDigestInvalidFormat if no separator (: ) is present, so Encoded / Algorithm is only called if a colon was found.

The concern would require a future change to Parse's error semantics. Since checking only for : would also weaken handling of malformed digests, I'd prefer to keep the current behavior; a fuzz test could cover the invariant separately.

@sudo-bmitch sudo-bmitch Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it can't currently panic (hence non-blocking review). I'm just concerned at the mental effort to verify that's the case, and a future change to a separate package's error handling could break that in a way we may not notice.

alg = dgst.Algorithm()
searchFunc = func(i int) bool {
if dst.entries[i].val == hex {
return dst.entries[i].alg >= alg
}
return dst.entries[i].val >= hex
}
}
idx := sort.Search(len(dst.entries), searchFunc)

// Entries whose value has hex as a prefix form a contiguous run starting
// at idx. Digests of a different algorithm may sort within that run, so a
// second matching entry is not necessarily adjacent to the first; scan the
// whole run instead of only inspecting idx and idx+1.
var match *digestEntry
for i := idx; i < len(dst.entries) && strings.HasPrefix(dst.entries[i].val, hex); i++ {
if !checkShortMatch(dst.entries[i].alg, dst.entries[i].val, string(alg), hex) {
idx := sort.Search(len(dst.entries), func(i int) bool {
return dst.entries[i].val >= hexPrefix
})

// Entries whose values start with hexPrefix form a contiguous range beginning
// at idx. Entries for other algorithms may appear between matching entries,
// so scan the entire prefix range rather than only idx and idx+1.
var match digest.Digest
for _, entry := range dst.entries[idx:] {
if !strings.HasPrefix(entry.val, hexPrefix) {
break
}
if alg != "" && entry.alg != alg {
// Non-matching algorithm.
continue
}
if dst.entries[i].alg == alg && dst.entries[i].val == hex {
// An exact whole-value match is unambiguous.
return dst.entries[i].digest, nil
if entry.val == hexPrefix {

@sudo-bmitch sudo-bmitch Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is now missing a needed check since alg could be "" which would allow a sha256 entry to be an exact match when a sha512 prefix also exists (and therefore should return ErrDigestAmbiguous). This is where the prior logic was getting confusing with assumptions based on the above if conditions, where all we really want to check is whether this digest is an exact match.

@AkihiroSuda AkihiroSuda Oct 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thaJeztah PTAL

// An exact encoded-value match is unambiguous.
return entry.digest, nil
}
if match != nil {
if match != "" {
return "", ErrDigestAmbiguous
}
match = dst.entries[i]
match = entry.digest
}
if match == nil {
if match == "" {
return "", ErrDigestNotFound
}

return match.digest, nil
return match, nil
}

// Add adds the given digest to the set. An error will be returned
Expand All @@ -145,13 +129,12 @@ func (dst *Set) Add(d digest.Digest) error {
dst.mutex.Lock()
defer dst.mutex.Unlock()
entry := &digestEntry{alg: d.Algorithm(), val: d.Encoded(), digest: d}
searchFunc := func(i int) bool {
idx := sort.Search(len(dst.entries), func(i int) bool {
if dst.entries[i].val == entry.val {
return dst.entries[i].alg >= entry.alg
}
return dst.entries[i].val >= entry.val
}
idx := sort.Search(len(dst.entries), searchFunc)
})
if idx == len(dst.entries) {
dst.entries = append(dst.entries, entry)
return nil
Expand All @@ -175,14 +158,13 @@ func (dst *Set) Remove(d digest.Digest) error {
}
dst.mutex.Lock()
defer dst.mutex.Unlock()
entry := &digestEntry{alg: d.Algorithm(), val: d.Encoded(), digest: d}
searchFunc := func(i int) bool {
if dst.entries[i].val == entry.val {
return dst.entries[i].alg >= entry.alg
alg, val := d.Algorithm(), d.Encoded()
idx := sort.Search(len(dst.entries), func(i int) bool {
if dst.entries[i].val == val {
return dst.entries[i].alg >= alg
}
return dst.entries[i].val >= entry.val
}
idx := sort.Search(len(dst.entries), searchFunc)
return dst.entries[i].val >= val
})
// Not found if idx is after or value at idx is not digest
if idx == len(dst.entries) || dst.entries[idx].digest != d {
return nil
Expand All @@ -196,7 +178,7 @@ func (dst *Set) Remove(d digest.Digest) error {
return nil
}

// All returns all the digests in the set
// All returns a copy of all digests in the set.
func (dst *Set) All() []digest.Digest {
dst.mutex.RLock()
defer dst.mutex.RUnlock()
Expand All @@ -208,68 +190,50 @@ func (dst *Set) All() []digest.Digest {
return retValues
}

// ShortCodeTable returns a map of Digest to unique short codes. The
// length represents the minimum value, the maximum length may be the
// entire value of digest if uniqueness cannot be achieved without the
// full value. This function will attempt to make short codes as short
// as possible to be unique.
// ShortCodeTable returns the shortest unique code for each digest in dst.
//
// Codes are at least length characters long. A code may be extended up to the
// digest's full string representation when its encoded value is not unique at
// a shorter length.
func ShortCodeTable(dst *Set, length int) map[digest.Digest]string {
dst.mutex.RLock()
defer dst.mutex.RUnlock()
m := make(map[digest.Digest]string, len(dst.entries))
l := length
shortCodes := make(map[digest.Digest]string, len(dst.entries))
codeLength := length
resetIdx := 0
for i := 0; i < len(dst.entries); i++ {
var short string
extended := true
for extended {
extended = false
if len(dst.entries[i].val) <= l {
short = dst.entries[i].digest.String()
} else {
short = dst.entries[i].val[:l]
for j := i + 1; j < len(dst.entries); j++ {
if checkShortMatch(dst.entries[j].alg, dst.entries[j].val, "", short) {
if j > resetIdx {
resetIdx = j
}
extended = true
} else {
break
}
for i, entry := range dst.entries {
for {
if len(entry.val) <= codeLength {
shortCodes[entry.digest] = entry.digest.String()
break
}

short := entry.val[:codeLength]
extended := false
for j := i + 1; j < len(dst.entries); j++ {
if !strings.HasPrefix(dst.entries[j].val, short) {
break
}
if extended {
l++
if j > resetIdx {
resetIdx = j
}
extended = true
}
if !extended {
shortCodes[entry.digest] = short
break
}
codeLength++
}
m[dst.entries[i].digest] = short
if i >= resetIdx {
l = length
codeLength = length
}
}
return m
return shortCodes
}

type digestEntry struct {
alg digest.Algorithm
val string
digest digest.Digest
}

type digestEntries []*digestEntry

func (d digestEntries) Len() int {
return len(d)
}

func (d digestEntries) Less(i, j int) bool {
if d[i].val != d[j].val {
return d[i].val < d[j].val
}
return d[i].alg < d[j].alg
}

func (d digestEntries) Swap(i, j int) {
d[i], d[j] = d[j], d[i]
}
18 changes: 9 additions & 9 deletions digestset/set_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestLookup(t *testing.T) {
"sha256:6532111111111111111111111111111111111111111111111111111111111111",
}

dset := NewSet()
var dset Set
for i := range digests {
if err := dset.Add(digests[i]); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -118,7 +118,7 @@ func TestAddDuplication(t *testing.T) {
"sha512:65321111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111",
}

dset := NewSet()
var dset Set
for i := range digests {
if err := dset.Add(digests[i]); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -154,7 +154,7 @@ func TestRemove(t *testing.T) {
t.Fatal(err)
}

dset := NewSet()
var dset Set
for i := range digests {
if err := dset.Add(digests[i]); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -225,14 +225,14 @@ func TestShortCodeTable(t *testing.T) {
"sha256:6532111111111111111111111111111111111111111111111111111111111111",
}

dset := NewSet()
var dset Set
for i := range digests {
if err := dset.Add(digests[i]); err != nil {
t.Fatal(err)
}
}

dump := ShortCodeTable(dset, 2)
dump := ShortCodeTable(&dset, 2)

if len(dump) < len(digests) {
t.Fatalf("Error unexpected size: %d, expecting %d", len(dump), len(digests))
Expand Down Expand Up @@ -268,7 +268,7 @@ func benchAddNTable(b *testing.B, n int) {
}
b.ResetTimer()
for i := 0; i < b.N; i++ {
dset := &Set{entries: digestEntries(make([]*digestEntry, 0, n))}
dset := &Set{entries: make([]*digestEntry, 0, n)}
for j := range digests {
if err = dset.Add(digests[j]); err != nil {
b.Fatal(err)
Expand All @@ -283,7 +283,7 @@ func benchLookupNTable(b *testing.B, n int, shortLen int) {
if err != nil {
b.Fatal(err)
}
dset := &Set{entries: digestEntries(make([]*digestEntry, 0, n))}
dset := &Set{entries: make([]*digestEntry, 0, n)}
for i := range digests {
if err := dset.Add(digests[i]); err != nil {
b.Fatal(err)
Expand All @@ -310,7 +310,7 @@ func benchRemoveNTable(b *testing.B, n int) {
}
b.ResetTimer()
for i := 0; i < b.N; i++ {
dset := &Set{entries: digestEntries(make([]*digestEntry, 0, n))}
dset := &Set{entries: make([]*digestEntry, 0, n)}
b.StopTimer()
for j := range digests {
if err = dset.Add(digests[j]); err != nil {
Expand All @@ -332,7 +332,7 @@ func benchShortCodeNTable(b *testing.B, n int, shortLen int) {
if err != nil {
b.Fatal(err)
}
dset := &Set{entries: digestEntries(make([]*digestEntry, 0, n))}
dset := &Set{entries: make([]*digestEntry, 0, n)}
for i := range digests {
if err := dset.Add(digests[i]); err != nil {
b.Fatal(err)
Expand Down
Loading