Hi community,
I feel a bit frustrated by how code review is going recently at osmocom.
Let me compare two larger feature branches:
There are weeks of review on 19 patches in osmo-hnbgw/cnpool,
and <24h from submit to merge on 14 patches in osmo-msc.
That may in some situations be ok and justified, but I feel that it is not:
The cnpool patch series for osmo-hnbgw spawned huge discussions and long delays
on account of minor issues: log lines, event names and comments. I said so, yet
it droned on and even spawned offspring.
None of the important features was part of code review:
larger design choices of the API, efficiency in handling the actual signalling,
correctness of the signalling. Maybe that's because all those were mint perfect
and no bugs anywhere, but I doubt that I'm really that good =)
And at the same time I was "just now" asked to review some patches for
osmo-msc, which changes an API that I have recently added and had ideas and
reasons for. Before I get a chance to even glance, I see that all the patches
are already merged! It is now basically too late to bring in my feedback,
because the patches are in, and why spend more time on it. Suck it up, bygones.
This very API that was being changed in osmo-msc in under 24 hours was itself
under code review scrutiny for at least three years.
Also I have (in my own free time) tried to get an improved logging timestamp
into libosmocore. This is tagging along for years and years now, because every
time over it is being blocked by minor details that are matters of taste. For
years I lose precious moments every time i have to read an 'extended-timestamp'
in osmo logging and identify the boundary of hours, minutes and seconds. This I
consider important. AFAIR the reasons for blocking the patches were,
objectively, all not important.
These examples are starkly unbalanced.
I'm sure you're aware of the bikeshed phenomenon -- it is a precise description
of what is happening. It would be great if we could turn that around a bit?
http://www.catb.org/jargon/html/B/bikeshedding.html
Let's say, I know from experience that it is hard to give good code review, so
count me in on this advice.
Let's still give all the code review we can, but let's keep in mind the actual
impact the particular code change has. If it goes in the "fringe detail"
bucket, let's mention it, but just let it pass if the author disagrees.
Hope you understand my frustration and maybe even agree.
Thanks,
~N