On Fri, 27 Oct 2023 at 08:46, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> One of the critical parts is review. Good reviews are often insanely
> expensive and they are very much a factor in burning people out. If one
> only ever reviews and the load never ends that's going to fsck with you
> in the long run.
I absolutely despise the review requirement that several companies
have. I very much understand why it happens, but I think it's actively
detrimental to the workflow.
It's not just that reviewing is hard, the review requirement tends to
be a serialization point where now you as a developer are waiting for
others to review it, and those others are not nearly as motivated to
do so or are easily going to be nitpicking about the non-critical
things.
So it's not just the reviewers that get burned out, I think the
process ends up being horrific for developers too, and easily leads to
the "let's send out version 17 of this patch based on previous
review". At which point everybody is completely fed up with the whole
process.
And if it doesn't get to version 17, it's because the reviewers too
have gotten so fed up that by version three they go "whatever, I've
seen this before, they fixed the obvious thing I noticed, I'll mark it
reviewed".
The other dynamic with reviews is that you end up getting
review-cliques, either due to company pressure or just a very natural
"you review mine, I review yours" back-scratching.
Don't get me wrong - it can work, and it can even work well, but I
think the times it works really well is when people have gotten so
used to each others, and know each other's quirks and workflows and
they just work well together. But that also means that some people are
having a much easier time getting reviews, because they are part of
that "this group works well together" crowd.
Maybe it's a necessary evil. I certainly do *not* think the "lone
developer goes his own way" model works all that well. But the reason
I said that I wish we had more maintainers, is that I think we would
often be better off with not a "review process" back-and-forth. but a
_pipeline_ through a few levels of maintainers. Not the "hold things
up and send it back to the developer" kind of thing, but "Oh, this
looks fine, I'll just send it on - possibly with the fixes I think are
needed".
So I think a pipeline of "Signed-off-by" (or just merges) might be
something to strive for as at least a partial replacement for reviews.
Sure, you might get Acked-by's or Reviewed-by's or Tested-by's along
the way *too*, or - when people are just merging directly through git
- you'd just get a merge commit with commentary and perhaps extra
stuff on top.
Back when we started doing the whole "Signed-off-by" - for legal
reasons, not development reasons - the big requirement for me was that
"it needs to work as a pipeline, not as some kind of back-and-forth
that holds up development". And I think the whole sign-off chain has
worked really well, and we've never needed to use it for the original
legal purposes (and hopefully just by virtue of it existing, we never
will), but it has been a huge success from a development standpoint.
When something goes wrong, I think it's been great to have that whole
chain of how it got merged, and in fact one of my least favorite parts
of git ended up being how we never made it easy to see the merge chain
after it's been committed (you can technically get it with "git
name-rev", but it sure is not obvious).
I dunno. Maybe I'm just dreaming. But the complaints about reviews -
or lack of them - do tend to come up a lot, and I feel like the whole
review process is a very big part of the problem.
Linus
