Hi,
I wanted to push this patch in 18.02, but when looking more closely,
I see few things to improve.
As it is a tool, there is no harm to wait one more week and push it
early in 18.05.
09/02/2018 16:21, Neil Horman:
> check () { # <patch> <commit> <title>
> + local reta
> total=$(($total + 1))
> ! $verbose || printf '\n### %s\n\n' "$3"
> if [ -n "$1" ] ; then
> @@ -96,9 +100,26 @@ check () { # <patch> <commit> <title>
> else
> report=$($DPDK_CHECKPATCH_PATH $options - 2>/dev/null)
> fi
> - [ $? -ne 0 ] || return 0
You are removing the return, so the report will be always printed.
You must print the report only in case of error.
> + reta=$?
> +
> $verbose || printf '\n### %s\n\n' "$3"
> printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> +
> + ! $verbose || echo
> + ! $verbose || echo "Checking API additions/removals:"
You can use printf to combine these lines.
> +
> + if [ -n "$1" ] ; then
> + report=$($VALIDATE_NEW_API $1)
Beware of spaces in file names: use quoted "$1".
> + elif [ -n "$2" ] ; then
> + report=$(git format-patch \
> + --find-renames --no-stat --stdout -1 $commit |
> + $VALIDATE_NEW_API -)
> + else
> + report=$($VALIDATE_NEW_API -)
So your script supports "-" for stdin? Nice
> + fi
> + [ $? -ne 0 -o $reta -ne 0 ] || return 0
Suggestion of more explicit variable naming:
$reta -> style_result
$? -> symbol_result
> + printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
Wrong copy/paste: the sed is useless for the API report.
