[Engine-devel] Development process wiki page

[Moti Asayag]

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 at redhat.com>
>>>>>>> To: engine-devel at 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.