| Message ID | 20210810172624.104634-1-tredaelli@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] checkpatch: check if some tags are wrongly written | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-Build_and_Test | fail | github build: failed |
Timothy Redaelli <tredaelli@redhat.com> writes: > Currently, there are some patches with the tags wrongly written (with > space instead of dash ) and this may prevent some automatic system or CI > to detect them correctly. > > This commit adds a check in checkpatch to be sure the tag is written > correctly with dash and not with space. > > The tags supported by the commit are: > Reported-by, Requested-by, Suggested-by, Reported-at, and Submitted-at > > It's not necessary to add "Signed-off-by" since it's already checked in > checkpatch. > > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> > --- Hi Timothy - thanks for this patch! There's some flake8 errors introduced here. Python doesn't like aligning the entities inside the corrections array (for some crazy reason). utilities/checkpatch.py:753:26: E241 multiple spaces after ':' utilities/checkpatch.py:756:26: E241 multiple spaces after ':' With those fixed - Acked-by: Aaron Conole <aconole@redhat.com> CC'd Sal - looks like the recent change introduced for multiple workflow support broke the way we report errors. At the very least, we have an error message and link to the failing builds, but the actual step failures isn't included. See: https://mail.openvswitch.org/pipermail/ovs-build/2021-August/016479.html > tests/checkpatch.at | 44 +++++++++++++++++++++++++++++++++++++++++ > utilities/checkpatch.py | 13 ++++++++++++ > 2 files changed, 57 insertions(+) > > diff --git a/tests/checkpatch.at b/tests/checkpatch.at > index 0718acd99..8eb6a7558 100755 > --- a/tests/checkpatch.at > +++ b/tests/checkpatch.at > @@ -348,3 +348,47 @@ try_checkpatch \ > " > > AT_CLEANUP > + > +AT_SETUP([checkpatch - malformed tags]) > +try_checkpatch \ > + " Author: A > + > + Reported by: foo... > + Signed-off-by: A" \ > + "ERROR: Reported-by tag is malformed. > + 1: Reported by: foo... > +" > +try_checkpatch \ > + " Author: A > + > + Requested by: foo... > + Signed-off-by: A" \ > + "ERROR: Requested-by tag is malformed. > + 1: Requested by: foo... > +" > +try_checkpatch \ > + " Author: A > + > + Suggested by: foo... > + Signed-off-by: A" \ > + "ERROR: Suggested-by tag is malformed. > + 1: Suggested by: foo... > +" > +try_checkpatch \ > + " Author: A > + > + Reported at: foo... > + Signed-off-by: A" \ > + "ERROR: Reported-at tag is malformed. > + 1: Reported at: foo... > +" > +try_checkpatch \ > + " Author: A > + > + Submitted at: foo... > + Signed-off-by: A" \ > + "ERROR: Submitted-at tag is malformed. > + 1: Submitted at: foo... > +" > + > +AT_CLEANUP > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > index 699fb4b02..97e21eeaa 100755 > --- a/utilities/checkpatch.py > +++ b/utilities/checkpatch.py > @@ -749,6 +749,14 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None): > is_gerrit_change_id = re.compile(r'(\s*(change-id: )(.*))$', > re.I | re.M | re.S) > > + tags_typos = { > + r'^Reported by:': 'Reported-by:', > + r'^Requested by:': 'Requested-by:', > + r'^Suggested by:': 'Suggested-by:', > + r'^Reported at:': 'Reported-at:', > + r'^Submitted at:': 'Submitted-at:' > + } > + > reset_counters() > > for line in text.splitlines(): > @@ -838,6 +846,11 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None): > print("%d: %s\n" % (lineno, line)) > elif spellcheck: > check_spelling(line, False) > + for typo, correct in tags_typos.items(): > + m = re.match(typo, line, re.I) > + if m: > + print_error("%s tag is malformed." % (correct[:-1])) > + print("%d: %s\n" % (lineno, line)) > > elif parse == PARSE_STATE_CHANGE_BODY: > newfile = hunks.match(line)
On Wed, Aug 11, 2021 at 8:27 AM Aaron Conole <aconole@redhat.com> wrote: > Timothy Redaelli <tredaelli@redhat.com> writes: > > > Currently, there are some patches with the tags wrongly written (with > > space instead of dash ) and this may prevent some automatic system or CI > > to detect them correctly. > > > > This commit adds a check in checkpatch to be sure the tag is written > > correctly with dash and not with space. > > > > The tags supported by the commit are: > > Reported-by, Requested-by, Suggested-by, Reported-at, and Submitted-at > > > > It's not necessary to add "Signed-off-by" since it's already checked in > > checkpatch. > > > > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> > > --- > > Hi Timothy - thanks for this patch! > > There's some flake8 errors introduced here. Python doesn't like > aligning the entities inside the corrections array (for some crazy > reason). > > utilities/checkpatch.py:753:26: E241 multiple spaces after ':' > utilities/checkpatch.py:756:26: E241 multiple spaces after ':' > > With those fixed - > > Acked-by: Aaron Conole <aconole@redhat.com> > > CC'd Sal - looks like the recent change introduced for multiple workflow > support broke the way we report errors. At the very least, we have an > error message and link to the failing builds, but the actual step > failures isn't included. > > Noted, I will take a look at this. > See: > https://mail.openvswitch.org/pipermail/ovs-build/2021-August/016479.html > > > tests/checkpatch.at | 44 +++++++++++++++++++++++++++++++++++++++++ > > utilities/checkpatch.py | 13 ++++++++++++ > > 2 files changed, 57 insertions(+) > > > > diff --git a/tests/checkpatch.at b/tests/checkpatch.at > > index 0718acd99..8eb6a7558 100755 > > --- a/tests/checkpatch.at > > +++ b/tests/checkpatch.at > > @@ -348,3 +348,47 @@ try_checkpatch \ > > " > > > > AT_CLEANUP > > + > > +AT_SETUP([checkpatch - malformed tags]) > > +try_checkpatch \ > > + " Author: A > > + > > + Reported by: foo... > > + Signed-off-by: A" \ > > + "ERROR: Reported-by tag is malformed. > > + 1: Reported by: foo... > > +" > > +try_checkpatch \ > > + " Author: A > > + > > + Requested by: foo... > > + Signed-off-by: A" \ > > + "ERROR: Requested-by tag is malformed. > > + 1: Requested by: foo... > > +" > > +try_checkpatch \ > > + " Author: A > > + > > + Suggested by: foo... > > + Signed-off-by: A" \ > > + "ERROR: Suggested-by tag is malformed. > > + 1: Suggested by: foo... > > +" > > +try_checkpatch \ > > + " Author: A > > + > > + Reported at: foo... > > + Signed-off-by: A" \ > > + "ERROR: Reported-at tag is malformed. > > + 1: Reported at: foo... > > +" > > +try_checkpatch \ > > + " Author: A > > + > > + Submitted at: foo... > > + Signed-off-by: A" \ > > + "ERROR: Submitted-at tag is malformed. > > + 1: Submitted at: foo... > > +" > > + > > +AT_CLEANUP > > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > > index 699fb4b02..97e21eeaa 100755 > > --- a/utilities/checkpatch.py > > +++ b/utilities/checkpatch.py > > @@ -749,6 +749,14 @@ def ovs_checkpatch_parse(text, filename, > author=None, committer=None): > > is_gerrit_change_id = re.compile(r'(\s*(change-id: )(.*))$', > > re.I | re.M | re.S) > > > > + tags_typos = { > > + r'^Reported by:': 'Reported-by:', > > + r'^Requested by:': 'Requested-by:', > > + r'^Suggested by:': 'Suggested-by:', > > + r'^Reported at:': 'Reported-at:', > > + r'^Submitted at:': 'Submitted-at:' > > + } > > + > > reset_counters() > > > > for line in text.splitlines(): > > @@ -838,6 +846,11 @@ def ovs_checkpatch_parse(text, filename, > author=None, committer=None): > > print("%d: %s\n" % (lineno, line)) > > elif spellcheck: > > check_spelling(line, False) > > + for typo, correct in tags_typos.items(): > > + m = re.match(typo, line, re.I) > > + if m: > > + print_error("%s tag is malformed." % (correct[:-1])) > > + print("%d: %s\n" % (lineno, line)) > > > > elif parse == PARSE_STATE_CHANGE_BODY: > > newfile = hunks.match(line) > >
Hi, the patch looks good to me, there's only one remark. Timothy Redaelli <tredaelli@redhat.com> writes: > Currently, there are some patches with the tags wrongly written (with > space instead of dash ) and this may prevent some automatic system or CI > to detect them correctly. > > This commit adds a check in checkpatch to be sure the tag is written > correctly with dash and not with space. > > The tags supported by the commit are: > Reported-by, Requested-by, Suggested-by, Reported-at, and Submitted-at > > It's not necessary to add "Signed-off-by" since it's already checked in > checkpatch. > > Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> > --- > tests/checkpatch.at | 44 +++++++++++++++++++++++++++++++++++++++++ > utilities/checkpatch.py | 13 ++++++++++++ > 2 files changed, 57 insertions(+) > > diff --git a/tests/checkpatch.at b/tests/checkpatch.at > index 0718acd99..8eb6a7558 100755 > --- a/tests/checkpatch.at > +++ b/tests/checkpatch.at > @@ -348,3 +348,47 @@ try_checkpatch \ > " > > AT_CLEANUP > + > +AT_SETUP([checkpatch - malformed tags]) > +try_checkpatch \ > + " Author: A > + > + Reported by: foo... > + Signed-off-by: A" \ > + "ERROR: Reported-by tag is malformed. > + 1: Reported by: foo... > +" > +try_checkpatch \ > + " Author: A > + > + Requested by: foo... > + Signed-off-by: A" \ > + "ERROR: Requested-by tag is malformed. > + 1: Requested by: foo... > +" > +try_checkpatch \ > + " Author: A > + > + Suggested by: foo... > + Signed-off-by: A" \ > + "ERROR: Suggested-by tag is malformed. > + 1: Suggested by: foo... > +" > +try_checkpatch \ > + " Author: A > + > + Reported at: foo... > + Signed-off-by: A" \ > + "ERROR: Reported-at tag is malformed. > + 1: Reported at: foo... > +" > +try_checkpatch \ > + " Author: A > + > + Submitted at: foo... > + Signed-off-by: A" \ > + "ERROR: Submitted-at tag is malformed. > + 1: Submitted at: foo... > +" > + > +AT_CLEANUP > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > index 699fb4b02..97e21eeaa 100755 > --- a/utilities/checkpatch.py > +++ b/utilities/checkpatch.py > @@ -749,6 +749,14 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None): > is_gerrit_change_id = re.compile(r'(\s*(change-id: )(.*))$', > re.I | re.M | re.S) > > + tags_typos = { > + r'^Reported by:': 'Reported-by:', > + r'^Requested by:': 'Requested-by:', > + r'^Suggested by:': 'Suggested-by:', > + r'^Reported at:': 'Reported-at:', > + r'^Submitted at:': 'Submitted-at:' > + } > + Some patches may already include Acked-by and Reviewed-by. Does it make sense to include those as well? > reset_counters() > > for line in text.splitlines(): > @@ -838,6 +846,11 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None): > print("%d: %s\n" % (lineno, line)) > elif spellcheck: > check_spelling(line, False) > + for typo, correct in tags_typos.items(): > + m = re.match(typo, line, re.I) > + if m: > + print_error("%s tag is malformed." % (correct[:-1])) > + print("%d: %s\n" % (lineno, line)) > > elif parse == PARSE_STATE_CHANGE_BODY: > newfile = hunks.match(line) > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/tests/checkpatch.at b/tests/checkpatch.at index 0718acd99..8eb6a7558 100755 --- a/tests/checkpatch.at +++ b/tests/checkpatch.at @@ -348,3 +348,47 @@ try_checkpatch \ " AT_CLEANUP + +AT_SETUP([checkpatch - malformed tags]) +try_checkpatch \ + " Author: A + + Reported by: foo... + Signed-off-by: A" \ + "ERROR: Reported-by tag is malformed. + 1: Reported by: foo... +" +try_checkpatch \ + " Author: A + + Requested by: foo... + Signed-off-by: A" \ + "ERROR: Requested-by tag is malformed. + 1: Requested by: foo... +" +try_checkpatch \ + " Author: A + + Suggested by: foo... + Signed-off-by: A" \ + "ERROR: Suggested-by tag is malformed. + 1: Suggested by: foo... +" +try_checkpatch \ + " Author: A + + Reported at: foo... + Signed-off-by: A" \ + "ERROR: Reported-at tag is malformed. + 1: Reported at: foo... +" +try_checkpatch \ + " Author: A + + Submitted at: foo... + Signed-off-by: A" \ + "ERROR: Submitted-at tag is malformed. + 1: Submitted at: foo... +" + +AT_CLEANUP diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index 699fb4b02..97e21eeaa 100755 --- a/utilities/checkpatch.py +++ b/utilities/checkpatch.py @@ -749,6 +749,14 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None): is_gerrit_change_id = re.compile(r'(\s*(change-id: )(.*))$', re.I | re.M | re.S) + tags_typos = { + r'^Reported by:': 'Reported-by:', + r'^Requested by:': 'Requested-by:', + r'^Suggested by:': 'Suggested-by:', + r'^Reported at:': 'Reported-at:', + r'^Submitted at:': 'Submitted-at:' + } + reset_counters() for line in text.splitlines(): @@ -838,6 +846,11 @@ def ovs_checkpatch_parse(text, filename, author=None, committer=None): print("%d: %s\n" % (lineno, line)) elif spellcheck: check_spelling(line, False) + for typo, correct in tags_typos.items(): + m = re.match(typo, line, re.I) + if m: + print_error("%s tag is malformed." % (correct[:-1])) + print("%d: %s\n" % (lineno, line)) elif parse == PARSE_STATE_CHANGE_BODY: newfile = hunks.match(line)
Currently, there are some patches with the tags wrongly written (with space instead of dash ) and this may prevent some automatic system or CI to detect them correctly. This commit adds a check in checkpatch to be sure the tag is written correctly with dash and not with space. The tags supported by the commit are: Reported-by, Requested-by, Suggested-by, Reported-at, and Submitted-at It's not necessary to add "Signed-off-by" since it's already checked in checkpatch. Signed-off-by: Timothy Redaelli <tredaelli@redhat.com> --- tests/checkpatch.at | 44 +++++++++++++++++++++++++++++++++++++++++ utilities/checkpatch.py | 13 ++++++++++++ 2 files changed, 57 insertions(+)