[dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs

[Andy Green]

On 05/10/2018 05:11 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 10 May 2018 14:46:42 +0800
>> From: Andy Green <andy at warmcat.com>
>> To: Jerin Jacob <jerin.jacob at caviumnetworks.com>
>> CC: dev at dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
>> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
>>   Thunderbird/52.7.0
>>
>>
>>
>> On 05/10/2018 02:17 PM, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Thu, 10 May 2018 10:46:18 +0800
>>>> From: Andy Green <andy at warmcat.com>
>>>> To: dev at dpdk.org
>>>> Subject: [dpdk-dev] [PATCH v3 00/40] Fix build on gcc8 and various bugs
>>>> User-Agent: StGit/unknown-version
>>>>
>>> ./devtools/check-git-log.sh
>>
>> Ugh...
>>
>> Wrong headline format:
>> 	drivers/net/nfp: fix buffer overflow in fw_name
>>
>> ... snip something "wrong" about every patch title...
>>
>> It's just doing this
>>
>> # check headline format (spacing, no punctuation, no code)
>> bad=$(echo "$headlines" | grep --color=always \
>>          -e '    ' \
>>          -e '^ ' \
>>          -e ' $' \
>>          -e '\.$' \
>>          -e '[,;!?&|]' \
>>          -e ':.*_' \
>>          -e '^[^:]\+$' \
>>          -e ':[^ ]' \
>>          -e ' :' \
>>          | sed 's,^,\t,')
>> [ -z "$bad" ] || printf "Wrong headline format:\n$bad\n"
>>
>> It probably seems to whoever wrote it that adds "quality", but actually
>> inflexible rules like this do nothing to help quality of the patch payload
>> and actively put off contribution.
>>
>> So on this first one it's hitting the rule ':.*_', ie, this project believes
>> there should never be a patch title mentioning anything with an underscore
>> after a colon.
>>
>> Can you help me understand in what way banning mentioning relevant strings
>> in the patch title is a good idea?  It's actively reducing the value of the
>> patch title, isn't it?
> 
> I think, the underscore check is to make sure that the subject should not have
> C symbols.

Right, that seems to be the intention.

But if the patch is entirely about doing something to a specific C 
symbol or function, it's not a bad thing if it mentions that in the 
title is it?  In itself, most projects would consider it a good thing to 
concisely explain what it's doing.  Eg, "fix NULL pointer exception in 
my_function if unconfigured" is illegal for this project.  It's strange 
actually.

I don't understand what negative outcome the check is saving us from. 
If nothing, maybe it should be patched to not do that any more.

> Change to following will work:
> 
> net/nfp: fix buffer overflow

Sure, I studied the script to find out what its problem was.  I just 
don't think its problem is reasonable.

If nobody cares, sure I will go through removing useful information from 
my patch titles to keep this script happy.

-Andy

>>
>> -Andy