Eelco Chaudron <[email protected]> writes:
> 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.
FYI, I did roll the SOURCE change in on commit. Thanks!
>>>> +# 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
