There are obvious problems with the current +3 enforcing scheme on gerrit.
a) if a patch has +2, the summary shows a tick mark, you don't see that another +1 is needed. b) if a patch has three +1s, the summary shows +1, you don't see that it can be merged. c) voting +1 for an own patch has an effect on the mergability.
Ideas to improve:
a) +2 hides missing +1 On IRC, we agreed that it is a good idea to require two reviews (by two people) -- three is too much. Currently that would be one +2 vote plus one +1 vote. (alternatively 3x +1, too).
Idea: if we remove the "+2" ability, we avoid the problem that a +2 hides the missing +1.
Everyone has only +1 votes, and we add up using that plugin.
To require only two reviews, we merge once a sum of +2 is reached.
b) 3x +1 == "+1" There's no solution short of fixing what gerrit shows in the summary. Everyone will still click on the patch and then see that it already has three votes.
c) +1 to self The solution is that you don't vote for your own patches, unless it is really super trivial and not worth wasting others' review time on.
Let's see for a few days how we fare with these quirks.
~N
Hi Neels and Oliver,
thanks for your work and investigation. To me, it seems, after all, gerrit is not suited for what we try to achieve.
Confusing messages in the UI are dangerous, particularly if there's no clear solution in sight.
I would hence rather go back to Neels' proposal to simply give all [known] developers +2 capacity with a social contract.
Please let's avoid re-starting the discussion from scratch.
Let's see for a few days how we fare with these quirks.
Ok if you'd like we can do that, but to me a broken UI/representation with no fix expected to show up makes it quite clear that this is not a situation which we want to persist.
To me, the behaviour you describe as the current status quo is a no-go, sorry.
Regards, Harald
Hi,
given that the UI has so many issues with the plugin, let's move to the other approach then (everybody can +2, that's it, right?).
On Wed, Dec 12, 2018 at 10:17:51PM +0100, Harald Welte wrote:
I would hence rather go back to Neels' proposal to simply give all [known] developers +2 capacity with a social contract.
Ok, I like that, but wanted to give the other way a chance / didn't want to be the one to press for it.
osmith, can you remove the special repository config again plz, and then I'll take care of reconfiguring gerrit tomorrow.
~N
On 12/13/18 3:06 AM, Neels Hofmeyr wrote:
On Wed, Dec 12, 2018 at 10:17:51PM +0100, Harald Welte wrote:
I would hence rather go back to Neels' proposal to simply give all [known] developers +2 capacity with a social contract.
Ok, I like that, but wanted to give the other way a chance / didn't want to be the one to press for it.
osmith, can you remove the special repository config again plz,
Done.
See voting rules draft at https://osmocom.org/projects/cellular-infrastructure/wiki/Gerrit
Give feedback, otherwise these are now in force.
~N
On Fri, Dec 14, 2018 at 06:08:34PM +0100, Neels Hofmeyr wrote:
See voting rules draft at https://osmocom.org/projects/cellular-infrastructure/wiki/Gerrit
Give feedback, otherwise these are now in force.
The new section about voting rules looks good to me. There's some historical baggage throughout this document which should be cleaned up, though.
For instance, further down the page in the section https://osmocom.org/projects/cellular-infrastructure/wiki/Gerrit#Rationale under "The advantages of Gerrit" it still says:
* developers + maintainers can formally vote on a patch (developer: -1/0/+1, maintainer: -2/0/+2) * once a patch has +2 score, it can be (automatically) merged into master
I suppose we could just remove those lines? They simply describe the default configuration of gerrit.
Similarly, the section https://osmocom.org/projects/cellular-infrastructure/wiki/Gerrit#Merge-patch... begins with "A patch can be merged when it has CR+2 and V+1 votes, ..." and duplicates information which is already covered at the top of the page.
Apart from that, there is some obsolete information such as a description of a gerrit bug which has been fixed upstream -- do we really need to keep such information around? https://osmocom.org/projects/cellular-infrastructure/wiki/Gerrit#Rebase-if-n...
I am wondering if there isn't a better way to integrate the new voting rules in the existing text, rather than putting them right at the top of the page? The page is about general gerrit usage in Osmocom. It doesn't really make much sense to explain voting etiquette before explaining basic details such as how to access gerrit in the first place.
Any volunteers for cleaning this page up? Did I just volunteer involuntarily? :)
On Mon, Dec 17, 2018 at 04:02:39PM +0100, Stefan Sperling wrote:
I suppose we could just remove those lines?
Sure, go ahead!
Any volunteers for cleaning this page up? Did I just volunteer involuntarily? :)
Usually this stuff sticks to my shoes after I step into it, would very much welcome spreeeeading that around a bit.
Well but I often am quite nitpicky on prose, so could be my fault, too, scaring off others too much.
~N
-
Harald Welte -
Neels Hofmeyr -
Oliver Smith -
Pau Espin Pedrol -
Stefan Sperling