formula_{auditor,versions}: handle sharding. #15865
Conversation
Ensure that `FormulaVersions` correctly also looks at older paths for sharded formulae. While we're here, also cleanup `FormulaVersions` a bit to have more signatures, cleanup dead code, make more code private, improve variable naming.
MikeMcQuaid
force-pushed
the
formula_versions_sharding
branch
from
b343938
to
8d1064c
Compare
| [relative_path, old_relative_path].compact.each do |entry| | ||
| Utils.popen_read(*rev_list_cmd, branch, "--", entry) do |io| | ||
| yield [io.readline.chomp, entry] until io.eof? | ||
| end |
There was a problem hiding this comment.
I think you can just pass both paths to git rev-list? Probably will be faster. Unless you need to pass them separately in order to correctly process the output? But it doesn't seem like it.
There was a problem hiding this comment.
@carlocab You can do both at once but it only returns a Git SHA and then you don't know what entry the SHA refers to for git cat-file later (which spits out errors to stdout if paths don't exist).
It's a bit slower but not dramatically:
$ hyperfine "git rev-list --abbrev-commit --remove-empty master -- Formula/a/ack.rb Formula/ack.rb"
Benchmark 1: git rev-list --abbrev-commit --remove-empty master -- Formula/a/ack.rb Formula/ack.rb
Time (mean ± σ): 7.874 s ± 0.164 s [User: 7.459 s, System: 0.396 s]
Range (min … max): 7.596 s … 8.032 s 10 runs
$ hyperfine "git rev-list --abbrev-commit --remove-empty master -- Formula/a/ack.rb; git rev-list --abbrev-commit --remove-empty master -- Formula/ack.rb"
Benchmark 1: git rev-list --abbrev-commit --remove-empty master -- Formula/a/ack.rb; git rev-list --abbrev-commit --remove-empty master -- Formula/ack.rb
Time (mean ± σ): 8.310 s ± 0.572 s [User: 7.475 s, System: 0.806 s]
Range (min … max): 7.769 s … 9.825 s 10 runs| } | ||
| def formula_at_revision(rev, &_block) | ||
| def formula_at_revision(revision, formula_relative_path = relative_path, &_block) |
There was a problem hiding this comment.
I have a vague memory of using this in test-bot. I'll double check later, but it would be good to fix any callers there too.
There was a problem hiding this comment.
I don't see any uses outside of Homebrew/brew.
|
Looks like I might have misdiagnosed the |
I think it still could have been based on looking at the uses of this? Even although I didn't change the caller: it will still be checking two names now when it only changed one before. |
Ensure that
FormulaVersionscorrectly also looks at older paths for sharded formulae.While we're here, also cleanup
FormulaVersionsa bit to have more signatures, cleanup dead code, make more code private, improve variable naming.Partial solution for #15856