[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]