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