digestset: cleanup/refactor, and touch-up GoDoc - #120
Open
thaJeztah wants to merge 11 commits into
Open
Conversation
Member
Author
|
Split this in small, incremental changes to make the logic easier to review. There's some cleanups we can do once we update the minimum Go version ( |
thaJeztah
marked this pull request as ready for review
August 6, 2026 14:39
thaJeztah
requested review from
AkihiroSuda,
dmcgowan,
stevvooe and
vbatts
as code owners
August 6, 2026 14:39
Use strings.HasPrefix for full-length and shortened values, consolidate the optional algorithm check, and use strings.HasPrefix directly where no algorithm is specified. This preserves the existing behavior while making the matching logic easier to follow. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The digestEntries type was added as part of the original implementation in [distribution@8aacddd]; it implements `sort.Interface`, but this was never used (there's no `sort.Sort` on the slice, and `sort.Search` does not use it). Replace it with a plain slice, and remove initialization from `NewSet`, because the zero-value is usable for this package. [distribution@8aacddd]: distribution/distribution@7258fda Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Document that the zero-value of Set is usable, borrowing some wording from stdlib's bytes.NewBuffer (which is similar), and replace uses of NewSet in our tests. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The hex value is either a full hex (encoded) value, or a prefix; rename the var to more clearly indicate it may be partial. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This avoids a function-scoped variable for the searchFunc, whereas we're only interested in the result (idx). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use a basic range loop, starting at the idx from the binary-search, and store the digest itself as match, instead of using digestEntry as intermediate. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Now that it's only used in a single place, we can open-code it to make it more transparent what logic is used. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Use a single binary search to find the start of the matching encoded-value prefix range. The subsequent scan must already inspect the complete range to detect ambiguity and already filters by algorithm, making the second algorithm-aware search redundant. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Range over entries directly and refactor the loop to make the prefix extension and completion logic easier to follow. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
force-pushed
the
cleanup_digestset
branch
from
August 6, 2026 20:15
fb704c3 to
003d27e
Compare
sudo-bmitch
reviewed
Aug 12, 2026
sudo-bmitch
left a comment
sudo-bmitch
left a comment
There was a problem hiding this comment.
Non-blocking suggestions. I think everything in here looks good even without my suggested changes, but I'm less familiar with this package. Separate from this PR, I'd like to throw a fuzzer at it, to make sure user input can't trigger a panic.
Comment on lines
+80
to
+87
| 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() |
There was a problem hiding this comment.
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)
| 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 alg != "" && entry.val == hexPrefix { |
There was a problem hiding this comment.
Would it be more clear to do an exact match on the full digest string? This could then be moved up to the first condition in the loop to cover a common use case.
Suggested change
| if alg != "" && entry.val == hexPrefix { | |
| if entry.digest.String() == d { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor / cleanup the code; slight improvements in Benchmarks, and very minor regression in some, but probably mostly noise;
digestset: simplify short digest matching
Use strings.HasPrefix for full-length and shortened values, consolidate the
optional algorithm check, and use strings.HasPrefix directly where no
algorithm is specified.
This preserves the existing behavior while making the matching logic easier
to follow.
digestset: remove redundant digestEntries type
The digestEntries type was added as part of the original implementation in
distribution@8aacddd; it implements
sort.Interface, but this was neverused (there's no
sort.Sorton the slice, andsort.Searchdoes not useit).
Replace it with a plain slice, and remove initialization from
NewSet,because the zero-value is usable for this package.
digestset: document, and use zero values for Set
Document that the zero-value of Set is usable, borrowing some wording
from stdlib's bytes.NewBuffer (which is similar), and replace uses of
NewSet in our tests.
digestset: rename hex var for clarity
The hex value is either a full hex (encoded) value, or a prefix;
rename the var to more clearly indicate it may be partial.
digestset: inline searchFuncs
This avoids a function-scoped variable for the searchFunc, whereas
we're only interested in the result (idx).
digestset: simplify entries loop
Use a basic range loop, starting at the idx from the binary-search,
and store the digest itself as match, instead of using digestEntry
as intermediate.
digestset: inline checkShortMatch
Now that it's only used in a single place, we can open-code it to
make it more transparent what logic is used.
digestset: remove redundant lookup search
Use a single binary search to find the start of the matching encoded-value
prefix range. The subsequent scan must already inspect the complete range to
detect ambiguity and already filters by algorithm, making the second
algorithm-aware search redundant.
digestset: Remove: remove intermediate digestEntry struct
digestset: simplify short-code generation
Range over entries directly and refactor the loop to make the
prefix extension and completion logic easier to follow.
digestset: touch-up some GoDoc