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

Proposal: use tooling that encourages reviewers #22120

Closed
mattfarina opened this Issue Oct 3, 2017 · 18 comments

Comments

Projects
None yet
@mattfarina
Member

mattfarina commented Oct 3, 2017

First off, I'd like to start by saying I appreciate the work the Go team does. Working with the community while developing a programming language, compiler, and toolchain is no small task. And, everyone has opinions which can be difficult to navigate.

Motivation
In a recent proposal on switching to GitHub it was noted by @ianlancetaylor that:

Even with our current contribution process, there are many many more contributors than there are reviewers.

I can understand how scaling qualified reviewers can be difficult. Many open source projects deal with the same problem. All to often they write their own custom tools to support them along the way. I currently work on Kubernetes which is struggling with scaling out people while writing custom tools to work around GitHub shortcomings.

But, I can appreciate how tool choice can have an impact on reviewers. In a post on lwn, Jonathan Corbet noted:

Gerrit, he said, makes patch submission quite hard; Repo helps a bit in that regard, but not many projects use it. Gerrit can be scripted, but few people do that. An audience member jumped in to say that using Gerrit was like doing one's taxes every time one submits a patch. The review interface makes it clear that the Gerrit developers do not actually spend time reviewing code; he pointed in particular at the need to separately click through to view every file that a patch touches. It is hard to do local testing of patches in Gerrit, and tracking a patch series is impossible. All discussions are done through a web interface. Nobody, Greg said, will do reviews in Gerrit unless it's part of their job.

Having used Gerrit quite a bit on OpenStack I appreciate this problem for reviewers.

Back in 2015 I started to investigate bettering the Gerrit usage situation. For example, here's a twitter conversation with Emma Jane (author of the book Git for Teams). I note this conversation because it's public.

For most developers and reviewers, Gerrit is not an ideal tool. It's not a tool of choice or one they enjoy using. As was noted...

Nobody, Greg said, will do reviews in Gerrit unless it's part of their job.

This may not be a universal truth but it appears to be the rule with some who are exceptions to it.

Proposal
To evaluate and use tooling that encourages more reviewers.

This proposal is open ended at the moment without a suggestion for a tool because it's worth knowing if there is acceptance of going down this path prior to putting in the effort to do so. If the Go team is not interested than the proposal need not progress further. If the Go team is honestly interested then criteria, user stories, and other details can be collected along with tools and processes being evaluated against them.

Is there interest in tooling to increase the number of reviewers?

@gopherbot gopherbot added this to the Proposal milestone Oct 3, 2017

@gopherbot gopherbot added the Proposal label Oct 3, 2017

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 3, 2017

I would also like to see better review tooling. But Gerrit, at least as used by the Go team, is better than what you are describing. You quote Jonathan Corbet quoting an unnamed person:

Gerrit, he said, makes patch submission quite hard; Repo helps a bit in that regard, but not many projects use it. Gerrit can be scripted, but few people do that. An audience member jumped in to say that using Gerrit was like doing one's taxes every time one submits a patch. The review interface makes it clear that the Gerrit developers do not actually spend time reviewing code; he pointed in particular at the need to separately click through to view every file that a patch touches. It is hard to do local testing of patches in Gerrit, and tracking a patch series is impossible. All discussions are done through a web interface. Nobody, Greg said, will do reviews in Gerrit unless it's part of their job.

The Go team has scripted Gerrit using the git-codereview tool (https://godoc.org/golang.org/x/review/git-codereview).

While Gerrit does show each file separately, you can move between files easily using the [ and ] keyboard shortcuts. No clicking is required. I agree that it would occasionally be nicer to have a view for looking at all the files at once; when I want that, which for me is only occasoinally, I look at the complete patch as Gerrit permits, but I agree that that interface could be improved.

I'm not sure what local testing of patches means, but the Go trybots are fast and effective.

Tracking a patch series is the opposite of impossible: it is easy. See "Multiple-Commit Work Branches" at https://godoc.org/golang.org/x/review/git-codereview.

While it is true that most discussion in Gerrit are through a web interface, it does in fact support e-mail.

Nobody, Greg said, will do reviews in Gerrit unless it's part of their job.

I can not disagree more. I've done reviews in e-mail and Github, and I strongly prefer Gerrit.

@mattfarina

This comment has been minimized.

Member

mattfarina commented Oct 3, 2017

@ianlancetaylor I'd like to provide a few clarifications...

You quote Jonathan Corbet quoting an unnamed person

I'm sorry I didn't make it more clear. At the start of the linked article he notes the person is Greg Kroah-Hartman who, among other things, is the "the current Linux kernel maintainer for the -stable branch" (from Wikipedia).

use tooling that encourages reviewers

This is my proposal title on purpose. I didn't suggest moving away from Gerrit. And, you note that there are some tools on top of Gerrit that the Go team has built to help improve the process. That's great and a start.

But, I will posit that the current setup doesn't encourage new reviewers who aren't being paid to do it. Unless you are not looking for more reviewers, wouldn't it be a good idea to look at tooling that encourages reviewers? Including new ones.

I've done reviews in e-mail and Github, and I strongly prefer Gerrit.

When I used Gerrit I became comfortable with it. Like Go, the OpenStack team also had tooling to make working with Gerrit easier which I appreciated.

But, you and I are not typical people. We are both in the "tribe" when it comes to using Gerrit. As Emma Jane noted, "people haven’t begged for info about it." In fact, finding good details on navigating Gerrit is hard. Even the contribution guide is focused on contributing and not reviewing. The contribution guide is long (rather than intuitive to new folks).

To get into reviewing you need to figure things out, much of which is not documented well and is not intuitive, or more likely know people in the "tribe" who can teach you.

Do you want to "use tooling that encourages reviewers" or to use tooling that the current reviewer team is comfortable with? Even if the current tooling is a turn off is getting new reviewers.

Please note, I'm not talking about what I prefer or would prefer. I'm approaching this as a people problem and figuring out how to encourage more reviewers and lowering that bar to entry and repeat performance with a bit of joy.

@randall77

This comment has been minimized.

Contributor

randall77 commented Oct 3, 2017

I think the barrier to entry for reviewers isn't the tool they use. It's the need to understand the codebase that the change is modifying. Making changes to the review tool for reviewers, in order to bring in more reviewers, seems like just tinkering around the edges.

Just about every Go reviewer started out as a contributor. I think the contributor experience is where any effort should be spent making the process better (e.g. issue #18517). Having more contributors is by far the best way to have lots of people understand the codebase, which is the best way to expand the reviewer pool.

@ALTree

This comment has been minimized.

Member

ALTree commented Oct 3, 2017

I don't believe that the 'outsiders' you'll (maybe) please by switching away from gerrit will be able to contribute to the review process in a meaningful way .

You can (roughly) partition present or potential contributors in three groups:

  • go team members
  • long time contributors from the open source community
  • everyone else

The people in the first group do 90% of the review work. This is expected, because they are the 'code owners', the people that have the last word about a proposed change, and the most knowledge. Those people seem to like gerrit.

The people in the second group are the ones you should try to please if you want to increase the number of reviewers. I rarely see people in this group help with reviews. There are (almost) no 'code-owners' there, but they can still make meaningful contributions to the review process. I suspect that most people in this group actually like gerrit. Well, we could ask them. I'm in this group, and I like gerrit.

People in the third group are the ones that we can hope to board as contributors (i.e. people who report bugs and send patches), but they do not have the knowledge necessary to contribute to the review process, and I believe it would be a mistake to try to cater to them. And when someone from this group moves to the second, it's more than likely that she'll understand gerrit enough to do reviews.

@dsnet

This comment has been minimized.

Member

dsnet commented Oct 3, 2017

But, you and I are not typical people. We are both in the "tribe" when it comes to using Gerrit.

I do work for Google (now), but my background is that I started contributing to Go as just some random contributor and I favor Gerrit. I still have to use GitHub PR's on a regular basis, and it makes me sad. Sure, I wasn't begging to use Gerrit, but when I starting using it more, I saw how it was a superior tool to GitHub.

@davecheney

This comment has been minimized.

Contributor

davecheney commented Oct 3, 2017

As someone who uses both Github and Gerrit on a daily basis there is no question in my mind that the latter is a superior tool for code review,

@ericlagergren

This comment has been minimized.

Contributor

ericlagergren commented Oct 3, 2017

@ALTree, wrt

I rarely see people in this group help with reviews.

As somebody who's contributed a handful of times, I can't even say I know the proper process to help review code. I think the docs for the entire lifetime of a contribution to Go could use some TLC to make them simpler.

@jimmyfrasche

This comment has been minimized.

Member

jimmyfrasche commented Oct 3, 2017

@ianlancetaylor

While it is true that most discussion in Gerrit are through a web interface, it does in fact support e-mail.

The contribution guidelines say that it does not. Are they simply out of date or are you referring to something different than the guidelines? Specifically it says:

You will receive a mail notification when this happens. You must reply through the web interface. (Unlike with the old Rietveld review system, replying by mail has no effect.)

and

You must respond to review comments through the web interface. (Unlike with the old Rietveld review system, responding by mail has no effect.)

(Regardless the mention of Rietveld seems past its expiration date at this point).


I've heard enough people whose opinions that I trust say that Gerrit is superior to be willing to believe it, though I am not yet familiar enough with it to form that opinion for myself.

Even if optimizing for contributors is the goal, contributors—at least currently—need to use most of the same features of gerrit as reviewers so documenting common tasks and workflows and increasing automation seem like good ideas regardless.

I'm also not sure that more casual reviewers would be a bad thing. If someone comes along before the code owner and points out some typos or possible simplifications to the code, etc., then, sure, there's still all the important stuff to work out but all the misc. simple stuff has been cleared out of the way so that can be focused on. Personally, I lurked in reviews for quite a while and pointed out some incorrect docs on a CL before even attempting to contribute to get an idea of the process and how to use gerrit at just the most basic level. Basic gardening in code reviews can be a gateway to contribution.

As far as updating and simplifying the contributing guidelines I filed #22007 and #21983 and any help or comments are welcome.

@davecheney

This comment has been minimized.

Contributor

davecheney commented Oct 3, 2017

The contribution guidelines say that it does not. Are they simply out of date or are you referring to something different than the guidelines? Specifically it says:

@andybons ping, this was an action item from the contributors summit in July,

@gopherbot

This comment has been minimized.

gopherbot commented Oct 4, 2017

Change https://golang.org/cl/68021 mentions this issue: doc: update contribute.html to clarify replying to reviews via email

@andybons

This comment has been minimized.

Member

andybons commented Oct 4, 2017

The contribution guidelines are out of date. You reply to emails sent by Gerrit and the response is reflected in the review. More info here: https://gerrit-review.googlesource.com/Documentation/intro-user.html#reply-by-email

@andybons

This comment has been minimized.

Member

andybons commented Oct 4, 2017

While Gerrit does show each file separately, you can move between files easily using the [ and ] keyboard shortcuts. No clicking is required. I agree that it would occasionally be nicer to have a view for looking at all the files at once; when I want that, which for me is only occasoinally, I look at the complete patch as Gerrit permits, but I agree that that interface could be improved.

There is a setting to toggle inline diffs in PolyGerrit by default so that you can see them all on one page:
screen shot 2017-10-04 at 12 09 58 am

...or you can click the triangle to expand them individually.

In fact, I would encourage everyone to look around in settings to see if there’s anything that would make your workflow a little better. There are a few good ones in there.

Both the Twitter conversation and post on lwn predate the broader launch of the PolyGerrit UI that the Go team now uses by default. This project has been a multi-year effort to address the usability problems and feature desires of the many developers using Gerrit and will usurp the old UI once it reaches feature parity.

This is all being done in the open (bugs and source). So while I completely agree that Go could be doing a better job of making things more approachable and clear, deficiencies with Gerrit itself should be reported to them as they are much better equipped to handle those.

gopherbot pushed a commit that referenced this issue Oct 4, 2017

doc: update contribute.html to clarify replying to reviews via email
Update #22120

Change-Id: Ie7dbd0e7b01b116c960243a8cd3fa9fd121f317d
Reviewed-on: https://go-review.googlesource.com/68021
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Dave Cheney <dave@cheney.net>
@mattfarina

This comment has been minimized.

Member

mattfarina commented Oct 4, 2017

A few quick comments...

  • I didn't say switch to GitHub
  • I didn't say switch away from Gerrit
  • I did note that Gerrit is unfamiliar to many and many who learn it do not like it
  • I did note that the current review experience doesn't encourage new reviewers. A lack of reviewers compared to contributors was noted elsewhere

Instead of debating between GitHub and Gerrit (a common conversation which I'm guilty of falling into the trap of sometimes), is there interest is looking at toolchains to improve the reviewer experience with a goal of bringing in more reviewers?

@andybons

This comment has been minimized.

Member

andybons commented Oct 4, 2017

is there interest is looking at toolchains to improve the reviewer experience with a goal of bringing in more reviewers?

Sure thing! If you'd like to present ways to improve the reviewer experience so that more reviewers will contribute, we are all ears.

@mattfarina

This comment has been minimized.

Member

mattfarina commented Oct 4, 2017

@andybons The impressions that I get from this thread is "you can drag my current workflow from my cold dead hands" and "Gerrit is better than X". I'm left with the feeling that suggests are welcome and will them be met with same attitude. Why will someone feel it's really worth their time?

@andybons

This comment has been minimized.

Member

andybons commented Oct 4, 2017

Because cogent, detailed proposals that outline plans to encourage more reviewers are a good thing.

From reading the above thread again, it doesn't seem like there are any concrete proposals on how you wish to encourage more reviewers; only that you would like to investigate possible solutions.

Yes. Absolutely feel free to investigate and present possible solutions to the problems you outline. We will treat them as we would any other proposal, so I encourage you to go for it.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Oct 4, 2017

@mattfarina I'm sorry if we seem to be rebuffing your suggestion. I don't see anybody on this thread saying that Gerrit is perfect. It's not. It's a very long way from perfect. We're still recovering from #21956 and that is probably coloring our responses somewhat.

I guess I don't know how to evaluate your proposal. Everything is a trade off. Switching away from Gerrit carries some real costs. Whatever we switch to would have to be clearly and significantly better. You seem to be asking whether the Go team is interested in switching. My answer: the Go team would love to switch to something that is clearly and significantly better. The Go team is not interested in switching to something that approximately the same.

It seems to me that the simpler approach is improving Gerrit. Gerrit has completely revamped their UI, and in general they seem to be open to bug reports and suggestions. But I'm not wedded to Gerrit. I just think that there is a reasonably high bar to changing.

You are positioning this as tooling that will bring in more reviewers. Like @randall77, I don't think the tooling is the barrier. Of course we should make the tooling better if we can. But I don't see that as likely to change the set of reviewers one way or another.

@rsc

This comment has been minimized.

Contributor

rsc commented Oct 16, 2017

The proposal process is for concrete, specific suggestions that we can evaluate. It sounds like you want to have a discussion. One of the mailing lists would be a better forum for a discussion.

As Ian said, the Go team is not interested in switching to something that is approximately the same, so I am honestly skeptical about the framing here. You'd do better by focusing on what pain points are and how we might get them improved. There's a TON about Gerrit that works very very very well for us, and it's doubtful any other systems are as well molded to our processes at this point.

But again, this is not a proposal, so closing.

@rsc rsc closed this Oct 16, 2017

@golang golang locked and limited conversation to collaborators Oct 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.