From: "Moti Asayag" <masayag(a)redhat.com&gt; To: engine-devel(a)ovirt.org Sent: Monday, January 30, 2012 10:30:01 AM Subject: Re: [Engine-devel] Development process wiki page We should add some guideline for the review process as well to ease the process for the reviewers. When pushing a newer version of a patch to gerrit we can distinguish between a patch which was already reviewed to non-reviewed patch: 1. A reviewed patch: the previous patch-set contains reviewer comments, therefore the expected change should be described there, as part of the conversation around a specific piece of code (either as a reply or using the 'Done' button). 2. A new reviewed patch: A general message should be added on the patch level describing what have changed since previous patch-set. Thoughts ?

On 01/30/2012 09:45 AM, Oved Ourfalli wrote: > Hey all, > > I've wrote a wiki page on the oVirt development process. > It contains mostly information on the patch review process, patch > submission and some git guidelines. > > http://www.ovirt.org/wiki/DevProcess > > I added a link to it from the main wiki page. > > Your comments are welcome. > > Thank you, > Oved > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel

From: "Oved Ourfalli" <ovedo(a)redhat.com&gt; To: engine-devel(a)ovirt.org Sent: Monday, January 30, 2012 9:45:47 AM Subject: [Engine-devel] Development process wiki page Hey all, I've wrote a wiki page on the oVirt development process. It contains mostly information on the patch review process, patch submission and some git guidelines. http://www.ovirt.org/wiki/DevProcess I added a link to it from the main wiki page. Your comments are welcome.

Thank you, Oved _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel

On 01/31/2012 03:03 AM, Eli Mesika wrote: > > > ----- Original Message ----- >> From: "Oved Ourfalli&quot;&lt;ovedo(a)redhat.com&gt; >> To: engine-devel(a)ovirt.org >> Sent: Monday, January 30, 2012 9:45:47 AM >> Subject: [Engine-devel] Development process wiki page >> >> Hey all, >> >> I've wrote a wiki page on the oVirt development process. >> It contains mostly information on the patch review process, patch >> submission and some git guidelines. >> >> http://www.ovirt.org/wiki/DevProcess >> >> I added a link to it from the main wiki page. >> >> Your comments are welcome. > > my 2 cents : (obvious , but should be in the wiki) > > 1) Regarding patch separation, still each commit should not break > the build > 2) When having more than one patch set due to comments and applied > fixes add to the commit message V[n] :<changes description> where > [n] is the patch set number I agree we need this, but the problem is we are lacking the [0/nnn] cover letter email you get for such commentary when using gerrit. and there is a concept of the commit itself should not include the history of the patch being reviewed. so we need to adapt this to gerrit. maybe a convention that when uploading a new patch set, contributor also adds a review comment of the changes from previous version (similar to the cover letter).

I wonder if the git/gerrit utility some adopted from openstack can help with such a step to allow adding a comment when pushing the patch for review. _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel

On 01/31/2012 12:04 PM, Mike Kolesnik wrote: > > ----- Original Message ----- >> On 01/31/2012 03:03 AM, Eli Mesika wrote: >>> >>> >>> ----- Original Message ----- >>>> From: "Oved Ourfalli&quot;&lt;ovedo(a)redhat.com&gt; >>>> To: engine-devel(a)ovirt.org >>>> Sent: Monday, January 30, 2012 9:45:47 AM >>>> Subject: [Engine-devel] Development process wiki page >>>> >>>> Hey all, >>>> >>>> I've wrote a wiki page on the oVirt development process. >>>> It contains mostly information on the patch review process, patch >>>> submission and some git guidelines. >>>> >>>> http://www.ovirt.org/wiki/DevProcess >>>> >>>> I added a link to it from the main wiki page. >>>> >>>> Your comments are welcome. >>> >>> my 2 cents : (obvious , but should be in the wiki) >>> >>> 1) Regarding patch separation, still each commit should not break >>> the build >>> 2) When having more than one patch set due to comments and applied >>> fixes add to the commit message V[n] :<changes description> where >>> [n] is the patch set number >> >> I agree we need this, but the problem is we are lacking the [0/nnn] >> cover letter email you get for such commentary when using gerrit. >> and there is a concept of the commit itself should not include the >> history of the patch being reviewed. >> so we need to adapt this to gerrit. >> maybe a convention that when uploading a new patch set, contributor >> also >> adds a review comment of the changes from previous version (similar >> to >> the cover letter). > > I think if the commit history already exists in gerrit, and the commit > itself is pushed through gerrit, then this will be redundant. this is not about the patch history, it is about providing reviewers a focus point as to what changed in this new version of the patch set.

> >> I wonder if the git/gerrit utility some adopted from openstack can >> help >> with such a step to allow adding a comment when pushing the patch for >> review. >> _______________________________________________ >> Engine-devel mailing list >> Engine-devel(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/engine-devel >> _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel

On 02/01/2012 12:56 AM, Moti Asayag wrote: > On 01/31/2012 01:31 PM, Itamar Heim wrote: >> On 01/31/2012 12:04 PM, Mike Kolesnik wrote: >>> >>> ----- Original Message ----- >>>> On 01/31/2012 03:03 AM, Eli Mesika wrote: >>>>> >>>>> >>>>> ----- Original Message ----- >>>>>> From: "Oved Ourfalli&quot;&lt;ovedo(a)redhat.com&gt; >>>>>> To: engine-devel(a)ovirt.org >>>>>> Sent: Monday, January 30, 2012 9:45:47 AM >>>>>> Subject: [Engine-devel] Development process wiki page >>>>>> >>>>>> Hey all, >>>>>> >>>>>> I've wrote a wiki page on the oVirt development process. >>>>>> It contains mostly information on the patch review process, patch >>>>>> submission and some git guidelines. >>>>>> >>>>>> http://www.ovirt.org/wiki/DevProcess >>>>>> >>>>>> I added a link to it from the main wiki page. >>>>>> >>>>>> Your comments are welcome. >>>>> >>>>> my 2 cents : (obvious , but should be in the wiki) >>>>> >>>>> 1) Regarding patch separation, still each commit should not break >>>>> the build >>>>> 2) When having more than one patch set due to comments and applied >>>>> fixes add to the commit message V[n] :<changes description> where >>>>> [n] is the patch set number >>>> >>>> I agree we need this, but the problem is we are lacking the [0/nnn] >>>> cover letter email you get for such commentary when using gerrit. >>>> and there is a concept of the commit itself should not include the >>>> history of the patch being reviewed. >>>> so we need to adapt this to gerrit. >>>> maybe a convention that when uploading a new patch set, contributor >>>> also >>>> adds a review comment of the changes from previous version (similar >>>> to >>>> the cover letter). >>> >>> I think if the commit history already exists in gerrit, and the commit >>> itself is pushed through gerrit, then this will be redundant. >> >> this is not about the patch history, it is about providing reviewers a >> focus point as to what changed in this new version of the patch set. >> > > However for the +2 the entire patch should be reviewed again as there > might be changes after rebasing the code (even for a new file, e.g. db > upgrade scripts with already used version in the file name). > > Following the small patches convention should ease the review, combining > with the short message after the push. > > However many times the newer version of a patch is just a rebase, so it > seems redundant adding just the "patch was rebased" message. why - wouldn't it save a lot of time for the reviewers to know this is the only change in this patchset, rather than trying to see there are no changes in it? I agree message shouldn't be in the commit, but a review comment by the patch submitter would go a long way helping reviewers.