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

[Andy Green <andy@warmcat.com>]

On 05/10/2018 07:58 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 10 May 2018 19:44:34 +0800
>> From: Andy Green <andy@warmcat.com>
>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>> CC: dev@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 05:11 PM, Jerin Jacob wrote:
>>> -----Original Message-----
>>>> Date: Thu, 10 May 2018 14:46:42 +0800
>>>> From: Andy Green <andy@warmcat.com>
>>>> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>>> CC: dev@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@warmcat.com>
>>>>>> To: dev@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 think, the rational is
> # In subject you have minimal information
> # In commit log, you can have DETAILED info
> 
> That translated to following in your example:
> 
> module: fix a NULL pointer exception
> 
> fix a NULL pointer exception in my_function if unconfigured due to
> so and so reason
> 
>>
>> 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.
> 
> IMO, Keep all useful information in description not in subject.

I appreciate the reply.

But why bother having a subject line at all if it is going to be 
mechanically enforced that nothing in it is allowed to be "useful"? 
That really doesn't make sense does it.

Stuff like line-length enforcement automatically is generally good I 
think, whitespace checking is also good.  Obviously compilers, lint, 
coverity making complaints are all good, and the tools can judge the 
things they look at better than I can most times.

But what is expressive, concise, meaningful or "useful" in a commit 
subject line can't be judged by grep IMHO.

Anyway no worries as I say if nobody cares in the next try I will nobly 
save the project from being contaminated by anything useful in my 
subject lines.

-Andy

>>
>> -Andy
>>
>>>>
>>>> -Andy