[PATCH v2] devtools: ensure proper tag sequence

[PATCH v2] devtools: ensure proper tag sequence

[jpalider]

From: Jakub Palider <jpalider@marvell.com>

This change to log checking procedure ensures that certain
tags are in proper order.
The order of tags is as follows:
 * Coverity issue
 * Bugzilla ID
 * Fixes
 * Cc
 * <BLANK LINE>
 * Suggested-by
 * Reported-by
 + Signed-off-by
where:
 * => 0 or more than one instance possible
 + => more than once instance possible
In order to satisfy the above requirements an extra check
is performed for obligatory tags.

Signed-off-by: Jakub Palider <jpalider@marvell.com>

v2:
* narrowed down the tags under check
---
 devtools/check-git-log.sh           | 48 +++++++++++++++++++++++++++++
 doc/guides/contributing/patches.rst | 24 +++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
index 23c6a7d9bb..c4d1d475c2 100755
--- a/devtools/check-git-log.sh
+++ b/devtools/check-git-log.sh
@@ -54,6 +54,7 @@ fixes=$(git log --format='%h %s' --reverse $range | grep -i ': *fix' | cut -d' '
 stablefixes=$($selfdir/git-log-fixes.sh $range | sed '/(N\/A)$/d'  | cut -d' ' -f2)
 tags=$(git log --format='%b' --reverse $range | grep -i -e 'by *:' -e 'fix.*:')
 bytag='\(Reported\|Suggested\|Signed-off\|Acked\|Reviewed\|Tested\)-by:'
+exttag='Coverity issue:\|Bugzilla ID:\|Fixes:\|Cc:'
 
 failure=false
 
@@ -203,6 +204,53 @@ done)
 [ -z "$bad" ] || { printf "Is it candidate for Cc: stable@dpdk.org backport?\n$bad\n"\
 	&& failure=true;}
 
+# check tag sequence
+bad=$(for commit in $commits; do
+	body=$(git log --format='%b' -1 $commit)
+	echo "$body" |\
+	grep -o -e "$exttag\|^[[:blank:]]*$\|$bytag" | \
+	# retrieve tags only
+	cut -f1 -d":" |\
+	# it is okay to have several tags of the same type but for processing
+	# we need to squash them
+	uniq |\
+	# make sure the tags are in the proper order as presented in SEQ
+	awk -v cmt="$commit" 'BEGIN{
+		SEQ[0] = "Coverity issue";
+		SEQ[1] = "Bugzilla ID";
+		SEQ[2] = "Fixes";
+		SEQ[3] = "Cc";
+		SEQ[4] = "^$";
+		SEQ[5] = "Suggested-by";
+		SEQ[6] = "Reported-by";
+		SEQ[7] = "Signed-off-by";
+		latest = 0;
+	}
+	{
+		for (seq = 0; seq < length(SEQ); seq++) {
+			if (match($0, SEQ[seq])) {
+				if (seq < latest) {
+					print "\tCommit " cmt " (" $0 ":)";
+					break;
+				} else {
+					latest = seq;
+				}
+			}
+		}
+	 }'
+done)
+[ -z "$bad" ] || { printf "Wrong tag order: \n$bad\n"\
+	&& failure=true;}
+
+# check required tag
+bad=$(for commit in $commits; do
+	  body=$(git log --format='%b' -1 $commit)
+	  echo $body | grep -q "Signed-off-by:" \
+	      || echo "\tCommit" $commit "(Signed-off-by:)"
+      done)
+[ -z "$bad" ] || { printf "Missing obligatory tag: \n$bad\n"\
+	&& failure=true;}
+
 total=$(echo "$commits" | wc -l)
 if $failure ; then
 	printf "\nInvalid patch(es) found - checked $total patch"
diff --git a/doc/guides/contributing/patches.rst b/doc/guides/contributing/patches.rst
index bebcaf3925..bbaaf10a42 100644
--- a/doc/guides/contributing/patches.rst
+++ b/doc/guides/contributing/patches.rst
@@ -360,6 +360,30 @@ Where ``NNNNN`` is patchwork ID for patch or series::
      ---
      Depends-on: series-10000 ("Title of the series")
 
+Tag order
+~~~~~~~~~
+
+There is a pattern indicating how certain tags should relate to each other.
+
+Example of proper tag sequence::
+
+     Coverity issue:
+     Bugzilla ID:
+     Fixes:
+     Cc:
+
+     Suggested-by:
+     Reported-by:
+     Signed-off-by:
+
+Between first and second tag section there is and empty line.
+
+While ``Signed-off-by:`` is an obligatory tag and must exists in each commit,
+all other tags are optional. Any tag, as long as it is in proper location
+to other adjacent tags (if present), may occur multiple times.
+
+Other tags shall be laid out in a chronological order.
+
 Creating Patches
 ----------------
 
-- 
2.25.1

Re: [PATCH v2] devtools: ensure proper tag sequence

[Thomas Monjalon]

14/06/2022 00:21, jpalider@marvell.com:
> +		SEQ[0] = "Coverity issue";
> +		SEQ[1] = "Bugzilla ID";
> +		SEQ[2] = "Fixes";
> +		SEQ[3] = "Cc";
> +		SEQ[4] = "^$";
> +		SEQ[5] = "Suggested-by";
> +		SEQ[6] = "Reported-by";
> +		SEQ[7] = "Signed-off-by";
> +		latest = 0;

Do you think you could check that Review, Ack and Test are added
after the first Signed-off?

> +Tag order
> +~~~~~~~~~
> +
> +There is a pattern indicating how certain tags should relate to each other.
> +
> +Example of proper tag sequence::
> +
> +     Coverity issue:
> +     Bugzilla ID:
> +     Fixes:
> +     Cc:
> +
> +     Suggested-by:
> +     Reported-by:
> +     Signed-off-by:

Given it is an example, you could add Reviewed-by, Acked-by and Tested-by.
> +
> +Between first and second tag section there is and empty line.
> +
> +While ``Signed-off-by:`` is an obligatory tag and must exists in each commit,
> +all other tags are optional. Any tag, as long as it is in proper location
> +to other adjacent tags (if present), may occur multiple times.
> +
> +Other tags shall be laid out in a chronological order.

Yes, after the first Signed-off-by.

RE: [EXT] Re: [PATCH v2] devtools: ensure proper tag sequence

[Jakub Palider]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, June 15, 2022 8:21 AM
> To: Jakub Palider <jpalider@marvell.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com
> Subject: [EXT] Re: [PATCH v2] devtools: ensure proper tag sequence
> 
> External Email
> 
> ----------------------------------------------------------------------
> 14/06/2022 00:21, jpalider@marvell.com:
> > +		SEQ[0] = "Coverity issue";
> > +		SEQ[1] = "Bugzilla ID";
> > +		SEQ[2] = "Fixes";
> > +		SEQ[3] = "Cc";
> > +		SEQ[4] = "^$";
> > +		SEQ[5] = "Suggested-by";
> > +		SEQ[6] = "Reported-by";
> > +		SEQ[7] = "Signed-off-by";
> > +		latest = 0;
> 
> Do you think you could check that Review, Ack and Test are added after the
> first Signed-off?

Ok, thank you for clarification. So any further Signed-offs need not to be in a particular 
sequence as long as the first one precedes Review/Ack/Test (these 3 in any order).

> 
> > +Tag order
> > +~~~~~~~~~
> > +
> > +There is a pattern indicating how certain tags should relate to each other.
> > +
> > +Example of proper tag sequence::
> > +
> > +     Coverity issue:
> > +     Bugzilla ID:
> > +     Fixes:
> > +     Cc:
> > +
> > +     Suggested-by:
> > +     Reported-by:
> > +     Signed-off-by:
> 
> Given it is an example, you could add Reviewed-by, Acked-by and Tested-by.

Sure, I will restore it.

> > +
> > +Between first and second tag section there is and empty line.
> > +
> > +While ``Signed-off-by:`` is an obligatory tag and must exists in each
> > +commit, all other tags are optional. Any tag, as long as it is in
> > +proper location to other adjacent tags (if present), may occur multiple
> times.
> > +
> > +Other tags shall be laid out in a chronological order.
> 
> Yes, after the first Signed-off-by.

Side question: what about Sponsored-by, I noticed it appeared just recently?

Re: [EXT] Re: [PATCH v2] devtools: ensure proper tag sequence

[Thomas Monjalon]

15/06/2022 10:46, Jakub Palider:
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Wednesday, June 15, 2022 8:21 AM
> > To: Jakub Palider <jpalider@marvell.com>
> > Cc: dev@dpdk.org; david.marchand@redhat.com
> > Subject: [EXT] Re: [PATCH v2] devtools: ensure proper tag sequence
> > 
> > External Email
> > 
> > ----------------------------------------------------------------------
> > 14/06/2022 00:21, jpalider@marvell.com:
> > > +		SEQ[0] = "Coverity issue";
> > > +		SEQ[1] = "Bugzilla ID";
> > > +		SEQ[2] = "Fixes";
> > > +		SEQ[3] = "Cc";
> > > +		SEQ[4] = "^$";
> > > +		SEQ[5] = "Suggested-by";
> > > +		SEQ[6] = "Reported-by";
> > > +		SEQ[7] = "Signed-off-by";
> > > +		latest = 0;
> > 
> > Do you think you could check that Review, Ack and Test are added after the
> > first Signed-off?
> 
> Ok, thank you for clarification. So any further Signed-offs need not to be in a particular 
> sequence as long as the first one precedes Review/Ack/Test (these 3 in any order).

Yes

> > > +Tag order
> > > +~~~~~~~~~
> > > +
> > > +There is a pattern indicating how certain tags should relate to each other.
> > > +
> > > +Example of proper tag sequence::
> > > +
> > > +     Coverity issue:
> > > +     Bugzilla ID:
> > > +     Fixes:
> > > +     Cc:
> > > +
> > > +     Suggested-by:
> > > +     Reported-by:
> > > +     Signed-off-by:
> > 
> > Given it is an example, you could add Reviewed-by, Acked-by and Tested-by.
> 
> Sure, I will restore it.
> 
> > > +
> > > +Between first and second tag section there is and empty line.
> > > +
> > > +While ``Signed-off-by:`` is an obligatory tag and must exists in each
> > > +commit, all other tags are optional. Any tag, as long as it is in
> > > +proper location to other adjacent tags (if present), may occur multiple
> > times.
> > > +
> > > +Other tags shall be laid out in a chronological order.
> > 
> > Yes, after the first Signed-off-by.
> 
> Side question: what about Sponsored-by, I noticed it appeared just recently?

It is a way to add the name of a company wishing to appear.
There is no constraint on it, it can be anywhere I think.
If it is in chronological order, it looks better before the Signed-off.
I think we should not advertise it here, because real names are more useful.

[PATCH v3] devtools: ensure proper tag sequence

[jpalider]

From: Jakub Palider <jpalider@marvell.com>

This change to log checking procedure ensures that certain
tags are in proper order.
The order of tags is as follows:
 * Coverity issue
 * Bugzilla ID
 * Fixes
 * Cc
 * <BLANK LINE>
 * Suggested-by
 * Reported-by
 + Signed-off-by
 * Acked-by
 * Reviewed-by
 * Tested-by
where:
 * => 0 or more than one instance possible
 + => more than once instance possible
In order to satisfy the above requirements an extra check
is performed for obligatory tags.

v3:
* restored some tags under check
* defined chronological order after Signed-off

v2:
* narrowed down the tags under check

Signed-off-by: Jakub Palider <jpalider@marvell.com>
---
 devtools/check-git-log.sh           | 56 +++++++++++++++++++++++++++++
 doc/guides/contributing/patches.rst | 28 +++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
index 23c6a7d9bb..9b98207f3a 100755
--- a/devtools/check-git-log.sh
+++ b/devtools/check-git-log.sh
@@ -54,6 +54,7 @@ fixes=$(git log --format='%h %s' --reverse $range | grep -i ': *fix' | cut -d' '
 stablefixes=$($selfdir/git-log-fixes.sh $range | sed '/(N\/A)$/d'  | cut -d' ' -f2)
 tags=$(git log --format='%b' --reverse $range | grep -i -e 'by *:' -e 'fix.*:')
 bytag='\(Reported\|Suggested\|Signed-off\|Acked\|Reviewed\|Tested\)-by:'
+exttag='Coverity issue:\|Bugzilla ID:\|Fixes:\|Cc:'
 
 failure=false
 
@@ -203,6 +204,61 @@ done)
 [ -z "$bad" ] || { printf "Is it candidate for Cc: stable@dpdk.org backport?\n$bad\n"\
 	&& failure=true;}
 
+# check tag sequence
+bad=$(for commit in $commits; do
+	body=$(git log --format='%b' -1 $commit)
+	echo "$body" |\
+	grep -o -e "$exttag\|^[[:blank:]]*$\|$bytag" | \
+	# retrieve tags only
+	cut -f1 -d":" |\
+	# it is okay to have several tags of the same type but for processing
+	# we need to squash them
+	uniq |\
+	# make sure the tags are in the proper order as presented in SEQ
+	awk -v cmt="$commit" 'BEGIN{
+		SEQ[0] = "Coverity issue";
+		SEQ[1] = "Bugzilla ID";
+		SEQ[2] = "Fixes";
+		SEQ[3] = "Cc";
+		SEQ[4] = "^$";
+		SEQ[5] = "Suggested-by";
+		SEQ[6] = "Reported-by";
+		SEQ[7] = "Signed-off-by";
+		SEQ[8] = "Acked-by";
+		SEQ[9] = "Reviewed-by";
+		SEQ[10] = "Tested-by";
+		latest = 0;
+		chronological = 0;
+	}
+	{
+		for (seq = 0; seq < length(SEQ); seq++) {
+			if (chronological == 1)
+				continue;
+			if (match($0, SEQ[seq])) {
+				if (seq < latest) {
+					print "\tCommit " cmt " (" $0 ":)";
+					break;
+				} else {
+					latest = seq;
+				}
+			}
+		}
+		if (match($0, "Signed-off-by"))
+			chronological = 1;
+	 }'
+done)
+[ -z "$bad" ] || { printf "Wrong tag order: \n$bad\n"\
+	&& failure=true;}
+
+# check required tag
+bad=$(for commit in $commits; do
+	  body=$(git log --format='%b' -1 $commit)
+	  echo $body | grep -q "Signed-off-by:" \
+	      || echo "\tCommit" $commit "(Signed-off-by:)"
+      done)
+[ -z "$bad" ] || { printf "Missing obligatory tag: \n$bad\n"\
+	&& failure=true;}
+
 total=$(echo "$commits" | wc -l)
 if $failure ; then
 	printf "\nInvalid patch(es) found - checked $total patch"
diff --git a/doc/guides/contributing/patches.rst b/doc/guides/contributing/patches.rst
index bebcaf3925..996089f2eb 100644
--- a/doc/guides/contributing/patches.rst
+++ b/doc/guides/contributing/patches.rst
@@ -360,6 +360,34 @@ Where ``NNNNN`` is patchwork ID for patch or series::
      ---
      Depends-on: series-10000 ("Title of the series")
 
+Tag order
+~~~~~~~~~
+
+There is a pattern indicating how certain tags should relate to each other.
+
+Example of proper tag sequence::
+
+     Coverity issue:
+     Bugzilla ID:
+     Fixes:
+     Cc:
+
+     Suggested-by:
+     Reported-by:
+     Signed-off-by:
+     Acked-by:
+     Reviewed-by:
+     Tested-by:
+
+Between first and second tag section there is and empty line.
+
+While ``Signed-off-by:`` is an obligatory tag and must exists in each commit,
+all other tags are optional. Any tag, as long as it is in proper location
+to other adjacent tags (if present), may occur multiple times.
+
+Other tags after the first occurrence of ``Signed-off-by:`` shall be laid out in
+a chronological order.
+
 Creating Patches
 ----------------
 
-- 
2.25.1

RE: [PATCH v3] devtools: ensure proper tag sequence

[Jakub Palider]

> -----Original Message-----
> From: jpalider@marvell.com <jpalider@marvell.com>
> Sent: Tuesday, July 19, 2022 2:01 PM
> To: Jakub Palider <jpalider@marvell.com>
> Cc: david.marchand@redhat.com; dev@dpdk.org; thomas@monjalon.net
> Subject: [PATCH v3] devtools: ensure proper tag sequence
> 
> From: Jakub Palider <jpalider@marvell.com>
> 
> This change to log checking procedure ensures that certain tags are in proper
> order.
> The order of tags is as follows:
>  * Coverity issue
>  * Bugzilla ID
>  * Fixes
>  * Cc
>  * <BLANK LINE>
>  * Suggested-by
>  * Reported-by
>  + Signed-off-by
>  * Acked-by
>  * Reviewed-by
>  * Tested-by
> where:
>  * => 0 or more than one instance possible  + => more than once instance
> possible In order to satisfy the above requirements an extra check is
> performed for obligatory tags.
> 
> v3:
> * restored some tags under check
> * defined chronological order after Signed-off
> 
> v2:
> * narrowed down the tags under check
> 
> Signed-off-by: Jakub Palider <jpalider@marvell.com>
> ---
>  devtools/check-git-log.sh           | 56 +++++++++++++++++++++++++++++
>  doc/guides/contributing/patches.rst | 28 +++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh index
> 23c6a7d9bb..9b98207f3a 100755
> --- a/devtools/check-git-log.sh
> +++ b/devtools/check-git-log.sh
> @@ -54,6 +54,7 @@ fixes=$(git log --format='%h %s' --reverse $range | grep
> -i ': *fix' | cut -d' '
>  stablefixes=$($selfdir/git-log-fixes.sh $range | sed '/(N\/A)$/d'  | cut -d' ' -
> f2)  tags=$(git log --format='%b' --reverse $range | grep -i -e 'by *:' -e 'fix.*:')
> bytag='\(Reported\|Suggested\|Signed-off\|Acked\|Reviewed\|Tested\)-
> by:'
> +exttag='Coverity issue:\|Bugzilla ID:\|Fixes:\|Cc:'
> 
>  failure=false
> 
> @@ -203,6 +204,61 @@ done)
>  [ -z "$bad" ] || { printf "Is it candidate for Cc: stable@dpdk.org
> backport?\n$bad\n"\
>  	&& failure=true;}
> 
> +# check tag sequence
> +bad=$(for commit in $commits; do
> +	body=$(git log --format='%b' -1 $commit)
> +	echo "$body" |\
> +	grep -o -e "$exttag\|^[[:blank:]]*$\|$bytag" | \
> +	# retrieve tags only
> +	cut -f1 -d":" |\
> +	# it is okay to have several tags of the same type but for processing
> +	# we need to squash them
> +	uniq |\
> +	# make sure the tags are in the proper order as presented in SEQ
> +	awk -v cmt="$commit" 'BEGIN{
> +		SEQ[0] = "Coverity issue";
> +		SEQ[1] = "Bugzilla ID";
> +		SEQ[2] = "Fixes";
> +		SEQ[3] = "Cc";
> +		SEQ[4] = "^$";
> +		SEQ[5] = "Suggested-by";
> +		SEQ[6] = "Reported-by";
> +		SEQ[7] = "Signed-off-by";
> +		SEQ[8] = "Acked-by";
> +		SEQ[9] = "Reviewed-by";
> +		SEQ[10] = "Tested-by";
> +		latest = 0;
> +		chronological = 0;
> +	}
> +	{
> +		for (seq = 0; seq < length(SEQ); seq++) {
> +			if (chronological == 1)
> +				continue;
> +			if (match($0, SEQ[seq])) {
> +				if (seq < latest) {
> +					print "\tCommit " cmt " (" $0 ":)";
> +					break;
> +				} else {
> +					latest = seq;
> +				}
> +			}
> +		}
> +		if (match($0, "Signed-off-by"))
> +			chronological = 1;
> +	 }'
> +done)
> +[ -z "$bad" ] || { printf "Wrong tag order: \n$bad\n"\
> +	&& failure=true;}
> +
> +# check required tag
> +bad=$(for commit in $commits; do
> +	  body=$(git log --format='%b' -1 $commit)
> +	  echo $body | grep -q "Signed-off-by:" \
> +	      || echo "\tCommit" $commit "(Signed-off-by:)"
> +      done)
> +[ -z "$bad" ] || { printf "Missing obligatory tag: \n$bad\n"\
> +	&& failure=true;}
> +
>  total=$(echo "$commits" | wc -l)
>  if $failure ; then
>  	printf "\nInvalid patch(es) found - checked $total patch"
> diff --git a/doc/guides/contributing/patches.rst
> b/doc/guides/contributing/patches.rst
> index bebcaf3925..996089f2eb 100644
> --- a/doc/guides/contributing/patches.rst
> +++ b/doc/guides/contributing/patches.rst
> @@ -360,6 +360,34 @@ Where ``NNNNN`` is patchwork ID for patch or
> series::
>       ---
>       Depends-on: series-10000 ("Title of the series")
> 
> +Tag order
> +~~~~~~~~~
> +
> +There is a pattern indicating how certain tags should relate to each other.
> +
> +Example of proper tag sequence::
> +
> +     Coverity issue:
> +     Bugzilla ID:
> +     Fixes:
> +     Cc:
> +
> +     Suggested-by:
> +     Reported-by:
> +     Signed-off-by:
> +     Acked-by:
> +     Reviewed-by:
> +     Tested-by:
> +
> +Between first and second tag section there is and empty line.
> +
> +While ``Signed-off-by:`` is an obligatory tag and must exists in each
> +commit, all other tags are optional. Any tag, as long as it is in
> +proper location to other adjacent tags (if present), may occur multiple
> times.
> +
> +Other tags after the first occurrence of ``Signed-off-by:`` shall be
> +laid out in a chronological order.
> +
>  Creating Patches
>  ----------------
> 
> --
> 2.25.1

Hello,

as the automatic tests reported performance only degradation on patch analysis change would it be feasible to integrate this patch?
I think all concerns from previous versions are addressed.

Regards,
Jakub

Re: [PATCH v3] devtools: ensure proper tag sequence

[Thomas Monjalon]

19/07/2022 14:01, jpalider@marvell.com:
> From: Jakub Palider <jpalider@marvell.com>
> 
> This change to log checking procedure ensures that certain
> tags are in proper order.
> The order of tags is as follows:
>  * Coverity issue
>  * Bugzilla ID
>  * Fixes
>  * Cc
>  * <BLANK LINE>
>  * Suggested-by
>  * Reported-by
>  + Signed-off-by
>  * Acked-by
>  * Reviewed-by
>  * Tested-by
> where:
>  * => 0 or more than one instance possible
>  + => more than once instance possible
> In order to satisfy the above requirements an extra check
> is performed for obligatory tags.
> 
> v3:
> * restored some tags under check
> * defined chronological order after Signed-off
> 
> v2:
> * narrowed down the tags under check
> 
> Signed-off-by: Jakub Palider <jpalider@marvell.com>

I've moved "Reported-by" before "Suggested-by" and
made the output more consistent with other checks.

Applied, thanks.