Attention is currently required from: Felix Singer, Nico Huber, Richard Hughes, Edward O'Callaghan, Daniel Campello, Anastasia Klimchuk.
1 comment:
Patchset:
Also you had a remark about code quality which has dropped below expected levels in your opinion. Code quality is an important subject too, if you could start a thread on the mailing list about it?
You could explain how you measure it, what are metrics?
What is expected level? (especially in light of Development guidelines saying “As usual there is no quality promise for the state of the code on the master branch”)
Ideally if you give examples of quality drops so that we can fix those and prevent from happening again.
While recent efforts have been focused on *improving* code quality, we can think of two examples of code quality dropping substantially in the past. These memories are related to the grudges/trauma we held before. We had just been entrusted with the "flashrom developer" and "flashrom owner" roles, so we felt our duty was to review flashrom patches. We were mostly on our own (we had Nico at the start, but he then had to take a break) and we (A&R) did not notice the problems with the code during review, and then we felt guilty and personally responsible for allowing this to get in. We had failed to do what we perceived was our duty. The trauma was so strong we couldn't logically talk about these issues (awful emotions would overwhelm us). Buuuuut... We just grabbed all the trauma and threw it in the trash, so we're now free to talk about what happened.
== Some context ==
When people from CrOS decided to upstream patches, Stefan Reinauer added Edward to the "flashrom developers" group without any notice. Nico and we (A&R) were rather annoyed by this; not necessarily because of Edward, but because we were not asked. This made us (A&R, not sure if Nico as well) treat Edward from a distance, instead of onboarding him into the "flashrom developer" group. That being said, we (A&R) didn't know much either, as Nico had entrusted us with the roles not too long ago.
We (A&R) have zero reasons to believe that Googlers have malicious intentions, but when the upstreaming started, there were *too many* people: we (A&R) just can't review patches from several active contributors, it's just too much. The sudden spike in workload affected Nico much sooner (we'll let Nico elaborate on this, if he wants to) and we ended up alone trying to catch balls (review patches) from multiple people at the same time. The ease of upstreaming stuff from CrOS flashrom made this even worse, as there were *lots* of patches flying around. Moreover, we didn't have others to help with reviews yet: People like Felix, Thomas and you, Anastasia, were not (so deeply) involved in flashrom yet. We didn't know who to ask for help, and we were too exhausted to continue; we collapsed.
Edward, with no one else to provide guidance, tried to do the best he could. However, this involves assuming a great amount of responsibility, and any wrongdoings (even honest mistakes) got amplified by the bad feelings stemming from the way Edward was given "flashrom developer" rights. Looking at old emails, we (A&R) can tell that Edward did, in fact, assume this responsibility, and later struggled when Nico confronted him about what happened. It was awful for everyone involved: Nico, Edward, us (A&R), and anyone else who got caught in the crossfire.
== Two unpleasant situations ==
Situation 1: A bunch of programmers were upstreamed from CrOS flashrom a few years ago, but many of them couldn't be tested anymore. They were in a similar situation as the `parade_lspcon` programmer (zero checks to avoid causing damage to unsupported devices). Eventually, these untested programmers were dropped from upstream: what is untested code, if not technical debt? Plus, the time spent reviewing these patches could've invested elsewhere.
Lessons learned: Let's not add large amounts of code if it's not tested and there's no plan to test it. Untested programmer code is very dangerous (it85spi code was disabled and eventually dropped because it would cause problems with some laptops), and there are better things to spend authors' and reviewers' time on.
Situation 2: When variable SPI size support was added to dummyflasher, the implementation was so poorly done that it caused all sorts of issues (including some build errors when trying to disable some programmers). If memory serves us right, these issues have been fixed, but we believe the effort of fixing that mess could've been avoided in the first place with a more thorough review. The main problem is that there were next to no reviewers back then.
Lessons learned: There's a point where help from others becomes necessary. Instead of giving people the fish, teach them how to fish (fish == code reviews, in this case).
Thank you Angel, let’s work together.
Thank you so much for your understanding, let's indeed work together.
To view, visit change 64663. To unsubscribe, or for help writing mail filters, visit settings.