On Thu, May 21, 2015 at 4:43 AM, Quim Gil <qgil(a)wikimedia.org> wrote: We also allow for self-merging of documentation fixes that don't change any code (e.g. "Oops, I forgot the RELEASE-NOTES entry!"). And if I manually rebase someone else's patch, or amend it to clean up whitespace, or cherry-pick it to another branch (automatically? manually because of a merge conflict?), would it let me merge or count that as "my own" now? I've also had opportunity for self-merge when someone else +2s, but Jenkins decides to be stupid and either not run or have a bogus failure that needs another +2 to tell it to try again. For that matter, I've had opportunity for using "Submit and merge" when Jenkins is being broken or excessively slow during a SWAT deployment. -- Brad Jorsch (Anomie) Software Engineer Wikimedia Foundation

Hi! I think it's not a good idea, as it trains people to do the opposite thing of what it's intended to train people for. I.e. "mandatory, but put 'whatever' there" IMHO is worse than non-mandatory but supported by common consensus. If we're setting up a new system, we shouldn't set it up so we constantly work around it. In theory, this is true. In practice, there are numerous occasions where people have to self-+2 - typoes, forgotten files, CI glitches, rebases, etc. Well, ok, "have to" is a strong word here - all of it can be worked around by dragging in somebody and asking them "please +2 this" - but again, that would be working against the setup and also training people that the system sucks and they have to work around it to be effective. -- Stas Malyshev smalyshev(a)wikimedia.org

On Thu, May 21, 2015 at 12:05 PM, Stas Malyshev &lt;smalyshev(a)wikimedia.org&gt; wrote: I don't agree, or I don't understand, "Test Plan:" is fantastic and I've wanted it in commit messages for years. Thinking about how to test is essential to writing good code, and a reminder to express your thought process is invaluable, whether it's "qunit tests now pass", or "it should no longer break when you visit the special page", or "obvious fix". Expecting reviewers to be Nostradamus and infer what the engineer might have thought about testing, or whether the engineer tested at all, is dumb. If someone writes "Test Plan: whatever", they'll get -1s until they respect reviewers enough to write "Test Plan: trivial untested fix in an area that lacks tests". In the last week my instance crashed on two checkins that were +2'd but never run; both useful improvements to MediaWiki-Vagrant roles that looked completely legit. I appreciate people writing needed code and others reviewing it promptly, but typing: "Test Plan: I didn't test this patch at all" might have saved me 20 minutes of wondering where I screwed up, re-provisioning, hunting down log files, etc. (Both are fixed BTW.) Can/will we add a "Test Plan: " line to the default commit.message <https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#idp25045696> just to get developers in the mood and encourage a standard format? Independent of Differential and separately from whether we make it mandatory. It seems developers have to do git config --global commit.template ~/path/to/better/git_commit.message , but we could put the file in a MediaWiki project like core Regards, -- =S Page WMF Tech writer

For Parsoid, we have comprehensive tests. When you fix a bug, you are expected to add a test showing the bug, and verifying that you've fixed it. I don't think it would help us at all to have to write "Test Plan: run the test suite." for every commit. There are a few issues which are not easily testable with our test infrastructure (which is motivation for us to improve it). Writing a "Test Plan" for those would be unobjectionable, and perhaps helpful. But they are the exception, not the rule. Which is all to say that mandatory fields are annoying, and we should have the ability on a project-by-project basis to enable/disable these things. --scott