|SUCCESS| [RFC,4/4] devtools: demote tag ordering to warning

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162838

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-02

git sha c9f8b2d5a3e1f4c6b7d8e9f0a1b2c3d4e5f6g7h8
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [RFC 1/4] devtools: use git trailer parser for tag extraction

This patch series updates check-git-log.sh to use git's native trailer
parsing instead of hand-rolled regex, adds support for kernel-style
trailers, removes the blank line requirement between trailers, and
demotes tag ordering violations to warnings.

> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
> index 45a58fab4f..1e04bca01b 100755
> --- a/devtools/check-git-log.sh
> +++ b/devtools/check-git-log.sh
> @@ -52,7 +52,9 @@ headlines=$(git log --format='%s' --reverse $range)
>  bodylines=$(git log --format='%b' --reverse $range)
>  fixes=$(git log --format='%h %s' --reverse $range | grep -i ': *fix' | cut -d' ' -f1)
>  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.*:')
> +# Use git's own trailer parser rather than hand-rolled regex.
> +# %(trailers:unfold) gives us exactly what git-interpret-trailers --parse sees.
> +tags=$(git log --format='%(trailers:unfold)' --reverse $range | grep -v '^$')

Does the trailing "| grep -v '^$'" correctly handle the case where
git-log produces output containing multiple commits, each potentially
having blank lines between their trailer blocks?  Since %(trailers:unfold)
operates per-commit, does this filter preserve the per-commit structure
needed by the subsequent tag validation logic that processes $tags?

[ ... ]

> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
> index 1e04bca01b..3775b1587f 100755
> --- a/devtools/check-git-log.sh
> +++ b/devtools/check-git-log.sh
> @@ -55,8 +55,8 @@ stablefixes=$($selfdir/git-log-fixes.sh $range | sed '/(N\/A)$/d'  | cut -d' ' -
>  # Use git's own trailer parser rather than hand-rolled regex.
>  # %(trailers:unfold) gives us exactly what git-interpret-trailers --parse sees.
>  tags=$(git log --format='%(trailers:unfold)' --reverse $range | grep -v '^$')
> -bytag='\(Reported\|Suggested\|Signed-off\|Acked\|Reviewed\|Tested\)-by:'
> -reltag='Coverity issue:\|Bugzilla ID:\|Fixes:\|Cc:'
> +bytag='\(Reported\|Suggested\|Signed-off\|Co-developed\|Acked\|Reviewed\|Tested\)-by:'
> +reltag='Coverity issue:\|Bugzilla ID:\|Fixes:\|Cc:\|Closes:\|Link:'

[ ... ]

> @@ -149,6 +149,7 @@ bad=$(echo "$headlines" |
>  
>  # check body lines length (75 max)
>  bad=$(echo "$bodylines" | grep -v '^Fixes:' |
> +	grep -v '^Closes: https\?://' | grep -v '^Link: https\?://' |

Does this pattern correctly exempt Closes: and Link: tags that use
http:// URLs (without TLS)?  The regex allows both https? and http?,
but is http:// expected to be valid for these tags?

[ ... ]

> @@ -165,6 +166,11 @@ done | sed 's,^,\t,')
>  bad=$(echo "$tags" |
>  	grep -v "^$bytag [^,]* <.*@.*>$" |
>  	grep -v '^Fixes: [0-9a-f]\{7\}[0-9a-f]* (".*")$' |
> +	grep -v '^Closes: https\?://.*' |
> +	grep -v '^Link: https\?://.*' |
> +	grep -v '^Cc: .*' |
> +	grep -v '^Coverity issue: .*' |
> +	grep -v '^Bugzilla ID: .*' |

Does the validation regex "^Cc: .*" adequately check the format of Cc:
tags?  The pattern for $bytag requires an email address format, but
this pattern accepts any content after "Cc: ".  Should Cc: tags follow
the same email format validation as other attribution tags?

[ ... ]

> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
> index 3775b1587f..94cf68b926 100755
> --- a/devtools/check-git-log.sh
> +++ b/devtools/check-git-log.sh
> @@ -223,28 +223,29 @@ done)
>  	&& failure=true;}
>  
>  # check tag sequence
> +# All trailers form a single contiguous block with no blank lines,
> +# matching kernel/netdev convention and git-interpret-trailers(1).
>  bad=$(for commit in $commits; do
> -	body=$(git log --format='%b' -1 $commit)
> -	echo "$body" |
> -	grep -o -e "$reltag\|^[[:blank:]]*$\|$bytag" |
> -	# retrieve tags only
> -	cut -f1 -d":" |
> +	git log --format='%(trailers:key-only)' -1 $commit |
> +	grep -v '^$' |

Does the removal of blank line filtering here interact correctly with
the tag ordering validation below?  The previous code explicitly
expected a blank line (SEQ[4] = "^$") between relation and attribution
tags, but now that blank lines are removed entirely, does the awk
script still function correctly when it encounters consecutive tags?

[ ... ]

> diff --git a/devtools/check-git-log.sh b/devtools/check-git-log.sh
> index 94cf68b926..5b621f5c8d 100755
> --- a/devtools/check-git-log.sh
> +++ b/devtools/check-git-log.sh
> @@ -266,8 +266,7 @@ bad=$(for commit in $commits; do
>  			chronological = 1;
>  	 }'
>  done)
> -[ -z "$bad" ] || { printf "Wrong tag order: \n$bad\n"\
> -	&& failure=true;}
> +[ -z "$bad" ] || printf "WARNING: non-preferred tag order: \n$bad\n"

This changes the script behavior to no longer fail CI when tags are
out of order.  Does this align with DPDK's patch acceptance policy?
The AGENTS.md file may not cover this, but is there documentation
elsewhere that would need updating to reflect this policy change?