Let's not put "emeritus" or "on leave" lines into owner files

[Nico Weber]

Hi,

a handful of OWNERS files have grown "emeritus" or "on leave" sections. I think OWNERS files should only contain active OWNERS, else owner-suggesting scripts get harder to write and OWNERS files get harder to read.

If someone goes on leave, they can remove themselves from OWNERS files and then add themselves back when they return.

Unless someone shouts, I'll send a CL to remove "emeritus" and "on leave" sections in a few days.

Nico

[Charlie Reis]

Hmm, I would disagree about "emeritus."  There's a difference between "I'm busy with other things but still happy to share my deep knowledge about a part of the code base" (emeritus) and "I've moved on" (removing from OWNERS).

I feel less strongly about "on leave."  I think editing your display name in Rietveld is fine for that, since people will see it if they try to add you as a reviewer.  That may not be needed in OWNERs files.

Charlie

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Peter Kasting]

On Thu, Mar 12, 2015 at 9:22 AM, Charlie Reis <cr...@chromium.org> wrote:

Hmm, I would disagree about "emeritus."  There's a difference between "I'm busy with other things but still happy to share my deep knowledge about a part of the code base" (emeritus) and "I've moved on" (removing from OWNERS).

I have seen "emeritus" and I certainly didn't think it meant I could send reviews to that person.  My reaction was "what does that mean?  Either be an OWNER or don't, don't sit on the fence."  Then I moved on to someone else who wasn't listed as a weird status.

I fully support these files being black and white rather than grey

PK 

[Dana Jansens]

On Thu, Mar 12, 2015 at 8:35 AM, Nico Weber <tha...@chromium.org> wrote:

What if we moved emeritus such people into comments in the OWNERS file instead of removing them entirely?

[Nico Weber]

On Thu, Mar 12, 2015 at 12:31 PM, Dana Jansens <dan...@chromium.org> wrote:

On Thu, Mar 12, 2015 at 8:35 AM, Nico Weber <tha...@chromium.org> wrote:

Hi,

a handful of OWNERS files have grown "emeritus" or "on leave" sections. I think OWNERS files should only contain active OWNERS, else owner-suggesting scripts get harder to write and OWNERS files get harder to read.

If someone goes on leave, they can remove themselves from OWNERS files and then add themselves back when they return.

Unless someone shouts, I'll send a CL to remove "emeritus" and "on leave" sections in a few days.

What if we moved emeritus such people into comments in the OWNERS file instead of removing them entirely?

I'm with Peter, I don't know what "emeritus" means, other than "this person used to work on this". If you want to find former OWNERS, they're in source control (similar how we don't keep code around that we don't need at the moment).

If people want to keep "# XXX is no longer doing reviews, but is happy to consult on design" comments around, that's fine with me – but even then, it's probably better to get the design consultation from the person who will also do the code review. The current owners will know which previous owners to ask, if necessary, no?

[Thiago Farina]

On Thu, Mar 12, 2015 at 2:06 PM, Nico Weber <tha...@chromium.org> wrote:

On Thu, Mar 12, 2015 at 12:31 PM, Dana Jansens <dan...@chromium.org> wrote:

On Thu, Mar 12, 2015 at 8:35 AM, Nico Weber <tha...@chromium.org> wrote:

Hi,

a handful of OWNERS files have grown "emeritus" or "on leave" sections. I think OWNERS files should only contain active OWNERS, else owner-suggesting scripts get harder to write and OWNERS files get harder to read.

If someone goes on leave, they can remove themselves from OWNERS files and then add themselves back when they return.

Unless someone shouts, I'll send a CL to remove "emeritus" and "on leave" sections in a few days.

What if we moved emeritus such people into comments in the OWNERS file instead of removing them entirely?

I'm with Peter, I don't know what "emeritus" means, other than "this person used to work on this". If you want to find former OWNERS, they're in source control (similar how we don't keep code around that we don't need at the moment).

I see the "emeritus" from another way. In my view it is just a respectful recognition dedicated to someone how made big contributions to the project and no longer works on it. Take Ben as example, he used to be the lead designer for the Chrome Browser (if he isn't still). It is just a note to remember him and his large impact contributions.

 

--

Thiago Farina

[Nico Weber]

This strengthens the claim that it's not clear what "emeritus" means :-)

Imho, OWNERS files aren't for shout-outs, they're for getting work done. If folks want to add a "# We want to thank XXX" to an OWNERS file in a comment, I guess I'm fine with that, but I'm not sure if an OWNERS file is the best place for that.

[Charlie Reis]

Sure, but that recognition isn't functionally needed in the OWNERS file.

I don't feel strongly about keeping emeritus.  If the people listed that way still want to receive OWNERS code reviews, I suppose they don't need a special label in the OWNERS file.

Charlie

[Mike Stipicevic]

We have people who fit the 'emeritus' status in infra, but I didn't know that was entered into OWNERS files until I saw this thread. To me, `git blame` is enough cred. OWNERS are gatekeepers, so if you're not willing to 'own' the codebase (keep bad code out, revert and 'handle it' when it gets in) then you probably shouldn't be in the file.

[Dana Jansens]

Fair points, I agree.

[Vincent Scheib]

+1 OWNERS should contain only actively reviewing owners.

The practice of changing the codereview.chromium.org name for short periods of not being available for review works well, even for a few months if needed (e.g. after a new child). 

--

[Darin Fisher]

Hmm... hmmmm...

In addition to OWNERS files being a list of folks who provide reviews, it is also a list of folks who are trusted to make code changes to an area without seeking out approval from other OWNERS. In other words, it is the right to contribute code more freely to an area of the repository. It is not clear such a right (or such earned trust) should simply evaporate because someone leaves the project for a period of time.

Not only that, and maybe I'm just a softy, but I can't help but feel like this is akin to kicking someone on their way out the door when you would much rather say "thank you, hope to see you again." The emeritus tag is meant to capture this--perhaps too sentimental--point of view. I agree it isn't a great fit for automation tools.

My preference would be to simply invent an annotation that our automation tools can understand.

-Darin

[Peter Kasting]

On Thu, Mar 12, 2015 at 11:48 PM, Darin Fisher <da...@chromium.org> wrote:

Not only that, and maybe I'm just a softy, but I can't help but feel like this is akin to kicking someone on their way out the door when you would much rather say "thank you, hope to see you again." The emeritus tag is meant to capture this--perhaps too sentimental--point of view. I agree it isn't a great fit for automation tools.

This sounds a bit like what Thiago said.  I find it interesting since I had literally never considered that purpose before this thread, but now two people have mentioned it.

I've seen very few of these tags, so I wonder if it's really worth the time to try and support some use case for them.  Have I just missed them?  (I went and did a code search and found six total across the codebase.  Is that the size of a thing you feel is worth a special annotation system?)

I'm concerned that the ability to contribute in a particular area without any other OWNER oversight and the ability to do code reviews (or at least to make good decisions about who else can do reviews) in that same area seem to go hand in hand.  I can't think of a scenario where someone is technically unable to do the latter but is still fine to do the former.  (If you're too _busy_ to do the latter that's a bit different -- then again, if you're too busy to ever do reviews, do you have time to write code?)

That said, I can see how, if being in an OWNERS file is meant as some sort of recognition and gratitude -- as Thiago mentioned, and you sort of imply -- then what I'm saying sounds cold-hearted.  I don't mean it that way.  People listed in these tags, like you and Ben, have contributed at least as much if not more to the project as anyone I can think of.  Removal doesn't mean we're glad to be rid of you.  It means you've moved on from doing code reviews of those parts of the codebase to other important responsibilities.  I still value what you're doing there -- and I'd also be happy to have you writing patches again instead of holding meetings 24/7 any time :)

Perhaps we should have some other system to formally recognize people's contributions, that's clearer to more people that it's meant to show respect.  (I recall that back in 2007ish I argued for about:credits, a la Firefox, but lost; that may not capture the intent you're looking for anyway.)

PK

[John Abd-El-Malek]

Folks have different ways of avoiding reviews because they're overloaded. Some put their name under "emeritus", or "owner only, no code reviews" in the OWNERS files. Others ignore reviews or take a long time. Others denote their Rietveld username with "slow" or "busy". But they still make changes to the codebase and we don't want to slow them down, as others have mentioned. I don't think we're leaving them in OWNERS files because of a shout-out.

I think we should update the tools to recognize this. This can be done with a simple "#owners_but_not_reviewers" tag that the find-owners scripts ignores anything below. It would take much less time to implement than the discussion that this can spiral to ;)

--

[Vincent Scheib]

I highly respect folks substantial contributions, many of which will forever shadow my cumulative contribution. My support of designing the OWNERS files form to match a functional need is not intended to dishonor those contributions. I believe the OWNERS files should stay focused on their initial functional purpose which is to specify the set of reviewer coverage necessary for landing changes. This function alone is already a non-negligible mental load for many patches where a contributor spans multiple areas and must find a set of owners who have the right domain knowledge, aren't so many reviewers as to increase the review work load and reduce comprehension of the patch, and be as compatible as possible with reviewer's time availability due to time off or other calendar events and time zones. We have scripts that help with some of this (e.g. git cl owners, and the Chromite Butler extension among others) but it can still be friction to just getting work done. Every additional comment or special handling in OWNERS files makes the tools less functional and requires additional manual effort and research in the process. This run on paragraph's point is to emphasize the value of keeping OWNERS files as simple as possible.

I suspect the status of OWNERS who no longer review, but would like to retain the ability to land code without other OWNERS review, may be fairly short lived. At some point either limited activity in the code will reduce their deep knowledge of the system as code is constantly changing, at which point they shouldn't find it onerous to have an OWNER be a reviewer. Or, they're maintaining knowledge and there remains an imbalance between individual and community contribution, and I question what is better for the project as a whole.

Providing credit/reverence/documentation of previous domain experts to previous OWNERS seems fine, but I recommend doing so via commented out lines. That will be clearer to humans via file search tools like grep, and already supported by the various scripts and extensions.

[Ken Rockot]

+1 for moving inactive (i.e. not-really-gonna-do-a-review) OWNER information into comments.

[Peter Kasting]

On Fri, Mar 13, 2015 at 7:57 AM, John Abd-El-Malek <j...@chromium.org> wrote:

Folks have different ways of avoiding reviews because they're overloaded. Some put their name under "emeritus", or "owner only, no code reviews" in the OWNERS files. Others ignore reviews or take a long time. Others denote their Rietveld username with "slow" or "busy". But they still make changes to the codebase and we don't want to slow them down, as others have mentioned. I don't think we're leaving them in OWNERS files because of a shout-out.

I think we should update the tools to recognize this. This can be done with a simple "#owners_but_not_reviewers" tag that the find-owners scripts ignores anything below. It would take much less time to implement than the discussion that this can spiral to ;)

OWNER reviews are still supposed to be reviews.  If you're too overloaded to do reviews, and the problem is not short-lived, then I don't think you should be in the OWNERS file, and I don't think you should have the privilege of still getting to write code in this area that doesn't need an OWNER review.

Spending time reviewing changes is part of the tradeoff cost you pay when you have the benefit of contributing code to some area without as much oversight.  I don't think we should extend the benefits to people who are in a long term situation of not paying those costs, for whatever reason.

If the problem is short-lived but unavoidable (e.g. vacation), we should be encouraging people to change their Rietveld username rather than adding a comment in an OWNERS file.  (I think this is a non-issue today; I haven't seen people using "emeritus" as a short-lived status in OWNERS.)

If the problem is extremely short-lived (one day), or avoidable, do what you can and don't worry about it.  Occasional review latency is OK.  Habitual review latency is not; if you regularly take more than a day or so to get to reviews, then again, you should prioritize them more highly, at the expense of your coding work, as that's part of the responsibility of being an OWNER.

PK

[John Abd-El-Malek]

In general, whenever we propose a new rule, I think of Linus' guidance of doing what google3 does unless there's a very good reason to diverge. Searching for "emeritus file:owners" in cs/ finds 861 hits. I do not see a very good reason to diverge here.

On Fri, Mar 13, 2015 at 1:48 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Mar 13, 2015 at 7:57 AM, John Abd-El-Malek <j...@chromium.org> wrote:

Folks have different ways of avoiding reviews because they're overloaded. Some put their name under "emeritus", or "owner only, no code reviews" in the OWNERS files. Others ignore reviews or take a long time. Others denote their Rietveld username with "slow" or "busy". But they still make changes to the codebase and we don't want to slow them down, as others have mentioned. I don't think we're leaving them in OWNERS files because of a shout-out.

I think we should update the tools to recognize this. This can be done with a simple "#owners_but_not_reviewers" tag that the find-owners scripts ignores anything below. It would take much less time to implement than the discussion that this can spiral to ;)

OWNER reviews are still supposed to be reviews.

Not everyone (at least me) agrees with this statement.

 

  If you're too overloaded to do reviews, and the problem is not short-lived, then I don't think you should be in the OWNERS file, and I don't think you should have the privilege of still getting to write code in this area that doesn't need an OWNER review.

Ditto

 

Spending time reviewing changes is part of the tradeoff cost you pay when you have the benefit of contributing code to some area without as much oversight. 

Ditto

 

I don't think we should extend the benefits to people who are in a long term situation of not paying those costs, for whatever reason.

Ditto

[Vincent Scheib]

On Fri, Mar 13, 2015 at 1:57 PM, John Abd-El-Malek <j...@chromium.org> wrote:

In general, whenever we propose a new rule, I think of Linus' guidance of doing what google3 does unless there's a very good reason to diverge. Searching for "emeritus file:owners" in cs/ finds 861 hits.

As this is a public list, I'll not report the numbers, but the ratio of 

"emeritus file:owners" / "file:owners"

is very, very low.

[John Abd-El-Malek]

On Fri, Mar 13, 2015 at 2:05 PM, Vincent Scheib <sch...@chromium.org> wrote:

On Fri, Mar 13, 2015 at 1:57 PM, John Abd-El-Malek <j...@chromium.org> wrote:

In general, whenever we propose a new rule, I think of Linus' guidance of doing what google3 does unless there's a very good reason to diverge. Searching for "emeritus file:owners" in cs/ finds 861 hits.

As this is a public list, I'll not report the numbers, but the ratio of 

"emeritus file:owners" / "file:owners"

is very, very low.

I'm not sure what ratio has to do with it; the discussion is whether to have them or not.

But if you really care, the ratio for (latest) chromium code is similar to googel3.

[Peter Kasting]

On Fri, Mar 13, 2015 at 2:35 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Mar 13, 2015 at 2:05 PM, Vincent Scheib <sch...@chromium.org> wrote:

On Fri, Mar 13, 2015 at 1:57 PM, John Abd-El-Malek <j...@chromium.org> wrote:

In general, whenever we propose a new rule, I think of Linus' guidance of doing what google3 does unless there's a very good reason to diverge. Searching for "emeritus file:owners" in cs/ finds 861 hits.

As this is a public list, I'll not report the numbers, but the ratio of 

"emeritus file:owners" / "file:owners"

is very, very low.

I'm not sure what ratio has to do with it; the discussion is whether to have them or not.

But if you really care, the ratio for (latest) chromium code is similar to googel3.

In both cases, we're saying this usage is extremely rare.  Rare enough in the case of google3 that it's not clear to me the majority of teams would accept something like this practice or understand it.  (For practically anything, one can find people in google3 doing it all possible ways, but it's normally the common case that should guide our understanding of what is standard practice, and it's hard to tell in this case whether these are so uncommon in google3 because it's not actually a standard practice, or because it is an accepted practice but almost no one uses it.)

I think when there's a clear Google policy that addresses a particular question -- e.g. a style guide rule on an issue -- there's a strong argument for not diverging without cause.  I don't think this cases is a clear one.

One of my concerns here is that it's not clear to me what use case this is needed to solve.  Based on your responses to my previous email, it seems like you're concerned that we have a set of people for whom it's not possible to do reviews of code but who should be granted above-normal privileges such that their changes in those areas don't get OWNER approval.  Is that really something our tooling needs to support?  What kinds of cases are these?

I really struggle to understand why it's such a burden to simply not be in an OWNERS file and simply get one of the other OWNERS to be the one reviewing one's code in such a scenario.  That's what most of us do for most of our changes, and it doesn't seem to be a problem.

Without a motivating use case, I don't see how we can argue conclusively about whether we're diverging from common Google practice, because the question is how other teams at Google solve the problem we're trying to solve.  I'm not convinced that finding a few hundred emeritus lines -- or even MANY emeritus lines -- across the many OWNERS files in google3 means that whatever problem we have is one others have, and this is how they solve it.  So I'd prefer to not close off debate with the hammer of "Google does it this way internally".

As to the rest of your responses, you seem to have a very different view on what being an OWNER implies than I do, and while perhaps this is part of the reason we disagree, I don't think we'll reach any meaningful behavioral conclusions by continuing that argument here, so I'll avoid retort.

My summary of this thread so far is that there's more support for removing these than keeping them, the people who do want to keep them have all posed different meanings for what "emeritus" is supposed to mean and achieve, and some of the use cases this is posed as addressing can (and normally are) addressed with other practices.  I think we need a more clear and compelling case for what this does, that is best done this way, and will be clear to all project contributors.

PK

[Thiago Farina]

On Fri, Mar 13, 2015 at 9:08 PM, Peter Kasting <pkas...@chromium.org> wrote:

My summary of this thread so far is that there's more support for removing these than keeping them, the people who do want to keep them have all posed different meanings for what "emeritus" is supposed to mean and achieve, and some of the use cases this is posed as addressing can (and normally are) addressed with other practices.  I think we need a more clear and compelling case for what this does, that is best done this way, and will be clear to all project contributors.

I think there was a campus that said it would be OK to have them in the form of a comment.

emeritus file:OWNERS in the cs.chromium.org shows there are just 6 entries. 

https://code.google.com/p/chromium/codesearch#search/&q=emeritus%20file:OWNERS&sq=package:chromium&type=cs

What is the problem of having this respectful note to them in this special file?

Can we keep these entries but perhaps mark them as comments, so people don't think to send reviews to them (would that address the tooling concern?)? Seeing 'emeritus' I wouldn't thought in sending reviews to them though, as to me 'emeritus' imply inactive or that he already departed from the project.

--

Thiago Farina

[Peter Kasting]

On Fri, Mar 13, 2015 at 5:18 PM, Thiago Farina <tfa...@chromium.org> wrote:

What is the problem of having this respectful note to them in this special file?

Well, John just argued that we don't have people in here because it's a respectful note.  So one problem is that we're not in agreement on what the intent is.

Other than that, I think I and others have already responded why we're not sure an entry in an OWNERS file is the right place/method to express what you're describing.  That doesn't mean I have a problem with the sentiment itself :)

PK

[Dirk Pranke]

On Fri, Mar 13, 2015 at 5:08 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Mar 13, 2015 at 2:35 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Fri, Mar 13, 2015 at 2:05 PM, Vincent Scheib <sch...@chromium.org> wrote:

On Fri, Mar 13, 2015 at 1:57 PM, John Abd-El-Malek <j...@chromium.org> wrote:

In general, whenever we propose a new rule, I think of Linus' guidance of doing what google3 does unless there's a very good reason to diverge. Searching for "emeritus file:owners" in cs/ finds 861 hits.

As this is a public list, I'll not report the numbers, but the ratio of 

"emeritus file:owners" / "file:owners"

is very, very low.

I'm not sure what ratio has to do with it; the discussion is whether to have them or not.

But if you really care, the ratio for (latest) chromium code is similar to googel3.

In both cases, we're saying this usage is extremely rare.  Rare enough in the case of google3 that it's not clear to me the majority of teams would accept something like this practice or understand it.  (For practically anything, one can find people in google3 doing it all possible ways, but it's normally the common case that should guide our understanding of what is standard practice, and it's hard to tell in this case whether these are so uncommon in google3 because it's not actually a standard practice, or because it is an accepted practice but almost no one uses it.)

I think when there's a clear Google policy that addresses a particular question -- e.g. a style guide rule on an issue -- there's a strong argument for not diverging without cause.  I don't think this cases is a clear one.

One of my concerns here is that it's not clear to me what use case this is needed to solve.  Based on your responses to my previous email, it seems like you're concerned that we have a set of people for whom it's not possible to do reviews of code but who should be granted above-normal privileges such that their changes in those areas don't get OWNER approval.  Is that really something our tooling needs to support?  What kinds of cases are these?

On the tooling note, as Darin said, OWNERs files in Chromium have always had two meanings: to indicate people that needed to approve a change *and* to indicate people who did not need to have other owners approve their changes. We have never needed to explicitly indicate which group any particular individual falls into, and I'm not yet convinced that we need to change things now.

Google3 is no different; their OWNERs files also support the "emeritus" comment, but it is only a comment at the reader. The tools do nothing special with it as far as I know.

As one last data point, WebKit actually had (and has) a different policy: every change need to be approved by an owner (reviewer), even changes by another owner. When we did the Blink fork, I actually added code to the owners check to support this model, but we never actually used it, no one missed it, and I deleted the code a few months ago.

-- Dirk