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

[Jakub Palider <jpalider@marvell.com>]

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