[libvirt] [PATCH] maint: fix syntax-check sc_prohibit_int_ijk exclude rule
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@redhat.com> --- I'm surprised that it even worked so far. I've noticed this after system update few days ago. 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]$$ -- 2.8.3
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@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. 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.
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. 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)$$ We might go even farther and turn it into ^(cfg\.mk|include/libvirt/libvirt.+|src/.+_protocol-structs|src/.+/.+_protocol\.x)$$ but we probably want to avoid adding more int variable to the protocol definitions. -- Andrea Bolognani Software Engineer - Virtualization Team
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@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
participants (2)
-
Andrea Bolognani -
Pavel Hrdina