Re: [coreboot] [RFC] Time until patches should be submitted (was: Document for review: coreboot Gerrit Etiquette and Guidelines)

[Martin Roth]

As others have stated, my concern is the tradeoff between long and
short times.  Too short a time, and people don't getting the chance to
review patches.  Too long a time and it delays development, frustrates
developers, and requires rebasing the patch, maybe requiring MORE
review.

Alternate thoughts or proposals:
- "Note that some contributors prefer to work on coreboot during
business hours, so consider leaving patches up longer than 24 hours
during weekends and holidays"
This of course presents other issues of dealing with holidays in
various countries, time zones, etc.

- Trivial patches (Whitespace, spelling, and the like) can be
submitted with no delay. Non-trivial commits touching fewer than 20
lines of code should be submitted no sooner than 24 hours after the
last major change.  Changes larger than that 20 lines should wait at
least 48? 72? hours after the last major change.

Paul also mentioned that a method of getting non-trivial patches in
quickly should be addressed as well.  This should not be a regular
occurrence, but sometimes we need or desire to push a patch quickly.
He suggested 2 +2s and a statement to the mailing list. To me, this
defeats the purpose of quick.

Yesterday tpearson requested on IRC that we get a patch in quickly.
It was, in my opinion significant, although just a single line change,
which made it easy to understand and review.  We got 3 +2s on it in
just a few minutes.  I thought that was a good barrier and would like
to propose that as the rule:  If you can get 3 +2s, knowing that it's
going to be submitted immediately, it can be submitted.  If this gets
abused, it can be reviewed and changed at some point in the future.

Martin

On Tue, Nov 3, 2015 at 2:48 PM, David Hendricks  wrote:
>
>
> On Tue, Nov 3, 2015 at 1: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.
>>
>> 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.
>
>
> 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 (dhendrix)
> Systems Software Engineer, Google Inc.
>
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot

-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] [RFC] Time until patches should be submitted

[Timothy Pearson]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/04/2015 10:07 AM, Martin Roth wrote:
> As others have stated, my concern is the tradeoff between long and
> short times.  Too short a time, and people don't getting the chance to
> review patches.  Too long a time and it delays development, frustrates
> developers, and requires rebasing the patch, maybe requiring MORE
> review.
> 
> Alternate thoughts or proposals:
> - "Note that some contributors prefer to work on coreboot during
> business hours, so consider leaving patches up longer than 24 hours
> during weekends and holidays"
> This of course presents other issues of dealing with holidays in
> various countries, time zones, etc.
> 
> - Trivial patches (Whitespace, spelling, and the like) can be
> submitted with no delay. Non-trivial commits touching fewer than 20
> lines of code should be submitted no sooner than 24 hours after the
> last major change.  Changes larger than that 20 lines should wait at
> least 48? 72? hours after the last major change.
> 
> Paul also mentioned that a method of getting non-trivial patches in
> quickly should be addressed as well.  This should not be a regular
> occurrence, but sometimes we need or desire to push a patch quickly.
> He suggested 2 +2s and a statement to the mailing list. To me, this
> defeats the purpose of quick.
> 
> Yesterday tpearson requested on IRC that we get a patch in quickly.
> It was, in my opinion significant, although just a single line change,
> which made it easy to understand and review.  We got 3 +2s on it in
> just a few minutes.  I thought that was a good barrier and would like
> to propose that as the rule:  If you can get 3 +2s, knowing that it's
> going to be submitted immediately, it can be submitted.  If this gets
> abused, it can be reviewed and changed at some point in the future.
> 
> Martin

Yes, this is a good compromise.  There isn't very much we can do about
latency on non-emergency patches; however due to the increased delays
per-patch, patch trains will become the dominant method of submitting
large changes (I am expecting a variant of Little's Law to hold here).
As a result, the improved patch train handling set out in the CL becomes
critical.  I assume all reviewers are on board with the idea of not
rebasing unless something is broken, and handling formatting or other
non-functional changes at the end of a series?

Thanks!

- -- 
Timothy Pearson
Raptor Engineering
+1 (415) 727-8645 (direct line)
+1 (512) 690-0200 (switchboard)
http://www.raptorengineeringinc.com
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJWOjtbAAoJEK+E3vEXDOFbTCoH/1d2B+2xZSpo+IdV+FWQaUrE
XmkJzUDBZMGX0EcnIj5+sEArMHsRZdNeG86ovmx+mFhj5LLJ8i84QG6Ayhduwxm5
Anv8ccDCvyB8QpJ1Ju6NOv2esfLYhi8d4EBdhv4fj6w4eJzZPoLI3p6FIDYSwaFp
jJy1K190oQYnQ1ff2gFdBK6zFm1V30cxyz9ogVJgUQJPvFe1KHa1QbwzDPYhguoH
cMcdyYaWJLihy0T3W+jQSbw06K39D2MnA7D/InCgseQRkHdfVqcuLggie98DuEXg
LrzjnwRp0d+cwBoRDCEfvfwF5UX6JjAOu0Q8rMGRShAk8rqXnXeWsNv4wO/cwyE=
=Bbai
-END PGP SIGNATURE-

-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] [RFC] Time until patches should be submitted

[Timothy Pearson]

-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.
> 
> 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

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
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJWOSqVAAoJEK+E3vEXDOFbW/4H/RAkgFdo6AwMi3KuMApGR9qU
y1lu6bClRUcvwx0Qs9wBolWuB+YiMzG1bcpgopbPXAozx3NMmkj3w73Mp14U2NDt
ZqCHZvClvQXUk4psDFTbRmFv4n3B6CX0EIVmy0Kcfrqd34YCNI8GHeYuXy4EOTrt
oWAIZgpvh8maOPx2UWv0EmJsYwnycrUiZ1S9RqV3OM0OEjLvZ7r6RJamzTjcBqrs
dq0rw6ihTM8M0vcfWqPokkL35Aw4WxalRilauW/SJc06Wgov22XksiWYulg7/4DN
WexwvbEuELHXLxLSxwcGcI9k/DV8dyiuBuogQy33Cpa8gjJPWIqI9QYJiL8A+e8=
=XGjb
-END PGP SIGNATURE-

-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

[coreboot] [RFC] Time until patches should be submitted (was: Document for review: coreboot Gerrit Etiquette and Guidelines)

[Paul Menzel]

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
> 

signature.asc
Description: This is a digitally signed message part
-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

Re: [coreboot] [RFC] Time until patches should be submitted (was: Document for review: coreboot Gerrit Etiquette and Guidelines)

[David Hendricks]

On Tue, Nov 3, 2015 at 1:36 PM, Paul Menzel <
paulepan...@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.
>
> 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.
>

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 (dhendrix)
Systems Software Engineer, Google Inc.
-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot