Eelco Chaudron <[email protected]> writes:
> On 27 Mar 2025, at 16:10, Aaron Conole wrote:
>
>> Checkpatch has the ability to scan source files rather than
>> exclusively scanning patch files. In this mode, some checks
>> get disabled (ex: all of the header and commit message
>> parsing) and the display modes are adjusted to simply print
>> the file and line, rather than including the patch line
>> details.
>>
>> When checkpatch was updated for subject parsing, it was
>> inadvertently ignoring the source file mode. This means
>> any checks run against a source file will always print a
>> bogus subject suggestion, and warning. Fix this by not
>> printing these warnings when in source file mode. This
>> should have no effect on checkpatch against actual patch
>> files.
>>
>> Additionally, introduce two simple tests to verify that
>> the source file mode still functions.
>>
>> Fixes: 1b8fa4a66aa4 ("checkpatch: Add checks for the subject line.")
>> Signed-off-by: Aaron Conole <[email protected]>
>
> The patch looks good to me with one small comment below. If applied during
> commit, it looks good to me.
>
> Acked-by: Eelco Chaudron <[email protected]>
>
>> ---
>> v3: Address Eelco's feedback
>> v2: Added unit tests to make sure we have some basic checks
>> for source file mode.
>>
>> tests/checkpatch.at | 69 +++++++++++++++++++++++++++++++++++++++++
>> utilities/checkpatch.py | 8 +++--
>> 2 files changed, 74 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
>> index 2ed2ec878b..29a25fef44 100755
>> --- a/tests/checkpatch.at
>> +++ b/tests/checkpatch.at
>> @@ -38,6 +38,31 @@ Subject: Patch this is.
>> $top_srcdir/utilities/checkpatch.py $3 -q test.patch])
>> fi
>> }
>> +
>> +# try_checkpatch_c_file FILE [ERRORS] [CHECKPATCH-ARGS]
>> +#
>> +# Runs checkpatch against test FILE expecting the set of specified
>
> Should FILE not be FILE_CONTENTS also, there is a double space after FILE.
I thought about it when moving from 2->3, but I looked at the
try_checkpatch documentation, and that uses PATCH instead of
PATCH_CONTENTS or PATCH_FILE. Maybe it actually could make more sense
as SOURCE. WDYT?
>> +# ERRORS (and warnings). Optionally, sets [CHECKPATCH-ARGS]
>> +try_checkpatch_c_file() {
>> + echo "$1" | sed 's/^ //' > test.c
>> +
>> + # Take expected output from $2.
>> + if test -n "$2"; then
>> + echo "$2" | sed 's/^ //' > expout
>> + else
>> + : > expout
>> + fi
>> +
>> + if test -s expout; then
>> + AT_CHECK([OVS_SRC_DIR=$top_srcdir $PYTHON3 \
>> + $top_srcdir/utilities/checkpatch.py $3 -q -f test.c],
>> + [1], [stdout])
>> + AT_CHECK([sed '/^Lines checked:/,$d' stdout], [0], [expout])
>> + else
>> + AT_CHECK([OVS_SRC_DIR=$top_srcdir $PYTHON3 \
>> + $top_srcdir/utilities/checkpatch.py $3 -q -f test.c])
>> + fi
>> +}
>> OVS_END_SHELL_HELPERS
>>
>> AT_SETUP([checkpatch - sign-offs])
>> @@ -657,3 +682,47 @@ try_checkpatch \
>> "-a"
>>
>> AT_CLEANUP
>> +
>> +AT_SETUP([checkpatch - file contents checks - bare return])
>> +try_checkpatch_c_file \
>> + "#include <foo.h>
>> + #include <bar.h>
>> +
>> + void foo() {
>> + return;
>> + }" \
>> + "WARNING: Empty return followed by brace, consider omitting
>> + test.c:6:
>> + }
>> + "
>> +AT_CLEANUP
>> +
>> +AT_SETUP([checkpatch - file contents checks - parenthesized constructs])
>> +
>> +for ctr in 'if' 'while' 'switch' 'HMAP_FOR_EACH' 'BITMAP_FOR_EACH_1'; do
>> +try_checkpatch_c_file \
>> + "#include <foo.h>
>> + #include <bar.h>
>> +
>> + void foo() {
>> + $ctr (check_node) {
>> + something(check_node);
>> + }
>> + }
>> + "
>> +
>> +try_checkpatch_c_file \
>> + "#include <foo.h>
>> + #include <bar.h>
>> +
>> + void foo() {
>> + $ctr ( first_run) {
>> + something(check_node);
>> + }
>> + }" \
>> + "ERROR: Improper whitespace around control block
>> + test.c:5:
>> + $ctr ( first_run) {
>> + "
>> +done
>> +AT_CLEANUP
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 3c09b94cf5..b356731ea4 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -1189,10 +1189,12 @@ def ovs_checkpatch_file(filename):
>> else:
>> mail.add_header('Subject', sys.argv[-1])
>>
>> - print("Subject missing! Your provisional subject is",
>> - mail['Subject'])
>> + if not checking_file:
>> + print("Subject missing! Your provisional subject is",
>> + mail['Subject'])
>>
>> - if run_subject_checks('Subject: ' + mail['Subject'], spellcheck):
>> + if not checking_file and run_subject_checks('Subject: ' +
>> mail['Subject'],
>> + spellcheck):
>> result = True
>>
>> ovs_checkpatch_print_result()
>> --
>> 2.47.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
