Help me get a patch into Wine

[Charles Davis]

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.

Again, thanks for your input, guys. And the fact that you said something
really helped me.

Chip