Hacker News Reader

I am yet to know people who Review code and use Gerrit to name a better solution.

I belonged to a team that used Gerrit for Review and Hosting, we changed to hosted Gitlab because people missed a "GitHub-like UI" they were used to. It was unanimous that Code Review on Gerrit was way better:

1. You start reviewing the commit message, that is the first touch point with a change everyone has 2. Navigation is done from file to file 3. On Gerrit there isn't two people commenting the same thing, because: 3.a. Messages from different people on the same line change are displayed together, not as different threads. 3.b. The review of a previous version is displayed with the next version, so you can continue the same discussion

I understand that GitHub/GitLab interface is more friendly, but their code-review really stands in the way of producing good software by not favoring good commit messages and long discussions.

> I am yet to know people who Review code and use Gerrit to name a better solution.

What about reviews via patches sent to a mailing list?

I haven't looked into Gerrit for a while, so one question I have is how it handles related commits? The mailing list approach can group them in a single thread tied by a cover letter message where each commit along with the associated diff from the parent working tree is a message which is a reply to the cover letter message.

To be polite, I think the target audience of these tools might not include you. While that workflow works for you and, apparently, scales for some very large projects like the Linux kernel, it isn't a good solution for an enormous number of people which is why tools like Github, Gerrit, GitLab, and others exist.

> To be polite, I think the target audience of tools might not include you.

Insinuating that I'm a special case doesn't add to the discussion.

You aren't a special case, but if you don't see any flaws with your method of code review then I don't think you're the target market for code review tools.

I have no dog in this fight, but you stated:

> it isn't a good solution for an enormous number of people

.. without providing any insight or justification for that belief. So an interesting and productive tangent might be to elucidate what you believe those flaws are?

Unpacking this question might even lead to some good UX ideas that can be applied to today's review systems?

I would provide one from someone who is not a dev or SE.

I honestly never fully understand how to read the maillist patch. The plain text format makes it very hard to understand what's going on. I'm sure I will understand it better or even prefer it if I use it long enough, but then again I can instantly understand code reviews on GitHub/GitLab/Gerrit.

I've never had to use a maillist patch myself, but from the ones that I've glanced at, I have this same problem, that formatting, syntax highlighting is absent. It seems like this would be an easy problem to fix by using a better viewer, so that might be enough to make it comprehensible.

> I honestly never fully understand how to read the maillist patch. The plain text format makes it very hard to understand what's going on.

That may be due to the settings in your mail client. If it's displaying plain text in a variable width font and/or not applying syntax highlighting in terms of showing added and removed lines in different colors, that would make the diff more difficult to read.

But some mail clients can do that and it makes reading the diff much easier.

> I'm sure I will understand it better or even prefer it if I use it long enough, but then again I can instantly understand code reviews on GitHub/GitLab/Gerrit.

Essentially, code reviews in a mailing list are much like a discussion thread in Hacker News or Reddit where the thread structure is very similar. The only difference is that most mail clients only allow you to display one message at a time.

In my mail client, Thunderbird, you can see the overall thread structure of a patchset discussion. This is the root message for the thread which serves as the cover letter (which is the equivalent of the PR description) [2]. The first commit in the patch is displayed here [3] (note that I have a plugin that enables diff syntax highlighting). The email subject is the commit message title (with the [PATCH v2 1/3] tag prepended to it). The commit message itself is the beginning of the email, and the diff follows.

Unlike Github (and maybe Gitlab), the commit message and diffstat is treated at the same level as the diff itself. That means you can comment on it just like you would on the diff.

Here, you can see the Junio C Hamano's comments on the second commit in the patch set [4]. He's commenting on the diffstat line which shows 391 lines lines added to the builtin/submodule--helper.c file. Further down in the same message [5], he's commenting on the code inline, much like someone would quote a message here on HN and reply inline to multiple sections of it. It's not really that different compared to comments on a diff in Github or Gitlab other than the fact that it's a reply to an email message rather than a web page.

[1] https://i.imgur.com/QmqUWR8.png

[2] https://i.imgur.com/mILREtf.png

[3] https://i.imgur.com/gdoy5zs.png

[4] https://i.imgur.com/BcTdRRe.png

[5] https://i.imgur.com/cCpqsOL.png

I will be honest, I don't even use email client :/

True, I suppose most people use Gmail or one of the other major email providers through a webmail interface. I haven't been able to get Gmail or Hotmail to display threaded messages the way they're displayed in Thunderbird and they tend to display messages using a variable width font.

In that context, reviewing code would be difficult, if not impossible, to do via email.

It is one advantage to mailpatch though. A lack of vendor lock-in means you can view the patch using whatever application you want. And there's room for a better mailpatch viewer, if anyone could be bothered to make one.

I'm guessing one disadvantage is the method of diff is hard-coded into the patch. It would be good to switch to word-diff, or ignore whitespace, but I'd imagine these could be applied as transformations on the generic format.

> I'm guessing one disadvantage is the method of diff is hard-coded into the patch. It would be good to switch to word-diff, or ignore whitespace, but I'd imagine these could be applied as transformations on the generic format.

The plugin I use in Thunderbird can switch between unified, context, and side-by-side diff views based on the same email. Adding the transformations you mentioned could be done.

But one limitation that email has over other review tools is the lack of ability to expand the view of the context within the client. The only way I could think of is to have git format-patch generate the diff with the entire context included and then have the client limit the display of that context. But that not have a reasonable fallback for those using clients that aren't capable or configured to do that.

Which plugin are you using? it seems life-changing to me

This is the one I'm using: https://github.com/Qeole/colorediffs. I installed it several years ago, so I'm not entirely sure whether you can install it on a current version of Thunderbird, but it is still working with my installation.

I might try it out as well! Thanks for it.

That should be an easy problem to fix:

Just send out email in HTML format with the code portions set to fixed width font and syntax highlighting already applied. Should display just fine in GMail.

> Just send out email in HTML format with the code portions set to fixed width font and syntax highlighting already applied

That won't work because people actually download those email messages and apply them to their local repository with the git am command and they also expect to be able to reply to the email inline when commenting on a patch. Also, for mail clients that don't support HTML rendering, it would be much more difficult to read or respond to an HTML email.

I would assume you'd sent both a text message and an HTML message?

Now you're claiming that I'm making an argument that I never made.

To be clear, the original statement was whether there was a better review tool compared to Gerrit for those who review code and use that tool for that purpose. I responded by suggesting the patch review via mailing list method and asked a follow up question about how Gerrit handled related commits and explained how that case was handled by the mailing list method.

Then you, not the person I originally responded to, decided to interject and, while claiming not to be rude, claims I'm saying something entirely different than what I actually stated.

Personally, I found that very off putting and extremely rude on your part.

I'm sorry I was rude to you, I didn't mean that and I apologize.

I didn't mean anything more than what I literally wrote, which is I don't think you're the target market for code review tools and your suggestion may not be a good fit for people looking for review tools.

Gerrit handles related commits in a similar way I guess.

A Gerrit changeset is like a GitHub pr, it has many commits, and commits are usually rebased against master, this is crucial to track the review properly over time. (Its very easy to switch to earlier revisions of the same changeset and you never see external changes.)

Honestly, I don't think Gerrit is anything special, I think it simply has the right approach to development (rebased/ff-only) that enables easy review.

Based on the documentation I read, it looks like Gerrit handles this by grouping changes by topics [1]. I'm not sure whether that can be done automatically when pushing up changes that span multiple commits in a local branch.

If I create a branch with several commits where one commit adds a new method with associated unit tests, and a subsequent commit adds several calls to that new method in the code base (while updating any affected tests), then how would Gerrit handle the ordering of those commits. Even if they're in the same topic, I don't know if there's a way to ensure that the first commit is reachable from the subsequent commit.

[1] https://gerrit-review.googlesource.com/Documentation/intro-u...

I'm a Product Designer at GitLab and I appreciate your feedback.

We are well aware of the advantages that Gerrit has over GitLab and how these are emphasized when teams migrate to GitLab. We are working on improvements that will help mitigate this. Specifically, targeting your points:

1. We're discussing ability to comment on commit messages: https://gitlab.com/gitlab-org/gitlab/-/issues/19691 2. From version 13.2 you can opt to show one file at a time, in your user preferences: https://gitlab.com/gitlab-org/gitlab/-/issues/222790. We have also listed a number of improvements to this feature in https://gitlab.com/groups/gitlab-org/-/epics/516.

Could you expand on point 3.b? If the commented line hasn't changed from version A to B, you should be seeing the comment in version B. Or maybe you're referring to something else?

Gerrit is the one where each "pull request" has to be a single commit, right?

I'm not particularly happy about GitHub, but I think it's less about GH and more about workflow and source code evolution, and I'm not sure if Gerrit solves anything here.

First, PRs being heavyweight encourages large branches instead of smaller incremental changes. Secondly, a large change (such as a big new feature) ends up living in a branch until it's ready to be released, not until it's reviewed. This means that any code that cannot be immediately merged to master need to live on that branch for a while, and gets rapidly out of date, requiring constant rebasing to keep it from rotting.

I'd prefer to merge as soon as something is accepted and use master at the main development branch, but that causes challenges. If you've merged something you don't want to release yet, you're faced with having to build release branches through cherry-picking, which can be really difficult or even impossible. You can hide features behind flags, but sometimes a branch is a big risky refactoring or some structural change that isn't providing features that can be isolated. Plus, once you merge something, any following changes, even if unrelated, often end up depending on the things you want to exclude.

I think something like Pijul (with some discipline, like always doing small incremental commits) could make this easier by being able to treat individual commits as moving pieces that can be rearranged for a release, but it wouldn't solve everything.

Any thoughts on this and how Gerrit would fit in?

> Gerrit is the one where each "pull request" has to be a single commit, right?

Yes, but you can have changesets depend on each other, so its not that big a deal. (But you ca end up in rebase hell if you do that). You also get a version history of all the different versions of your commit.

Ancedotally, during my use of gerrit, i never really wished to have multiple commits on a single changeset.

> I'd prefer to merge as soon as something is accepted and use master at the main development branch

That's what Wikimedia did, mostly. (There were weekly deployment branches but it was unusual to have something in master but reverted out of the deploy branch). It seemed to mostly work fine afaik (of course i wasnt on the team doing deploys, for all i know they might have horror stories)

> Ancedotally, during my use of gerrit, i never really wished to have multiple commits on a single changeset.

People's tools shape their workflows; projects that use Gerrit tend to do more squashing of commits, because there's much more per-commit overhead. When I encountered Gerrit, I found it really frustrating to work with for this exact reason. Other aspects of it were great, but if you're used to "one logical change per commit" and end up with a dozen commits in a PR, that can be painful with Gerrit.

The whole point of Gerrit is to keep doing "one logical change per commit", but reviewing them individually. Anecdotally, this results in much higher-quality reviews.

You can still group them by "feature" and merge them atomically by using topics.

> # Why

> For the past two years, our developer satisfaction survey has shown that there is some level of dissatisfaction with Gerrit, our code review system. This dissatisfaction is particularly evident for our volunteer communities. The evident dissatisfaction with code review, coupled with an internal review of our CI tooling and practice makes this an opportune moment to revisit our code review choices.

and then further down

> # FAQ

> * Why is GitHub not considered?

> - GitHub would be the first tool required to participate in the Wikimedia technical community that would be non Free Software and non self-hosted.

> - GitHub also does not meet all of our needs; for example, GitHub grants little control of metadata, no influence over privacy policy/data retention, sanctions and bans, little control over backups and data integrity checks, and no long-term guaranteed access to underlying repository settings and configuration.

Wikimedia was already using mirroring to github (however we didn't accept pull requests).

I'm pretty sure most of the anti-gerrit sentiment at wikimedia was about gerrit as a code review tool.

My personal experience with it (as a mediawiki developer) is gerrit has a lot of UI bugs (although it has gotten better). I also suspect it encourages a code review culture that is overly nitpicky and risk averse (but perhaps that is just cultural forces at wikimedia)

I'm not familiar with any UI bugs of any kind, but we don't use the "live editing" feature, maybe that's where you had problems. The big issue with Gerrit is on the "getting plugins to work" side of things, as they are kind of ad-hoc and almost totally undocumented, as well as the access model is too complicated but once that's all working, there is no need to deal with it.

nitpicky culture, we maybe have that problem with Openstack where there are thousands of developers, but for our projects in SQLAlchemy we're a team of about five people and I'm more or less a BDFL type of role, to the degree that we are nitpicky about things it only prevents much bigger problems from happening later, if a review has little things that are bugging me I'll just fix them myself and push a new change up rather than bothering them with it, also something you can't usually do with pull requests.

Wikimedia was using a super old version of gerrit for a long time. They upgraded recently (although the upgrade happened at roughly the same time as i left my job and took a step back from wikimedia so i dont have much experience with the new version).

I think Wikimedia struggles a lot with code review culture in general. Different people have conflicting ideas about what good code looks like. It used to be very nitpicky (i've had code rejected in the past for using (php's) intval() instead of casting to int. I've also had code rejected for casting to int instead intval().) But that's improved quite a bit with better precommit lint tools. The length of the feedback cycle is very long and sometimes feels like its mostly about who you know (e.g. the last patch i submitted i did on sept 9 for a decently serious bug. First actionable feedback (which was relatively minor things of the form use a constant named ONE_MINUTE instead of 60) was on oct 15. Thats kind of a long time to wait for code review imo). Anyways, its just not fun to contribute when code review is so unpredictable and long.

Hmm. Guess i got off on a bit of a tagent there. I do think gerrit has some usability issues, but i think that's hardly the main problem.

those sound like managerial / organizational / social issues. technology isn't going to solve those without good guidance and controls for the overall system. building that up for a very large organization is extremely difficult, I'd not want to have to do that :).

> My personal experience with it (as a mediawiki developer) is gerrit has a lot of UI bugs (although it has gotten better). I also suspect it encourages a code review culture that is overly nitpicky and risk averse (but perhaps that is just cultural forces at wikimedia)

Can strongly recommend to remove "-1 code review" and require all comments to be resolved instead. Accomplishes the same goal why being more positive.

I wonder how gerrit compares to reviewboard (https://www.reviewboard.org/)