Ran scripted-diff from 2d9d752e4fc30aabf2ddd36ca7a63432d26d4fea:
0sed -i --regexp-extended -e 's/\buint256S\("(0x)?([^"]{64})"\)/uint256{"\2"}/g' $(git grep -l uint256S)
Follow-up to Testnet4 introduction #29775 which overlapped with work on uint256 consteval ctor #30560 (the latter includes the scripted-diff commit).
Going forward uint256{} should be used for constants instead of uint256S().
Ran scripted-diff from 2d9d752e4fc30aabf2ddd36ca7a63432d26d4fea.
Follow-up to #29775 which overlapped with work on #30560 (the latter includes the scripted-diff commit).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
review-ACK 49f9b645ea3f42c1b9e0a83b26af8fc8f6fa159d 🐮
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review-ACK 49f9b645ea3f42c1b9e0a83b26af8fc8f6fa159d 🐮
3DBaDDPWYrgqPiqxeTlRzcfIADMMqBxcLr0V/soQ4agaKh7fHhMUyEW77K17VoAMEla59o3GiThk4mPy14oGtAw==
ACK 49f9b645ea3f42c1b9e0a83b26af8fc8f6fa159d
For the future: you could have made the commit here an actual scripted diff too.
For the future: you could have made the commit here an actual scripted diff too.
Good point, thanks, still getting the hang of scripted diffs. Makes sense in case new stuff using uint256S gets merged into master before this.
For the future: you could have made the commit here an actual scripted diff too.
Good point, thanks, still getting the hang of scripted diffs. Makes sense in case new stuff using
uint256Sgets merged into master before this.
A scripted-diff won’t help here, because it is applied on top of the commit it is based on, which won’t change when master is changed, unless you rebase on every push to master.
Moreover, I think scripted-diffs should only be used when it makes sense. As a rule of thumb, I think when the script is larger than the diff, they do not make sense, because:
It seems easier for reviewers who like grep to just call git grep --extended-regexp '\buint256S\("(0x)?([^"]{64})"\)' 49f9b645ea3f42c1b9e0a83b26af8fc8f6fa159d~1 (and ...~0 to see the difference).
A scripted-diff won’t help here, because it is applied on top of the commit it is based on, which won’t change when master is changed, unless you rebase on every push to master.
Couldn’t CI at least run scripted diffs when the PR they belong to gets merged into master - so it could be caught post-merge?
When a commit from a pull request (which may contain a scripted-diff) is merged into another branch, the commit itself does not change, so it won’t affect the execution or the result of the scripted-diff in that commit.
In theory, the commits could be rebased/cherry-picked instead of merged. However, that’d make auditing harder, because it makes it easier to inject malicious code accidentally or intentionally. Also, it wouldn’t help here, because then the scripted diff check may permanently fail on the rebased/cherry-picked commits. (It could only be fixed up in a follow-up, which seems easier to do as-is done today).
#29775 was merged into master as da083d4bbdb37737f5080fada97bd15f5a8bfb2d, and #30560 was merged into master as 21c2879f37ce336af6df878d43ab090eb9d02157.
I’m thinking CI running on 21c2879f37ce336af6df878d43ab090eb9d02157 upon merge, could detect a new scripted-diff, re-run it on 21c2879f37ce336af6df878d43ab090eb9d02157 and in theory detect issues with #29775 if it had been merged into master earlier (wouldn’t have caught this case though, as da083d4bbdb37737f5080fada97bd15f5a8bfb2d was merged later).
Is that mental model of Git correct?
Are you concerned that scripted-diffs should only be run on the tree of their own commit, and might cause issues when run on merge-commits when PRs include later commits after their scripted-diff that happen to introduce text that would be replaced?
What I’m after is less reliance on having to remember to re-run scripted-diffs.
Needs backport to 28.x branch
Unless this fixes a bug that I’m missing, refactors generally are not candidates for backport.