[v2] devtools: ensure proper tag sequence
Checks
Commit Message
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(+)
Comments
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.
> -----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?
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.
@@ -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"
@@ -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
----------------