Re: debian/watch version 5
Otto Kekäläinen <otto@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 (rra@debian.org) <https://www.eyrie.org/~eagle/>
Reply to: