Re: [Engine-devel] Development process wiki page

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.