| Message ID | 20170501201409.12116-8-aconole@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, May 01, 2017 at 04:14:09PM -0400, Aaron Conole wrote: > A common way of expressing 'raise to the power of' when authoring > comments uses **. This is currently getting caught by the pointer > spacing warning. So, catch it here. > > Reported-by: Lance Richardson <lrichard@redhat.com> > Signed-off-by: Aaron Conole <aconole@redhat.com> Thanks a lot for improving checkpatch, it should be helpful for review. Maybe I'll start using it in my review process. I applied all of these patches to master.
Ben Pfaff <blp@ovn.org> writes: > On Mon, May 01, 2017 at 04:14:09PM -0400, Aaron Conole wrote: >> A common way of expressing 'raise to the power of' when authoring >> comments uses **. This is currently getting caught by the pointer >> spacing warning. So, catch it here. >> >> Reported-by: Lance Richardson <lrichard@redhat.com> >> Signed-off-by: Aaron Conole <aconole@redhat.com> > > Thanks a lot for improving checkpatch, it should be helpful for > review. Thank you for the incrementals, and review. > Maybe I'll start using it in my review process. I use it by default in all of my development. The following is my .git/hooks/pre-commit: #!/bin/sh if git rev-parse --verify HEAD 2>/dev/null then git diff-index -p --cached HEAD else : fi | utilities/checkpatch.py -s > I applied all of these patches to master. Thanks, Ben!
Ben Pfaff <blp@ovn.org> writes: > On Mon, May 01, 2017 at 04:39:30PM -0400, Aaron Conole wrote: >> Ben Pfaff <blp@ovn.org> writes: >> >> > On Mon, May 01, 2017 at 04:14:09PM -0400, Aaron Conole wrote: >> >> A common way of expressing 'raise to the power of' when authoring >> >> comments uses **. This is currently getting caught by the pointer >> >> spacing warning. So, catch it here. >> >> >> >> Reported-by: Lance Richardson <lrichard@redhat.com> >> >> Signed-off-by: Aaron Conole <aconole@redhat.com> >> > >> > Thanks a lot for improving checkpatch, it should be helpful for >> > review. >> >> Thank you for the incrementals, and review. >> >> > Maybe I'll start using it in my review process. >> >> I use it by default in all of my development. The following is my >> .git/hooks/pre-commit: >> >> #!/bin/sh >> if git rev-parse --verify HEAD 2>/dev/null >> then >> git diff-index -p --cached HEAD >> else >> : >> fi | utilities/checkpatch.py -s >> >> > I applied all of these patches to master. > > Oh, interesting. > > I'm playing with it by applying the following and then changing my > "mutt" shortcut for applying a patch from > cd ~/nicira/ovs && git am -s > to > cd ~/nicira/ovs && checkpatch.py -p | git am -s > > I'm not sure whether my patch makes sense, but it makes me feel clever > either way. :) If it works - I don't know what the erroring output will do to the git am input parser, but my guess is blow up (which is probably what you want anyway). I looked for solutions that use a git hook, but it seems they all blow up whenever anything more trivial happens, so there's probably not yet a slick solution. With that said, Acked-by: Aaron Conole <aconole@redhat.com> > --8<--------------------------cut here-------------------------->8-- > > From: Ben Pfaff <blp@ovn.org> > Date: Mon, 1 May 2017 13:42:46 -0700 > Subject: [PATCH] checkpatch: Add support for "pass-through" mode. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > utilities/checkpatch.py | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py > index 387549afe3f6..ced5e9f8c241 100755 > --- a/utilities/checkpatch.py > +++ b/utilities/checkpatch.py > @@ -342,6 +342,8 @@ def usage(): > "Do not emit an error if no Signed-off-by line is present") > print("-t|--skip-trailing-whitespace\t" > "Skips the trailing whitespace test") > + print("-p|--pass-through\t" > + "Print a copy of the input as output, when reading from stdin") > > > def ovs_checkpatch_file(filename): > @@ -366,17 +368,19 @@ def ovs_checkpatch_file(filename): > > if __name__ == '__main__': > try: > - optlist, args = getopt.getopt(sys.argv[1:], 'bhlstf', > + optlist, args = getopt.getopt(sys.argv[1:], 'bhlstfp', > ["check-file", > "help", > "skip-block-whitespace", > "skip-leading-whitespace", > "skip-signoff-lines", > - "skip-trailing-whitespace"]) > + "skip-trailing-whitespace", > + "pass-through"]) > except: > print("Unknown option encountered. Please rerun with -h for help.") > sys.exit(-1) > > + pass_through = False > for o, a in optlist: > if o in ("-h", "--help"): > usage() > @@ -391,6 +395,8 @@ if __name__ == '__main__': > skip_trailing_whitespace_check = True > elif o in ("-f", "--check-file"): > checking_file = True > + elif o in ("-p", "--pass-through"): > + pass_through = True > else: > print("Unknown option '%s'" % o) > sys.exit(-1) > @@ -404,5 +410,10 @@ if __name__ == '__main__': > if sys.stdin.isatty(): > usage() > sys.exit(-1) > - sys.exit(ovs_checkpatch_parse(sys.stdin.read())) > + > + content = sys.stdin.read() > + exit_code = ovs_checkpatch_parse(content) > + if pass_through: > + sys.stdout.write(content) > + sys.exit(exit_code) > sys.exit(ovs_checkpatch_file(filename))
On Tue, May 02, 2017 at 12:24:53PM -0400, Aaron Conole wrote: > Ben Pfaff <blp@ovn.org> writes: > > > On Mon, May 01, 2017 at 04:39:30PM -0400, Aaron Conole wrote: > >> Ben Pfaff <blp@ovn.org> writes: > >> > >> > On Mon, May 01, 2017 at 04:14:09PM -0400, Aaron Conole wrote: > >> >> A common way of expressing 'raise to the power of' when authoring > >> >> comments uses **. This is currently getting caught by the pointer > >> >> spacing warning. So, catch it here. > >> >> > >> >> Reported-by: Lance Richardson <lrichard@redhat.com> > >> >> Signed-off-by: Aaron Conole <aconole@redhat.com> > >> > > >> > Thanks a lot for improving checkpatch, it should be helpful for > >> > review. > >> > >> Thank you for the incrementals, and review. > >> > >> > Maybe I'll start using it in my review process. > >> > >> I use it by default in all of my development. The following is my > >> .git/hooks/pre-commit: > >> > >> #!/bin/sh > >> if git rev-parse --verify HEAD 2>/dev/null > >> then > >> git diff-index -p --cached HEAD > >> else > >> : > >> fi | utilities/checkpatch.py -s > >> > >> > I applied all of these patches to master. > > > > Oh, interesting. > > > > I'm playing with it by applying the following and then changing my > > "mutt" shortcut for applying a patch from > > cd ~/nicira/ovs && git am -s > > to > > cd ~/nicira/ovs && checkpatch.py -p | git am -s > > > > I'm not sure whether my patch makes sense, but it makes me feel clever > > either way. > > :) If it works - I don't know what the erroring output will do to the > git am input parser, but my guess is blow up (which is probably what you > want anyway). I looked for solutions that use a git hook, but it seems > they all blow up whenever anything more trivial happens, so there's > probably not yet a slick solution. With that said, > > Acked-by: Aaron Conole <aconole@redhat.com> I'm experimenting with it, I'm not aiming to commit it, at least not yet. I had the idea that checkpatch sent its errors to stderr, but I was wrong. I ended up modifying my patch to make it do that. I don't know whether it's ideal. Maybe I should use it in a way that doesn't require pass-through, which isn't a customary mode for this kind of tool.
Ben Pfaff <blp@ovn.org> writes: > On Tue, May 02, 2017 at 12:24:53PM -0400, Aaron Conole wrote: >> Ben Pfaff <blp@ovn.org> writes: >> >> > On Mon, May 01, 2017 at 04:39:30PM -0400, Aaron Conole wrote: >> >> Ben Pfaff <blp@ovn.org> writes: >> >> >> >> > On Mon, May 01, 2017 at 04:14:09PM -0400, Aaron Conole wrote: >> >> >> A common way of expressing 'raise to the power of' when authoring >> >> >> comments uses **. This is currently getting caught by the pointer >> >> >> spacing warning. So, catch it here. >> >> >> >> >> >> Reported-by: Lance Richardson <lrichard@redhat.com> >> >> >> Signed-off-by: Aaron Conole <aconole@redhat.com> >> >> > >> >> > Thanks a lot for improving checkpatch, it should be helpful for >> >> > review. >> >> >> >> Thank you for the incrementals, and review. >> >> >> >> > Maybe I'll start using it in my review process. >> >> >> >> I use it by default in all of my development. The following is my >> >> .git/hooks/pre-commit: >> >> >> >> #!/bin/sh >> >> if git rev-parse --verify HEAD 2>/dev/null >> >> then >> >> git diff-index -p --cached HEAD >> >> else >> >> : >> >> fi | utilities/checkpatch.py -s >> >> >> >> > I applied all of these patches to master. >> > >> > Oh, interesting. >> > >> > I'm playing with it by applying the following and then changing my >> > "mutt" shortcut for applying a patch from >> > cd ~/nicira/ovs && git am -s >> > to >> > cd ~/nicira/ovs && checkpatch.py -p | git am -s >> > >> > I'm not sure whether my patch makes sense, but it makes me feel clever >> > either way. >> >> :) If it works - I don't know what the erroring output will do to the >> git am input parser, but my guess is blow up (which is probably what you >> want anyway). I looked for solutions that use a git hook, but it seems >> they all blow up whenever anything more trivial happens, so there's >> probably not yet a slick solution. With that said, >> >> Acked-by: Aaron Conole <aconole@redhat.com> > > I'm experimenting with it, I'm not aiming to commit it, at least not > yet. > > I had the idea that checkpatch sent its errors to stderr, but I was > wrong. I ended up modifying my patch to make it do that. I don't know > whether it's ideal. Maybe I should use it in a way that doesn't require > pass-through, which isn't a customary mode for this kind of tool. According to the git docs, the git pre-commit hook is invoked by `git am` when the default pre-applypatch hook is enabled. I have my pre-commit setup already. So, I just did (from top level): $ cp .git/hooks/pre-applypatch.sample .git/hooks/pre-applypatch Then I took a random patch file, and modified a file to have bad spacing, then did `git am`, with these results: $ cp .git/hooks/pre-applypatch.sample .git/hooks/pre-applypatch $ git am 0001-borked-patch.patch Applying: rhel: fix the fedora spec WARNING: Line has non-spaces leading whitespace #10 FILE: b/rhel/openvswitch-fedora.spec.in:233: install -d -m 0755 $RPM_BUILD_ROOT%{_rundir}/openvswitch WARNING: Line has non-spaces leading whitespace #11 FILE: b/rhel/openvswitch-fedora.spec.in:234: install -d -m 0755 $RPM_BUILD_ROOT%{_localstatedir}/log/openvswitch $ git status On branch master You are in the middle of an am session. (fix conflicts and then run "git am --continue") (use "git am --skip" to skip this patch) (use "git am --abort" to restore the original branch) Changes to be committed: (use "git reset HEAD <file>..." to unstage) modified: rhel/openvswitch-fedora.spec.in May be that is what you're looking for?
On Wed, May 03, 2017 at 11:25:03AM -0400, Aaron Conole wrote: > Ben Pfaff <blp@ovn.org> writes: > > > On Tue, May 02, 2017 at 12:24:53PM -0400, Aaron Conole wrote: > >> Ben Pfaff <blp@ovn.org> writes: > >> > >> > On Mon, May 01, 2017 at 04:39:30PM -0400, Aaron Conole wrote: > >> >> Ben Pfaff <blp@ovn.org> writes: > >> >> > >> >> > On Mon, May 01, 2017 at 04:14:09PM -0400, Aaron Conole wrote: > >> >> >> A common way of expressing 'raise to the power of' when authoring > >> >> >> comments uses **. This is currently getting caught by the pointer > >> >> >> spacing warning. So, catch it here. > >> >> >> > >> >> >> Reported-by: Lance Richardson <lrichard@redhat.com> > >> >> >> Signed-off-by: Aaron Conole <aconole@redhat.com> > >> >> > > >> >> > Thanks a lot for improving checkpatch, it should be helpful for > >> >> > review. > >> >> > >> >> Thank you for the incrementals, and review. > >> >> > >> >> > Maybe I'll start using it in my review process. > >> >> > >> >> I use it by default in all of my development. The following is my > >> >> .git/hooks/pre-commit: > >> >> > >> >> #!/bin/sh > >> >> if git rev-parse --verify HEAD 2>/dev/null > >> >> then > >> >> git diff-index -p --cached HEAD > >> >> else > >> >> : > >> >> fi | utilities/checkpatch.py -s > >> >> > >> >> > I applied all of these patches to master. > >> > > >> > Oh, interesting. > >> > > >> > I'm playing with it by applying the following and then changing my > >> > "mutt" shortcut for applying a patch from > >> > cd ~/nicira/ovs && git am -s > >> > to > >> > cd ~/nicira/ovs && checkpatch.py -p | git am -s > >> > > >> > I'm not sure whether my patch makes sense, but it makes me feel clever > >> > either way. > >> > >> :) If it works - I don't know what the erroring output will do to the > >> git am input parser, but my guess is blow up (which is probably what you > >> want anyway). I looked for solutions that use a git hook, but it seems > >> they all blow up whenever anything more trivial happens, so there's > >> probably not yet a slick solution. With that said, > >> > >> Acked-by: Aaron Conole <aconole@redhat.com> > > > > I'm experimenting with it, I'm not aiming to commit it, at least not > > yet. > > > > I had the idea that checkpatch sent its errors to stderr, but I was > > wrong. I ended up modifying my patch to make it do that. I don't know > > whether it's ideal. Maybe I should use it in a way that doesn't require > > pass-through, which isn't a customary mode for this kind of tool. > > According to the git docs, the git pre-commit hook is invoked by `git > am` when the default pre-applypatch hook is enabled. I have my > pre-commit setup already. So, I just did (from top level): > > $ cp .git/hooks/pre-applypatch.sample .git/hooks/pre-applypatch > > Then I took a random patch file, and modified a file to have bad spacing, > then did `git am`, with these results: > > $ cp .git/hooks/pre-applypatch.sample .git/hooks/pre-applypatch > $ git am 0001-borked-patch.patch > Applying: rhel: fix the fedora spec > WARNING: Line has non-spaces leading whitespace > #10 FILE: b/rhel/openvswitch-fedora.spec.in:233: > install -d -m 0755 $RPM_BUILD_ROOT%{_rundir}/openvswitch > > WARNING: Line has non-spaces leading whitespace > #11 FILE: b/rhel/openvswitch-fedora.spec.in:234: > install -d -m 0755 $RPM_BUILD_ROOT%{_localstatedir}/log/openvswitch > > $ git status > On branch master > You are in the middle of an am session. > (fix conflicts and then run "git am --continue") > (use "git am --skip" to skip this patch) > (use "git am --abort" to restore the original branch) > > Changes to be committed: > (use "git reset HEAD <file>..." to unstage) > > modified: rhel/openvswitch-fedora.spec.in > > May be that is what you're looking for? Maybe I should use that, sure.
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index acf6f15..f22ceac 100755 --- a/utilities/checkpatch.py +++ b/utilities/checkpatch.py @@ -76,7 +76,7 @@ __regex_is_for_if_single_line_bracket = \ re.compile(r'^ +(if|for|while) \(.*\)') __regex_ends_with_bracket = \ re.compile(r'[^\s]\) {(\s+/\*[\s\Sa-zA-Z0-9\.,\?\*/+-]*)?$') -__regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*') +__regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*[^*]') skip_leading_whitespace_check = False skip_trailing_whitespace_check = False
A common way of expressing 'raise to the power of' when authoring comments uses **. This is currently getting caught by the pointer spacing warning. So, catch it here. Reported-by: Lance Richardson <lrichard@redhat.com> Signed-off-by: Aaron Conole <aconole@redhat.com> --- utilities/checkpatch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)