Skip to content
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

[SUPERCEDED] Standardize and disambiguate the return from digest.Parse #76

Closed
wants to merge 1 commit into from
Closed

[SUPERCEDED] Standardize and disambiguate the return from digest.Parse #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 15 additions & 2 deletions digest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ var (
// be returned if the format is invalid.
func Parse(s string) (Digest, error) {
d := Digest(s)
return d, d.Validate()
if err := d.Validate(); err != nil {
return "", err
}
Comment on lines +83 to +85
Copy link
Member

Choose a reason for hiding this comment

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

🤔 the existing code looks a bit odd indeed (always returning the digest, even if it didn't validate).

However, it looks like there's some code that uses the returned value even if there's an error;

go-digest/digestset/set.go

Lines 95 to 103 in bde1400

dgst, err := digest.Parse(d)
if err == digest.ErrDigestInvalidFormat {
hex = d
searchFunc = func(i int) bool {
return dst.entries[i].val >= d
}
} else {
hex = dgst.Hex()
alg = dgst.Algorithm()

(i.e., it only considers the error if it's a digest.ErrDigestInvalidFormat). Not sure if that's a bug, or if that's intentional (in either case, the documentation of this function should be updated to explain, and the code I linked to could use some comments as well)

@dmcgowan I see you wrote that code in 77570c9 (#55) perhaps you recall?

Copy link
Author

@nima nima Jul 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I've made a change that I think should address this (valid) issue that you've picked up with the change I made. Let me know if you think it seems reasonable to you.

Copy link
Member

Choose a reason for hiding this comment

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

Why not keep the previous code?

return d, nil
}

// FromReader consumes the content of rd until io.EOF, returning canonical digest.
Expand Down Expand Up @@ -137,11 +140,21 @@ func (d Digest) Encoded() string {
return string(d[d.sepIndex()+1:])
}

// Hex is deprecated. Please use Digest.Encoded.
// IsIdenticalTo compares this digest to any other, inclusive of the hashing algorithm used
func (d Digest) IsIdenticalTo(o Digest) bool {
return string(d) == string(o)
}
Copy link
Member

Choose a reason for hiding this comment

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

This should be probably a separate PR


// Deprecated: Hex has been deprecated in favor of Digest.Encoded
func (d Digest) Hex() string {
return d.Encoded()
}

// IsEmpty does what it says (an empty digest is an empty string)
func (d Digest) IsEmpty() bool {
Copy link
Member

Choose a reason for hiding this comment

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

This one too

return len(d.String()) == 0
}

func (d Digest) String() string {
return string(d)
}
Expand Down
3 changes: 2 additions & 1 deletion digestset/set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,14 @@ func (dst *Set) Lookup(d string) (digest.Digest, error) {
alg digest.Algorithm
hex string
)
dgst, err := digest.Parse(d)
_, err := digest.Parse(d)
if err == digest.ErrDigestInvalidFormat {
hex = d
searchFunc = func(i int) bool {
return dst.entries[i].val >= d
}
} else {
dgst := digest.Digest(d)
hex = dgst.Hex()
alg = dgst.Algorithm()
searchFunc = func(i int) bool {
Expand Down