> Not exactly - since this includes post commit review too. Since we> don't have any positive acknowledgement of post-commit review (when > there are no comments being provided) a code owner will generally end > up having to perform any cases of post-commit review in there area > (since they have no way of knowing that someone else also did so if > they provided no feedback). > > Certainly if a code owner comes up with some way to manage the > post-commit review of their trusted reviewers using a shared queue, > that could be a way to alleviate some of this load. >I think it would be good policy to have anyone doing a formal (thorough) review send a mail to the code owner with comments (or an OK). Code owners can then organize reviewers the way they want. Keeping track of what has been reviewed is essential, since the majority of commits (I am guessing) are post-commit reviews. Having a person quickly scan over a patch, especially if it ends up being a self-review in case the code owner is committing code, is unlikely to improve things. I don't want a heavy-weight process, but if people are going to come in and drop out etc, there has to be some way of keeping track of things. - Jan
On Mon, Nov 12, 2012 at 10:09 AM, Jan Sjodin <jan_sjodin at yahoo.com> wrote:>> Not exactly - since this includes post commit review too. Since we > >> don't have any positive acknowledgement of post-commit review (when >> there are no comments being provided) a code owner will generally end >> up having to perform any cases of post-commit review in there area >> (since they have no way of knowing that someone else also did so if >> they provided no feedback). >> >> Certainly if a code owner comes up with some way to manage the >> post-commit review of their trusted reviewers using a shared queue, >> that could be a way to alleviate some of this load. >> > > I think it would be good policy to have anyone doing a formal (thorough) review > send a mail to the code owner with comments (or an OK). Code owners can > then organize reviewers the way they want. > Keeping track of what has been reviewed is essential, since the > majority of commits (I am guessing) are post-commit reviews. Having a person > quickly scan over a patch, especially if it ends up being a self-review in case > the code owner is committing code, is unlikely to improve things. I don't want > a heavy-weight process, but if people are going to come in and drop out etc, > there has to be some way of keeping track of things.Phabricator provides a way to set up notifications for changes to certain parts of the codebase & you can then "accept" those changes which is probably the right way to do this, but I haven't tried the workflow myself. - David
On Mon, Nov 12, 2012 at 10:40 AM, David Blaikie <dblaikie at gmail.com> wrote:> Phabricator provides a way to set up notifications for changes to > certain parts of the codebase & you can then "accept" those changes > which is probably the right way to do this, but I haven't tried the > workflow myself. > > - DavidI currently use this and find it works quite well. The only issues with it that I've found are that everyone has to be registered and have the same username as svn for "raise concern" to alert them. The other is that you still get an audit even if the patch was approved pre-commit. The first issue is already being worked on. The second isn't really a generally solvable problem unless you include the phabricator Differential number in the commit message, as quite a few reviews end with "change x and commit". - Michael Spencer