Sean Silva <silvas at purdue.edu> writes:>> - I have to *remember* I submitted the patch (not hard, but it is a >> cost). > > If you forgot, the chances are high that the patch was unimportant. I > do my development on local git branches, so every time I do `git > branch`, I'm reminded. There's really no overhead.Every time you have to check it's overhead. It may be small overhead, but it's nonzero.>> - I have to save that e-mail from llvm-commits so I can refer to it when >> the inevitable ping is necessary. > > Maybe you need a better mail-reader? In gmail I just look in my "sent > mail" or if I have sent a lot of mail recently I search "is:sent > has:attachment". It literally takes 10 seconds.Ok, I'll give you this one. :)>> - I have to wade through tons and tons of commit e-mails searching for a >> response to my patch (for some reason the mailing list software often >> breaks threading). This is a more general problem with the current >> review process, not strictly a timeliness issue. > > Sounds like you need a better mail reader. I have never heard of this > even remotely being a problem in any way, at all, ever.Really? You've never heard comments about the huge volume of mail on llvm-commits? I just read another message about it today.>> - I have to send a ping e-mail. > > Once again, this takes like what, 10 seconds? I find it highly > unlikely that 10 seconds is a non-negligible amount of time compared > to the time spent coding, building, and testing any non-obvious (i.e. > needing review) change.Again, it's non-zero. I have to break out of whatever I'm doing, context switch and send the mail.>> - Now I have to look for responses to *two* e-mails. > > You definitely need a better mail reader. In every mail reader I've > ever seen, both mails get clustered into a thread. Maybe this is the > root cause of your vehement dissatisfaction with the current > email-based system?Even if threads are clustered, one still has to look through tons of subject lines to find it.> Have you considered the amount of quality code that gets produced with > the current process?Code quality doesn't reflect how efficient the review system is. It reflects how good the reviewers are and we are fortunate to have excellent ones. I find llvm-commits daunting. So much that I hesitate to do reviews. As Chris commented, I am not very active on that list. There's a reason for that beyond lack of time. By definition, making the process simpler and more organized should result in less time for reviews and thus hopefully more review participation.> What kind of improvements can we expect from such a system?A one-stop shop for reviewers to find patches. Something searchable by subsystem, file name, etc. I can't review every patch that goes by but I certainly can review patches for files I'm currently working in. If I could start my day by doing a patch search on whatever file I'm currently working on, I'd make it a daily practice to do some reviews. Right now that is tedious to do with e-mail.> Are there advantages that email has over a more structured > sort of review system like Phabricator?I don't know. I would like to know if there are any. I suppose it's super convenient to open the list of messages but beyond that I get overwhelmed.> Are you sure that the bottleneck > which causes high review latency isn't elsewhere in the system?Of course I'm not sure. I'm not sure anyone is sure. :) All I know is that there's a problem.> Or, as counterintuitive as it may seem, perhaps all of the things you > are pointing to as problematic (e.g. review latency, patches getting > dropped) are actually *beneficial* (maybe it prevents less-determined > people from becoming contributors?If that's our goal then I will just say right out it is not a good one. -David
David A. Green wrote:> I find llvm-commits daunting. So much that I hesitate to do reviews. > As Chris commented, I am not very active on that list. There's a reason > for that beyond lack of time.So the goal is to make it easier for a member of the community to review only commits to a sub-tree that interests them? Let's say it may or may not be easier for reviewers to monitor the Pull Requests of a spork than to write a clever filter for llvm-commits. And we'll also say that it may or may not be easier for reviewers to comment on a patch on Github than trying to reference code blocks in llvm-commits email. At this point we really don't know if one solution is better than the other, but we have good reason to believe Pull Requests might be a big win. So rather than all the talk, can we baby-step forward in a noncommittal way? How about allowing the code owner to add a line to CODE_OWNERS.TXT of the location to submit patches? If no location is given, assume llvm-commits. If the URI is a Github spork, the contributor should make a Pull Request. Thanks, Greg On Fri, Nov 16, 2012 at 3:50 PM, <dag at cray.com> wrote:> Sean Silva <silvas at purdue.edu> writes: > >>> - I have to *remember* I submitted the patch (not hard, but it is a >>> cost). >> >> If you forgot, the chances are high that the patch was unimportant. I >> do my development on local git branches, so every time I do `git >> branch`, I'm reminded. There's really no overhead. > > Every time you have to check it's overhead. It may be small overhead, > but it's nonzero. > >>> - I have to save that e-mail from llvm-commits so I can refer to it when >>> the inevitable ping is necessary. >> >> Maybe you need a better mail-reader? In gmail I just look in my "sent >> mail" or if I have sent a lot of mail recently I search "is:sent >> has:attachment". It literally takes 10 seconds. > > Ok, I'll give you this one. :) > >>> - I have to wade through tons and tons of commit e-mails searching for a >>> response to my patch (for some reason the mailing list software often >>> breaks threading). This is a more general problem with the current >>> review process, not strictly a timeliness issue. >> >> Sounds like you need a better mail reader. I have never heard of this >> even remotely being a problem in any way, at all, ever. > > Really? You've never heard comments about the huge volume of mail on > llvm-commits? I just read another message about it today. > >>> - I have to send a ping e-mail. >> >> Once again, this takes like what, 10 seconds? I find it highly >> unlikely that 10 seconds is a non-negligible amount of time compared >> to the time spent coding, building, and testing any non-obvious (i.e. >> needing review) change. > > Again, it's non-zero. I have to break out of whatever I'm doing, > context switch and send the mail. > >>> - Now I have to look for responses to *two* e-mails. >> >> You definitely need a better mail reader. In every mail reader I've >> ever seen, both mails get clustered into a thread. Maybe this is the >> root cause of your vehement dissatisfaction with the current >> email-based system? > > Even if threads are clustered, one still has to look through tons of > subject lines to find it. > >> Have you considered the amount of quality code that gets produced with >> the current process? > > Code quality doesn't reflect how efficient the review system is. It > reflects how good the reviewers are and we are fortunate to have > excellent ones. > > I find llvm-commits daunting. So much that I hesitate to do reviews. > As Chris commented, I am not very active on that list. There's a reason > for that beyond lack of time. By definition, making the process simpler > and more organized should result in less time for reviews and thus > hopefully more review participation. > >> What kind of improvements can we expect from such a system? > > A one-stop shop for reviewers to find patches. Something searchable by > subsystem, file name, etc. I can't review every patch that goes by but > I certainly can review patches for files I'm currently working in. If I > could start my day by doing a patch search on whatever file I'm > currently working on, I'd make it a daily practice to do some reviews. > Right now that is tedious to do with e-mail. > >> Are there advantages that email has over a more structured >> sort of review system like Phabricator? > > I don't know. I would like to know if there are any. I suppose it's > super convenient to open the list of messages but beyond that I get > overwhelmed. > >> Are you sure that the bottleneck >> which causes high review latency isn't elsewhere in the system? > > Of course I'm not sure. I'm not sure anyone is sure. :) All I know is > that there's a problem. > >> Or, as counterintuitive as it may seem, perhaps all of the things you >> are pointing to as problematic (e.g. review latency, patches getting >> dropped) are actually *beneficial* (maybe it prevents less-determined >> people from becoming contributors? > > If that's our goal then I will just say right out it is not a good one. > > -David > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On Fri, Nov 16, 2012 at 4:39 PM, Greg Fitzgerald <garious at gmail.com> wrote:> David A. Green wrote: >> I find llvm-commits daunting. So much that I hesitate to do reviews. >> As Chris commented, I am not very active on that list. There's a reason >> for that beyond lack of time. > > So the goal is to make it easier for a member of the community to > review only commits to a sub-tree that interests them? > > Let's say it may or may not be easier for reviewers to monitor the > Pull Requests of a spork than to write a clever filter for > llvm-commits. And we'll also say that it may or may not be easier for > reviewers to comment on a patch on Github than trying to reference > code blocks in llvm-commits email. At this point we really don't know > if one solution is better than the other, but we have good reason to > believe Pull Requests might be a big win. So rather than all the > talk, can we baby-step forward in a noncommittal way? > > How about allowing the code owner to add a line to CODE_OWNERS.TXT of > the location to submit patches? If no location is given, assume > llvm-commits. If the URI is a Github spork, the contributor should > make a Pull Request.This isn't viable; Github pull requests aren't visible on llvm-commits. -Eli
> code blocks in llvm-commits email. At this point we really don't know > if one solution is better than the other, but we have good reason to > believe Pull Requests might be a big win.I feel the opposite, TBH. All of LLVM's development history to date points to llvm-commits being awesome with a long, proven history, and pull requests are "unknown". -- Sean Silva On Fri, Nov 16, 2012 at 7:39 PM, Greg Fitzgerald <garious at gmail.com> wrote:> David A. Green wrote: >> I find llvm-commits daunting. So much that I hesitate to do reviews. >> As Chris commented, I am not very active on that list. There's a reason >> for that beyond lack of time. > > So the goal is to make it easier for a member of the community to > review only commits to a sub-tree that interests them? > > Let's say it may or may not be easier for reviewers to monitor the > Pull Requests of a spork than to write a clever filter for > llvm-commits. And we'll also say that it may or may not be easier for > reviewers to comment on a patch on Github than trying to reference > code blocks in llvm-commits email. At this point we really don't know > if one solution is better than the other, but we have good reason to > believe Pull Requests might be a big win. So rather than all the > talk, can we baby-step forward in a noncommittal way? > > How about allowing the code owner to add a line to CODE_OWNERS.TXT of > the location to submit patches? If no location is given, assume > llvm-commits. If the URI is a Github spork, the contributor should > make a Pull Request. > > Thanks, > Greg > > On Fri, Nov 16, 2012 at 3:50 PM, <dag at cray.com> wrote: >> Sean Silva <silvas at purdue.edu> writes: >> >>>> - I have to *remember* I submitted the patch (not hard, but it is a >>>> cost). >>> >>> If you forgot, the chances are high that the patch was unimportant. I >>> do my development on local git branches, so every time I do `git >>> branch`, I'm reminded. There's really no overhead. >> >> Every time you have to check it's overhead. It may be small overhead, >> but it's nonzero. >> >>>> - I have to save that e-mail from llvm-commits so I can refer to it when >>>> the inevitable ping is necessary. >>> >>> Maybe you need a better mail-reader? In gmail I just look in my "sent >>> mail" or if I have sent a lot of mail recently I search "is:sent >>> has:attachment". It literally takes 10 seconds. >> >> Ok, I'll give you this one. :) >> >>>> - I have to wade through tons and tons of commit e-mails searching for a >>>> response to my patch (for some reason the mailing list software often >>>> breaks threading). This is a more general problem with the current >>>> review process, not strictly a timeliness issue. >>> >>> Sounds like you need a better mail reader. I have never heard of this >>> even remotely being a problem in any way, at all, ever. >> >> Really? You've never heard comments about the huge volume of mail on >> llvm-commits? I just read another message about it today. >> >>>> - I have to send a ping e-mail. >>> >>> Once again, this takes like what, 10 seconds? I find it highly >>> unlikely that 10 seconds is a non-negligible amount of time compared >>> to the time spent coding, building, and testing any non-obvious (i.e. >>> needing review) change. >> >> Again, it's non-zero. I have to break out of whatever I'm doing, >> context switch and send the mail. >> >>>> - Now I have to look for responses to *two* e-mails. >>> >>> You definitely need a better mail reader. In every mail reader I've >>> ever seen, both mails get clustered into a thread. Maybe this is the >>> root cause of your vehement dissatisfaction with the current >>> email-based system? >> >> Even if threads are clustered, one still has to look through tons of >> subject lines to find it. >> >>> Have you considered the amount of quality code that gets produced with >>> the current process? >> >> Code quality doesn't reflect how efficient the review system is. It >> reflects how good the reviewers are and we are fortunate to have >> excellent ones. >> >> I find llvm-commits daunting. So much that I hesitate to do reviews. >> As Chris commented, I am not very active on that list. There's a reason >> for that beyond lack of time. By definition, making the process simpler >> and more organized should result in less time for reviews and thus >> hopefully more review participation. >> >>> What kind of improvements can we expect from such a system? >> >> A one-stop shop for reviewers to find patches. Something searchable by >> subsystem, file name, etc. I can't review every patch that goes by but >> I certainly can review patches for files I'm currently working in. If I >> could start my day by doing a patch search on whatever file I'm >> currently working on, I'd make it a daily practice to do some reviews. >> Right now that is tedious to do with e-mail. >> >>> Are there advantages that email has over a more structured >>> sort of review system like Phabricator? >> >> I don't know. I would like to know if there are any. I suppose it's >> super convenient to open the list of messages but beyond that I get >> overwhelmed. >> >>> Are you sure that the bottleneck >>> which causes high review latency isn't elsewhere in the system? >> >> Of course I'm not sure. I'm not sure anyone is sure. :) All I know is >> that there's a problem. >> >>> Or, as counterintuitive as it may seem, perhaps all of the things you >>> are pointing to as problematic (e.g. review latency, patches getting >>> dropped) are actually *beneficial* (maybe it prevents less-determined >>> people from becoming contributors? >> >> If that's our goal then I will just say right out it is not a good one. >> >> -David >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Greg Fitzgerald <garious at gmail.com> writes:> David A. Green wrote: >> I find llvm-commits daunting. So much that I hesitate to do reviews. >> As Chris commented, I am not very active on that list. There's a reason >> for that beyond lack of time. > > So the goal is to make it easier for a member of the community to > review only commits to a sub-tree that interests them?C'mon, don't put words in my mouth. The reality is that reviewer time is limited. I have more expertise in certain areas of LLVM, so I'm naturally going to gravitate there because that's where the benefit/time is maximized. Frankly, I don't feel _comfortable_ reviewing code for subsystems I've never worked in. I don't know the design or the design goals of those areas. To me, reviewing syntax and spacing is next to worthless. I'd rather give a meaty review about design and that requires familiarity. -David