Talk:Gerrit/Code review

"Reversions themselves are always "ok"."

What if person A makes rX, which fixes bug P, and then person B reverts rX in rY because he thought it added bug Q, which was actually caused by something else. Since rY re-introduces bug P, can't it be marked fixme? Aaron 23:43, 15 November 2010 (UTC)

We mark reversions "ok" because it's OK to be cautious. The reviewer may have reintroduced the bug, but at least the situation is no worse than it was in the previous release. The onus is on the developer to convince the reviewer that their changes are worth having. The change that introduced the bug in the first place can be set from "resolved" back to "fixme", and the relevant bug reopened, while the developer is working on a solution that will make everyone happy. If it proves that the revert was a mistake, then the developer (or someone else) can reapply the change, once consensus is established that that's the right thing to do. -- Tim Starling 05:16, 3 February 2011 (UTC)

Inspected and tested

Hallo. What is exactly the meaning of "inspected" and "tested" in code review?

If i have to guess, "inspected" = "i read the code" and "tested" = "i tried to run the code". And in both cases it also means "i think that it's fine, but not sure enough to mark as OK".

Did i understand correctly? --Amir E. Aharoni 00:26, 18 October 2011 (UTC)

Yes! Sumana Harihareswara, Wikimedia Foundation Volunteer Development Coordinator (talk) 18:37, 17 February 2012 (UTC)

To add re Gerrit

-2 does not veto a change or prevent a merge going through. It's a strong disapproval. But another project owner can still come along, add a +2, and merge. The only way to actually cancel a commit and take it out of the merge queue is to click Abandon Change. Sumana Harihareswara, Wikimedia Foundation Volunteer Development Coordinator (talk) 23:36, 27 February 2012 (UTC)

"the code will still make sense in five years"

This page says:

Remember that when you mark a commit with +2, you're saying: ...

• the code will still make sense in five years.

Aren't we exagerating a bit here? --JGonera (WMF) (talk) 18:21, 27 December 2012 (UTC)

I don't know. Maybe run git blame on the codebase and find out how much of the codebase is older than 5 or 10 years? --Nemo 15:37, 15 January 2015 (UTC)

Moving +2 specific info to +2

This page would benefit from better neighborhood with +2. The content that applies only to developers with +2 permissions could be moved there, making this page lighter and easier for new developers willing to learn how to exercise their +1/-1 powers.--Qgil (talk) 22:09, 28 February 2013 (UTC)

Why for MediaWiki code review only?

This is the only document we have here for people willing to contribute their time reviewing changes in Gerrit. However it is focused in MediaWiki when the changeset queue is full with changes of many other projects and we welcome feedback on all. The basic advice for any reviewer could be placed in a first, simple section. Then MediaWiki or any other project could have whatever specific advice in their own sections.--Qgil (talk) 22:15, 28 February 2013 (UTC)

Adding a "privacy" section

We at the Wikimedia Foundation are in the process of revising our privacy policy, which now refers to our code review practice. We'd like to implement a change to this document that reflects that. I don't have time right now to make this work in a less WMF-centric way, and it may not be possible, but I'd like to create a new section which covers these two points:

• Does the feature generally conform to the Wikimedia Foundation privacy policy?
• If this creates a new operational requirement in order to conform, is there a realistic plan to ensure that the responsible parties know and are ready to comply with the new requirement?

Thoughts? -- RobLa-WMF (talk) 23:04, 29 January 2014 (UTC)

A few other points that Rob and I had discussed (though so long ago that he may have forgotten :). These are less about privacy per se and more about data retention; not sure which frame works best. I tried to frame these in the style of the current document, particularly the security review section:

* The reviewer should have read and understood the data retention guidelines and should be aware of the retention issues discussed there.

* Check what data is retained by the code being reviewed.

* Check that, if required by the data retention policy, that the stored data is deleted in no more than the time permitted by the policy.

This could perhaps be strengthened by adding separate points for PII (as opposed to data generally), but I'm not sure how best to phrase that. —LVilla (WMF) (talk) 18:26, 30 January 2014 (UTC)

The only issue I see is that sometimes the retention is tied to the operating environment (like relying on a cron-job to run deletion scripts). But it seems like a lot for a developer (especially our volunteers with +2) to have to ensure Ops is setup correctly before they merge a change. Should we also require some sort of document in the repo that tracks any operational considerations for data retention? CSteipp (talk) 23:55, 30 January 2014 (UTC)

I'm tempted to say "if you can't guarantee it is deleted, you probably shouldn't be recording it". (Certainly, there are lots of examples of data retention problems that boiled down to "I kept it because I forgot to delete it, and I could have deleted it immediately.") But I definitely see the point about burdening non-WMF reviewers and committers. More generally, I really don't know enough about how platform/ops communicate with each other to say how best to implement this in practice. —LVilla (WMF) (talk) 23:32, 11 February 2014 (UTC)

+2 your own patches

I quickly scanned and didnt see any guidelines about +2'ing your own patches. Did I miss it? Does mediawiki allow +2'ing own patches? If so, in which (hopefully limited) circumstances? Where it gets complex is if a 'component tech leader' doesnt have anyone else 'willing' to +2 a scary looking changeset, or a reviewer doing a few minor alterations to someone elses changeset and then +2'ing it. John Vandenberg (talk) 10:16, 24 August 2014 (UTC)

@John Vandenberg: see Gerrit/+2#.2B2_is_for_code_review.2C_not_merging_your_own_stuff.--Qgil (talk) 10:06, 25 August 2014 (UTC)

@Qgil: , wow, thanks for mentioning that page, which covers exactly what I was expecting to find, and failed. I didnt see that +2 was linked to that page. Gerrit/+2 needs to be more prominently linked. Im not sure how. John Vandenberg (talk) 08:25, 4 September 2014 (UTC)

Choosing revisions

This section seems excessively detailed and focused on a specific workflow. Can we trim the most "trivial" parts like "open a text editor" etc.? I think it's fine to leave that to each person's habits and choice. --Nemo 15:52, 15 January 2015 (UTC)

Code Review should be separated from gerrit specifics

I think we should have a general code review article that focuses on the process around code review, without getting into gerrit specifics.

Specifically, Gerrit/Code_review/Code_review_hours doesn't belong under Gerrit hierarchy and we are moving away from gerrit generally/gradually.

Automated tests?

Should a code review look for the presence of automated tests? If they don't exist, or are not as robust as they "should be", would that warrant a -1? --KSmith (WMF) (talk) 16:40, 12 August 2016 (UTC)

We haven't traditionally required, at code review time, unit tests for any new bit of code introduced. I've always wanted Jenkins to enforce such a rule, but we're in such a bad state as it is (wrt unit tests) for MW Core that it'd be a big uphill battle. Not saying we shouldn't, but that's why I didn't follow through with my original intent when I first started as Release Manager. A non-voting job wouldn't do much good (people will just ignore it) so we would need to be more intelligent about how we get from ~11% to anything higher :). Greg (WMF) (talk) 16:53, 12 August 2016 (UTC)

If the revision is trivial, broken and has no obvious value, mark the commit as "-2"

In practice, I have always (I think?) seen "broken" revisions (meaning: the revision does not completely do what the commit message or linked phab task says it should due to some logic error or bug) receive -1 instead of -2. Could we have examples of when -2 is appropriate? KHarlan (WMF) (talk) 13:31, 12 August 2019 (UTC)