Help me get a patch into Wine

[Austin English]

On Mon, Oct 12, 2009 at 2:18 PM, Charles Davis <cdavis at mymail.mines.edu> wrote:
> Keith Muir wrote:
>> Charles Davis wrote:
>>> Damn it, I keep forgetting to reply all.
>>>
>>> Charles Davis wrote:
>>>
>>>> Vitaliy Margolen wrote:
>>>>
>>>>> Charles Davis wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> My name is Charles Davis, but you all can call me "Chip".
>>>>>>
>>>>>> I'm new to Wine development, and frankly, I'm kinda scared because I've
>>>>>> heard and read that getting patches into Wine is very difficult. In
>>>>>> fact, every time I got ready to send this first patch, I chickened out
>>>>>> and couldn't do it. So, I want someone to review this patch to see if
>>>>>> there's anything wrong with it.
>>>>>>
>>>>>>
>>>>> Thanks for writing the patch.
>>>>>
>>>>> Few things about your patch:
>>>>>
>>>>>> + * Copyright 2008, 2009 Charles Davis
>>>>>>
>>>>> You need to add substantial amount of code to or be working for a long time
>>>>> on this file to add yourself to the copyright note. As for your contribution
>>>>> to the project, all authors are tracked and AUTHORS file updated automatically.
>>>>>
>>>> Oh. I see. But I have many more patches for this file.
>>>>
>>>>>> +    switch(fmt->Format)
>>>>>> +    {
>>>>>> +    case IOCTL_CDROM_CURRENT_POSITION:
>>>>>> +        memset(&data->CurrentPosition, 0, sizeof(data->CurrentPosition));
>>>>>> +        break;
>>>>>> +    case IOCTL_CDROM_MEDIA_CATALOG:
>>>>>> +        memset(&data->MediaCatalog, 0, sizeof(data->MediaCatalog));
>>>>>> +        break;
>>>>>> +    case IOCTL_CDROM_TRACK_ISRC:
>>>>>> +        memset(&data->TrackIsrc, 0, sizeof(data->TrackIsrc));
>>>>>> +    }
>>>>>> +    /* We need IOCDAudioControl for IOCTL_CDROM_CURRENT_POSITION */
>>>>>> +    if (fmt->Format == IOCTL_CDROM_CURRENT_POSITION)
>>>>>> +    {
>>>>>> +        ERR("This version of Mac OS X does not support IOCDAudioControl\n");
>>>>>> +        return STATUS_NOT_SUPPORTED;
>>>>>> +    }
>>>>>>
>>>>> This doesn't seems right. If it's not supported (aka error) you shouldn't be
>>>>> touching returned data. Also why are you zeroing return buffers? I don't see
>>>>> other implementations do it.
>>>>>
>>>> Good catch. I should have done the check first.
>>>>
>>>>>> +    case IOCTL_CDROM_MEDIA_CATALOG:
>>>>>> +        memset(&ioc, 0, sizeof(ioc));
>>>>>>
>>>>> and
>>>>>
>>>>>> +    case IOCTL_CDROM_TRACK_ISRC:
>>>>>> +        memset(&ioc, 0, sizeof(ioc));
>>>>>>
>>>>> You shouldn't need to zero buffer you pass to system ioctl.
>>>>>
>>>> Good point.
>>>>
>>>>>> +        if((io = ioctl(fd, DKIOCCDREADMCN, &ioc.mcn)) == -1) break;
>>>>>> +        if((io = ioctl(fd, DKIOCCDREADISRC, &ioc.isrc)) == -1) break;
>>>>>>
>>>>> Please keep formatting consistent with the rest of the function/file. Here
>>>>> you need to add space after "if".
>>>>>
>>>> You're right. I read about that, but that must have slipped by me.
>>>>
>>>>> Vitaliy.
>>>>>
>>>> Thanks for your input.
>>>>
>>>> Chip
>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>
>> The Julliard rank is something worth explaining. Wine developers have
>> long wondered exactly *HOW* Alexandre determines whether or not a
>> specific patch gets committed. The process is something as follows:
>>
>>     * Does the patch use C++ style comments? Yes: REJECT
>>     * Does the patch change coding style from the surrounding code? Yes:
>>       REJECT
>>     * Is the code obviously wrong in any way? Yes: REJECT
>>     * Does it take more than a few seconds to see that the code is
>>       correct? If today is a busy day: IGNORE
>>     * Is the author on the s**t list? Yes: REJECT
>>     * Are the planets aligned in just the right way and none of the
>>       above reject? Yes : ACCEPT
>>
>> And of course now we need some sort of measurement for inclusion in the
>> crap list. For every 'good' patch set an author moves further off the
>> list, for every bad patch the author's reputation is diminished and put
>> further onto the list. Most Wine developers have come to realize that
>> that the Julliard Rank is very similar to Google's PageRank algorithm:
>> everybody who cares understands how it is 'supposed' to work but could
>> never replicate it in a million years.
>>
> OK, I've sent a better version of my patch to wine-patches. Now we'll
> see what AJ thinks of it.

Apparently he liked it, it was committed today
http://source.winehq.org/git/wine.git/?a=commitdiff;h=c7992a8d260e0ce4073f85988c81b5c09a8c127f

Well done, now get some more patches in ;-)

-- 
-Austin