14/06/2018 15:30, Neil Horman:
> * found a way to eliminate the use of filterdiff (new awk rules)
Thanks a lot for not requiring filterdiff dependency.
[...]
> + # Just inform the user of this occurrence, but
> + # don't flag it as an error
> + echo -n "INFO: symbol $syname is added but "
> + echo -n "patch has insuficient context "
> + echo -n "to determine the section name "
> + echo -n "please ensure the version is "
> + echo "EXPERIMENTAL"
For info, I think nowadays "printf" is preferred over "echo -n"
But if you prefer "echo -n" for any reason, no problem.
[...]
> +exit $exit_code
> +
> +
Ironically, this patch doesn't pass checkpatch test because of
the trailing new lines.
[...]
> +clean_tmp_files() {
> + echo $TMPINPUT | grep -q checkpaches
Two comments here.
Since TMPINPUT is not supposed to be overwritten by environment,
I think it is better to make it lowercase (kind of convention).
What the grep is supposed to match?
(side note, there is a typo: checkpaches -> checkpatches)
Is it to remove file only in case of mktemp?
I think it is a risky pattern matching. I suggest '^checkpatches\.'
> + if [ $? -eq 0 ]; then
Could be easier to read if combining "if" and "grep":
if echo $tmpinput | grep -q '^checkpatches\.' ; then
> + rm -f $TMPINPUT
> + fi
> +}
[...]
> + TMPINPUT=$(mktemp checkpatches.XXXXXX)
Open to discussion: do we prefer local dir or /tmp?
Some tools are using /tmp.
[...]
> + report=$($DPDK_CHECKPATCH_PATH $options $TMPINPUT 2>/dev/null)
> +
Please, no blank line between command and test.
> + if [ $? -ne 0 ]
> + then
> + $verbose || printf '\n### %s\n\n' "$3"
> + printf '%s\n' "$report" | sed -n '1,/^total:.*lines checked$/p'
> + ret=1
> + fi
> +
> + ! $verbose || printf '\nChecking API additions/removals:\n'
> +
> + report=$($VALIDATE_NEW_API "$TMPINPUT")
> +
Same comments about blank lines.
> + if [ $? -ne 0 ]; then
> + printf '%s\n' "$report"
> + ret=1
> + fi
> +
> + clean_tmp_files
> + if [ $ret -eq 0 ]; then
> + return 0
> fi
> - [ $? -ne 0 ] || return 0
Why replacing this oneliner by a longer "if" block?
After this review, I think I won't have any more comment.
Thanks Neil
