Thoughts on code review

Development
1

I want to discuss how we should do code review. Ideally, the discussion should result in a set of guidelines we can agree on. This post is not a suggestion for those guidelines; it is merely my thoughts on code review in a more or less structured manner as a start, things to agree or disagree on.

Purpose of code review

There are several reasons for code review. On one hand, it’s a filter against bad stuff. Obviously, we want to avoid shipping broken stuff. We want to catch bugs earlier and not afflict our users with them. Perhaps more importantly, we want to avoid adopting code we’re uncomfortable maintaining. Any given piece of code is a little bit like a pet; it will grow and need care and feeding over many years.

On the other hand, especially with newer contributors, it’s also a potential teaching moment, a conversation with someone who maybe never interacted with our project before.

Stages of code review

When looking at a new PR, I apply review in stages. This goes from the more general to the more specific. Something like this, where we don’t reach the next stage until we’ve cleared the previous ones:

  1. Conceptual. To begin with, is this a feature or change we want? Does it fit into Syncthing? Does it solve a problem we want to solve? Does it do it reasonably, or is it entirely misguided? Problems at this level typically generate a “wait, let’s back off and talk about what you’re trying to do” response without looking at the details of what the code does.
  2. Architectural. This is looking at the code but at a high level. Does it attempt to shoehorn something into the model that should rather be added as a filesystem layer? Is it adding many config options to a versioner when it should instead create a new versioning kind? That sort of thing. If the architecture is problematic, there’s no reason to look at details in the code because it will likely need to be largely rewritten.
  3. Technical. Now, we really look at the code. We read to understand, find bugs, tease out unclear or missing things, etc.
  4. Stylistic. I avoid this type of review unless it is egregious, in which case it should have been an issue already at the architectural step… The code won’t look like I wrote it, and that’s fine; we all do things differently.

In many cases the first steps are implicit and never seen; it’s unlikely that a minor bugfix PR has conceptual problems. However, it’s a mistake to perform a technical review on an architecturally or conceptually unsound change – it wastes everyone’s time and communicates the wrong expectations.

Amount and intensity of review

There is also such a thing as over-review. The effort put into the review should be proportionate to the consequences of getting it wrong. The cost of mistakenly accepting a bug might just be that we need to fix it afterwards – unless it’s a bug that causes data corruption or another catastrophic failure.

The cost of trying to perfect every little detail before merging may be that the merge never happens, and we lose both the contribution and the contributor. Judgement calls need to be made, considering the complexity and risk of the change and the surrounding code.

Rejecting a change

At some point, we may need to say no. This might be at the conceptual review step; it doesn’t matter how nice the code is if we don’t want to do what it does. This is fairly unusual.

Much more common is that a PR gets stuck at the architectural review stage: it’s not good enough to merge, even with the best of intentions, and it’s not moving forward for a long time. Perhaps someone else can step up and adopt the PR, doing the required changes and taking it over the finish line. When that doesn’t happen and it lingers on for months or years, it needs to be closed and removed from consideration.

Getting stuck at technical review is worse. We’ve already implicitly agreed that the change is in principle sound and should be accepted and we’re looking at possibly minor details. At some point the reviewer might consider just doing the final adjustments themselves and merging it. If that doesn’t seem feasible, closing may be the better option.

Timeliness

This is a tough nut to crack. Ideally, code review would be quick & immediate. In practice, it’s both hard and sometimes boring to review changes…

The recipient matters; old, grizzled contributors can take some “abuse” in terms of having their PR reviews linger, while that’s a very bad first impression to give a new contributor who enthusiastically wanders into our project. Prioritising the new and the simple is reasonable.

There are ways of having GitHub systematically assign reviewers, but I’m not sure we’re a large or diverse enough team for that to work out. There are areas of the code that only a single person really understands well (and it’s not always me).

Ganging up

As a reviewer, inviting someone else to review for a second opinion is, of course, fine. Otherwise, I often feel that the fewer cooks the better the soup. For a newer contributor, getting conflicting advice from multiple reviewers will be very confusing and possibly a blocker since they may not be able to judge whose advice to take, and what is just advice as opposed to must-follow directions. The more the contributor and the reviewers know and understand each other the easier it gets, obviously.

6 Likes
2

Thanks for the insights! I’m very much on the “newer contributor” side of the audience here at Syncthing - and although I had some intuition as to how the review process went, this is definitely a post I am bookmarking!

To your point on timeliness, what should I do as a new contributor if I feel that a PR may be stuck in the merge limbo? I’ve been sitting on gui/core: added search bar for logging by DAlba-sudo · Pull Request #9255 · syncthing/syncthing · GitHub for about a week now, but I don’t feel it’s important enough to warrant a post about getting it reviewed (or @-ing someone to look at it).

I’m in no rush to have it merged, nor am I sure that it’s something that the entire maintainer team agrees to have added, but as of the last comment on that thread I was under the impression it was somewhat good to go. I also understand you all have other things going on, which is why I was just going to leave it be until I heard otherwise. Given the nature of open source projects, what’s a generally accepted way to ‘revive’ a PR that may be lost in the thick of things? I don’t want to come across as someone that just wants to get the merge request in and leave, which is why I ask.

Best, DAlba-sudo

3

There is much truth in your analysis. Some points I’d like to add that shift it more toward a case-by-case consideration:

  1. Reviewers often don’t know all the details or even the overall logic how a certain part of the code works. I’ve personally learned a lot about the code while doing review, with it being the first time looking at some parts at all. Some conversation is thus required to reach a common understanding first.

  2. Some changes require extensive testing in different environments. When it has worked with the contributor’s build in one test, it can still fail miserably for the reviewer. Thus the reviewer’s role is shifted toward being a quite involved alpha-tester, asking back questions for the contributor to explain why it might have failed, and uncovering lurking bugs. That’s a very valuable type of cooperation for everyone involved though. It means more work for the contributor and might scare them off, but getting contributions that work only one time and disregard any failure modes is the worst kind, so rightfully rejected at that point. Such cooperative debugging however tends to result in a massive conversation log and makes it hard to follow for any other reviewer. I couldn’t think of a better place to work on that though, than the PR thread on GitHub.

  1. Regarding coding style, I think this is not the whole truth, and we shouldn’t be too strict about separating the review stages. There might be architectural issues still, but I’m also getting distracted from the code by typos, unusual logic constructs, bad naming, etc. Pointing out these issues at an early stage is fine IMHO, for educating the contributor and hopefully reducing similar distractions later on. That’s not bikeshedding to me, but calling out style issues as early as possible. It sucks to get the architectural issues addressed with new code, which then requires the same nit-picking again.

It matters very much how those nit-picks are conveyed though. I tend to just clean up the style issues locally, then copy the result as a suggestion for the contributor to apply it with a simple button click. Another option is for the reviewer to simply push the corrections themselves before merging, but that doesn’t have an educating effect on the contributor. Leading to possibly bad style again on the next PR.

Of course we could simply not be pedantic about this and merge PRs without perfecting style (limiting effort because of little consequences). But there are hidden consequences with this approach. I for one highly value the high standards and clean, readable code we have in most areas, and would hate to see that deteriorating. But if it’s left as an exercise to be carried out after a PR is merged, then who really volunteers to clean it up? And because that results in a follow-up PR, it actually binds review capacity again, but for a much smaller benefit. Thus I find it important to get these issues out of the way right during the original PR review.

Yes, that can be demotivating for the contributor who just wants to see their contribution accepted. It’s a matter of closely watching the reaction to see if it’s still acceptable and taken as positive criticism. When it gets unnerving and tedious, the reviewer should definitely back off and consider applying their desired clean-ups directly without further discussion. That should reduce friction on both sides.

Patience

Yes, immediate reviews would be great, but that can often only happen on a code basis. Valuable reviews also involve testing, thus establishing the proper environment first, and thinking about the corner cases. Given that most reviewers donate their free time, delays are the norm (on non-critical issues / features) and it’s absolutely normal for a PR to drag on over weeks or months. Contributors should be prepared for that and given an appropriate notice about what to expect.

The same applies in the opposite direction of course. A review sometimes does not get timely attention / reaction from the contributor. Patience is mostly not the problem then, but if a PR times out and gets closed automatically, that’s when a previous reviewer should seriously consider adopting it if there’s clearly an improvement and there were only small issues left.

Release timing shouldn’t affect most of this, but in practice I know that it’s annoying to see an RC (or more) being released with one’s pet feature still not merged. Or as a reviewer, worrying about whether it’s the right time to merge something, because we’re in RC phase and I don’t want to be the one “messing up” the Git history because an RC bugfix pops up and requires branching because the feature was already merged. Clearly separating a “current testing” branch from the “approved for the next RC” material would help there, maybe at the expense of a strictly linear history. This all gets complicated by the translation workflow btw, because the RC period should still allow translations for added features, until short before the final release. But translations happen on main, therefore might include already merged “for-next” features.

That’s a whole lot of timing constraints and thoughts on why something could be taking longer than some party wishes. Thus the more both sides know about these intricacies, the easier it becomes to be patient while waiting for the other side to move along.

2 Likes
4

I think when something has been stalled for about a week with no clear next action, it’s fair to ask for an update. You may not get one, immediately, but at least it keeps the ball rolling.

Yeah, I guess I disagree on this point. If the code is going to get thrown out and rewritten I see no reason to point out typos or naming issues. We’ll get to those later, and pointing them out may give the impression that the code is fine apart from those issues.

However, to be clear, that does not mean letting it slide indefinitely. I just think it’s better to get the big picture right first, and then say something like “awesome, we’re pretty much there, now just these nits: [all the nits]”.

This is not at all what I’m suggesting. :slight_smile: If it “needs cleanup” it’s not a style issue. A style issue is something that is fine either way and it just comes down to personal preference. For example, here, I’m pretty sure I wouldn’t have defined those one-line functions for comparisons and instead just added a comment where the comparison happened if I felt it needed clarification. Eric apparently felt differently. Neither way is wrong or requires cleanup, and I’m fine with the change doing it the way I wouldn’t have; there’s no need to problematize it.

Agreed. This is the nature of things. I usually try to gather up any small or easy PRs lingering before RC time and get them in if possible, but sometimes that’s not realistic. If it’s not ready it’s not ready.

In any case, I don’t want the opposite (hesitating a merge due to release timing) to ever be a concern.

4 Likes
5

This post could/should be a blog post :slight_smile:
It’s great and exemplifies/reminds me how much I learned from this project and of course especial you and Audrius, and still do. Pretty random and off topic start, just felt the urge to say it here - thank you!

So back on topic:
Somewhat related to “Ganging up” we might need to talk about what it takes for a review to be done resp. a PR being ready to be merged. I feel like some confusion comes from long(er)-timers being very involved and doing reviews (just in case: which is great of course) and then there still might be the question: Does a maintainer need to green-light on top? I feel like wherever significant “conceptual”/stage-1 review is needed that’s the case, and often “architecutral”/stage-2 as well. Less so for straight forward, small changes where only technical review is required. That’s mostly because of the “puppy” aspect - maintainers have some more skin in the game and thus will need to carry the puppy, so wanna make sure they really want to adopt it.
However that distinction is often not clearcut. And even if, a reviewer might still not feel comfortable merging on their own. I think both happens however isn’t called out explicitly. Maybe we should have some (soft) process that if someone wants a maintainer to have a look, that should be stated explicitly tagging maintainers. And vice-versa if a bunch of people are already engaging in reviews, but someone has stage 1-2 concerns, that’s expressed early to avoid too much review churn that then might become void.

Regarding doing technical/nit-picky review at the same time as conceptual/architectural: I am guilty of that to a large degree, and have come to regret it many times. It happens naturally as I am trying to understand code well enough for stage 1-2 feedback. I still wont do it anymore (or well hope I’ll always remember not to). It feels extremely bad when stage 1-2 tend more and more towards a “no”, and I already used up a lot of the reviewers time. Saying no is never nice and easy, I am really terrible at it, even more so like this.

3 Likes