Version with helpful links: https://www.mediawiki.org/wiki/Git/Code_review/Getting_reviews
1) Write small commits.
It's easier for other people to review small changes that only change one thing. We'd rather see five small commits than one big one.
2) Respond to test failures and feedback.
Check your Gerrit settings and make sure you're getting email notifications. If your code fails automated tests, or you got some review already, respond to it in a comment or resubmission. Or hit the Abandon button to remove your commit from the review queue while you revise it.
(To see why automated tests fail, click on the link in the "failed" comment in Gerrit, hover over the failed test's red dot, wait for the popup to show, and then click "console output.")
3) Don't mix rebases with changes.
When rebasing, only rebase. That makes it easier to use the "Old Version History" dropdown, which greatly quickens reviews. If non-rebase changes are made inside a rebase changeset, you have to read through a lot more code to find it and it's non-obvious.
4) Add reviewers.
I try to help with this. If I notice an unreviewed changeset lingering, then I add a review request or two. (These are requests -- there's no way to assign a review to someone in Gerrit.) But it's faster if you do it right after committing. Some tricks:
* Click the name of the repository ("Gerrit project"), e.g. operations/debs/squid , and remove "status:open" from the search box to find other changesets in that repository. The people who write and review those changesets would be good candidates to add as reviewers. * Search through other commit summaries and changesets. Example: Matmarex and Foxtrott are interested in reviewing frontend changes, so I search for "message:css" to find changesets that mention CSS in their commit summaries to add them to. You can use this and regexes to find changes that touch the same components you're touching, to find likely reviewers. Learn more at https://gerrit.wikimedia.org/r/Documentation/user-search.html .
5) Review more.
Many eyes make bugs shallow. Read the code review guide and help out with comments, "+1", and "-1". Those are nonbinding, won't cause merges or rejections, and have no formal effect on the code review. But you'll learn, gain reputation, and get people to return the favor by reviewing you in the future. "How to review code in Gerrit" has the step-by-step explanation. Example Gerrit search for MediaWiki commits that have not had +1, +2, -1, or -2 reviews yet: https://gerrit.wikimedia.org/r/#/q/-CodeReview%252B1+-CodeReview%252B2+-Code...
Am 29.08.2012 23:55, schrieb Sumana Harihareswara:
Version with helpful links: https://www.mediawiki.org/wiki/Git/Code_review/Getting_reviews
- Write small commits.
It's easier for other people to review small changes that only change one thing. We'd rather see five small commits than one big one.
- Respond to test failures and feedback.
Check your Gerrit settings and make sure you're getting email notifications. If your code fails automated tests, or you got some review already, respond to it in a comment or resubmission. Or hit the Abandon button to remove your commit from the review queue while you revise it.
(To see why automated tests fail, click on the link in the "failed" comment in Gerrit, hover over the failed test's red dot, wait for the popup to show, and then click "console output.")
- Don't mix rebases with changes.
When rebasing, only rebase. That makes it easier to use the "Old Version History" dropdown, which greatly quickens reviews. If non-rebase changes are made inside a rebase changeset, you have to read through a lot more code to find it and it's non-obvious.
- Add reviewers.
I try to help with this. If I notice an unreviewed changeset lingering, then I add a review request or two. (These are requests -- there's no way to assign a review to someone in Gerrit.) But it's faster if you do it right after committing. Some tricks:
- Click the name of the repository ("Gerrit project"), e.g.
operations/debs/squid , and remove "status:open" from the search box to find other changesets in that repository. The people who write and review those changesets would be good candidates to add as reviewers.
- Search through other commit summaries and changesets. Example:
Matmarex and Foxtrott are interested in reviewing frontend changes, so I search for "message:css" to find changesets that mention CSS in their commit summaries to add them to. You can use this and regexes to find changes that touch the same components you're touching, to find likely reviewers. Learn more at https://gerrit.wikimedia.org/r/Documentation/user-search.html .
- Review more.
Many eyes make bugs shallow. Read the code review guide and help out with comments, "+1", and "-1". Those are nonbinding, won't cause merges or rejections, and have no formal effect on the code review. But you'll learn, gain reputation, and get people to return the favor by reviewing you in the future. "How to review code in Gerrit" has the step-by-step explanation. Example Gerrit search for MediaWiki commits that have not had +1, +2, -1, or -2 reviews yet: https://gerrit.wikimedia.org/r/#/q/-CodeReview%252B1+-CodeReview%252B2+-Code...
Something which needs so much text, is broken is some respect. "Mies van der Rohe: Less is more."
Le 30/08/12 00:05, Thomas Gries a écrit :
Something which needs so much text, is broken is some respect. "Mies van der Rohe: Less is more."
Well we probably nickname the community team "too long did not read" for a reason, their job is indeed to write full reports which I myself find enjoying. Tip for the hurry people: only read the titles :-)
Le 29/08/12 23:55, Sumana Harihareswara wrote:
- Write small commits.
I cant stress how important this is. git has several ways to split a commit: - git rebase --interactive <parent commit sha1> - reset to master and git cherry-pick --no-commit <sha1> then use git add --patch to select the hunk to craft a new small commit. --> http://nuclearsquid.com/writings/git-add/
<snip>
- Don't mix rebases with changes.
When rebasing, only rebase. That makes it easier to use the "Old Version History" dropdown, which greatly quickens reviews. If non-rebase changes are made inside a rebase changeset, you have to read through a lot more code to find it and it's non-obvious.
The way to go is usually to git rebase and send that then do the new change. That make reviewing ten times easier.
It is also a good practice to add a cover message on the new patchset to explain what it changes compared to the previous one. For example:
PS3: rebased on latest master PS4: fix the, now obsolete, function() call
Where PS is used instead of "PatchSet".
- Add reviewers.
If you do not know who could review your change, do ask in #mediawiki or #wikimedia-dev. People with a long experience will be able to tell you who might be able to help.
- Review more.
Asking a question about code, commenting about anything (whitespaces, code you do not understand or dont like), doing +1 / -1, is NEVER going to harm anyone. I have seen reviews from newbie that were not understanding some part, and it ended up being a bug. Reading code is a great way to learn the framework, asking question is even better :)
On Aug 30, 2012, at 9:59 AM, Antoine Musso hashar+wmf@free.fr wrote:
It is also a good practice to add a cover message on the new patchset to explain what it changes compared to the previous one.
Yes, very important. If you submit a patch set, please do leave a quick comment explaining what you changed. I personally like to use bullet points for those comments like:
* Rebased
or
* Addressed comments from John. * Removed redundant code in foo.js.
PS3: rebased on latest master PS4: fix the, now obsolete, function() call
Where PS is used instead of "PatchSet".
This is not needed. Because if you leave a comment (and do it right, as in, click "Review" on the main gerrit change page under the correct "Patch set" heading) gerrit prefixes this to your comments automatically.
The only reason to need such a prefix is if you're putting it somewhere else, such as the commit-msg – where such comments don't belong in the first place.
Putting it in the commit message is imho annoying because: * It is not at all helpful when the commit is merged because only the last version is merged. The rest are in-between steps during the review process and discussion and details of that process belong in the review thread, not in the commit message. * And, as the author, it is kinda hard because you don't know the patch version number until it is submitted, and someone else can submit a version while you're working.
Having said that, do make sure that your commit message is still accurate and explains the full change (not just the first version of the patch, nor just the last amendment to the patch).
-- Krinkle
Antoine Musso wrote:
Le 29/08/12 23:55, Sumana Harihareswara wrote:
- Write small commits.
I cant stress how important this is. git has several ways to split a commit:
- git rebase --interactive <parent commit sha1>
- reset to master and git cherry-pick --no-commit <sha1> then use git
add --patch to select the hunk to craft a new small commit. --> http://nuclearsquid.com/writings/git-add/
In a previous wikitech-l thread from April 2012, Tim Starling wrote:
Larger things with more benefits tend to get a higher priority than smaller things. So it's usually quicker to get 1500 lines of code reviewed than 15.
This seems to be a nasty discrepancy.
Of course these are only snippets from two threads, but the underlying idea here seems to be that some reviewers prefer large branches of code that can be reviewed all at once rather than a lot of small commits. And some committers similarly prefer a "touch it once" approach (particularly if there are several bugs related to the same area of code) over a bunch of commits, for a variety of reasons, some of which are related to the annoyance of Gerrit (Git?) dependency linking and others of which are related to varying ideas of what's most efficient/easiest/sanest.
I think some kind of reconciliation is needed here in the advice to committers, new and old. I guess whether to split commits up or not depends on context?
MZMcBride
On 09/04/2012 07:38 PM, MZMcBride wrote:
Antoine Musso wrote:
Le 29/08/12 23:55, Sumana Harihareswara wrote:
- Write small commits.
I cant stress how important this is. git has several ways to split a commit:
- git rebase --interactive <parent commit sha1>
- reset to master and git cherry-pick --no-commit <sha1> then use git
add --patch to select the hunk to craft a new small commit. --> http://nuclearsquid.com/writings/git-add/
In a previous wikitech-l thread from April 2012, Tim Starling wrote:
Larger things with more benefits tend to get a higher priority than smaller things. So it's usually quicker to get 1500 lines of code reviewed than 15.
This seems to be a nasty discrepancy.
...
I think some kind of reconciliation is needed here in the advice to committers, new and old. I guess whether to split commits up or not depends on context?
Or maybe these simply the differences in the sorts of reviews that people like to do? Or maybe its a bit of both?
"If this is a major change you really want Tim to review, then make a single big commit. If you want Krinkle to review your code, don't batch it up into several seemingly unrelated changes in a big blob."
Because Sumana's initial instructions weren't about any particular developer, maybe this is just something she has noticed as a tendency?
Mark.
On 09/04/2012 05:31 PM, Mark A. Hershberger wrote:
On 09/04/2012 07:38 PM, MZMcBride wrote:
I think some kind of reconciliation is needed here in the advice to committers, new and old. I guess whether to split commits up or not depends on context?
Or maybe these simply the differences in the sorts of reviews that people like to do? Or maybe its a bit of both?
"If this is a major change you really want Tim to review, then make a single big commit. If you want Krinkle to review your code, don't batch it up into several seemingly unrelated changes in a big blob."
Because Sumana's initial instructions weren't about any particular developer, maybe this is just something she has noticed as a tendency?
Mark.
That particular suggestion -- writing small commits -- came from Daniel Kinzler, who gave feedback on the draft (see 19:55:45 in http://bots.wmflabs.org/~wm-bot/logs/%23mediawiki/20120828.txt ). I believe I have also heard similar advice from others.
I would love more clarifications from developers to help people decide when to lump commits together into a changeset and when to split things up.
On 9/4/12 5:57 PM, Sumana Harihareswara wrote:
I would love more clarifications from developers to help people decide when to lump commits together into a changeset and when to split things up.
If your commits are going to be touching the same files repeatedly, bundle them up into one large commit (using either --amend or squashing after the fact).
If your commits are going to be touching separate files and there's not a lot of dependency between them, it's probably best to keep them as smaller discreet commits.
Ryan Kaldari
On Tue, Sep 4, 2012 at 5:31 PM, Mark A. Hershberger mah@everybody.org wrote:
Or maybe these simply the differences in the sorts of reviews that people like to do? Or maybe its a bit of both?
I think you're right that stylistic differences are at play.
One possible bit of guidance we can give is "make things as simple as possible, but no simpler". That is, having a large change broken up into smaller bits can sometime mean creating a jigsaw puzzle for the reviewer to piece together prior to reviewing your big change.
Nonetheless, I know that, for example, some reviewers prefer to see things broken up, and others don't. An example might be a big change that requires a new API. Some reviewers don't mind reviewing that API in isolation, whereas other reviewers want to see the calling code. If the API is an obvious and welcome change that can stand on its own, then that's going to be the better strategy. However, if the calling code could have been done differently (and much better) with a different API, then refactoring might be made harder by the fact that there are now people depending on the previously reviewed/committed API.
I think most reviewers agree that several unrelated changes bundled up into the Omnibus MediaWiki Cleanup and Terrorism Prevention Commit of 2012 is a bad thing. And no, those reviewers are not in league with terrorists.
Rob
wikitech-l@lists.wikimedia.org