-
Notifications
You must be signed in to change notification settings - Fork 13
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.
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
Add hashGlobs query to state service #3484
Conversation
MDrakos
commented
Sep 9, 2024
•
edited by github-actions
bot
Loading
edited by github-actions
bot
DX-3032 We have a query on the state-svc for calculating a hash for a series of globs |
When I ran |
internal/hash/file_hasher.go
Outdated
"time" | ||
|
||
"github.com/ActiveState/cli/internal/errs" | ||
"github.com/cespare/xxhash" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xxhash is a fast hash algorithm. For more info see here: https://xxhash.com/
this link is safe for work
internal/hash/file_hasher.go
Outdated
} | ||
|
||
fh.cache.Set(cacheKey(file.Name(), fileInfo.ModTime()), hash, cache.NoExpiration) | ||
fmt.Fprintf(hasher, "%x", hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? Might need a comment, or drop if it's for debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment. The hasher is an io.Writer
so this is writing the hash of the current file to the overall hash.
internal/hash/file_hasher.go
Outdated
fmt.Fprintf(hasher, "%x", hash) | ||
} | ||
|
||
return base64.StdEncoding.EncodeToString(hasher.Sum(nil)), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the hasher itself not produce a usable string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasher.Sum()
returns a slice of bytes and I don't believe these values are guaranteed to be printable characters. They could be control characters or invalid UFT-8 sequences. The encoding ensures that the output is safe to print and transmit.
} | ||
} | ||
|
||
func (fh *FileHasher) HashFiles(files []string) (string, error) { | ||
func (fh *FileHasher) HashFiles(files []string) (hash string, rerr error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (fh *FileHasher) HashFiles(files []string) (hash string, rerr error) { | |
func (fh *FileHasher) HashFiles(files []string) (string, error) { |
Doesn't look like naming these is used for anything. And it can actually cause bug so better to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot that we need the named error return for the deferred error handling https://github.com/ActiveState/cli/pull/3484/files#diff-f95330cc381d368f14952fbad1b24b3a714997bab5851fba341014a2271774d0R41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see. In that case I suggest renaming hash
to _
, so it doesn't clash with your code. You are using a hash
var in the function body. I don't think it clashes as currently written, but it feels error-prone.
Co-authored-by: Nathan Rijksen <[email protected]>
} | ||
} | ||
|
||
func (fh *FileHasher) HashFiles(files []string) (string, error) { | ||
func (fh *FileHasher) HashFiles(files []string) (hash string, rerr error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see. In that case I suggest renaming hash
to _
, so it doesn't clash with your code. You are using a hash
var in the function body. I don't think it clashes as currently written, but it feels error-prone.