[dpdk-dev] [PATCH] scripts: add check for net driver names without net prefix

[dpdk-dev] [PATCH] scripts: add check for net driver names without net prefix

[Bruce Richardson]

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(+)

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].*:' \
-- 
2.5.5

Re: [dpdk-dev] [PATCH] scripts: add check for net driver names without net prefix

[Thomas Monjalon]

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.

Re: [dpdk-dev] [PATCH] scripts: add check for net driver names without net prefix

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

> 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

Re: [dpdk-dev] [PATCH] scripts: add check for net driver names without net prefix

[Thomas Monjalon]

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 ;)

Re: [dpdk-dev] [PATCH] scripts: add check for net driver names without net prefix

[Bruce Richardson]

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