A call to arms: Reviewers wanted. #2122

Closed
elmart opened this Issue Mar 9, 2015 · 73 comments

Projects

None yet
@elmart
Member
elmart commented Mar 9, 2015

The problem

It's been for some time now that a disturbing thought has been coming to my mind: Though neovim project is in good health, I feel that, somehow, we're losing traction.

Dont' get me wrong. We've achieved phenomenal progress, and have a steady contribution pace. But what I mean by "losing traction" is that I think our pace could be much higher, if we were able to handle it. In other words, I feel there are a lot of potential contributors that get pissed-off because of their first PR's taking too much time before getting reviewed/merged. A quick turnaround time on good PR's is essential to keep people engaged.

BTW, this is by no means a complain on @justinmk's work. On the contrary, I can only praise his impressive dedication and the amount of work he's been able to cope with. It's really difficult to find the time to do what he does, at a sustained pace, and for so long.

Having more people merging code, or trying to cope with PR's in a more FIFO way, as it's been suggested in #2056, could help, but, IMO, that's not the real bottleneck. The crux of the problem, to my understanding, is reviewing. To state it clearly: We need more reviewers. Less time put by people on doing their own things, and more time dedicated to review others'.

Now, why people isn't already reviewing more? To my understanding, there could be several reasons:

  • (1) It's more fun to do your own thing than to review others'.
  • (2) Good reviewing often requires not only a good knowledge of C, but also a fair understanding of the project status/direction.
  • (3) Review work doesn't get visible/rewarded/recognized.

There's little we can do about (1), apart from appealing to people's good citizenship and altruism, for the sake of project's success.
As of (2), I'd say many PR's don't require a project-wide vision. And also, if reviewing was somehow done more attractive, many people would quickly get to a good understanding by reading others' reviews for a little while.
Thus, we reach (3), for which something can indeed be done, IMO.

Problem is that good, thorough reviewing takes a lot of effort. Often, longer than it takes to make the changes themselves. But that effort is reflected nowhere. It has no public visibility. After corrections have been incorporated into original commits, even comments often dissappear due to rebasing, if people comment on commits instead of diffs. There are people out there doing an incredible review work (@oni-link comes first to my mind), who are altruistic enough not to care much about that. But, let's admit it: many others probably care, and there's nothing wrong about that. It's perfectly understandable that many people find frustrating putting so much effort on something that, very often, will leave no trace.

I find this situation a major pitfall of open-source development in general, and of GitHub workflow in particular. In fact, I wrote to GitHub's staff some time ago, asking for some way to publicly reward reviewers. They kindly responded it was an interesting suggestion, but it won't probably land soon. So, can we do something about that in the meantime?

A possible improvement?

I think we could try a simple approach to make reviewing more attractive: Let's credit reviewers by setting them as authors of review commits. I mean, instead of integrating corrections into original commits, let's add separated review commits, setting the author to the actual reviewer, instead of the PR's owner. You can easily do that with git commit --author=<...>. Final PR could reorder those review commits to follow original commit, but maintain them as separated commits. That would increase reviewer's commit count, and would push him up in the contributor list, which, IMO, is a fair recognition for his work.

I would also make some nuances to that:

  • doing it only for semantic corrections, not for styling corrections, as a way to discourage superficial, style-only reviews.
  • doing it only once per ammended-commit/reviewer combination.

Those are details that should be developed by the community. But the general approach, I think it could work. It would add some noise to the commit log (also some more faithful accountability for changes), but in exchange for that, we could get more people engaged in reviewing, thus boosting project's output, and what is more important, keeping contributors happier.

What do you guys think of this?

P.S. I had been thinking all this for a while, but it was #2056 to push me to write this. I think a separated thread is in order, though, not to hijack that PR.

@justinmk
Member
justinmk commented Mar 9, 2015

Definitely a good idea. Git has Helped-by: and Signed-off-by:, and maybe some others. If there is a git tag that GitHub would pay attention to, that would be ideal.

@elmart
Member
elmart commented Mar 9, 2015

Yes. I tried to explain them it seemed an easy thing to do, that could really impact the game, but I'm not sure they really considered it. Unfortunately, there's no place where feature requests for GitHub are publicly shown/discussed (and where a popular request can draw attention to it by many people upvoting it). You have to email them, instead, individually. Which is a pity, since that way is much less probable they can have good feedback. We can do nothing about that, though. So, let's see what others think about proposed "manual" approach.

@tarruda
Member
tarruda commented Mar 9, 2015

@elmart great initiative as always.

@jszakmeister
Member

Having more people merging code, or trying to cope with PR's in a more FIFO way, as it's been suggested in #2056, could help, but, IMO, that's not the real bottleneck.

How do you intend to address the problem of knowing whether the review that was done was useful or not? How do you address the issue of a review not being thorough enough? One nice feature of having a commit bit attached to your name is that we at least know some minimum bar was met. How do you see tracking this across a community of potential reviewers?

As of (2), I'd say many PR's don't require a project-wide vision.

It might not be project-wide, but it's certainly wide. Take the task of changing from Vim's types to our own new types. It seems like an easy thing, you just change the type and cast where you need to. Instead, you are mired in decisions that impact the project as a whole, and if you go about it the wrong way, we are no better off than where we started (and possibly worse off). That's just one example, but I think there are many more like it.

For me, at least, I'm blocked by issues that fall into a realm that I'm not familiar with yet, or don't know what the project policy is. I also think it would help to have a sketch of the architecture (or future architecture) of Neovim to know where things are going. I don't think simple things are getting blocked nearly as much as ones where project policy comes into play.

(3) Review work doesn't get visible/rewarded/recognized.

I honestly believe this issue is not the number one problem with getting reviews done and code merged. Perhaps I'm out of touch, but in the numerous projects that I've worked on over the years, recognition for reviews has never been the prime mover. I think it would help, but I don't think it's going to solve the problem. We need to scale, and that means cultivating trusted reviewers and mergers (committers), IMHO.

I mean, instead of integrating corrections into original commits, let's add separated review commits, setting the author to the actual reviewer, instead of the PR's owner.

I've got several issues with this approach:

  1. Contributing just got even more more cumbersome, or merging just got more cumbersome.
  2. What do we do with fixups? Keep those commits separate?
  3. Email address may not be exposed for all reviewers (and perhaps they want it that way).

I'm all for giving credit (and just took a few minutes to contact GitHub asking them to recognize Reviewed-by and a few other tags in commit messages), but I'm not sure the approach of generating review commits is sustainable. Many contributors (at least in my experience of 15+ years of Open Source development) aren't very sophisticated with their tools. You end up having to guide them through this process, and working very closely with them to make it all happen/work. I spent over an hour this weekend going back and forth with someone on a different project to guide them through fixing up their history. It's tedious and time consuming.

If we do anything, I think it should be Git's (the project) model of putting tags into commit messages (Reviewed-by). It's at least a little easier to amend a commit that remembering to generate/regenerate it with right author.

All of that said, I hope there's a balance we can strike and still get more reviewers and committers on board.

@jebaum
Contributor
jebaum commented Mar 9, 2015

I have a good working knowledge of C and follow the project relatively closely. What keeps me from contributing or reviewing is lack of familiarity with the vim/nvim codebase. If I was going to review code, I would want to do it thoroughly and properly, and I don't feel like I can do that without taking a lot of time to become acquainted with the structure of the whole codebase by slowly going through it, which is time I'm not likely to have any time soon.

If there was some very thorough documentation about:

  1. the overall structure of the code (for instance how the event loop works. what do all the functions called in main_loop() do?)
  2. which files are responsible for what (more specifically than just the names of the files)
  3. what a lot of the types actually represent (linenr_T I can take a guess, buf_T I probably could too, not as sure about win_T, or msgchunk_T, or msg_hist, scid_T, etc...)
  4. a description of some of the most common functions (ml_get*)

I think it would go a long way. Perhaps structured as a guided walkthrough (e.g., open up main.c, look at this and this, here's what they do, this important thing calls this function in this file, go open up that one and take a look...). Sure anyone with good knowledge of C could take some time and figure these things out for themselves, but it makes the entry barrier a lot higher. The obvious downside is that writing all of this well would take a lot of time too, and there are probably not a lot of people with the knowledge needed to do it.

@tarruda
Member
tarruda commented Mar 9, 2015

If there was some very thorough documentation about:

I will take an afternoon this weekend to document the code structure

@jebaum
Contributor
jebaum commented Mar 9, 2015

Another thing I thought of - I took a crack a month and a half ago at implementing some of the functions suggested in #901, but had a difficult time figuring out how to make a new ex command. I ended up getting there after a couple hours, but I don't think it was a plesant experience. My memory is already fading on that though.

@jdavis
Member
jdavis commented Mar 9, 2015

@tarruda Just a reminder, there is this: https://github.com/neovim/neovim/wiki/Code-overview

It's probably just out of date and in need of some love.

@tarruda
Member
tarruda commented Mar 9, 2015

@tarruda Just a reminder, there is this: https://github.com/neovim/neovim/wiki/Code-overview

It's probably just out of date and in need of some love.

I will update it, thanks

@ZyX-I
Contributor
ZyX-I commented Mar 9, 2015

Yes. I tried to explain them it seemed an easy thing to do, that could really impact the game, but I'm not sure they really considered it. Unfortunately, there's no place where feature requests for GitHub are publicly shown/discussed (and where a popular request can draw attention to it by many people upvoting it). You have to email them, instead, individually. Which is a pity, since that way is much less probable they can have good feedback. We can do nothing about that, though. So, let's see what others think about proposed "manual" approach.

Maybe they are afraid of issues like this. Public bug tracker is good at sucking reputation if you don’t want to be responsive (though this issue). Though the only time I remember myself contacting github support they were fine and quick.

@doppioandante

As we are discussing reviewing, how do you usually review pull requests aimed at refactoring?
I find hard to understand if a variable whose type was changed isn't going to create any problem, because it's usually declared at top and github collapses all the code below it.

@jebaum
Contributor
jebaum commented Mar 9, 2015

@doppioandante as of ~6 months ago, this plugin worked well for me https://github.com/codegram/vim-codereview

hasn't been updated in a while. in any case, when I review nontrivial pull requests for other projects, I pull them locally so I can browse the code more easily, and take notes which I later paste back into github. that vim-codereview plugin could use some love

@ghost
ghost commented Mar 9, 2015

@elmart as you said at the end of #2056, actually reviewing things is more important than popularizing a FIFO methodology for PRs, which I now agree with. having Having said that, I have to agree with @jszakmeister that crediting reviewers isn't the number one problem. While I think that crediting reviewers isn't without merit, the suggested approach honestly sounds very tedious/inefficient.

Unless I'm missing how this will be done, I think this will be very inconsistent, that is to say we'll get people that amend their commits without adding a Signed Off by: field, people that add new "Review" commits without changing the author, etc. Of course the person who ends up merging the PR could end up fixing up the commits themselves, but I think it's unreasonable to add yet more load to commiters (who many times are also reviewing). I have very little confidence that this could be efficiently done without some sort of automation.

If we were to employ such a strategy, I think we should at least look into gerrit (or something like it). It looks like they have a GitHub plugin, but I'm not sure how complicated it would be to setup. Here's what the commit messages look like. Searching this repo yielded no results for gerrit, so I figure we might as well see what people think of it, given it can be considered a radical proposal.

If anything, it might be more work than it's worth (at least at this stage of development), so it might just be better to sign off on commits. There is GerritHub, but I'm not too keen on the permissions it requires when signing in with GitHub.

I'm all for giving credit (and just took a few minutes to contact GitHub asking them to recognize Reviewed-by and a few other tags in commit messages), but I'm not sure the approach of generating review commits is sustainable. Many contributors (at least in my experience of 15+ years of Open Source development) aren't very sophisticated with their tools. You end up having to guide them through this process, and working very closely with them to make it all happen/work. I spent over an hour this weekend going back and forth with someone on a different project to guide them through fixing up their history. It's tedious and time consuming.

Speaking of this, our CONTRIBUTING.md is lacking in a few places, e.g., we don't provide any links to how to rebase, make feature branches, or other stuff. Just a link to this page of the Git book would be helpful IMO, although there's probably more we can add. If anyone agrees, then I can submit a PR (doesn't really matter who submits it).

@ghost
ghost commented Mar 10, 2015

@doppioandante I use hub to locally review PRs. That way you can do things like git log -p -W, where -W shows the whole function as context in the diff.

@starcraftman

Hi, just commenting here as an outsider who ran across this from r/vim. I've just been watching neovim for a while. I have experience with C/C++, maybe I'll pitch in after getting reading up. This is a bunch of thoughts on what I read, a bit disjointed.

@Pyrohh I agree with you regarding CONTRIBUTING.md, it seems a bit light on details for a project with a scope this large. Regarding git, I agree with @jszakmeister a lot of people don't know their tools very well. Git rebasing/squashing can intimidate people, it is after all destructive. In addition to rewriting, you might also want to link to the rebasing page. You may still need some more directed comments, to avoid the back and forth as described above. Being verbose often saves time.

Another problem with CONTRIBUTING.md is it lacks a section on review process. It mainly seems to be written for the PR author. A simple checklist of what to do as a reviewer might help standardize. Checklists are proven to save a lot of time with repetitive tasks (I remember reading simple lists halved mortality at some hospitals). Another reason for them, is what seems obvious to YOU is not necessarily obvious to another. For example:

Reviewer Checklist

  1. Check Travis passes.
  2. Check style guide conformation (link to guide).
  3. Do Another Thing
  4. Test something....

As an outsider after having read CONTRIBUTING.md, all I know is that there are 3 stages of PR WIP/RFC/RDY. I don't see a clear guideline as want takes a PR from RFC to RDY. It says one person must have reviewed but who qualifies as that person? Does that mean someone with commit access, there seem to be 7 in the neovim group. Or can it be some random person like me? Does the originator of the PR say it is RDY? The reviewer? A commiter?

Regarding the consistency problem raised by @jszakmeister:

  • Maybe you should have a whitelist of people who are trusted reviewers in CONTRIBUTING.md so people know who counts.
  • Another idea, consider some mentoring model for reviewers? This has been used in large projects like KDE to on board devs.
  • Above checklist idea will standardize things somewhat.

Small side note, though perhaps relevant in coordinating review/commit & questions, do you plan on deciding regarding https://gitter.im/ ? This was raised in #1658, but left unresolved. Ability to discuss then use @/# notifications can be useful. As noted in the topic, lots of people use IRC for support, gitter could be a mainly reviewer/commiter area?

Hope that feedback helps.

@edthedev

i also just wandered in from /r/vim, but I'm probably the sort the tan is looking to attract, so here's some scattered thoughts:

Regrading point 1, some of us would rather work on the big picture than on adding new shiny features. The most common reason i don't advocate on Pull Requests is if the project doesn't have clear direction that such input is welcome. This post basically solves that concern for this project, for me. It's worth making a strong point in Contributing.md, of course.

Redarding point 2, I second the suggestion of a checklist. I can likely self-moderate against a checklist, identifying in comments which steps i was qualified to perform and which not. Seems like a move on the right direction.

Regarding 3, I can speak from personal experience. The two open source contributions i am most proud of in the last week both disappeared from the project commit history. As motivated by pride as i am, i
I regret nothing. :) But i do appreciate your drawing attention to the current state of short stick that these contributions get.

I'm not sure a process that introduces commits is really necessary though. A thanks.md file in the project wiki would be credit enough, and might be simpler to implement.

@ghost
ghost commented Mar 10, 2015

I opened a PR for the CONTRIBUTING.md improvements.

@ghost
ghost commented Mar 10, 2015

@starcraftman forgot to say, the checklist is a great idea IMO, so I added it to the aforementioned PR. I also think gitter is a great idea (especially since it has IRC integration https://irc.gitter.im/) but it should probably be discussed in the issue you mentioned as to keep this one on track.

@elmart
Member
elmart commented Mar 10, 2015

Sorry for late response. I couldn't answer before. I'll try to answer everybody (in separated posts, not to make a monster one).

@elmart
Member
elmart commented Mar 10, 2015

@jszakmeister

How do you intend to address the problem of knowing whether the review that was done was useful or not?

I think I may have been misunderstood here. What I'm proposing doesn't differ to what we do now to this respect. I mean, you, as a PR owner, still discuss with your reviewers which proposed changes you accept and which you don't, until everybody reaches an agreement. And it's you to write those changes and add them to your PR. It's just that instead of putting your name on them, you put the name of the reviewer on extra commits implementing those changes.

How do you address the issue of a review not being thorough enough?

Same as now, too. You can't know for sure, but you develop confidence in some people that consistently show you they do thorough reviews. Note also that we have always required more than one review. So, new reviewers shouldn't be ever alone. Their pitfalls should be signaled by non-new reviewers (and/or PR owner), from which they would learn. We may have in fact relaxed sometimes about the more-than-one review rule, precisely because a lack of reviewers.

It might not be project-wide, but it's certainly wide. Take the task of changing from Vim's types to our own new types. It seems like an easy thing

Well, I know. I've taken a very active role in type refactoring, and I know it's all but easy. But those are the kind of things a new reviewer with a good knowledge of C could quickly get acquainted with if a more experienced reviewer showed him the way.

For me, at least, I'm blocked by issues that fall into a realm that I'm not familiar with yet

Me too. And that's the reason I myself haven't reviewed much up to now. There's little we can do about that, too. I'm just appealing for us to try some more often. Specially core contributors.

(3) Review work doesn't get visible/rewarded/recognized.
I honestly believe this issue is not the number one problem with getting reviews done and code merged.

Note it's not number 1 for me, either, but 3. My point is this is something we can do something about, while that's more difficult for the previous 2 points.

I've got several issues with this approach:

  1. Contributing just got even more more cumbersome, or merging just got more cumbersome.

I don't see why. Simplest workflow we currently follow is:

  • someone signals a pitfall in your code, and suggests a better alternative
  • you discuss with him until finding an agreement
  • you do the changes, commit that, and then, by git rebase -i you integrate that commit with the original one (usually, by reordering it after original commit and choosing "fix").
  • you repush

What I'm proposing is just that on the new commit implementing suggested changes, you put the name of the reviewer instead of yours (using git commit --author=...), and, though you may reorder it after the original one, you don't "fixup" the original one, but leave the new commit there. So, it's not more complicated than we currently do. In fact, it would be simpler (no fixup step).

  1. What do we do with fixups? Keep those commits separate?

As said above, in principle, yes. I wouldn't do it for just stylistic changes, though, only for semantic changes. That admittedly introduces noise in the commit log, but also provides better accountability for changes.

  1. Email address may not be exposed for all reviewers (and perhaps they want it that way).

Possible, but rare. To be a reviewer, it's not much asking you've done some commits before, just to get the feel about process and know what's expected from reviewers. And to commit you need an email.

I'm not sure the approach of generating review commits is sustainable. Many contributors (at least in my experience of 15+ years of Open Source development) aren't very sophisticated with their tools. You end up having to guide them through this process, and working very closely with them to make it all happen/work. I spent over an hour this weekend going back and forth with someone on a different project to guide them through fixing up their history. It's tedious and time consuming.

I think that's true. But note we are already requiring some basic git abilities with our current approach. As explained above, what proposed here doesn't change that.

If we do anything, I think it should be Git's (the project) model of putting tags into commit messages (Reviewed-by). It's at least a little easier to amend a commit that remembering to generate/regenerate it with right author.

I certainly would prefer such an approach too. We could maintain a cleaner log. The point is: GitHub is not going to support such a thing soon, and I doubt there's anything we can do by our own automation and GitHub's API, though that's something that could be explored. Maybe recognizing those tags in master branch and automatically doing bogus commits somewhere just to increment reviewers' commit count?

All of that said, I hope there's a balance we can strike and still get more reviewers and committers on board.

Same here. What proposed here is the only thing I could think of we can do right now. But I do agree is far from perfect, and I'd be glad if a better alternative is found.

Note also that I do agree that rewarding reviewers is not the main reason why we lack reviewers. Most of us are here mainly driven by another forces (basically, we love vim, and want to help having a better one). But I know from experience how addictive a little gamification can turn out to be for many people, too. ;-)

@elmart
Member
elmart commented Mar 10, 2015

@doppioandante

As we are discussing reviewing, how do you usually review pull requests aimed at refactoring?
I find hard to understand if a variable whose type was changed isn't going to create any problem, because it's usually declared at top and github collapses all the code below it.

I use GitHub's interface only for small/simple PR's. Note, though, that you can expand folded blocks by clicking on hunk's header.

For non-trivial PR's, I prefer to navigate the code locally. For that, I also use hub, like in hub checkout https://github.com/neovim/neovim/pull/2125.

@elmart
Member
elmart commented Mar 10, 2015

@Pyrohh

I think most of your doubts should be answered by my message to @jszakmeister.
GerritHub could be interesting. Worth having a look. It could streamline review workflow, though it wouldn't address the review work recognition thing.

@elmart
Member
elmart commented Mar 10, 2015

@starcraftman I'm all in for checklists and improving wiki pages to better describe workflow.

EDIT: I have a reservation about them, though. Those kind of artifacts are good while serving as a guide for new people. They start being not so good when/if people become dogmatic about them. Good judgment about particular circumstances, as well as openness to changing guidelines when showed wrong should always prevail, IMO.

@elmart
Member
elmart commented Mar 10, 2015

@edthedev

Regrading point 1, some of us would rather work on the big picture than on adding new shiny features.

Note that, by now, we have restrained ourselves to 1) eliminating old cruft; 2) introducing key architectural changes; 3) making low-level refactoring that will enable much broader refactoring in a later iteration.

I mean, we have tried to keep "new shiny features" to a minimum. Those are targeted for future iterations. The recent terminal emulator would be one of the few exceptions to this.

Regarding 3, I can speak from personal experience. The two open source contributions i am most proud of in the last week both disappeared from the project commit history. As motivated by pride as i am, i
I regret nothing. :) But i do appreciate your drawing attention to the current state of short stick that these contributions get.

As said before in another post, I know most of us main motivation is well another one, and will continue doing it no matter other things. But, that said, I still think it's important rewarding work somehow, for people to feel appreciated.

@fwalch
Member
fwalch commented Mar 10, 2015

Regarding review tools, see also #2121.

@ghost
ghost commented Mar 10, 2015

@elmart

I don't see why. Simplest workflow we currently follow is:

someone signals a pitfall in your code, and suggests a better alternative
you discuss with him until finding an agreement
you do the changes, commit that, and then, by git rebase -i you integrate that commit with the original one (usually, by reordering it after original commit and choosing "fix").
you repush
What I'm proposing is just that on the new commit implementing suggested changes, you put the name of the reviewer instead of yours (using git commit --author=...), and, though you may reorder it after the original one, you don't "fixup" the original one, but leave the new commit there. So, it's not more complicated than we currently do. In fact, it would be simpler (no fixup step).

I see, that doesn't sound too complicated, but I still think it might get tedious. Large PRs would end up having 10, 20, 30 of these commits, which would just make our history hard to review. I might be misunderstanding you here...

I think most of your doubts should be answered by my message to @jszakmeister.
GerritHub could be interesting. Worth having a look. It could streamline review workflow, though it wouldn't address the review work recognition thing.

It may not address it by adding review commits with individual authors, but it does add Signed off by: fields to requests where reviewers +1 or +2 a commit. I'm not sure if this is done automatically, or if a server-side commit hook must be used.

@jszakmeister
Member

I think I may have been misunderstood here. What I'm proposing doesn't differ
to what we do now to this respect. I mean, you, as a PR owner, still discuss
with your reviewers which proposed changes you accept and which you don't,
until everybody reaches an agreement. And it's you to write those changes and
add them to your PR. It's just that instead of putting your name on them, you
put the name of the reviewer on extra commits implementing those changes.

Sorry, I was responding to your comment in #2056:

I think main problem is not centralized merging

I took this as an argument against offering more people commit bits to the main
repo, and as saying that merging a PR is not a problem.

Right now, any time I merge a PR, I'm not simply looking for RDY to be set and
then opening the PR and merging it. I'm doing a review of the PR and either
providing feedback or merging it. I do it for several reasons: sometimes people
put RDY in the title instead of RFC, sometimes something important was missed
during a review, and primarily because I feel it's my responsibility to help
ensure that the process is being followed and reviews are being done well.

So, now we've involved more people, but I'm still doing my thing. Where is the
speed benefit? To help scale, I think we need to give the good folks who are
doing quality reviews the ability to merge the PRs themselves, or we need a
group of "trusted lieutenants" that when they say "Ready for merge", we can
trust that it actually is (I can skip my end and just merge it).

Otherwise, we're still bottlenecked at the merge step.

Same as now, too. You can't know for sure, but you develop confidence in some
people that consistently show you they do thorough reviews. Note also that we
have always required more than one review. So, new reviewers shouldn't be ever
alone. Their pitfalls should be signaled by non-new reviewers (and/or PR
owner), from which they would learn. We may have in fact relaxed sometimes
about the more-than-one review rule, precisely because a lack of reviewers.

I'm fine with keeping a list of folks in my head that I've come to appreciate.
I'd like to know who you appreciate too though. :-) If I have to develop that
same relationship, then we end up with a star-topology that doesn't scale well.
I think in practice it's not quite that bad, but it's certainly not the same as
naming the reviewer outright.

I think our more-than-one review rule is nice, but I agree that we have trouble
meeting that requirement right now.

Well, I know. I've taken a very active role in type refactoring, and I know
it's all but easy. But those are the kind of things a new reviewer with a good
knowledge of C could quickly get acquainted with if a more experienced
reviewer showed him the way.

There it is: "if a more experienced reviewer showed him the way." It's not
something I can just walk in off the street and learn, which is how you made it
sound. :-)

Me too. And that's the reason I myself haven't reviewed much up to now.
There's little we can do about that, too. I'm just appealing for us to try
some more often. Specially core contributors.

Sorry, but I cannot. I honestly give nearly every free moment I have to Neovim
and other open source work that I do. I sleep only 5-6 hours a night, I don't
watch any TV, and the only free time I have to contribute anything is generally
the hours before work in the morning (generally from 3:30 to 6:30)--which is
where much of my open work happens. I don't have more time to give unless I
short-change my family, and I'm not willing to do that. We need to scale and
asking the current core contributors to do more doesn't fix the problem. :-(

Note it's not number 1 for me, either, but 3. My point is this is something we
can do something about, while that's more difficult for the previous 2 points.

I agree giving credit is something that is well within our powers to do, and we
should find ways to do it. If what you are really purposing is let's take this
small step towards fixing the issue, then great, I'll drop the rest of this
discussion. But, I'd like to take steps to address the other issues if we can
too. I see "A call to arms" as implying that we should. :-)

I don't see why. Simplest workflow we currently follow is:

  • someone signals a pitfall in your code, and suggests a better alternative
  • you discuss with him until finding an agreement
  • you do the changes, commit that, and then, by git rebase -i you integrate that commit with the original one (usually, by reordering it after original commit and choosing "fix").
  • you repush

What I'm proposing is just that on the new commit implementing suggested
changes, you put the name of the reviewer instead of yours (using git commit --author=...), and, though you may reorder it after the original one, you
don't "fixup" the original one, but leave the new commit there. So, it's not
more complicated than we currently do. In fact, it would be simpler (no fixup
step).

My issue is precisely the git commit --author=... step. It's just not
something you use in a typical workflow. And fixing it when you screw it up
(read: forget) is a little tougher since you need to amend the commit with both
--author and --reset-author to have it take affect.

As said above, in principle, yes. I wouldn't do it for just stylistic changes,
though, only for semantic changes. That admittedly introduces noise in the
commit log, but also provides better accountability for changes.

Don't they deserve credit even if there are no changes? I don't think having it
in the commit log provides better accountability. It may currently show up
better on their GitHub profile page, but their name is on public data being
indexed by Google and other engines. It's pretty easy to point to and say
"yeah, I helped do that." And it's good on the other end when someone is
searching on them for the next job. :-)

Possible, but rare. To be a reviewer, it's not much asking you've done some
commits before, just to get the feel about process and know what's expected
from reviewers. And to commit you need an email.

Let me just point out that in helping to maintain Nose, emails weren't always
easy to come by. In fact, it sometimes took days to get a response. Most had
them visible, but when they didn't, it was work. Committing helps, but I'm
still searching through logs to discover it. As a result, changing the author
of a commit is not a half-second task. :-(

Add a Reviewed-by tag suffers similar problems (and it's the solution that I
advocate), but I'd like the email not to hold up getting the commits into shape.
If we were to add a Reviewed-by tag to the merge commit, then we could give
credit whether or not there were any changes to be made.

I think that's true. But note we are already requiring some basic git
abilities with our current approach. As explained above, what proposed here
doesn't change that.

No it doesn't. It is just a bit harder to dig up help on it though, since it's
not often done.

I certainly would prefer such an approach too. We could maintain a cleaner
log. The point is: GitHub is not going to support such a thing soon, and I
doubt there's anything we can do by our own automation and GitHub's API,
though that's something that could be explored. Maybe recognizing those tags
in master branch and automatically doing bogus commits somewhere just to
increment reviewers' commit count?

Why does GitHub have to support it? I know: because then you can see it on the
timeline. :-( I'm definitely a -1 on the bogus commits. And that doesn't help
anyone who goes to take a look at them.

Why not just make our trusted reviewers committers when we think they've reached
the bar? I think it speaks far more to have them be a part of the team and I
think it goes farther to helping us scale if we're willing to give out commit
access to more people.

I do realize that the downside here is that we give out commit access and wish
we hadn't. We should definitely keep the bar high to reduce this potential, and
give enough time to recognize whether interactions with others are what we
expect.

But I know from experience how addictive a little gamification can turn out to
be for many people, too. ;-)

Yes, it definitely can be. :-) I still want to go farther though. I want to
scale, and that means more than getting points to me. It means taking on a real
and recognized role in the project either through commit access, or by naming
them as an experienced reviewer that we trust.

I know the argument has been scattered all over, but let me summarize things as
I see them:

  • Crediting reviewers is important and we should take steps to do it
  • The problem is deeper than just giving credit though, and there are
    definite hurdles to doing a good review:
    • Getting to know the code base
    • What is the current and future architecture
    • Knowing the project's policies
  • That we should (as the Neovim team) be willing to help coach reviewers (likely
    by example) and work to develop a trusted relationship with them
  • Acknowledge that either through commit bits (especially if we are expecting
    reviewers to have contributed) or by naming them outright in some manner (a
    reviewers file publicly thanking those who have stood with us and achieved
    trusted reviewer status). This is addition to crediting reviewers in the
    way @elmart has suggested or using Reviewed-by tags in the commit.

If you want to start with the first step, I'm all for it. But I think we need
the other steps to be successful in achieving what we need.

PS. I know tone can be is often lost in text-based communications, so I want you
to know that I'm happy you brought up the topic and that you are willing to
discuss it! :-)

@starcraftman

@jszakmeister Very well put, I think the summary is spot on. Perhaps you should have added the bottleneck note there too as I think that is also important. More cooks in the kitchen without organization sometimes just means a bigger mess! :)

It is just like bringing new people into a regular programming team. You can't expect them to know all your written/unwritten rules/goals/methods since they weren't there day 1. They learn when a) you tell them directly or b) read the process/info written down somewhere or c) they observe it over time. C doesn't work so well over the internet due to the non-daily meetings/contact.

@elmart
Member
elmart commented Mar 12, 2015

I think the title I chose for this thread has created some confusion.
I didn't pretend for people outside the project flocking here to do reviews from day one.
I rather pretended to appeal for core contributors (the ones who know the code/project most) to try to dedicate more time to reviewing. Also, I didn't mean "more time, in addition to what you currently do". I understand time is limited and we all do what we can. I rather meant "trying to divide your time differently: more time to review / less time to write new code".

Of course, anyone wanting to help reviewing is welcomed. But it's true they probably won't be able to do much if they don't have a good amount of already existing reviewers to learn from.
And, of course too, this is open source and everybody can do whatever he feels like with his free time. I'm just trying to say I think it would be good for the project if we did so.

Regarding proposed commit strategy, I won't do anything by now. Something like that requires a strong consensus, which clearly isn't there.

I'll leave this open some more time, in case anybody can come with a better proposal.

@jszakmeister
Member

I think the title I chose for this thread has created some confusion.
I didn't pretend for people outside the project flocking here to do reviews from day one.
I rather pretended to appeal for core contributors (the ones who know the code/project most) to try to dedicate more time to reviewing. Also, I didn't mean "more time, in addition to what you currently do". I understand time is limited and we all do what we can. I rather meant "trying to divide your time differently: more time to review / less time to write new code".

Understood. Though, things like this will just end up taking longer to do. :-(

And, of course too, this is open source and everybody can do whatever he feels like with his free time. I'm just trying to say I think it would be good for the project if we did so.

I assume you mean the core team spending more time on reviews in this last statement?

Regarding proposed commit strategy, I won't do anything by now. Something like that requires a strong consensus, which clearly isn't there.

It's definitely hard choosing from several solutions with none of the being a clear winner and all of them having some unfortunate down sides.

I'll leave this open some more time, in case anybody can come with a better proposal.

I'd still like to see us expand the base of the core team. You never know when any one of us will face downtime from other things going on in our lives, and to ensure the vitality of the project.

@elmart
Member
elmart commented Mar 13, 2015

I assume you mean the core team spending more time on reviews in this last statement?

Yes, that's what I mean.

You never know when any one of us will face downtime from other things going on in our lives

I myself will probably be contributing less for the next months to come. I will be landing on a new day-job and you know what happens until you get up to speed.

@jszakmeister
Member

@jszakmeister Very well put, I think the summary is spot on. Perhaps you should have added the bottleneck note there too as I think that is also important. More cooks in the kitchen without organization sometimes just means a bigger mess! :)

Thank you, and exactly! :-)

It is just like bringing new people into a regular programming team. You can't expect them to know all your written/unwritten rules/goals/methods since they weren't there day 1. They learn when a) you tell them directly or b) read the process/info written down somewhere or c) they observe it over time. C doesn't work so well over the internet due to the non-daily meetings/contact.

Actually, I think C can work quite well since so much interaction is out in the open--assuming there's not much back channel stuff happening (there's not in Neovim). It's A that I find harder. Reaching out to everyone and guiding them is hard to scale. The Law of Conservation of Energy ultimately wins. :-)

@fwalch fwalch referenced this issue Mar 15, 2015
Closed

gitter.im #1658

@jebaum
Contributor
jebaum commented Mar 19, 2015

For whatever it's worth, spring break for most US universities is beginning next week. I'm still a student, and I wouldn't be surprised if a number of other potential neovim contributors are as well. If @tarruda or anyone else can find the time to write some documentation by this weekend, I would definitely spend a lot of time next week while I'm not busy with classes going through it and learning the codebase, and hopefully start reviewing code and tackling some of the simpler issues.

I was planning on doing that anyway, but documentation would definitely speed up the process.
If I'm the only person keeping an eye on this who's still in school, sorry for the noise

@tarruda
Member
tarruda commented Mar 19, 2015

For whatever it's worth, spring break for most US universities is beginning next week. I'm still a student, and I wouldn't be surprised if a number of other potential neovim contributors are as well. If @tarruda or anyone else can find the time to write some documentation by this weekend, I would definitely spend a lot of time next week while I'm not busy with classes going through it and learning the codebase, and hopefully start reviewing code and tackling some of the simpler issues.

After #2076 is merged I will create a tutorial on how to develop/debug Neovim, including information about code structure.

@jwhitley jwhitley referenced this issue in codegram/vim-codereview Mar 19, 2015
Merged

Fix help syntax problems in docs #13

@jwhitley
Contributor

@elmart As someone who has been following neovim since it started, and has been looking to get into contributing more directly, I'll chime in that reviewing is a great and now-obvious way to start that. I'll agree that bringing in droves of random reviewers probably isn't what's needed. OTOH, using reviewing as one concrete way to ramp-up existing would-be contributors such as myself would probably be helpful to the project.

@jwhitley
Contributor

Random idea: how about a needs review issue label, as a general way of calling out PRs which need some (or extra) scrutiny? Or should just the existence of a PR be enough for that?

@jwhitley
Contributor

@justinmk Thanks for the clarification. I've seen RFC here but misinterpreted it, as it had a somewhat different usage on some prior projects I've worked on.

@elmart
Member
elmart commented Mar 22, 2015

As my proposal of leaving review commits authored by reviewers didn't have much success (and I agree it's not ideal, because of log pollution), I've opted for the other suggested alternative: adding Reviewed-by and Helped-by tags to commit messages. I hope someday GitHub can honour them.

An example can be found at #2184, and its merge commit 4bc62c7.
I've added Helped-by tags to commits where a non-cosmetic change has been introduced due to another person's suggestion, and a Reviewed-by tag to the merge commit (which means it applies to the full group). I'll be doing this from now on.
If others want to follow this practice, I'd suggest the following conventions, to ease future recognition by tools:

  • Tag lines (of the form <tag-name>: <tag-value>) go at the end of commit message.
  • Each tag must be on a separated line, being its only content.
  • Tags can be repeated, to support multiple values. Add a new <tag-name>: <tag-value> line for each value. That eases a lot usage of that info by tools. For example, git log --grep='Helped-by: <name>' can be directly used to get all commits where someone has contributed something, not being the author.

Well, that's it by now.
More suggestions are welcome.
I'll be closing this after some days if no activity.

@tarruda
Member
tarruda commented Mar 22, 2015

dding Reviewed-by and Helped-by tags to commit messages.

Are you using a commit template for that? If so it might be a good idea to add it to the contrib directory for other contributors.

@elmart
Member
elmart commented Mar 22, 2015

Are you using a commit template for that?

Well, no. It's as simple as adding Helped-by: $name <$email> at the end.
The name and the email, you get them through git log --author=<something>.
Same with Reviewed-by.

I do have some ultisnips templates for certain kinds of message bodies, like the

Problem    : 
Diagnostic :
Rationale  :
Resolution :

pattern I like to follow in coverity/clang-analysis commits, but that's unrelated to this.

@tarruda
Member
tarruda commented Mar 22, 2015

Well, no. It's as simple as adding Helped-by: $name <$email> at the end.

I know, but it is useful for less disciplined contributors(like myself). Having that automatically inserted in each commit template might result in more people following this pattern.

@jszakmeister
Member

Tag lines (of the form : ) go at the end of commit message.

FWIW, Git calls them "trailers" and has been building up more tooling to support the concept. For instance, git interpret-trailers is a fairly recent addition.

@elmart
Member
elmart commented Mar 22, 2015

@jszakmeister

Git calls them "trailers"

Great! Didn't know about that, thx.

@tarruda

Ok. I'll have a look to see if can contribute some things to that respect. A git hook on prepare-commit-msg can provide a default template containing those tags. And another git hook on commit-msg could automatically delete empty tags, to avoid you having to do it manually for normal commits where you haven't been helped/reviewed yet.

UPDATE: I'm also tempted to see if we can use git interpret-trailers for this, as it seems really nice. I doubt, though, cause that would leave out users with a not-so-recent version of git.

@tarruda
Member
tarruda commented Mar 22, 2015

Ok. I'll have a look to see if can contribute some things to that respect. A git hook on prepare-commit-msg can provide a default template containing those tags. And another git hook on commit-msg could automatically delete empty tags, to avoid you having to do it manually for normal commits where you haven't been helped/reviewed yet.

That would be great 😄

But since this is aimed at "encouraging" people to use this commit pattern, maybe a more strict behavior would be better: The commit is aborted on empty tags. This would force the committer to manually deal with the tags(either by removing or by filling with some information)

@elmart
Member
elmart commented Mar 22, 2015

The commit is aborted on empty tags

I'll give it a try and see how it feels working with that. I'd like to encourage people doing it, but I wouldn't like to upset them by being too much rigid. Also, I have to think a bit about it, cause doing it blindly can preclude some instances where you create commits based on existing ones (like in rebase).

I think I can do it so that the tags only get inserted when fixing up a previous commit. If I can, that would be ideal, IMO.

@ghost
ghost commented Mar 22, 2015

Why not just git commit -s / --signoff? I imagine it could be forgotten though... and I am not aware of how a "signoff" would behave during a squash.

@elmart
Member
elmart commented Mar 22, 2015

Why not just git commit -s / --signoff?

Not exactly the same. By git commit -s, you can only sign-off as yourself. Here we are talking about doing it on behalf of others. You can manually add Signed-off-by: <another person>, but then you're at the same case we're discussing.

Regarding meaning, Signed-off-by would be equivalent to Reviewed-by. I just happen to like this last one more. Approved-by would be another good alternative. So, such tags would mean: the mentioned person stated he looked carefully at changes in this commit (or groups of commits), and he's ok with them. So, this is to express other people's conformity.

The other tag I proposed, Helped-by:, has a different meaning: It means, I (the commit's author) made some improvements over this commit's original content, due to kind suggestions by the mentioned person. So, this is a way to acknowledge co-authorship.

@ghost
ghost commented Mar 22, 2015

I see, thanks for explaining.

@mseeber
mseeber commented Mar 22, 2015

you could work with git tags to let reviewers sign a tag on a commit themselves:
https://stackoverflow.com/questions/2423520/trusted-development-path-in-git-with-signatures/2423637#2423637

But this will also complicate things, as signing a tag only makes sense after the Branch has been cleaned up and is ready to be merged. This would also create a lot of tags. And requires the Reviewers to be able to push their tags into the pull request. Not to forget the issues with distributing public keys, etc...

Looks very complicated to me and is probably not worth the effort.

@elmart
Member
elmart commented Mar 22, 2015

Note that we are in the context of a normal GitHub pull-request workflow.
So, nobody other than the author can push to the PR.
Thus the need for him to act on-behalf-of-others.
Also, I prefer to put work as much work as possible onto the author's, and not the reviewer's shoulders.
I think the proposed mechanism is easy enough and fulfills all those requisites.
I'll open a PR when I have some hooks/templates to help in implementing it.

@mseeber
mseeber commented Mar 22, 2015

Good points, 👍

@Shougo
Contributor
Shougo commented Mar 23, 2015

I'm confusing.
How I create for reviewed commit?

@ghost
ghost commented Mar 23, 2015

@shougo just git commit --author="Persons Name <email@address.com>"

@Shougo
Contributor
Shougo commented Mar 23, 2015

Thank you!

@elmart
Member
elmart commented Mar 23, 2015

Note that that approach (setting the author of review commit) has been turned down.
We are now proposing using trailers (tags of the form Reviewed-by: Some Person <some@mail> at the end of commit messages).
See this comment.

@Shougo
Contributor
Shougo commented Mar 23, 2015

Hmm.... I should use the approache?

@elmart
Member
elmart commented Mar 23, 2015

The current suggested approach is the one in the comment I linked in my previous post.
This is, work normally, but, as part of the final commit cleaning before getting merged, add Helped-by and Reviewed-by trailers to your commits if they apply.

@elmart
Member
elmart commented Mar 23, 2015

I'm currently working on some tools to help with this. When I'm finished, I'll add a wiki entry explaining the suggested approach and how those tools can help you to enforce that workflow.

@Shougo
Contributor
Shougo commented Mar 23, 2015

Thank you! It seems good.

@jdavis jdavis referenced this issue in neovim/neovim.github.io Mar 26, 2015
Merged

Upcoming Newsletter #82

22 of 22 tasks complete
@elmart
Member
elmart commented Mar 29, 2015

See #2219.

@ghost
ghost commented Mar 29, 2015

I think you mean #2291.

@splinterofchaos
Member

Haven't read through this whole discussion, so maybe this has been proposed, but I think one thing that might help would be to link to all PRs in [RFC] status in the README.md and encourage users to review and comment on them. Also this could be linked from the wiki on "how to contribute".

Here's the link: https://github.com/neovim/neovim/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+%5BRFC%5D

I also feel that reviewing parts of code one has no familiarity with, or that the author may be the for-most expert on, can be daunting because it can be difficult to tell how the changes affect nvim's behaviour as a whole. Listing the contributors most knowledgeable on various areas of the codebase in the wiki so that new contributors can ping them for a review or comment may help.

I like the idea of "reviewed-by" tags, although I'm a little fuzzy it'll exactly work.

@elmart
Member
elmart commented Mar 29, 2015

@Pyrohh Yes. Thanks!

@elmart
Member
elmart commented Mar 29, 2015

@splinterofchaos There's already such a link. See second badge on Readme header.

@ghost
ghost commented Mar 29, 2015

@elmart Well it's quite small, so might be easy to miss. Also, I noticed the link points to waffle.io, and I asked earlier but I'll ask again, does anyone actually use it?

@splinterofchaos
Member

Indeed, I did miss that badge, The more badges, the less each one stands out. Maybe we could move it to right below that graph of closed PRs/issues per week.

@philix
Contributor
philix commented Mar 30, 2015

I stopped reviewing code when the list of open PRs consisted mostly of refactoring PRs that are almost impossible to review (style changes, bad commit log). I'll have some free time in the coming months and will help with reviews.

@ghost
ghost commented Mar 31, 2015

It was mentioned earlier in the thread that mentors for reviewers would be helpful, but how about mentors for commiters? Of course having reviewers is good, but it seems like we currently have a bunch of PRs which probably wouldn't require much work to be merged, because they've already been reviewed by a few people.

I mention this because while we have a decent amount of people with commit bits, they don't seem to get used often, but let me be clear: I'm by no means asking people work more, just that we establish some sort of system of trust. The few times I've seen people who aren't @tarruda or @justinmk commit things, they usually explicitly ask to do so beforehand. It's very rarely that I see people who aren't the repo owners (as GitHub describes them) say things like "If no one objects, I'll merge this soon". I don't necessarily think it's fair to call them BDFL(s), because they don't wholly dictate the projects direction, but (_edit_: finish this sentence) they certainly dictate the flow of when things are merged.

I'm not knowledgeable enough in certain project aspects currently, but perhaps people like @oni-link, @philix, and other knowledgeable contributors could be encouraged to merge their own PRs after others have reviewed them (I'm not sure if they already do). Of course they already are allowed to merge PRs, but as I mentioned earlier it doesn't seem to be the norm. (I don't mention @elmart because he already seems to merge his own stuff, and @jszakmeister already seems to deal with build system PRs pretty quickly).

From a shallow perspective, I think it might be just be a lack of confidence, i.e. it might not be obvious what's required before merging things. There's a checklist for reviewers on the wiki, but how about a checklist for commiters? If core contributors are encouraged to merge their own things, IMHO this will logically progress to said contributors asking more questions about merging procedure, and eventually feel comfortable merging other PRs.

Maybe I'm wrong on this, but the people mentioned earlier seem very knowledgeable about programming in general as well as about specific (or broad) areas of nvim's source code, which I think should be rewarded with some degree of independence. Of course I don't mean full independence, this is a community project after all. I just mean less reliance on one or two commiters, so that the project won't stall in merging PRs if one or both of them are busy.

@elmart
Member
elmart commented Mar 31, 2015

For what regards me, since I was given commit permissions I've been merging my own PR's (after being reviewed by others, of course). I haven't merged other people's PR's, though, mainly because I haven't reviewed other people's PR's much, as I reckoned before. It is my intention to review more from now on, specially PR's dealing with tasks/areas-of-code I'm comfortable with. And those PR's I review, I can of course merge them at the end, so that we take some load off @justinmk's back.

I would encourage other people with commit rights to do the same. So, I do agree with you.
To avoid collision with main maintainer, we can use the Assignee field to the right (to know who is in charge of merging at the end).

@ghost
ghost commented Mar 31, 2015

To avoid collision with main maintainer, we can use the Assignee field to the right (to know who is in charge of merging at the end).

👍 This sounds like something that should be on the wiki. I can create a page if no-one objects.

@fwalch
Member
fwalch commented Apr 1, 2015

To avoid collision with main maintainer, we can use the Assignee field to the right (to know who is in charge of merging at the end).

👍

I'm not comfortable with most areas of the code, but I'll try to do some reviews/merges of simple(r) PRs.

@rev112 rev112 referenced this issue Apr 6, 2015
@rev112 rev112 Remove redundant NULL checks
menu_text() never returns NULL, because vim_strsave() and vim_strnsave()
never return NULL.
9890885
@justinmk
Member

We improved the docs. And, just a reminder to those with the commit-bit, please merge anything you judge to be low-risk and/or sufficiently reviewed. In particular, vim-patches need to be accelerated.

@justinmk justinmk closed this Aug 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment