[RFC] Time until patches should be submitted (was: Document for review: coreboot Gerrit Etiquette and Guidelines)
Dear coreboot folks,
Am Donnerstag, den 29.10.2015, 12:55 -0600 schrieb Martin Roth:
As the community has grown, so has the need to formalize some of the guidelines that the community lives by. When the community was small, it was easy to communicate these things just from one person to another.
first of, big thanks go to Martin for writing up the draft!
[…]
Expectations contributors should have:
- Don't expect that people will review your patch unless you ask them
to. Adding other people as reviewers is the easiest way. Asking for reviews for individual patches in the IRC channel, or by sending a direct request to an individual through your favorite messenger is usually the best way to get a patch reviewed quickly.
- Don't expect that your patch will be submitted immediately after
getting a +2. As stated previously, non-trivial patches should wait at least 24 hours before being submitted.
to get some context, at the coreboot conference in Bonn some people asked for longer review time to not wake up the next morning seeing something changed.
Even then, especially for non-payed developers, I think it’s hard to stay up to date, so the question is, if the time is long enough. On IRC somebody even mentioned, that patches should stay up for review for *a week* before getting merged, so there is enough time people can notice this.
To not complicate rules, it probably would be easiest to just ask around if people are alright with 24 hours. Especially, when people working on the code get added as reviewers, this should be alright.
On the other hand, more complicated rules could be drafted. The following rules just deal with the time issue. I am assuming, that +2 has been given before and the appropriate announcements are made.
1. Commits just touching a board can be submitted after 24 hours. 2. Commits touch more boards, should stay up for review for a week. They can be submitted earlier if an announcement to the list about the urgency has been sent and at least two people have given +2.
[…]
Thanks,
Paul
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 11/03/2015 03:36 PM, Paul Menzel wrote:
Dear coreboot folks,
Am Donnerstag, den 29.10.2015, 12:55 -0600 schrieb Martin Roth:
As the community has grown, so has the need to formalize some of the guidelines that the community lives by. When the community was small, it was easy to communicate these things just from one person to another.
first of, big thanks go to Martin for writing up the draft!
[…]
Expectations contributors should have:
- Don't expect that people will review your patch unless you ask them
to. Adding other people as reviewers is the easiest way. Asking for reviews for individual patches in the IRC channel, or by sending a direct request to an individual through your favorite messenger is usually the best way to get a patch reviewed quickly.
- Don't expect that your patch will be submitted immediately after
getting a +2. As stated previously, non-trivial patches should wait at least 24 hours before being submitted.
to get some context, at the coreboot conference in Bonn some people asked for longer review time to not wake up the next morning seeing something changed.
Even then, especially for non-payed developers, I think it’s hard to stay up to date, so the question is, if the time is long enough. On IRC somebody even mentioned, that patches should stay up for review for *a week* before getting merged, so there is enough time people can notice this.
To not complicate rules, it probably would be easiest to just ask around if people are alright with 24 hours. Especially, when people working on the code get added as reviewers, this should be alright.
On the other hand, more complicated rules could be drafted. The following rules just deal with the time issue. I am assuming, that +2 has been given before and the appropriate announcements are made.
- Commits just touching a board can be submitted after 24 hours.
- Commits touch more boards, should stay up for review for a week.
They can be submitted earlier if an announcement to the list about the urgency has been sent and at least two people have given +2.
[…]
Thanks,
Paul
The only thing I'd like to mention in here is that by increasing latency too far, you will increase the tendency for large patch trains to be dumped on Gerrit and possibly abandoned. There are conflicting goals at the moment; one is to reduce patch trains by merging incrementally, but almost no one aside from a hobby hacker is going to be OK with waiting at each development stage for over a week for the review process to finally get around to merging things.
A single data point is a patch I created to fix a kernel panic in the Linux kernel. Anyone who's familiar with the Linux development process knows that certain areas have an extreme latency due to poor maintainership; apparently I hit one of those areas. The latency for communication was several weeks (!) and as a result the patch was not taken up and the kernel will still panic under the original conditions that prompted the patch, even though there was no direct objection to the contents of the patch.
Now if the kernel panics, a reboot can fix it. If coreboot goes down, machines can be bricked.
Just something to consider -- like all things in life, it's a balance.
- -- Timothy Pearson Raptor Engineering +1 (415) 727-8645 (direct line) +1 (512) 690-0200 (switchboard) http://www.raptorengineeringinc.com
On Tue, Nov 3, 2015 at 1:36 PM, Paul Menzel < paulepanter@users.sourceforge.net> wrote:
Dear coreboot folks,
Am Donnerstag, den 29.10.2015, 12:55 -0600 schrieb Martin Roth:
As the community has grown, so has the need to formalize some of the guidelines that the community lives by. When the community was small, it was easy to communicate these things just from one person to another.
first of, big thanks go to Martin for writing up the draft!
[…]
Expectations contributors should have:
- Don't expect that people will review your patch unless you ask them
to. Adding other people as reviewers is the easiest way. Asking for reviews for individual patches in the IRC channel, or by sending a direct request to an individual through your favorite messenger is usually the best way to get a patch reviewed quickly.
- Don't expect that your patch will be submitted immediately after
getting a +2. As stated previously, non-trivial patches should wait at least 24 hours before being submitted.
to get some context, at the coreboot conference in Bonn some people asked for longer review time to not wake up the next morning seeing something changed.
Even then, especially for non-payed developers, I think it’s hard to stay up to date, so the question is, if the time is long enough. On IRC somebody even mentioned, that patches should stay up for review for *a week* before getting merged, so there is enough time people can notice this.
To not complicate rules, it probably would be easiest to just ask around if people are alright with 24 hours. Especially, when people working on the code get added as reviewers, this should be alright.
On the other hand, more complicated rules could be drafted. The following rules just deal with the time issue. I am assuming, that +2 has been given before and the appropriate announcements are made.
- Commits just touching a board can be submitted after 24 hours.
- Commits touch more boards, should stay up for review for a week.
They can be submitted earlier if an announcement to the list about the urgency has been sent and at least two people have given +2.
There's a catch-22 here: The kinds of patches that could benefit from >24 hours in limbo also tend to be the disruptive kinds that may require additional rebasing or changes should they remain in code review for too long. A lot can happen in a week and disruptive patches bitrot faster than normal ones.
24 hours should be considered a minimum, but I'd say "preferably a few days if possible." A >24hr grace period can be agreed upon by reviewers and the author depending on the complexity to mitigate bitrot.
-
David Hendricks -
Paul Menzel -
Timothy Pearson