[dpdk-dev] [PATCH] devtools: export title syntax for check-git-log

[dpdk-dev] [PATCH] devtools: export title syntax for check-git-log

[Sean Morrissey]

Moved title syntax to a separate file so that it improves code readability
and allows for easy addition of new correct title syntax in future cases.

Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
---
 devtools/check-git-log.sh        | 60 ++++++++------------------------
 devtools/commit-title-syntax.txt | 45 ++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 45 deletions(-)
 create mode 100644 devtools/commit-title-syntax.txt

diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
index a763ccf78..4152f6dfa 100755
--- a/devtools/check-git-log.sh
+++ b/devtools/check-git-log.sh
@@ -83,51 +83,21 @@ bad=$(echo "$headlines" | grep --color=always \
 	| sed 's,^,\t,')
 [ -z "$bad" ] || printf "Wrong headline uppercase:\n$bad\n"
 
-# check headline uppercase (Rx/Tx, VF, L2, MAC, Linux, ARM...)
-bad=$(echo "$headlines" | grep -E --color=always \
-	-e ':.*\<(rx|tx|RX|TX)\>' \
-	-e ':.*\<[pv]f\>' \
-	-e ':.*\<[hsf]w\>' \
-	-e ':.*\<l[234]\>' \
-	-e ':.*\<api\>' \
-	-e ':.*\<ARM\>' \
-	-e ':.*\<(Aarch64|AArch64|AARCH64|Aarch32|AArch32|AARCH32)\>' \
-	-e ':.*\<(Armv7|ARMv7|ArmV7|armV7|ARMV7)\>' \
-	-e ':.*\<(Armv8|ARMv8|ArmV8|armV8|ARMV8)\>' \
-	-e ':.*\<crc\>' \
-	-e ':.*\<dcb\>' \
-	-e ':.*\<dma\>' \
-	-e ':.*\<eeprom\>' \
-	-e ':.*\<freebsd\>' \
-	-e ':.*\<iova\>' \
-	-e ':.*\<lacp\>' \
-	-e ':.*\<linux\>' \
-	-e ':.*\<lro\>' \
-	-e ':.*\<lsc\>' \
-	-e ':.*\<mac\>' \
-	-e ':.*\<mss\>' \
-	-e ':.*\<mtu\>' \
-	-e ':.*\<nic\>' \
-	-e ':.*\<nvm\>' \
-	-e ':.*\<numa\>' \
-	-e ':.*\<pci\>' \
-	-e ':.*\<phy\>' \
-	-e ':.*\<pmd\>' \
-	-e ':.*\<reta\>' \
-	-e ':.*\<rss\>' \
-	-e ':.*\<sctp\>' \
-	-e ':.*\<tos\>' \
-	-e ':.*\<tpid\>' \
-	-e ':.*\<tso\>' \
-	-e ':.*\<ttl\>' \
-	-e ':.*\<udp\>' \
-	-e ':.*\<[Vv]lan\>' \
-	-e ':.*\<vdpa\>' \
-	-e ':.*\<vsi\>' \
-	| grep \
-	-v ':.*\<OCTEON\ TX\>' \
-	| sed 's,^,\t,')
-[ -z "$bad" ] || printf "Wrong headline lowercase:\n$bad\n"
+# check headline case (Rx/Tx, VF, L2, MAC, Linux ...)
+data="$selfdir/commit-title-syntax.txt"
+while IFS= read -r line
+do
+	regex=":.*\<$line\>"
+	bad=$(echo "$headlines" | grep -i $regex | grep \
+		-v ':.*\<OCTEON\ TX\>' )
+	if ! [ -z "$bad" ]
+	then
+		bad=$(echo "$headlines" | grep --color=always -v $regex \
+			| grep --color=always -i $regex \
+			| sed 's,^,\t,')
+		[ -z "$bad" ] || printf "Wrong headline case:\n$bad\n"
+	fi
+done < "$data"
 
 # special case check for VMDq to give good error message
 bad=$(echo "$headlines" | grep -E --color=always \
diff --git a/devtools/commit-title-syntax.txt b/devtools/commit-title-syntax.txt
new file mode 100644
index 000000000..0d4b9af01
--- /dev/null
+++ b/devtools/commit-title-syntax.txt
@@ -0,0 +1,45 @@
+Rx
+Tx
+PF
+VF
+HW
+SW
+FW
+L2
+L3
+L4
+API
+arm
+aarch64
+armv7
+armv8
+CRC
+DCB
+DMA
+EEPROM
+FreeBSD
+IOVA
+LACP
+Linux
+LRO
+LSC
+MAC
+MSS
+MTU
+NIC
+NVM
+NUMA
+PCI
+PHY
+PMD
+RETA
+RSS
+SCTP
+TOS
+TPID
+TSO
+TTL
+UDP
+VLAN
+VDPA
+VSI
-- 
2.17.1

Re: [dpdk-dev] [PATCH] devtools: export title syntax for check-git-log

[Ferruh Yigit]

On 10/25/2019 10:59 AM, Sean Morrissey wrote:
> Moved title syntax to a separate file so that it improves code readability
> and allows for easy addition of new correct title syntax in future cases.
> 
> Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH] devtools: export title syntax for check-git-log

[Thomas Monjalon]

Hi,

Sorry for not looking at this patch before.

25/10/2019 11:59, Sean Morrissey:
> Moved title syntax to a separate file so that it improves code readability
> and allows for easy addition of new correct title syntax in future cases.

I think it is a good idea for a reason which is not said here:
the logic is switched from checking bad patterns to checking good wording.

> Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
[...]
> +data="$selfdir/commit-title-syntax.txt"

The file is processed to check specifically the case of words,
so it should be renamed to something like words-case.txt
The file need to be added to MAINTAINERS file as well.

The variable name "data" is too vague. I suggest "words".

> +while IFS= read -r line

We should avoid "while" loop because it is a sub-shell,
so it forbids setting global variables as it is done
in another patch collecting stats:
	https://patches.dpdk.org/patch/65243/

"for" loop should be fine.

The variable name should be "word" I think.

> +do
> +	regex=":.*\<$line\>"
> +	bad=$(echo "$headlines" | grep -i $regex | grep \

If using -o option of grep, we can capture the word and compare directly
with the correct word. It will be also better when printing the error.

> +		-v ':.*\<OCTEON\ TX\>' )

I don't like having this exception but I have no better idea.
We can move it before the first grep so it will still work with grep -o.

> +	if ! [ -z "$bad" ]
> +	then
> +		bad=$(echo "$headlines" | grep --color=always -v $regex \
> +			| grep --color=always -i $regex \
> +			| sed 's,^,\t,')
> +		[ -z "$bad" ] || printf "Wrong headline case:\n$bad\n"

As said above, the word can be printed alone if using grep -o.

> +	fi
> +done < "$data"
>  
>  # special case check for VMDq to give good error message
>  bad=$(echo "$headlines" | grep -E --color=always \

The VMDq case should be moved as well.

> --- /dev/null
> +++ b/devtools/commit-title-syntax.txt
> @@ -0,0 +1,45 @@
> +Rx
> +Tx
> +PF
> +VF
> +HW
> +SW
> +FW
> +L2
> +L3
> +L4
> +API
> +arm

The right syntax is Arm.

> +aarch64

aarch32 is missing.

> +armv7
> +armv8
> +CRC
> +DCB
> +DMA
> +EEPROM
> +FreeBSD
> +IOVA
> +LACP
> +Linux
> +LRO
> +LSC
> +MAC
> +MSS
> +MTU
> +NIC
> +NVM
> +NUMA
> +PCI
> +PHY
> +PMD
> +RETA
> +RSS
> +SCTP
> +TOS
> +TPID
> +TSO
> +TTL
> +UDP
> +VLAN
> +VDPA

The right syntax is vDPA

> +VSI

[dpdk-dev] [PATCH v2] devtools: export title syntax data for check-git-log

[Ferruh Yigit]

From: Sean Morrissey <sean.morrissey@intel.com>

Moved title syntax to a separate file so that it improves code
readability and allows easy addition.

Also logic changed from checking for bad pattern to checking good
pattern which documents the expected syntax more clearly, and does not
have gaps in the checks.

Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
v2:
* renamed data file to words-case.txt and added to MAINTAINERS file
* Updated script
* as of now vdpa as file prefix is giving false positive
---
 MAINTAINERS               |  1 +
 devtools/check-git-log.sh | 63 ++++++++-------------------------------
 devtools/words-case.txt   | 48 +++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 51 deletions(-)
 create mode 100644 devtools/words-case.txt

diff --git a/MAINTAINERS b/MAINTAINERS
index 1886b18c3..923b49680 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -95,6 +95,7 @@ F: devtools/check-dup-includes.sh
 F: devtools/check-maintainers.sh
 F: devtools/check-forbidden-tokens.awk
 F: devtools/check-git-log.sh
+F: devtools/words-case.txt
 F: devtools/check-includes.sh
 F: devtools/check-symbol-maps.sh
 F: devtools/checkpatches.sh
diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
index f9d055039..2ee5eefd7 100755
--- a/devtools/check-git-log.sh
+++ b/devtools/check-git-log.sh
@@ -83,57 +83,18 @@ bad=$(echo "$headlines" | grep --color=always \
 	| sed 's,^,\t,')
 [ -z "$bad" ] || printf "Wrong headline uppercase:\n$bad\n"
 
-# check headline uppercase (Rx/Tx, VF, L2, MAC, Linux, ARM...)
-bad=$(echo "$headlines" | grep -E --color=always \
-	-e ':.*\<(rx|tx|RX|TX)\>' \
-	-e ':.*\<[pv]f\>' \
-	-e ':.*\<[hsf]w\>' \
-	-e ':.*\<l[234]\>' \
-	-e ':.*\<api\>' \
-	-e ':.*\<ARM\>' \
-	-e ':.*\<(Aarch64|AArch64|AARCH64|Aarch32|AArch32|AARCH32)\>' \
-	-e ':.*\<(Armv7|ARMv7|ArmV7|armV7|ARMV7)\>' \
-	-e ':.*\<(Armv8|ARMv8|ArmV8|armV8|ARMV8)\>' \
-	-e ':.*\<crc\>' \
-	-e ':.*\<dcb\>' \
-	-e ':.*\<dma\>' \
-	-e ':.*\<eeprom\>' \
-	-e ':.*\<freebsd\>' \
-	-e ':.*\<iova\>' \
-	-e ':.*\<lacp\>' \
-	-e ':.*\<linux\>' \
-	-e ':.*\<lro\>' \
-	-e ':.*\<lsc\>' \
-	-e ':.*\<mac\>' \
-	-e ':.*\<mss\>' \
-	-e ':.*\<mtu\>' \
-	-e ':.*\<nic\>' \
-	-e ':.*\<nvm\>' \
-	-e ':.*\<numa\>' \
-	-e ':.*\<pci\>' \
-	-e ':.*\<phy\>' \
-	-e ':.*\<pmd\>' \
-	-e ':.*\<reta\>' \
-	-e ':.*\<rss\>' \
-	-e ':.*\<sctp\>' \
-	-e ':.*\<tos\>' \
-	-e ':.*\<tpid\>' \
-	-e ':.*\<tso\>' \
-	-e ':.*\<ttl\>' \
-	-e ':.*\<udp\>' \
-	-e ':.*\<[Vv]lan\>' \
-	-e ':.*\<vdpa\>' \
-	-e ':.*\<vsi\>' \
-	| grep \
-	-v ':.*\<OCTEON\ TX\>' \
-	| sed 's,^,\t,')
-[ -z "$bad" ] || printf "Wrong headline lowercase:\n$bad\n"
-
-# special case check for VMDq to give good error message
-bad=$(echo "$headlines" | grep -E --color=always \
-	-e '\<(vmdq|VMDQ)\>' \
-	| sed 's,^,\t,')
-[ -z "$bad" ] || printf "Wrong headline capitalization, use 'VMDq':\n$bad\n"
+# check headline case (Rx/Tx, VF, L2, MAC, Linux ...)
+words="$selfdir/words-case.txt"
+for word in $(cat $words); do
+	bad=$(echo "$headlines" | grep -iw $word | grep -v $word)
+	if [ "$word" = "Tx" ]; then
+		bad=$(echo $bad | grep -v 'OCTEON\ TX')
+	fi
+	if [ -n "$bad" ]; then
+		bad_word=$(echo $bad | grep -io $word)
+		printf "Wrong headline case:\n\"$bad\": $bad_word --> $word\n"
+	fi
+done
 
 # check headline length (60 max)
 bad=$(echo "$headlines" |
diff --git a/devtools/words-case.txt b/devtools/words-case.txt
new file mode 100644
index 000000000..5d5490577
--- /dev/null
+++ b/devtools/words-case.txt
@@ -0,0 +1,48 @@
+OCTEON_TX
+aarch32
+aarch64
+API
+Arm
+armv7
+armv8
+CRC
+DCB
+DMA
+EEPROM
+FreeBSD
+FW
+HW
+IOVA
+L2
+L3
+L4
+LACP
+Linux
+LRO
+LSC
+MAC
+MSS
+MTU
+NIC
+NUMA
+NVM
+PCI
+PF
+PHY
+PMD
+RETA
+RSS
+Rx
+SCTP
+SW
+TOS
+TPID
+TSO
+TTL
+Tx
+UDP
+vDPA
+VF
+VLAN
+VMDq
+VSI
-- 
2.24.1

Re: [dpdk-dev] [PATCH v2] devtools: export title syntax data for check-git-log

[Thomas Monjalon]

24/02/2020 13:02, Ferruh Yigit:
> From: Sean Morrissey <sean.morrissey@intel.com>
> 
> Moved title syntax to a separate file so that it improves code
> readability and allows easy addition.
> 
> Also logic changed from checking for bad pattern to checking good
> pattern which documents the expected syntax more clearly, and does not
> have gaps in the checks.
> 
> Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> v2:
> * renamed data file to words-case.txt and added to MAINTAINERS file
> * Updated script
> * as of now vdpa as file prefix is giving false positive

False positive can be avoided if filtering out what is before the first colon.

Re: [dpdk-dev] [PATCH v2] devtools: export title syntax data for check-git-log

[Ferruh Yigit]

On 2/24/2020 1:39 PM, Thomas Monjalon wrote:
> 24/02/2020 13:02, Ferruh Yigit:
>> From: Sean Morrissey <sean.morrissey@intel.com>
>>
>> Moved title syntax to a separate file so that it improves code
>> readability and allows easy addition.
>>
>> Also logic changed from checking for bad pattern to checking good
>> pattern which documents the expected syntax more clearly, and does not
>> have gaps in the checks.
>>
>> Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> v2:
>> * renamed data file to words-case.txt and added to MAINTAINERS file
>> * Updated script
>> * as of now vdpa as file prefix is giving false positive
> 
> False positive can be avoided if filtering out what is before the first colon.
> 

It can, wasn't sure to add more work/check just to cover this case.

Also it is possible to introduce additional check for 'vdpa' only, right now it
is the only case that hits the file prefix.

Do you have a preference?

[dpdk-dev] [PATCH v3] devtools: export title syntax data for check-git-log

[Ferruh Yigit]

From: Sean Morrissey <sean.morrissey@intel.com>

Moved title syntax to a separate file so that it improves code
readability and allows easy addition.

Also logic changed from checking for bad pattern to checking good
pattern which documents the expected syntax more clearly, and does not
have gaps in the checks.

Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
v2:
* renamed data file to words-case.txt and added to MAINTAINERS file
* Updated script
* as of now vdpa as file prefix is giving false positive

v3:
* add check excluding file prefix to address vdpa false positive
* remove OCTEON_TX line from words-case.txt which was a mistake
---
 MAINTAINERS               |  1 +
 devtools/check-git-log.sh | 69 +++++++++------------------------------
 devtools/words-case.txt   | 47 ++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 53 deletions(-)
 create mode 100644 devtools/words-case.txt

diff --git a/MAINTAINERS b/MAINTAINERS
index 1886b18c3..923b49680 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -95,6 +95,7 @@ F: devtools/check-dup-includes.sh
 F: devtools/check-maintainers.sh
 F: devtools/check-forbidden-tokens.awk
 F: devtools/check-git-log.sh
+F: devtools/words-case.txt
 F: devtools/check-includes.sh
 F: devtools/check-symbol-maps.sh
 F: devtools/checkpatches.sh
diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
index f9d055039..4e65be0e4 100755
--- a/devtools/check-git-log.sh
+++ b/devtools/check-git-log.sh
@@ -83,57 +83,22 @@ bad=$(echo "$headlines" | grep --color=always \
 	| sed 's,^,\t,')
 [ -z "$bad" ] || printf "Wrong headline uppercase:\n$bad\n"
 
-# check headline uppercase (Rx/Tx, VF, L2, MAC, Linux, ARM...)
-bad=$(echo "$headlines" | grep -E --color=always \
-	-e ':.*\<(rx|tx|RX|TX)\>' \
-	-e ':.*\<[pv]f\>' \
-	-e ':.*\<[hsf]w\>' \
-	-e ':.*\<l[234]\>' \
-	-e ':.*\<api\>' \
-	-e ':.*\<ARM\>' \
-	-e ':.*\<(Aarch64|AArch64|AARCH64|Aarch32|AArch32|AARCH32)\>' \
-	-e ':.*\<(Armv7|ARMv7|ArmV7|armV7|ARMV7)\>' \
-	-e ':.*\<(Armv8|ARMv8|ArmV8|armV8|ARMV8)\>' \
-	-e ':.*\<crc\>' \
-	-e ':.*\<dcb\>' \
-	-e ':.*\<dma\>' \
-	-e ':.*\<eeprom\>' \
-	-e ':.*\<freebsd\>' \
-	-e ':.*\<iova\>' \
-	-e ':.*\<lacp\>' \
-	-e ':.*\<linux\>' \
-	-e ':.*\<lro\>' \
-	-e ':.*\<lsc\>' \
-	-e ':.*\<mac\>' \
-	-e ':.*\<mss\>' \
-	-e ':.*\<mtu\>' \
-	-e ':.*\<nic\>' \
-	-e ':.*\<nvm\>' \
-	-e ':.*\<numa\>' \
-	-e ':.*\<pci\>' \
-	-e ':.*\<phy\>' \
-	-e ':.*\<pmd\>' \
-	-e ':.*\<reta\>' \
-	-e ':.*\<rss\>' \
-	-e ':.*\<sctp\>' \
-	-e ':.*\<tos\>' \
-	-e ':.*\<tpid\>' \
-	-e ':.*\<tso\>' \
-	-e ':.*\<ttl\>' \
-	-e ':.*\<udp\>' \
-	-e ':.*\<[Vv]lan\>' \
-	-e ':.*\<vdpa\>' \
-	-e ':.*\<vsi\>' \
-	| grep \
-	-v ':.*\<OCTEON\ TX\>' \
-	| sed 's,^,\t,')
-[ -z "$bad" ] || printf "Wrong headline lowercase:\n$bad\n"
-
-# special case check for VMDq to give good error message
-bad=$(echo "$headlines" | grep -E --color=always \
-	-e '\<(vmdq|VMDQ)\>' \
-	| sed 's,^,\t,')
-[ -z "$bad" ] || printf "Wrong headline capitalization, use 'VMDq':\n$bad\n"
+# check headline case (Rx/Tx, VF, L2, MAC, Linux ...)
+IFS='
+'
+words="$selfdir/words-case.txt"
+for word in $(cat $words); do
+	bad=$(echo "$headlines" | grep -iw $word | grep -v $word)
+	if [ "$word" = "Tx" ]; then
+		bad=$(echo $bad | grep -v 'OCTEON\ TX')
+	fi
+	for bad_line in $bad; do
+		bad_word=$(echo $bad_line | cut -d":" -f2 | grep -io $word)
+		if [ -n "$bad_word" ]; then
+			printf "Wrong headline case:\n\"$bad_line\": $bad_word --> $word\n"
+		fi
+	done
+done
 
 # check headline length (60 max)
 bad=$(echo "$headlines" |
@@ -187,8 +152,6 @@ done)
 [ -z "$bad" ] || printf "Missing 'Fixes' tag:\n$bad\n"
 
 # check Fixes: reference
-IFS='
-'
 fixtags=$(echo "$tags" | grep '^Fixes: ')
 bad=$(for fixtag in $fixtags ; do
 	hash=$(echo "$fixtag" | sed 's,^Fixes: \([0-9a-f]*\).*,\1,')
diff --git a/devtools/words-case.txt b/devtools/words-case.txt
new file mode 100644
index 000000000..6bfb81985
--- /dev/null
+++ b/devtools/words-case.txt
@@ -0,0 +1,47 @@
+aarch32
+aarch64
+API
+Arm
+armv7
+armv8
+CRC
+DCB
+DMA
+EEPROM
+FreeBSD
+FW
+HW
+IOVA
+L2
+L3
+L4
+LACP
+Linux
+LRO
+LSC
+MAC
+MSS
+MTU
+NIC
+NUMA
+NVM
+PCI
+PF
+PHY
+PMD
+RETA
+RSS
+Rx
+SCTP
+SW
+TOS
+TPID
+TSO
+TTL
+Tx
+UDP
+vDPA
+VF
+VLAN
+VMDq
+VSI
-- 
2.24.1

Re: [dpdk-dev] [PATCH v3] devtools: export title syntax data for check-git-log

[Thomas Monjalon]

24/02/2020 16:30, Ferruh Yigit:
> From: Sean Morrissey <sean.morrissey@intel.com>
> 
> Moved title syntax to a separate file so that it improves code
> readability and allows easy addition.
> 
> Also logic changed from checking for bad pattern to checking good
> pattern which documents the expected syntax more clearly, and does not
> have gaps in the checks.
> 
> Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> +words="$selfdir/words-case.txt"
> +for word in $(cat $words); do

I think we should exclude comments in the words file.

> +	bad=$(echo "$headlines" | grep -iw $word | grep -v $word)
> +	if [ "$word" = "Tx" ]; then
> +		bad=$(echo $bad | grep -v 'OCTEON\ TX')
> +	fi

We could try to manage such false positive automatically by excluding
valid patterns while patterns list is iterated.
If OCTEON TX is before Tx in the list, no confusion is possible.

> +	for bad_line in $bad; do
> +		bad_word=$(echo $bad_line | cut -d":" -f2 | grep -io $word)

The cut will ignore anything after a second colon.
Should not happen but a possible minor issue.

> +		if [ -n "$bad_word" ]; then
> +			printf "Wrong headline case:\n\"$bad_line\": $bad_word --> $word\n"
> +		fi
> +	done
> +done

I think it is good enough to be merged.
We can adress above improvements in a separate patch.

Applied, thanks