On 05/10/2018 07:58 PM, Jerin Jacob wrote:
-----Original Message-----
Date: Thu, 10 May 2018 19:44:34 +0800
From: Andy Green <a...@warmcat.com>
To: Jerin Jacob <jerin.ja...@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 <a...@warmcat.com>
To: Jerin Jacob <jerin.ja...@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 <a...@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
