Patchwork ["FIX-RE2] match: fix re2 compability broken in 2e2699af5649

login
register
mail settings
Submitter Pierre-Yves David
Date April 11, 2019, 4:46 p.m.
Message ID <ca4f233c0f5c4ca5604a.1555001209@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/39557/
State New
Headers show

Comments

Pierre-Yves David - April 11, 2019, 4:46 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1555000496 -7200
#      Thu Apr 11 18:34:56 2019 +0200
# Node ID ca4f233c0f5c4ca5604a53c01216a16d136c69d8
# Parent  509a0477b3a6e9f1ea0fe32bfb028c256aada7fa
# EXP-Topic fix-2e2699af5649
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r ca4f233c0f5c
match: fix re2 compability broken in 2e2699af5649

Apparently, the re2 regex matcher return False when no match are found. So since
2e2699af5649 running test on a machine with a re2 install fails in many places.
Instead we make the code a bit more general and everything goes back to normal.
via Mercurial-devel - April 11, 2019, 4:48 p.m.
On Thu, Apr 11, 2019 at 9:46 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1555000496 -7200
> #      Thu Apr 11 18:34:56 2019 +0200
> # Node ID ca4f233c0f5c4ca5604a53c01216a16d136c69d8
> # Parent  509a0477b3a6e9f1ea0fe32bfb028c256aada7fa
> # EXP-Topic fix-2e2699af5649
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r
> ca4f233c0f5c
> match: fix re2 compability broken in 2e2699af5649
>
> Apparently, the re2 regex matcher return False when no match are found.


This is not quite true; we explicitly use a different method on the
compiled regex when we use re2:
https://www.mercurial-scm.org/repo/hg/file/509a0477b3a6/mercurial/match.py#l41


> So since
> 2e2699af5649 running test on a machine with a re2 install fails in many
> places.
> Instead we make the code a bit more general and everything goes back to
> normal.
>
> diff --git a/mercurial/match.py b/mercurial/match.py
> --- a/mercurial/match.py
> +++ b/mercurial/match.py
> @@ -1288,7 +1288,7 @@ def _buildregexmatch(kindpats, globsuffi
>
>          if startidx == 0:
>              matcher = _rematcher(fullregexp)
> -            func = lambda s: matcher(s) is not None
> +            func = lambda s: bool(matcher(s))
>          else:
>              group = regexps[startidx:]
>              allgroups.append(_joinregexes(group))
>
Pierre-Yves David - April 11, 2019, 4:53 p.m.
On 4/11/19 6:48 PM, Martin von Zweigbergk wrote:
> 
> 
> On Thu, Apr 11, 2019 at 9:46 AM Pierre-Yves David 
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>> 
> wrote:
> 
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@octobus.net
>     <mailto:pierre-yves.david@octobus.net>>
>     # Date 1555000496 -7200
>     #      Thu Apr 11 18:34:56 2019 +0200
>     # Node ID ca4f233c0f5c4ca5604a53c01216a16d136c69d8
>     # Parent  509a0477b3a6e9f1ea0fe32bfb028c256aada7fa
>     # EXP-Topic fix-2e2699af5649
>     # Available At https://bitbucket.org/octobus/mercurial-devel/
>     #              hg pull
>     https://bitbucket.org/octobus/mercurial-devel/ -r ca4f233c0f5c
>     match: fix re2 compability broken in 2e2699af5649
> 
>     Apparently, the re2 regex matcher return False when no match are found.
> 
> 
> This is not quite true; we explicitly use a different method on the 
> compiled regex when we use re2: 
> https://www.mercurial-scm.org/repo/hg/file/509a0477b3a6/mercurial/match.py#l41

The point is that the `_rematcher` call returns False instead of None, 
breaking regex based matching.

> 
>     So since
>     2e2699af5649 running test on a machine with a re2 install fails in
>     many places.
>     Instead we make the code a bit more general and everything goes back
>     to normal.
> 
>     diff --git a/mercurial/match.py b/mercurial/match.py
>     --- a/mercurial/match.py
>     +++ b/mercurial/match.py
>     @@ -1288,7 +1288,7 @@ def _buildregexmatch(kindpats, globsuffi
> 
>               if startidx == 0:
>                   matcher = _rematcher(fullregexp)
>     -            func = lambda s: matcher(s) is not None
>     +            func = lambda s: bool(matcher(s))
>               else:
>                   group = regexps[startidx:]
>                   allgroups.append(_joinregexes(group))
>
via Mercurial-devel - April 11, 2019, 4:55 p.m.
On Thu, Apr 11, 2019 at 9:53 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 4/11/19 6:48 PM, Martin von Zweigbergk wrote:
> >
> >
> > On Thu, Apr 11, 2019 at 9:46 AM Pierre-Yves David
> > <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>
> > wrote:
> >
> >     # HG changeset patch
> >     # User Pierre-Yves David <pierre-yves.david@octobus.net
> >     <mailto:pierre-yves.david@octobus.net>>
> >     # Date 1555000496 -7200
> >     #      Thu Apr 11 18:34:56 2019 +0200
> >     # Node ID ca4f233c0f5c4ca5604a53c01216a16d136c69d8
> >     # Parent  509a0477b3a6e9f1ea0fe32bfb028c256aada7fa
> >     # EXP-Topic fix-2e2699af5649
> >     # Available At https://bitbucket.org/octobus/mercurial-devel/
> >     #              hg pull
> >     https://bitbucket.org/octobus/mercurial-devel/ -r ca4f233c0f5c
> >     match: fix re2 compability broken in 2e2699af5649
> >
> >     Apparently, the re2 regex matcher return False when no match are
> found.
> >
> >
> > This is not quite true; we explicitly use a different method on the
> > compiled regex when we use re2:
> >
> https://www.mercurial-scm.org/repo/hg/file/509a0477b3a6/mercurial/match.py#l41
>
> The point is that the `_rematcher` call returns False instead of None,
> breaking regex based matching.
>

Yes, I understand why it's broken. I just mean that "the re2 regex matcher
return False when no match are found" is true, but it's misleading, because
it makes it sounds like (to me, at least) like the re2 regex matcher
behaves differently.


>
> >
> >     So since
> >     2e2699af5649 running test on a machine with a re2 install fails in
> >     many places.
> >     Instead we make the code a bit more general and everything goes back
> >     to normal.
> >
> >     diff --git a/mercurial/match.py b/mercurial/match.py
> >     --- a/mercurial/match.py
> >     +++ b/mercurial/match.py
> >     @@ -1288,7 +1288,7 @@ def _buildregexmatch(kindpats, globsuffi
> >
> >               if startidx == 0:
> >                   matcher = _rematcher(fullregexp)
> >     -            func = lambda s: matcher(s) is not None
> >     +            func = lambda s: bool(matcher(s))
> >               else:
> >                   group = regexps[startidx:]
> >                   allgroups.append(_joinregexes(group))
> >
>
> --
> Pierre-Yves David
>
via Mercurial-devel - April 11, 2019, 5:08 p.m.
On Thu, Apr 11, 2019 at 9:55 AM Martin von Zweigbergk <martinvonz@google.com>
wrote:

>
>
> On Thu, Apr 11, 2019 at 9:53 AM Pierre-Yves David <
> pierre-yves.david@ens-lyon.org> wrote:
>
>>
>>
>> On 4/11/19 6:48 PM, Martin von Zweigbergk wrote:
>> >
>> >
>> > On Thu, Apr 11, 2019 at 9:46 AM Pierre-Yves David
>> > <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>>
>> > wrote:
>> >
>> >     # HG changeset patch
>> >     # User Pierre-Yves David <pierre-yves.david@octobus.net
>> >     <mailto:pierre-yves.david@octobus.net>>
>> >     # Date 1555000496 -7200
>> >     #      Thu Apr 11 18:34:56 2019 +0200
>> >     # Node ID ca4f233c0f5c4ca5604a53c01216a16d136c69d8
>> >     # Parent  509a0477b3a6e9f1ea0fe32bfb028c256aada7fa
>> >     # EXP-Topic fix-2e2699af5649
>> >     # Available At https://bitbucket.org/octobus/mercurial-devel/
>> >     #              hg pull
>> >     https://bitbucket.org/octobus/mercurial-devel/ -r ca4f233c0f5c
>> >     match: fix re2 compability broken in 2e2699af5649
>> >
>> >     Apparently, the re2 regex matcher return False when no match are
>> found.
>> >
>> >
>> > This is not quite true; we explicitly use a different method on the
>> > compiled regex when we use re2:
>> >
>> https://www.mercurial-scm.org/repo/hg/file/509a0477b3a6/mercurial/match.py#l41
>>
>> The point is that the `_rematcher` call returns False instead of None,
>> breaking regex based matching.
>>
>
> Yes, I understand why it's broken. I just mean that "the re2 regex matcher
> return False when no match are found" is true, but it's misleading, because
> it makes it sounds like (to me, at least) like the re2 regex matcher
> behaves differently.
>

I'll change the message to the following in flight if you're okay with that.

When using re2, we call test_match() instead of match() on the

compiled regex object. While match() returns a matcher object or None,

test_match() returns True or False. So since 2e2699af5649 running test

on a machine with a re2 install fails in many places.  Instead we make

the code a bit more general and everything goes back to normal.



>
>>
>> >
>> >     So since
>> >     2e2699af5649 running test on a machine with a re2 install fails in
>> >     many places.
>> >     Instead we make the code a bit more general and everything goes back
>> >     to normal.
>> >
>> >     diff --git a/mercurial/match.py b/mercurial/match.py
>> >     --- a/mercurial/match.py
>> >     +++ b/mercurial/match.py
>> >     @@ -1288,7 +1288,7 @@ def _buildregexmatch(kindpats, globsuffi
>> >
>> >               if startidx == 0:
>> >                   matcher = _rematcher(fullregexp)
>> >     -            func = lambda s: matcher(s) is not None
>> >     +            func = lambda s: bool(matcher(s))
>> >               else:
>> >                   group = regexps[startidx:]
>> >                   allgroups.append(_joinregexes(group))
>> >
>>
>> --
>> Pierre-Yves David
>>
>
Pierre-Yves David - April 11, 2019, 5:46 p.m.
On 4/11/19 7:08 PM, Martin von Zweigbergk wrote:
> 
> 
> On Thu, Apr 11, 2019 at 9:55 AM Martin von Zweigbergk 
> <martinvonz@google.com <mailto:martinvonz@google.com>> wrote:
> 
> 
> 
>     On Thu, Apr 11, 2019 at 9:53 AM Pierre-Yves David
>     <pierre-yves.david@ens-lyon.org
>     <mailto:pierre-yves.david@ens-lyon.org>> wrote:
> 
> 
> 
>         On 4/11/19 6:48 PM, Martin von Zweigbergk wrote:
>          >
>          >
>          > On Thu, Apr 11, 2019 at 9:46 AM Pierre-Yves David
>          > <pierre-yves.david@ens-lyon.org
>         <mailto:pierre-yves.david@ens-lyon.org>
>         <mailto:pierre-yves.david@ens-lyon.org
>         <mailto:pierre-yves.david@ens-lyon.org>>>
>          > wrote:
>          >
>          >     # HG changeset patch
>          >     # User Pierre-Yves David <pierre-yves.david@octobus.net
>         <mailto:pierre-yves.david@octobus.net>
>          >     <mailto:pierre-yves.david@octobus.net
>         <mailto:pierre-yves.david@octobus.net>>>
>          >     # Date 1555000496 -7200
>          >     #      Thu Apr 11 18:34:56 2019 +0200
>          >     # Node ID ca4f233c0f5c4ca5604a53c01216a16d136c69d8
>          >     # Parent  509a0477b3a6e9f1ea0fe32bfb028c256aada7fa
>          >     # EXP-Topic fix-2e2699af5649
>          >     # Available At https://bitbucket.org/octobus/mercurial-devel/
>          >     #              hg pull
>          > https://bitbucket.org/octobus/mercurial-devel/ -r ca4f233c0f5c
>          >     match: fix re2 compability broken in 2e2699af5649
>          >
>          >     Apparently, the re2 regex matcher return False when no
>         match are found.
>          >
>          >
>          > This is not quite true; we explicitly use a different method
>         on the
>          > compiled regex when we use re2:
>          >
>         https://www.mercurial-scm.org/repo/hg/file/509a0477b3a6/mercurial/match.py#l41
> 
>         The point is that the `_rematcher` call returns False instead of
>         None,
>         breaking regex based matching.
> 
> 
>     Yes, I understand why it's broken. I just mean that "the re2 regex
>     matcher return False when no match are found" is true, but it's
>     misleading, because it makes it sounds like (to me, at least) like
>     the re2 regex matcher behaves differently.
> 
> 
> I'll change the message to the following in flight if you're okay with that.
> 
> When using re2, we call test_match() instead of match() on the
> compiled regex object. While match() returns a matcher object or None,
> test_match() returns True or False. So since 2e2699af5649 running test
> on a machine with a re2 install fails in many places.  Instead we make
> the code a bit more general and everything goes back to normal.

Looks clearer, Go for it.

> 
> 
>          >
>          >     So since
>          >     2e2699af5649 running test on a machine with a re2 install
>         fails in
>          >     many places.
>          >     Instead we make the code a bit more general and
>         everything goes back
>          >     to normal.
>          >
>          >     diff --git a/mercurial/match.py b/mercurial/match.py
>          >     --- a/mercurial/match.py
>          >     +++ b/mercurial/match.py
>          >     @@ -1288,7 +1288,7 @@ def _buildregexmatch(kindpats,
>         globsuffi
>          >
>          >               if startidx == 0:
>          >                   matcher = _rematcher(fullregexp)
>          >     -            func = lambda s: matcher(s) is not None
>          >     +            func = lambda s: bool(matcher(s))
>          >               else:
>          >                   group = regexps[startidx:]
>          >                   allgroups.append(_joinregexes(group))
>          >
> 
>         -- 
>         Pierre-Yves David
>

Patch

diff --git a/mercurial/match.py b/mercurial/match.py
--- a/mercurial/match.py
+++ b/mercurial/match.py
@@ -1288,7 +1288,7 @@  def _buildregexmatch(kindpats, globsuffi
 
         if startidx == 0:
             matcher = _rematcher(fullregexp)
-            func = lambda s: matcher(s) is not None
+            func = lambda s: bool(matcher(s))
         else:
             group = regexps[startidx:]
             allgroups.append(_joinregexes(group))