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
x/build/cmd/gopherbot: -2 any CLs with large binary blobs unless declared in commit message #10658
Comments
Instead of a force commit, we can say that adding a large binary files requires an explicit magic phrase in the commit message, as acknowledgment of intent. |
A simple thing would be, to list the to be added binary files in the commit message, to make it visible and communicate intent. |
I don't know if git-codereview is the best place to enforce this, as its use is not mandatory (I don't use it any more, for example), Gerrit would be better. Codereview would be better than nothing, mind. |
Slightly off-topic, but why not, @mwhudson? What do you do instead? If this were added, it would be a pre-commit hook, so if you use the hooks, it'd be picked up. I agree that we should enforce some simple size rules on the gerrit side, but doing it here too lets us put in more subtle and more restrictive detection logic, which the user can intentionally overrule. I like @nightlyone's suggestion for that. Overruling a gerrit check is way beyond a reviewer sanity-check. |
On 4 May 2015 at 05:26, Josh Bleecher Snyder notifications@github.com
|
SGTM. #18548 is pretty busy. Let’s leave this open, so it doesn’t get lost in the noise of that issue. M |
I agree we should leave this open. This is a feature that should be implemented as part of that system. |
I've retitled this to reflect my new short-term implementation plan in light of all the recent accidents. We can do this pretty easily and quickly in gopherbot by making it vote -2 on any Gerrit CL it seems with large files if they're not declared in the commit message. |
Looking at the data in maintner for https://go-review.googlesource.com/c/go/+/151318 I see the diff summary:
Note that for issue2331.dir we just see:
No number for the size of the blob. We need to add that to maintner first. Or have gopherbot just git fetch the thing and run a local |
Seems to me we should just require explicit call out for all binary files regardless of size. They’re rare and are more likely to have license issues anyway. (Unless and until this becomes annoying.) |
@josharian, worth investigating. I'll collect some stats to see how often that would've fired. |
Another: #34688 |
Another, a 6 MB binary: https://go-review.googlesource.com/c/go/+/229863/ |
@dmitshur who else should be cc’d on this? |
I noticed Gerrit now has a "Checks" tab. I don't really know what it does, but it sounds like something we could use for this. |
Most people who should be cc'ed here already are, but @toothrot @cagedmantis @FiloSottile should also be aware of this issue.
It's a feature that was added to Gerrit quite recently (sometime in 2019 I believe). It does seem like it could be a good option to consider. It would need to be investigated whether we can and what it takes. I think it's https://gerrit.googlesource.com/plugins/checks, but I have a hard time finding more documentation about it. There was one example of a Gerrit CL I found that has 3 checks. |
Another case: #40740. |
At the very least, if you had the bot post a comment ("hi you have a very large file in here, please confirm that you really wanted to do this"), it would probably get the human reviewers to spend an extra second or two reviewing the patch contents before submit, and that would be easier to add than some sort of automated commit message parser. |
I see that there are many implementation ideas here already. That said, here is another one. Something somewhere prevents me from mailing a CL that has a crypto key, unless I use |
See CL 9560 for context.
There will be cases in which it is legit to add a binary file. In such cases, we could do a force commit. Other suggestions welcomed.
Edit by @dmitshur: This also happened in CL 149604, see #28899.
The text was updated successfully, but these errors were encountered: