Message ID | 1467729650-925-1-git-send-email-bruce.richardson@intel.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Delegated to: | Thomas Monjalon |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 9F0AA5693; Tue, 5 Jul 2016 16:41:10 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 359D85689 for <dev@dpdk.org>; Tue, 5 Jul 2016 16:41:09 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 05 Jul 2016 07:41:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos; i="5.26,580,1459839600"; d="scan'208"; a="1016030366" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by fmsmga002.fm.intel.com with ESMTP; 05 Jul 2016 07:41:08 -0700 Received: from sivswdev01.ir.intel.com (sivswdev01.ir.intel.com [10.237.217.45]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id u65Ef6pn022124; Tue, 5 Jul 2016 15:41:06 +0100 Received: from sivswdev01.ir.intel.com (localhost [127.0.0.1]) by sivswdev01.ir.intel.com with ESMTP id u65Ef6O1000968; Tue, 5 Jul 2016 15:41:06 +0100 Received: (from bricha3@localhost) by sivswdev01.ir.intel.com with id u65Ef6L3000964; Tue, 5 Jul 2016 15:41:06 +0100 From: Bruce Richardson <bruce.richardson@intel.com> To: dev@dpdk.org Cc: thomas.monjalon@6wind.com, Bruce Richardson <bruce.richardson@intel.com> Date: Tue, 5 Jul 2016 15:40:50 +0100 Message-Id: <1467729650-925-1-git-send-email-bruce.richardson@intel.com> X-Mailer: git-send-email 1.7.4.1 Subject: [dpdk-dev] [PATCH] scripts: add check for net driver names without net prefix X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Bruce Richardson
July 5, 2016, 2:40 p.m. UTC
Since the current standard for commit prefixes is to put the device type
e.g. "net", in front of the driver name, add a check for commit titles
which have a net driver name without any prefix in front of it.
Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
scripts/check-git-log.sh | 6 ++++++
1 file changed, 6 insertions(+)
Comments
2016-07-05 15:40, Bruce Richardson: > +# check headline label for net/ prefix if needed > +bad=$(echo "$headlines" | grep -E --color=always \ > + -e "^($(ls drivers/net | grep -v Makefile | tr '\n' '|')):" \ > + | sed 's,^,\t,') > +[ -z "$bad" ] || printf "Headline missing 'net/' prefix:\n$bad\n" This check is definitely a good idea. We need the same thing for crypto. Few nits: - -E is not needed for this regex - "/base:" cases are not handled because of the colon at the end of the regex. - patches touching several drivers are not handled. Examples: "mlx:" -> "net/mlx:" "drivers/net:" -> "net:" By the way, I am looking at an idea, that Nelio sent me, based on modified files. It can complete this check.
On Wed, Jul 06, 2016 at 11:10:48AM +0200, Thomas Monjalon wrote: > 2016-07-05 15:40, Bruce Richardson: > > +# check headline label for net/ prefix if needed > > +bad=$(echo "$headlines" | grep -E --color=always \ > > + -e "^($(ls drivers/net | grep -v Makefile | tr '\n' '|')):" \ > > + | sed 's,^,\t,') > > +[ -z "$bad" ] || printf "Headline missing 'net/' prefix:\n$bad\n" > > This check is definitely a good idea. > We need the same thing for crypto. > Few nits: > - -E is not needed for this regex > - "/base:" cases are not handled because of the colon at the end > of the regex. Good point. Easy enough to fix, I suspect. > - patches touching several drivers are not handled. Examples: > "mlx:" -> "net/mlx:" > "drivers/net:" -> "net:" > Yes, however, those are in the minority of cases, and we can always add special case extra regex's for those - hardcoded like the other regex's in the file. I did the pulling directory list so we don't have to update the checks every time a new driver was added. > By the way, I am looking at an idea, that Nelio sent me, based on > modified files. It can complete this check. Great. I'll look forward to seeing that. If it works out well, by all means drop this patch. If it's not coming any time soon though, I can do a V2 of this patch, but I won't bother right now in the expectation that a better solution is coming. /Bruce
2016-07-06 10:25, Bruce Richardson: > On Wed, Jul 06, 2016 at 11:10:48AM +0200, Thomas Monjalon wrote: > > 2016-07-05 15:40, Bruce Richardson: > > > +# check headline label for net/ prefix if needed > > > +bad=$(echo "$headlines" | grep -E --color=always \ > > > + -e "^($(ls drivers/net | grep -v Makefile | tr '\n' '|')):" \ > > > + | sed 's,^,\t,') > > > +[ -z "$bad" ] || printf "Headline missing 'net/' prefix:\n$bad\n" > > > > This check is definitely a good idea. > > We need the same thing for crypto. > > Few nits: > > - -E is not needed for this regex > > - "/base:" cases are not handled because of the colon at the end > > of the regex. > > Good point. Easy enough to fix, I suspect. > > > - patches touching several drivers are not handled. Examples: > > "mlx:" -> "net/mlx:" > > "drivers/net:" -> "net:" > > > Yes, however, those are in the minority of cases, and we can always add > special case extra regex's for those - hardcoded like the other regex's in the > file. I did the pulling directory list so we don't have to update the checks > every time a new driver was added. Another issue: I suspect you will have false positives with librte_ring and librte_vhost. > > By the way, I am looking at an idea, that Nelio sent me, based on > > modified files. It can complete this check. > > Great. I'll look forward to seeing that. If it works out well, by all means > drop this patch. If it's not coming any time soon though, I can do a V2 of > this patch, but I won't bother right now in the expectation that a better > solution is coming. My concern is that the file-based solution will handle only the patches where every changes are in drivers/ (not config/ or anything else). I'm thinking how to merge both solutions. I'm also concerned by the time passed developing these checks ;)
On Wed, Jul 06, 2016 at 11:46:23AM +0200, Thomas Monjalon wrote: > 2016-07-06 10:25, Bruce Richardson: > > On Wed, Jul 06, 2016 at 11:10:48AM +0200, Thomas Monjalon wrote: > > > 2016-07-05 15:40, Bruce Richardson: > > > > +# check headline label for net/ prefix if needed > > > > +bad=$(echo "$headlines" | grep -E --color=always \ > > > > + -e "^($(ls drivers/net | grep -v Makefile | tr '\n' '|')):" \ > > > > + | sed 's,^,\t,') > > > > +[ -z "$bad" ] || printf "Headline missing 'net/' prefix:\n$bad\n" > > > > > > This check is definitely a good idea. > > > We need the same thing for crypto. > > > Few nits: > > > - -E is not needed for this regex > > > - "/base:" cases are not handled because of the colon at the end > > > of the regex. > > > > Good point. Easy enough to fix, I suspect. > > > > > - patches touching several drivers are not handled. Examples: > > > "mlx:" -> "net/mlx:" > > > "drivers/net:" -> "net:" > > > > > Yes, however, those are in the minority of cases, and we can always add > > special case extra regex's for those - hardcoded like the other regex's in the > > file. I did the pulling directory list so we don't have to update the checks > > every time a new driver was added. > > Another issue: I suspect you will have false positives with librte_ring and > librte_vhost. Yep, good catch there. Need to tune up those filters if I proceed with this. > > > > By the way, I am looking at an idea, that Nelio sent me, based on > > > modified files. It can complete this check. > > > > Great. I'll look forward to seeing that. If it works out well, by all means > > drop this patch. If it's not coming any time soon though, I can do a V2 of > > this patch, but I won't bother right now in the expectation that a better > > solution is coming. > > My concern is that the file-based solution will handle only the patches where > every changes are in drivers/ (not config/ or anything else). > I'm thinking how to merge both solutions. > > I'm also concerned by the time passed developing these checks ;) "Time is an illusion. Lunchtime doubly so."
diff --git a/scripts/check-git-log.sh b/scripts/check-git-log.sh index 7d2c7ee..7dc1fd8 100755 --- a/scripts/check-git-log.sh +++ b/scripts/check-git-log.sh @@ -78,6 +78,12 @@ bad=$(echo "$headlines" | grep --color=always \ | sed 's,^,\t,') [ -z "$bad" ] || printf "Wrong headline label:\n$bad\n" +# check headline label for net/ prefix if needed +bad=$(echo "$headlines" | grep -E --color=always \ + -e "^($(ls drivers/net | grep -v Makefile | tr '\n' '|')):" \ + | sed 's,^,\t,') +[ -z "$bad" ] || printf "Headline missing 'net/' prefix:\n$bad\n" + # check headline lowercase for first words bad=$(echo "$headlines" | grep --color=always \ -e '^.*[A-Z].*:' \