Hi Russ, Thanks for writing a long reply! I just wanted to acknowledge I fully read it and look forward to hearing more learnings from your career over a pint of beer at the next event we attend, and I am glad to also share my learnings from various CI and CR systems and what I think works best for avoiding regressions while keeping maximum development velocity going.
I will try to reply to the most important points below briefly: On Fri, 15 Aug 2025 at 18:54, Russ Allbery <r...@debian.org> wrote: > 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. I did not at any point doubt that you would not have informed opinions. I was just trying to raise in my reply the bigger picture and what can be the cost of skipping code reviews and somebody having to fix up afterwards. > 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. There might be a language/culture thing at play here too, so I just wanted to clarify that by 'not expensive' I do not mean no cost or no effort. If I pay 1000€ for an ice cream or for a car, the cost in both cases is the same but only the ice cream was 'expensive', meaning the cost was high compared to value. I think that in the vast majority of MRs in Debian, the benefits far outweigh the cost, and in particular for package 'devscripts' which was the context here. > 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. I have experienced all the phenomena you describe above during my career too. I have also been involved in mitigating and solving all of them in ways that didn't conclude that code reviews should stop, but by making the code review process better. I also don't see the "easy" code reviews as a waste of time. They are cheap, and having an almost "rubberstamp" review is still better than not having any at all, or having people trying to guess in advance which code change has a mistake and ask reviews only for the MRs that are likely to have mistakes. Reviews also have more functions than just mistake catching. > 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. Sure, as a very senior software developer you can and should use more judgment. But keep in mind that you are also setting an example to others. I hope you could, in the future, encourage people to do more code reviews and develop collaboratively, but then silently opt out of it yourself and when asked resort to your seniority. Would that be a reasonable approach? I don't think you actually think code reviews are a bad thing in general, you just don't think you get much value of them yourself as you have the judgment to always do things correctly, you don't want to spend time on teaching others (as you wrote in an earlier email) and you probably don't have much to learn by looking at code from others either. I think that is a reasonable stance for you or other people with similar seniority to take, but I don't think that should be the culture we teach to "average" or new contributors. > 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 Thanks! > 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. But we should strike a balance on having fun for everyone. If somebody goes around and breaks stuff it is not fun for others to fix, in particular if it had been easily preventable. > > 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. My point was not about reviews in general, but specifically about Salsa MRs. I believe that if people do Salsa MRs frequently, they get a good routine on how to efficiently do it, and they no longer feel expensive, which goes back to the line you quoted on the first line of the email. ... > 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. I believe that if doing MRs is a main rule in a project, and the people who participate in it do it a lot, they will get fast and efficient at it. Both in using the tools and technology, and in the workflow/interaction and psychological aspects. Then we get away from feeling overhead and friction. Not having code reviews, of course, removes the "overhead and friction" of code reviews, but may introduce overhead and friction elsewhere. This boils back to the assessment of cost or it being "expensive." I hope to contribute to this by improving the tools and processes to make Salsa MRs specifically smoother overall, and I also try to lead by example and practice MRs myself so that I myself suffer from any friction or inconvenience there might be. In the case of 'devscripts' I have used MRs for all my contributions, apart from the changelog fix I did in this thread. I will also try to contribute by documenting the MR processes/practices that I feel work best like I did in https://optimizedbyotto.com/post/debian-salsa-merge-request-best-practices/ - Otto
