Re: [libvirt] [PATCH] maint: fix syntax-check sc_prohibit_int_ijk exclude rule
On Tue, May 24, 2016 at 11:42:11AM +0200, Andrea Bolognani wrote:
On Tue, 2016-05-24 at 09:41 +0200, Pavel Hrdina wrote:
> This fixes the "include/" path, we need to create a regex for the whole
path
> because we are matching the whole line.
>
> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com>
> ---
>
> I'm surprised that it even worked so far. I've noticed this after system
update
> few days ago.
So, is syntax-check failing for you without that patch? Because
it's working for me.
Yes it's failing right now for me, like i wrote as note it started after system
updated.
I think it worked so far because the line ends with '$'
instead
of '$$' - I'm not sure what a single '$' does in this context,
but it definitely doesn't force the regex to match the whole path.
That's probably why it started failing for me, because it started parsing the
single '$'.
> cfg.mk | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cfg.mk b/cfg.mk
> index c19f615..f688cbb 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -1259,7 +1259,7 @@
exclude_file_name_regexp--sc_prohibit_include_public_headers_brackets = \
> ^(tools/|examples/|include/libvirt/(virterror|libvirt(-(admin|qemu|lxc))?)\.h$$)
>
> exclude_file_name_regexp--sc_prohibit_int_ijk = \
>
- ^(src/remote_protocol-structs|src/remote/remote_protocol.x|cfg.mk|include/|src/admin_protocol-
> structs|src/admin/admin_protocol.x)$
>
+ ^(src/remote_protocol-structs|src/remote/remote_protocol.x|cfg.mk|include/libvirt/libvirt.+|src/admin_protocol-
> structs|src/admin/admin_protocol.x)$$
>
> exclude_file_name_regexp--sc_prohibit_getenv = \
> ^tests/.*\.[ch]$$
ACK to the change.
Thanks
You should also, as follow-up fixes, properly escape dots and
maybe even use a regex to match the other files instead of
listing them: the end result would look something like
^(cfg\.mk|include/libvirt/libvirt.+|src/(admin|remote)_protocol-structs|src/(admin|remote)/(admin|remote)_protocol\.x)$$
Escaping dots is a good idea and it should be done, I'll include it in this
patch, it's probably not worth to do in separate commit. I'll explain the
changes little bit more in the commit message.
We might go even farther and turn it into
^(cfg\.mk|include/libvirt/libvirt.+|src/.+_protocol-structs|src/.+/.+_protocol\.x)$$
but this is probably intentional to exclude only the files we know about so if
someone else update a different file the syntax-check will fail.
Pavel