Otto Kekäläinen <o...@debian.org> writes: > It is not expensive to do reviews.
I realize that it's a little difficult to keep all of the various people straight in these threads, and there are a lot of people weighing in with a wide variety of backgrounds. Some of them have said that they haven't used a merge request workflow or done a lot of code reviews, and I think you may be assuming that everyone disagrees with you is in that category. However, some of us have been doing code reviews for many years and have significant experience with merge-based workflows, with and without code review. We probably have informed opinions! And my informed opinion is that a simple, sweeping statement like the above is an exaggeration. It is *sometimes* not expensive to do reviews. Sometimes it is quite expensive. One of the places where it can be the most expensive is when waiting for review breaks a development flow and pushes the conclusion of a piece of work outside of the window in which one has to do it, which often results in the work being dropped entirely. I have been on both sides of this: I have been the person who has abandoned work because having to sequence it around code reviews makes it not worth the effort, and I have been (far too often) the person who has killed work because I didn't review it in a timely fashion because, you guessed it, the review was expensive for one reason or another. Another failure mode where review can be expensive in a different way is when code reviews are required for every change, which means that people spend a substantial amount of time doing code reviews, which then start to suffer from inattentional blindness. Now you're still paying most of the overhead of code reviews but the benefits are degrading. People are just passing their eyes over the change without really seeing it, so that they can get their code review chores done. One of the ways to avoid that problem is to *not* do code reviews for routine and uninteresting changes by experienced developers (which of course includes commit discipline so that mechanical changes are separated from semantic changes) to reduce code review volume and to make code reviews more interesting and flagged as requiring more concentrated attention. It's great that you like code reviews and merge requests and are very excited about this tool. But you are talking about them to other people in a way that reminds me of coworkers who just discovered test-driven development, or agile, or CI. These are all very useful tools, but like every other tool, they're situational. There are good ways to use them and bad ways to use them, and there are times when they're helpful and times when they're more of a religious ritual than something that improves the result. Figuring out the difference requires a lot of expertise, and it can be helpful for more junior developers to follow rules a bit blindly as they build up that expertise, but there comes a time when you look at the tool and think "this is not helping me here and it has a cost in this specific situation." This is part of what it means to have expertise. It's part of what's *fun* about having expertise: you can craft your working environment to maximize not just your productivity but the amount of enjoyment you can get from the work, and you adjust it dynamically based on the situation, such as available time and resources. I do at least somewhat disagree with the folks in this thread who are saying that merge requests are not a useful investment of their time. I think there are a lot of benefits to merge request workflows, and I wish that folks who have not used them much would give them a try with an open mind. But I realize that, on a volunteer project, harping on something often *increases* the resistance to trying it out rather than decreases it. One of the tricky things about free software is that a lot of people work on free software *precisely* so that they can do their work the way they want and not have to follow the demands of an employer, and while that too can be taken too far and break collaboration, it's important for volunteer enthusiasm and motivation to try to preserve it as much as possible. > You have authored any MRs since October 2023, so it is understandable > that you probably haven't developed a routine on doing lots of them, and > efficiently. I'm glad that you're trying to tailor your message to the person that you're talking to, but I suggest that you go check GitHub to get a better picture of my experience with using merge-based workflows. My account name is rra there as well. That will still give you a very incomplete picture since a lot of my past experience with code review was in an internal Phabricator instance that you wouldn't have access to, and before that in various other places such as a Gerrit instance that probably doesn't exist any more, but it will give you a more complete picture. > Maybe the conclusion from this and another email today is that people > feel that collaborating via a MR and doing reviews is "expensive" and > I should prove that it is actually quite quick and handy when > following a clear workflow, and what that ideal workflow could be. I am familiar with what is and isn't expensive about code reviews, not only in the tools but also for my personal psychology and work style. But sure, not everyone in this discussion may be familiar, so maybe that would be useful. My guess, though, is that most of your audience here are fairly experienced developers, who are not that interested in being told what workflow they should be using and are more interested in how to gradually add specific tools in a way that doesn't badly increase the level of friction involved in their current work. That advice will probably need to be at least somewhat tailored to how they're currently working and their preferences, as opposed to how you wish they were working or how you prefer to work yourself. Anyway, the tl;dr is that I have worked on teams before where code review was required for every change, and that workflow was both annoying and resulted in lower-quality code reviews when they actually mattered. I'm not likely to use it again without a really good justification for why a specific project needs that level of overhead and friction. This does not mean that I dislike code review! I both ask for and write code reviews on a regular basis and they're valuable for thinking more deeply about tricky bits of code. They're also valuable as a training tool for developers who are new to a particular code base. We should use them where appropriate, including for new contributors. But we shouldn't turn them into an off-putting religion. -- Russ Allbery (r...@debian.org) <https://www.eyrie.org/~eagle/>
