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