[Engine-devel] Development process wiki page

[Itamar Heim]

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.