bintray: check remote file hash prior to upload #8905
Conversation
Library/Homebrew/bintray.rb
Outdated
| false | ||
| result = curl_output "--fail", "--silent", "--head", url | ||
| if result.success? | ||
| [:present, result.stdout.match(/^X-Checksum-Sha2: ([0-9a-f]{64})/i)[1]] |
There was a problem hiding this comment.
| [:present, result.stdout.match(/^X-Checksum-Sha2: ([0-9a-f]{64})/i)[1]] | |
| { | |
| success: true, | |
| checksum: result.stdout.match(/^X-Checksum-Sha2: ([0-9a-f]{64})/i)[1]] | |
| } |
or similar; I think a hash makes this a little more readable.
Also: can this fail if it's a success but it can't find the checksum header? What about making the presence of a checksum imply success and checking explicitly for the header before returning a (possibly) missing match?
There was a problem hiding this comment.
Yeah, I haven't seen any cases where the checksum doesn't exist. But, also the Bintray API documentation is so sparse that it's not clear if the situation where the file exists but doesn't return a checksum is even possible.
How about this:
file exists and has checksum: return checksum
file exists but doesn't have checksum: return true
file doesn't exist: return false
That way, a truthy return value always means "the file exists", and you can additionally compare the return value against your local hash to look for a mismatch. (If the remote file exists but doesn't have a checksum, you can't tell if a previous upload already succeeded anyway, so this will still alert you of a mismatch and prompt you to delete the remote file).
There was a problem hiding this comment.
Yeah, I haven't seen any cases where the checksum doesn't exist. But, also the Bintray API documentation is so sparse that it's not clear if the situation where the file exists but doesn't return a checksum is even possible.
Yeh, with external APIs I always like to assume they will break at any point and start returning garbage 😂
file exists and has checksum: return checksum
file exists but doesn't have checksum: return true
file doesn't exist: return false
Didn't realise there was essentially three cases here. I'd say in that case return a checksum, empty string, nil (or false)?
Library/Homebrew/bintray.rb
Outdated
|
|
||
| if remote_status == :present | ||
| if remote_hash.presence == sha256 | ||
| odebug "#{filename} is published with matching hash." |
There was a problem hiding this comment.
| odebug "#{filename} is published with matching hash." | |
| odebug "#{filename} is already published with matching hash." |
If the remote file is published and has a hash matching the file we're about to upload, just skip uploading and publishing it entirely, rather than bailing out with an error.
|
Ok, revamped this a bit. Probably easier to check the w=1 diff: https://github.com/Homebrew/brew/pull/8905/files?w=1 |
|
Thanks for the review |
If the remote file is published and has a hash matching the file we're about to upload, just skip uploading and publishing it entirely, rather than bailing out with an error.
This should reduce the number of cases where maintainers have to go and delete remote bottles when those bottles were correctly published but the homebrew/core git repository didn't update due to transient network errors.
brew stylewith your changes locally?brew testswith your changes locally?brew manlocally and committed any changes?