Hello coreboot fellows,
first I want to say, I feel pretty awkward bringing this up. I'm not a fan of regulations. But as it turns out, our community is growing (\o/) and with that growth the number of situations where regulation seems necessary increases too.
So, we already have Gerrit guidelines [1]. While they are most often not worth a look (common sense usually is enough), some people like to be reminded of them regularly. The latter is pretty annoying and doesn't seem to help.
I propose to add some lines what should happen when somebody delibe- rately ignores the guidelines. One such case that I often see is rubber-stamping of huge commits. Our guidelines say
"As a reviewer who approves (+2) a patch, you are responsible for the patch and the effect it has on the codebase."
I've never seen anyone taking responsibility for rubber-stamped code. Bigger additions seem to be always cleaned up by other folks. This led me to the following idea:
Reviewers who rubber-stamp bigger code additions will be demoted by one group on Gerrit. They can only return to the higher group after one hour per rubber-stamped, added line.
Sounds easy and fair, I'd say. What do you think? Of course, there are many other kinds of violations; feel free to bring more up. I just want to start the discussion.
Nico
[1] https://doc.coreboot.org/getting_started/gerrit_guidelines.html
On Tue, Nov 05, 2019 at 02:26:25PM +0100, Nico Huber wrote:
So, we already have Gerrit guidelines [1]. While they are most often not worth a look (common sense usually is enough), some people like to be reminded of them regularly. The latter is pretty annoying and doesn't seem to help.
Most of them aren't easily testable, so I guess the best we could do to reduce the pain about that is to add some prominent link to it in the Gerrit UI. Would that help?
I propose to add some lines what should happen when somebody delibe- rately ignores the guidelines. One such case that I often see is rubber-stamping of huge commits.
One person's rubber-stamp is another person's "I know you plan to work on this, and you have a reason for doing it this way".
Reviewers who rubber-stamp bigger code additions will be demoted by one group on Gerrit. They can only return to the higher group after one hour per rubber-stamped, added line.
Sounds easy and fair, I'd say. What do you think?
I don't see how this is easy:
The first issue I see is "who is not taking responsibility?", a judgement that certainly differs from person to person, and context to context.
Next: Assuming multiple reviewers +2 and are found "not to take responsibility", does everybody get one hour per rubber-stamped, added line, or will the lines be split among them?
Next: When is the penalty implemented? If it happens right before my vacation, I might not care too much ;-)
In general, I'm not opposed to removing privileges on Gerrit (so that you can lose them over time just like you can gain them) based on unprofessional behavior, but the mechanics are far from clear to me.
I wouldn't do it time-based, either: you don't end up in the CR+2 and submit group just because your account lingered around on Gerrit for X days.
Patrick
Hi,
thanks for your response Patrick. I should have made it more clear that I'm talking about rather rare, but very extreme cases. I don't want to discuss the rubber-stamping example too much, rather if we want such rules at all.
On 05.11.19 17:47, Patrick Georgi via coreboot wrote:
On Tue, Nov 05, 2019 at 02:26:25PM +0100, Nico Huber wrote:
So, we already have Gerrit guidelines [1]. While they are most often not worth a look (common sense usually is enough), some people like to be reminded of them regularly. The latter is pretty annoying and doesn't seem to help.
Most of them aren't easily testable, so I guess the best we could do to reduce the pain about that is to add some prominent link to it in the Gerrit UI. Would that help?
I don't think so. I'm really talking about cases when experienced reviewers already know that there are guidelines and choose to ignore them. If somebody is unaware of the guidelines and does something wrong, that's ok, IMHO. We can just point them into the right direction for the future.
I propose to add some lines what should happen when somebody delibe- rately ignores the guidelines. One such case that I often see is rubber-stamping of huge commits.
One person's rubber-stamp is another person's "I know you plan to work on this, and you have a reason for doing it this way".
I mean "rubber-stamping of *huge* commits". That huge that it is obvious that no review happened, e.g. 1k+ LOC copy-pasta. Also, the guidelines say "This means you shouldn't +2 a patch just because you trust the author of a patch [...]" I didn't want to quote the whole paragraph, maybe I should have ;)
Reviewers who rubber-stamp bigger code additions will be demoted by one group on Gerrit. They can only return to the higher group after one hour per rubber-stamped, added line.
Sounds easy and fair, I'd say. What do you think?
I don't see how this is easy:
The first issue I see is "who is not taking responsibility?", a judgement that certainly differs from person to person, and context to context.
In case of really huge commits, the only way to return to a clean Git history is a revert. So that should be easy to measure. But really, what are we arguing about, I mean cases where a +2 already makes it obvious that the reviewer doesn't care about future maintenance nor a useful history.
Next: Assuming multiple reviewers +2 and are found "not to take responsibility", does everybody get one hour per rubber-stamped, added line, or will the lines be split among them?
Everybody. At least that's how I read it.
Next: When is the penalty implemented? If it happens right before my vacation, I might not care too much ;-)
I don't think it matters much when it starts.
In general, I'm not opposed to removing privileges on Gerrit (so that you can lose them over time just like you can gain them) based on unprofessional behavior, but the mechanics are far from clear to me.
I wouldn't do it time-based, either: you don't end up in the CR+2 and submit group just because your account lingered around on Gerrit for X days.
Not sure if this is a misunderstanding. "They can only return...", isn't supposed to mean that this would happen automatically.
Nico
On Tue, Nov 05, 2019 at 06:37:02PM +0100, Nico Huber wrote:
I mean "rubber-stamping of *huge* commits". That huge that it is obvious that no review happened, e.g. 1k+ LOC copy-pasta. Also, the guidelines say "This means you shouldn't +2 a patch just because you trust the author of a patch [...]" I didn't want to quote the whole paragraph, maybe I should have ;)
Some of the mega patches are copies of a predecessor chip (with the minimum amount of changes to integrate it in the build), that are then modified to fit the new chip.
The idea was precisely to ensure that changes between the generations are visible in the history.
That was considered an acceptable strategy for a long time (after we broke up 82801xx because fixing one variant broke another). We can certainly revisit that, but to me that change appears to have happened gradually and without much notice.
In case of really huge commits, the only way to return to a clean Git history is a revert. So that should be easy to measure. But really, what are we arguing about, I mean cases where a +2 already makes it obvious that the reviewer doesn't care about future maintenance nor a useful history.
Again, obvious to whom?
Next: When is the penalty implemented? If it happens right before my vacation, I might not care too much ;-)
I don't think it matters much when it starts.
Under your scheme, 1kloc added is about 7 weeks. Too long to wait it out, not long enough to be practically permanent (but as you note later, that penalty isn't automatically lifted after that time anyway, changing the mechanics), so timing can make a big difference.
Not sure if this is a misunderstanding. "They can only return...", isn't supposed to mean that this would happen automatically.
That wasn't clear to me, even if that's a possible interpretation.
Regards, Patrick
-
Nico Huber -
Nico Huber -
Patrick Georgi