On 02/01/2012 01:04 AM, Itamar Heim wrote:
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"<ovedo@redhat.com> > To: engine-devel@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.
If those patches are about to be submitted, there is a need to go all over them. If it is 'review-in-process' it will surely save time. Just thinking on multi-patchset (series of dependent patches) where each push to gerrit for review requires going over all of the patches.