On Wed, Oct 18, 2017 at 12:37 PM, Joerg Schilling < [email protected]> wrote:
> Peter Tribble <[email protected]> wrote: > > > Integration requires code review. One issue we do have is that we could > do > > with more code reviewers. > > Even though this seems to be correct for most putbacks, it is not true in > general. > > > As a statistic, here is (roughly) the distribution of how many reviewers > per > > commit. > > > > 0 12 > > 1 483 > > 2 1316 > > 3 884 > > 4 437 > > 5 139 > > 6 59 > > 7 13 > > 8 14 > > 9 3 > > 15 1 > > > > There's clearly a peak at 2-3 reviewers. I suspect that for many simple > > changes > > people see it's been reviewed and move on. > > > > (Of the 12 with no reviews, 10 are post-commit fixups, 1 is a typo in the > > commit > > message, only 1 [5524] appears to have no reviewers listed.) > > Statistically this looks OK, if there was no putback without code review. > > BTW: How did you create your statistic? > run git log through awk, counting "Reviewed By:" for each "Approved By:" > I see that 2964, 2408, 2831, 852, 509, 345, 225, 244, 59, 58, 38, 2, have > no > review OK, let's check that. 2964 need POSIX 2008 locale object support Reviewed by: Robert Mustacchi <[email protected]> Reviewed by: Gordon Ross <[email protected]> Approved by: Dan McDonald <[email protected]> Hm. That's been reviewed. Maybe the next one? 2408 CJK character width handled incorrectly in terminal emulators 3019 localedef(1) manpage is pretty out of date Reviewed by: Yuri Pankov <[email protected]> Reviewed by: Lauri Tirkkonen <[email protected]> Reviewed by: Andy Stormont <[email protected]> Approved by: Gordon Ross <[email protected]> Perhaps we're unlucky. 2831 bring Joyent/SmartOS OS-1186 and OS-1187 to Illumos Reviewed by: Theo Schlossnagle <[email protected]> Reviewed by: Robert Mustacchi <[email protected]> Reviewed by: Eric Schrock <[email protected]> Reviewed by: Garrett D'Amore <[email protected]> Approved by: Garrett D'Amore <[email protected]> Nope. Your statement about there being no review is false. and I know that related code is buggy in a way that could have been > avoided by a code review. In that case, why did you not review the code? And if you know of bugs, what issues have you logged? If you don't participate, you are the only one to blame. > In other words, a quick check identifies more than > 12 putbacks without code review and some of them have visible bugs; my > method > to look for those putbacks is very simple Too simple; I suspect you're finding the post-commit fixups rather than the genuine commit. > and I may have missed many other > putbacks without code review. > > In addition, there is 354 which has been added even though there have been > several unfixed bugs identified by the code review of the related code. > > BTW: These bugs are still unfixed. > Please quote the bugids you have logged to report that. > > There's nothing missing from the IPS format either, and that's trivial to > > convert > > to other formats. It's just much easier to only maintain the metadata > once. > > IPS misses the meta data that is needed to permit a split / & /usr > You can run split / and /usr with both IPS and SVR4 distributions today. (You probably wouldn't want to, as the dependency tree is getting sufficiently dense that it's pulling in too much of /usr, but that's not a packaging issue.) > > > The advantage of the format is that it supports a split / and /usr and > > > this is > > > something I do not like to miss for future options. > > > > > > > Split / and /usr is completely unrelated to packaging. > > > That may be your private opinion. I know that IFS misses meta data for this > purpose. > Please tell us exactly what metadata is missing. -- -Peter Tribble http://www.petertribble.co.uk/ - http://ptribble.blogspot.com/ ------------------------------------------ illumos-discuss Archives: https://illumos.topicbox.com/groups/discuss/discussions/T784fc87098d66577-M25a63885a95dd16f34844059 Powered by Topicbox: https://topicbox.com
