On 28 Mar 2025, at 14:22, Aaron Conole wrote:
> 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?
Yes, SOURCE would also work, as FILE is too general.
>>> +# 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
