<dag at cray.com> writes:> Sean Silva <silvas at purdue.edu> writes: > >>> Really, patches get dropped *all the time* to the point where pings are >>> a regular part of the development process. That's a huge waste of time >>> for everyone. >> >> It's only a waste of time if your workflow is entirely synchronous >> with patch review. Most of us have a number of things that we can work >> on, so letting a patch chill for a while on the list isn't a big deal >> because we just go on and do something else. > > Of course, that's easier with git. :)Just to follow up, I agree that people shouldn't be stuck for days waiting for their patch to be reviewed. The cost is more subtle than that. - I have to *remember* I submitted the patch (not hard, but it is a cost). - I have to save that e-mail from llvm-commits so I can refer to it when the inevitable ping is necessary. - 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. - I have to send a ping e-mail. - Now I have to look for responses to *two* e-mails. In short, e-mail is a very poor vehicle for managing this kind of process. I hope that the patch queue in testing helps alleviate the poor response time problem. Even just an acknowlegement, "hey I got your patch but it will be a few days before I can review it", would help a lot. -David
> - 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.> - 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.> - 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.> - 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.> - 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?> In short, e-mail is a very poor vehicle for managing this kind of > process. I hope that the patch queue in testing helps alleviate the > poor response time problem. Even just an acknowlegement, "hey I got > your patch but it will be a few days before I can review it", would help > a lot.Have you considered the amount of quality code that gets produced with the current process? What kind of improvements can we expect from such a system? Are there advantages that email has over a more structured sort of review system like Phabricator? If so, does the current development process take advantage of them (and hence would be *negatively* impacted by the switch)? Are you sure that the bottleneck which causes high review latency isn't elsewhere in the system? 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? idk, I'm not willing to rule it out without more careful analysis). -- Sean Silva On Fri, Nov 16, 2012 at 5:42 PM, <dag at cray.com> wrote:> <dag at cray.com> writes: > >> Sean Silva <silvas at purdue.edu> writes: >> >>>> Really, patches get dropped *all the time* to the point where pings are >>>> a regular part of the development process. That's a huge waste of time >>>> for everyone. >>> >>> It's only a waste of time if your workflow is entirely synchronous >>> with patch review. Most of us have a number of things that we can work >>> on, so letting a patch chill for a while on the list isn't a big deal >>> because we just go on and do something else. >> >> Of course, that's easier with git. :) > > Just to follow up, I agree that people shouldn't be stuck for days > waiting for their patch to be reviewed. The cost is more subtle than > that. > > - I have to *remember* I submitted the patch (not hard, but it is a > cost). > > - I have to save that e-mail from llvm-commits so I can refer to it when > the inevitable ping is necessary. > > - 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. > > - I have to send a ping e-mail. > > - Now I have to look for responses to *two* e-mails. > > In short, e-mail is a very poor vehicle for managing this kind of > process. I hope that the patch queue in testing helps alleviate the > poor response time problem. Even just an acknowlegement, "hey I got > your patch but it will be a few days before I can review it", would help > a lot. > > -David
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