Re: [vdsm] Patch review process
- Original Message - > From: "Ryan Harper" > To: "Alon Bar-Lev" > Cc: "Ryan Harper" , "Ryan Harper" > , "Anthony Liguori" > , vdsm-devel@lists.fedorahosted.org, "Adam > Litke" > Sent: Monday, September 10, 2012 9:41:14 PM > Subject: Re: [vdsm] Patch review process > > * Alon Bar-Lev [2012-09-10 12:44]: > > > > > > - Original Message - > > > From: "Ryan Harper" > > > To: "Alon Bar-Lev" > > > Cc: "Ryan Harper" , "Ryan Harper" > > > , "Anthony Liguori" > > > , vdsm-devel@lists.fedorahosted.org, > > > "Adam Litke" > > > Sent: Monday, September 10, 2012 8:33:53 PM > > > Subject: Re: [vdsm] Patch review process > > > > > > * Alon Bar-Lev [2012-09-10 12:22]: > > > > > > > > > > > > - Original Message - > > > > > From: "Ryan Harper" > > > > > To: "Adam Litke" > > > > > Cc: "Ryan Harper" , "Anthony > > > > > Liguori" > > > > > , > > > > > vdsm-devel@lists.fedorahosted.org > > > > > Sent: Monday, September 10, 2012 7:07:56 PM > > > > > Subject: Re: [vdsm] Patch review process > > > > > > > > > > * Adam Litke [2012-09-09 12:29]: > > > > > > > > > > > > > > > > > I'm certainly willing to review any patches that show up on > > > > > the > > > > > mailing > > > > > list directly, so if folks want to submit patches first for > > > > > review > > > > > before pushing into gerrit; I'm quite happy with reviewing > > > > > those. > > > > > > > > Why won't you subscribe to vdsm-patches[1] and comment within > > > > gerrit? > > > > > > I am subscribed. Commenting within gerrit is much slower for > > > myself. > > > I'm > > > always in my email client, so sending an email back involves far > > > less > > > work for myself and I don't have to write lots of words in a > > > small > > > text > > > box without a proper text editor. > > > > > > Gerrit comment workflow: > > > > > > 1) follow link from vdsm-patches email to gerrit patch in browser > > > 2) sign-in via openid > > > 3) find which patch file I want to comment upon > > > 4) select the line I want to comment upon to bring up text widget > > > 5) write up comment in a browswer gui box without any support > > > from > > > text > > > editor > > > 6) repeat (3-5) if there are multiple files touched > > > 7) go back to top > > > 8) hit publish > > > > > > > > > Email comment workflow after we know have inline patches. > > > > > > 1) Reply to email > > > 2) find lines in email for comments > > > 3) write up responses in editor > > > 4) close file and mail. > > > > > > > > > > > > > > This method is superior as the past/present/future comments and > > > > review > > > > > > I disagree here. > > > > > > > process is managed within productivity application, from birth > > > > to > > > > merge or death. > > > > > > Much like an email thread with inline patches that's archived for > > > everyone to read and review. > > > > > > > > > > > Each comment within gerrit is going to the list as if sent > > > > directly > > > > to > > > > the mailing list, so you can track the activity. > > > > > > What's the point of going to the list if not to be able to > > > respond to > > > email? > > > > You see the benefit of you as a reviewer, while there are other > > reviewers (past and future, members and guests) and there is the > > patch > > owner. > > > > While it may indeed be easier for you to "just" send a message, for > > the other people who are involved in the process it may not be as > > productive as managing the discussion within productivity > > application > > which supports the process quite well. > > Sure; There is a fixed set of folks who already have been working > with > gerrit for some time and I'm sure are
Re: [vdsm] Patch review process
On Sun, Sep 09, 2012 at 02:33:00PM -0400, Alon Bar-Lev wrote: > > > - Original Message - > > From: "Adam Litke" > > To: vdsm-devel@lists.fedorahosted.org > > Cc: "Ryan Harper" , "Anthony Liguori" > > > > Sent: Sunday, September 9, 2012 8:27:30 PM > > Subject: [vdsm] Patch review process > > > > While discussing gerrit recently, I learned that some people use > > gerrit simply > > to host work-in-progress patches and they don't intend for those to > > be reviewed. > > How can a reviewer recognize this and skip those patches when > > choosing what to > > review? Is there a way to mark certain patches as more important and > > others as > > drafts? > > Yes. > > See [1]. > > $ git push upstream HEAD:refs/drafts/master/description Thanks for pointing it out. It would be nice if we could get people to start pushing WIP patches to drafts now that we have this feature. > > [1] > http://gerrit-documentation.googlecode.com/svn/ReleaseNotes/ReleaseNotes-2.3.html > -- Adam Litke IBM Linux Technology Center ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
* Alon Bar-Lev [2012-09-10 12:44]: > > > - Original Message - > > From: "Ryan Harper" > > To: "Alon Bar-Lev" > > Cc: "Ryan Harper" , "Ryan Harper" > > , "Anthony Liguori" > > , vdsm-devel@lists.fedorahosted.org, "Adam > > Litke" > > Sent: Monday, September 10, 2012 8:33:53 PM > > Subject: Re: [vdsm] Patch review process > > > > * Alon Bar-Lev [2012-09-10 12:22]: > > > > > > > > > - Original Message - > > > > From: "Ryan Harper" > > > > To: "Adam Litke" > > > > Cc: "Ryan Harper" , "Anthony Liguori" > > > > , > > > > vdsm-devel@lists.fedorahosted.org > > > > Sent: Monday, September 10, 2012 7:07:56 PM > > > > Subject: Re: [vdsm] Patch review process > > > > > > > > * Adam Litke [2012-09-09 12:29]: > > > > > > > > > > > > > I'm certainly willing to review any patches that show up on the > > > > mailing > > > > list directly, so if folks want to submit patches first for > > > > review > > > > before pushing into gerrit; I'm quite happy with reviewing those. > > > > > > Why won't you subscribe to vdsm-patches[1] and comment within > > > gerrit? > > > > I am subscribed. Commenting within gerrit is much slower for myself. > > I'm > > always in my email client, so sending an email back involves far less > > work for myself and I don't have to write lots of words in a small > > text > > box without a proper text editor. > > > > Gerrit comment workflow: > > > > 1) follow link from vdsm-patches email to gerrit patch in browser > > 2) sign-in via openid > > 3) find which patch file I want to comment upon > > 4) select the line I want to comment upon to bring up text widget > > 5) write up comment in a browswer gui box without any support from > > text > > editor > > 6) repeat (3-5) if there are multiple files touched > > 7) go back to top > > 8) hit publish > > > > > > Email comment workflow after we know have inline patches. > > > > 1) Reply to email > > 2) find lines in email for comments > > 3) write up responses in editor > > 4) close file and mail. > > > > > > > > > > This method is superior as the past/present/future comments and > > > review > > > > I disagree here. > > > > > process is managed within productivity application, from birth to > > > merge or death. > > > > Much like an email thread with inline patches that's archived for > > everyone to read and review. > > > > > > > > Each comment within gerrit is going to the list as if sent directly > > > to > > > the mailing list, so you can track the activity. > > > > What's the point of going to the list if not to be able to respond to > > email? > > You see the benefit of you as a reviewer, while there are other > reviewers (past and future, members and guests) and there is the patch > owner. > > While it may indeed be easier for you to "just" send a message, for > the other people who are involved in the process it may not be as > productive as managing the discussion within productivity application > which supports the process quite well. Sure; There is a fixed set of folks who already have been working with gerrit for some time and I'm sure are quite comfortable with the process. Part of opening the community up is figuring out how best to attract and maintain additional developers. When growing a community, reducing the barrier for entry is a must. I'll posit that if contributions to gerrit-based communities require the additional openid, web-based commented/editing, some-what undocumented process for obtain review and approval then we're not going to see tremendous growth given the additional overhead of working with gerrit as a contributor. Only recently did we get gerrit to push the code out after submission, meaning that if someone wanted to lurk and read the code, it was always through the web-based viewing; some comments showup on the page of the changeset, the other in-line comments are only available if you know which file the comment was made against. All comments go to the vdsm-patches, but this is a one-way avenue; you don't see discussion happening in response; and if you want to reply, you have to sign-in to gerrit and hunt down the right file. None of this
Re: [vdsm] Patch review process
* Itamar Heim [2012-09-10 12:43]: > On 09/10/2012 08:33 PM, Ryan Harper wrote: > >What's the point of going to the list if not to be able to respond to > >email? > > to be able to see what's going on in bulk, in offline, via mail client. > but go on gerrit to reply/discuss, or some of your comments will get > lost from the patch activity. yes, I've been asking for replies to show up as gerrit comments; but I know that's not happing any time soon. > if you comment via email, other reviewers may not see your comments > when they review the patch in gerrit. I always cc vdsm-devel; this small group discussion of patches makes things not like a community. I like having everyone see the discussions about a patch, rather than just the reviewer. yes, I know all comments are pushed to the -patches list, but the whole point is to have code + comments + discussion in a single thread. -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ry...@us.ibm.com ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
- Original Message - > From: "Ryan Harper" > To: "Alon Bar-Lev" > Cc: "Ryan Harper" , "Ryan Harper" > , "Anthony Liguori" > , vdsm-devel@lists.fedorahosted.org, "Adam > Litke" > Sent: Monday, September 10, 2012 8:33:53 PM > Subject: Re: [vdsm] Patch review process > > * Alon Bar-Lev [2012-09-10 12:22]: > > > > > > - Original Message - > > > From: "Ryan Harper" > > > To: "Adam Litke" > > > Cc: "Ryan Harper" , "Anthony Liguori" > > > , > > > vdsm-devel@lists.fedorahosted.org > > > Sent: Monday, September 10, 2012 7:07:56 PM > > > Subject: Re: [vdsm] Patch review process > > > > > > * Adam Litke [2012-09-09 12:29]: > > > > > > > > > I'm certainly willing to review any patches that show up on the > > > mailing > > > list directly, so if folks want to submit patches first for > > > review > > > before pushing into gerrit; I'm quite happy with reviewing those. > > > > Why won't you subscribe to vdsm-patches[1] and comment within > > gerrit? > > I am subscribed. Commenting within gerrit is much slower for myself. > I'm > always in my email client, so sending an email back involves far less > work for myself and I don't have to write lots of words in a small > text > box without a proper text editor. > > Gerrit comment workflow: > > 1) follow link from vdsm-patches email to gerrit patch in browser > 2) sign-in via openid > 3) find which patch file I want to comment upon > 4) select the line I want to comment upon to bring up text widget > 5) write up comment in a browswer gui box without any support from > text > editor > 6) repeat (3-5) if there are multiple files touched > 7) go back to top > 8) hit publish > > > Email comment workflow after we know have inline patches. > > 1) Reply to email > 2) find lines in email for comments > 3) write up responses in editor > 4) close file and mail. > > > > > > This method is superior as the past/present/future comments and > > review > > I disagree here. > > > process is managed within productivity application, from birth to > > merge or death. > > Much like an email thread with inline patches that's archived for > everyone to read and review. > > > > > Each comment within gerrit is going to the list as if sent directly > > to > > the mailing list, so you can track the activity. > > What's the point of going to the list if not to be able to respond to > email? You see the benefit of you as a reviewer, while there are other reviewers (past and future, members and guests) and there is the patch owner. While it may indeed be easier for you to "just" send a message, for the other people who are involved in the process it may not be as productive as managing the discussion within productivity application which supports the process quite well. It is not that gerrit is perfect, it is far from being perfect, however has advantages over plain list. You wrote initially that this discussion already took place, is there any new factor from previous discussion that should be taken into consideration? Regards, Alon. ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
On 09/10/2012 08:33 PM, Ryan Harper wrote: What's the point of going to the list if not to be able to respond to email? to be able to see what's going on in bulk, in offline, via mail client. but go on gerrit to reply/discuss, or some of your comments will get lost from the patch activity. if you comment via email, other reviewers may not see your comments when they review the patch in gerrit. ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
* Alon Bar-Lev [2012-09-10 12:22]: > > > - Original Message - > > From: "Ryan Harper" > > To: "Adam Litke" > > Cc: "Ryan Harper" , "Anthony Liguori" > > , > > vdsm-devel@lists.fedorahosted.org > > Sent: Monday, September 10, 2012 7:07:56 PM > > Subject: Re: [vdsm] Patch review process > > > > * Adam Litke [2012-09-09 12:29]: > > > > > I'm certainly willing to review any patches that show up on the > > mailing > > list directly, so if folks want to submit patches first for review > > before pushing into gerrit; I'm quite happy with reviewing those. > > Why won't you subscribe to vdsm-patches[1] and comment within gerrit? I am subscribed. Commenting within gerrit is much slower for myself. I'm always in my email client, so sending an email back involves far less work for myself and I don't have to write lots of words in a small text box without a proper text editor. Gerrit comment workflow: 1) follow link from vdsm-patches email to gerrit patch in browser 2) sign-in via openid 3) find which patch file I want to comment upon 4) select the line I want to comment upon to bring up text widget 5) write up comment in a browswer gui box without any support from text editor 6) repeat (3-5) if there are multiple files touched 7) go back to top 8) hit publish Email comment workflow after we know have inline patches. 1) Reply to email 2) find lines in email for comments 3) write up responses in editor 4) close file and mail. > > This method is superior as the past/present/future comments and review I disagree here. > process is managed within productivity application, from birth to > merge or death. Much like an email thread with inline patches that's archived for everyone to read and review. > > Each comment within gerrit is going to the list as if sent directly to > the mailing list, so you can track the activity. What's the point of going to the list if not to be able to respond to email? > > Alon. > > [1] https://lists.fedorahosted.org/pipermail/vdsm-patches/ -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ry...@us.ibm.com ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
- Original Message - > From: "Ryan Harper" > To: "Adam Litke" > Cc: "Ryan Harper" , "Anthony Liguori" > , > vdsm-devel@lists.fedorahosted.org > Sent: Monday, September 10, 2012 7:07:56 PM > Subject: Re: [vdsm] Patch review process > > * Adam Litke [2012-09-09 12:29]: > I'm certainly willing to review any patches that show up on the > mailing > list directly, so if folks want to submit patches first for review > before pushing into gerrit; I'm quite happy with reviewing those. Why won't you subscribe to vdsm-patches[1] and comment within gerrit? This method is superior as the past/present/future comments and review process is managed within productivity application, from birth to merge or death. Each comment within gerrit is going to the list as if sent directly to the mailing list, so you can track the activity. Alon. [1] https://lists.fedorahosted.org/pipermail/vdsm-patches/ ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
* Adam Litke [2012-09-09 12:29]: > Hi, > > I want to open up a discussion about patch reviews in the vdsm project. I > believe everyone will agree that more code review needs to happen for the > betterment of the project. I want to ask everyone some questions and also > make > some observations. I hope to gain some insights and improve my own workflow. > And I hope the same for everyone else too. > > How much time in a week do you spend reviewing patches? > > We have a lot of open patches in gerrit. When deciding to review some code, > how > do you select a patch to review. I have heard people say that they only > select > patches which have named them specifically as a reviewer. How does a new > contributor know who to ask? Does anyone have a workflow (or gerrit query) to > select recent unreviewed patches? In non-gerrit communities, it's also common for patches to not get reviewed. The general approach has been to resubmit the patches to the mailing list for followup. I know in the past I've emailed this list for review requests; and that seemed to help somewhat, but I do worry that it's not obvious to developers how exactly this should be approached. If you've received some reviews (say even a +1) but are lacking the additional +1 then it seems email request is the best option since a resubmit will remove any previous reviews (+1). So, it's not clear to me what the best method (or even the preferred method of the community) is when soliciting review. I'm certainly willing to review any patches that show up on the mailing list directly, so if folks want to submit patches first for review before pushing into gerrit; I'm quite happy with reviewing those. > > While discussing gerrit recently, I learned that some people use gerrit simply > to host work-in-progress patches and they don't intend for those to be > reviewed. > How can a reviewer recognize this and skip those patches when choosing what to > review? Is there a way to mark certain patches as more important and others > as > drafts? > > Thanks for taking the time to share your thoughts. > > > -- > Adam Litke > IBM Linux Technology Center > > ___ > vdsm-devel mailing list > vdsm-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ry...@us.ibm.com ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
* Dan Kenigsberg [2012-09-10 06:46]: > On Sun, Sep 09, 2012 at 12:27:30PM -0500, Adam Litke wrote: > > Hi, > > > > I want to open up a discussion about patch reviews in the vdsm project. I > > believe everyone will agree that more code review needs to happen for the > > betterment of the project. I want to ask everyone some questions and also > > make > > some observations. I hope to gain some insights and improve my own > > workflow. > > And I hope the same for everyone else too. > > > > How much time in a week do you spend reviewing patches? > > For me it is a couple of hours every day... > > > > > We have a lot of open patches in gerrit. When deciding to review some > > code, how > > do you select a patch to review. I have heard people say that they only > > select > > patches which have named them specifically as a reviewer. How does a new > > contributor know who to ask? > > I think that `git blame` on the relevant code area would take him a > long way. I personally add people I trust as reviewers to patches > written by third parties. Is there anyway to progamatically do that? running git-blame to collect developer names, and then adding them on each push via the web seems like a lot of extra steps a new developer has to do to ensure they get any feedback. Ideally our barrier to contributions should be as low as possible. -- Ryan Harper Software Engineer; Linux Technology Center IBM Corp., Austin, Tx ry...@us.ibm.com ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
On Sun, Sep 09, 2012 at 12:27:30PM -0500, Adam Litke wrote: > Hi, > > I want to open up a discussion about patch reviews in the vdsm project. I > believe everyone will agree that more code review needs to happen for the > betterment of the project. I want to ask everyone some questions and also > make > some observations. I hope to gain some insights and improve my own workflow. > And I hope the same for everyone else too. > > How much time in a week do you spend reviewing patches? For me it is a couple of hours every day... > > We have a lot of open patches in gerrit. When deciding to review some code, > how > do you select a patch to review. I have heard people say that they only > select > patches which have named them specifically as a reviewer. How does a new > contributor know who to ask? I think that `git blame` on the relevant code area would take him a long way. I personally add people I trust as reviewers to patches written by third parties. > Does anyone have a workflow (or gerrit query) to > select recent unreviewed patches? Are you looking for something like http://gerrit.ovirt.org/#/q/status:open+project:vdsm+-codereview%253E%253D1+-codereview%253C%253D-1+-age:1w,n,z ? I cannot say that I use anything like that. But I do search regularly for changes that have all the acks, and wait for my submitting them. > > While discussing gerrit recently, I learned that some people use gerrit simply > to host work-in-progress patches and they don't intend for those to be > reviewed. > How can a reviewer recognize this and skip those patches when choosing what to > review? Is there a way to mark certain patches as more important and others > as > drafts? > > Thanks for taking the time to share your thoughts. Regards, Dan. ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
Re: [vdsm] Patch review process
- Original Message - > From: "Adam Litke" > To: vdsm-devel@lists.fedorahosted.org > Cc: "Ryan Harper" , "Anthony Liguori" > > Sent: Sunday, September 9, 2012 8:27:30 PM > Subject: [vdsm] Patch review process > > While discussing gerrit recently, I learned that some people use > gerrit simply > to host work-in-progress patches and they don't intend for those to > be reviewed. > How can a reviewer recognize this and skip those patches when > choosing what to > review? Is there a way to mark certain patches as more important and > others as > drafts? Yes. See [1]. $ git push upstream HEAD:refs/drafts/master/description [1] http://gerrit-documentation.googlecode.com/svn/ReleaseNotes/ReleaseNotes-2.3.html ___ vdsm-devel mailing list vdsm-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-devel
