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

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----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.

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