|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

I'll review this patch series against the DPDK coding standards and guidelines.

git sha: Series of 4 patches (162835-162838)
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [RFC 1/4] devtools: use git trailer parser for tag extraction

This patch series modifies devtools/check-git-log.sh to use git's native
trailer parsing and align with Linux kernel conventions for commit message
trailers.

The changes introduce git log --format="%(trailers:unfold)" to replace
manual grep-based tag extraction, add support for kernel-standard trailers
(Co-developed-by, Closes, Link), remove blank line requirements between
trailer blocks, and demote tag ordering enforcement from error to warning.

> 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 '^$')

[ ... ]

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

Does this change introduce a completeness issue? The Co-developed-by tag
requires specific handling in the kernel - it must be followed immediately
by a Signed-off-by from the co-developer. Does this script validate that
pairing requirement when Co-developed-by is present? Without checking for
the required Signed-off-by immediately after Co-developed-by, does this
code accept malformed Co-developed-by usage?

> @@ -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 filter correctly handle all valid URL schemes? The Linux kernel
accepts various URL schemes beyond http/https (including git://, ftp://,
mailto:, etc.). Does limiting to https\?:// reject valid kernel-style
Closes:/Link: tags with other schemes?

> @@ -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 pattern for Cc: allow malformed entries to pass? The
pattern '^Cc: .*' accepts literally any content after "Cc: ", including
invalid email addresses, missing email addresses, or garbage text. Should
this enforce that Cc: contains at least a valid-looking email address,
similar to how bytag validates "<.*@.*>"?

Similarly, do the patterns for Coverity issue and Bugzilla ID allow empty
or whitespace-only values? The '^Coverity issue: .*' and '^Bugzilla ID: .*'
patterns match even if there's only whitespace after the colon. Should
these require non-empty identifiers?

> 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 removing the blank line requirement between trailer groups create
ambiguity issues? When git's trailer parser encounters a blank line within
what check-git-log.sh previously considered a valid trailer block, does
git now split this into two separate trailer blocks and only parse the
lower block? This could cause the validation to miss trailers that appear
before a blank line, potentially allowing improperly placed tags to pass
undetected. Does the code handle this scenario?

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

Does demoting tag ordering to a warning create consistency issues with
DPDK's existing policy? The patch justifies this by saying the kernel
doesn't enforce ordering via tooling, but does DPDK's contribution guide
explicitly require a specific tag order? If contributors relied on
check-git-log.sh returning failure for ordering violations, does this
change silently allow commits that violate documented DPDK conventions?