Patchwork D8254: nodemap: fix missing r-prefix on regular expression

login
register
mail settings
Submitter phabricator
Date March 6, 2020, 7:04 p.m.
Message ID <differential-rev-PHID-DREV-spaxliwxfihhnqacvomk-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45571/
State Superseded
Headers show

Comments

phabricator - March 6, 2020, 7:04 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Looking at this regular expression, it's pretty obvious from reading
  it that it wanted to match literal ., but since the r was missing on
  the pattern it was matching any character. I guess we're just lucky
  nothing bad happened as a result. This was automatically fixed by
  pyupgrade, but I split it out into its own change because it seemed
  important.
  
  1. skip-blame just adding a missing r-prefix

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8254

AFFECTED FILES
  mercurial/revlogutils/nodemap.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - March 7, 2020, 9:01 a.m.
marmoute added a comment.
marmoute accepted this revision.


  Good catch, look good to me. Can we drop the `skip-blame just adding a missing r-prefix` inflight?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8254/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8254

To: durin42, #hg-reviewers, marmoute
Cc: marmoute, mercurial-devel
phabricator - March 7, 2020, 4:05 p.m.
martinvonz added a comment.


  In D8254#122978 <https://phab.mercurial-scm.org/D8254#122978>, @marmoute wrote:
  
  > Good catch, look good to me. Can we drop the `skip-blame just adding a missing r-prefix` inflight?
  
  563dfdfd01a4 <https://phab.mercurial-scm.org/rHG563dfdfd01a4e050b5ea01796843cf2fd2d33ef9> is public, so no?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8254/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8254

To: durin42, #hg-reviewers, marmoute
Cc: martinvonz, marmoute, mercurial-devel
phabricator - March 9, 2020, 10:17 a.m.
marmoute added a comment.


  In D8254#122981 <https://phab.mercurial-scm.org/D8254#122981>, @martinvonz wrote:
  
  > In D8254#122978 <https://phab.mercurial-scm.org/D8254#122978>, @marmoute wrote:
  >
  >> Good catch, look good to me. Can we drop the `skip-blame just adding a missing r-prefix` inflight?
  >
  > 563dfdfd01a4 <https://phab.mercurial-scm.org/rHG563dfdfd01a4e050b5ea01796843cf2fd2d33ef9> is public, so no?
  
  I am confused, because this diff was not even landed. when you commented this.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8254/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8254

To: durin42, #hg-reviewers, marmoute, pulkit
Cc: martinvonz, marmoute, mercurial-devel
phabricator - March 9, 2020, 1:57 p.m.
martinvonz added a comment.


  In D8254#123109 <https://phab.mercurial-scm.org/D8254#123109>, @marmoute wrote:
  
  > In D8254#122981 <https://phab.mercurial-scm.org/D8254#122981>, @martinvonz wrote:
  >
  >> In D8254#122978 <https://phab.mercurial-scm.org/D8254#122978>, @marmoute wrote:
  >>
  >>> Good catch, look good to me. Can we drop the `skip-blame just adding a missing r-prefix` inflight?
  >>
  >> 563dfdfd01a4 <https://phab.mercurial-scm.org/rHG563dfdfd01a4e050b5ea01796843cf2fd2d33ef9> is public, so no?
  >
  > I am confused, because this diff was not even landed. when you commented this.
  
  Note that my comment was not talking about this patch but about the commit that introduced bad regex.
  
  Anyway, I think I misunderstood your original comment because I didn't see the quotes in it, so I thought you were asking reviewers to add the missing r'' prefix in flight.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8254/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8254

To: durin42, #hg-reviewers, marmoute, pulkit
Cc: martinvonz, marmoute, mercurial-devel
phabricator - March 9, 2020, 2:08 p.m.
marmoute added a comment.


  In D8254#123110 <https://phab.mercurial-scm.org/D8254#123110>, @martinvonz wrote:
  
  > In D8254#123109 <https://phab.mercurial-scm.org/D8254#123109>, @marmoute wrote:
  >
  >> In D8254#122981 <https://phab.mercurial-scm.org/D8254#122981>, @martinvonz wrote:
  >>
  >>> In D8254#122978 <https://phab.mercurial-scm.org/D8254#122978>, @marmoute wrote:
  >>>
  >>>> Good catch, look good to me. Can we drop the `skip-blame just adding a missing r-prefix` inflight?
  >>>
  >>> 563dfdfd01a4 <https://phab.mercurial-scm.org/rHG563dfdfd01a4e050b5ea01796843cf2fd2d33ef9> is public, so no?
  >>
  >> I am confused, because this diff was not even landed. when you commented this.
  >
  > Note that my comment was not talking about this patch but about the commit that introduced bad regex.
  > Anyway, I think I misunderstood your original comment because I didn't see the quotes in it, so I thought you were asking reviewers to add the missing r'' prefix in flight.
  
  Haa, I understand the misunderstanding now. Now that things are clearer, Can we fix the queued changeset ?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8254/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8254

To: durin42, #hg-reviewers, marmoute, pulkit
Cc: martinvonz, marmoute, mercurial-devel
phabricator - March 9, 2020, 3:23 p.m.
martinvonz added a comment.


  In D8254#123111 <https://phab.mercurial-scm.org/D8254#123111>, @marmoute wrote:
  
  > In D8254#123110 <https://phab.mercurial-scm.org/D8254#123110>, @martinvonz wrote:
  >
  >> In D8254#123109 <https://phab.mercurial-scm.org/D8254#123109>, @marmoute wrote:
  >>
  >>> In D8254#122981 <https://phab.mercurial-scm.org/D8254#122981>, @martinvonz wrote:
  >>>
  >>>> In D8254#122978 <https://phab.mercurial-scm.org/D8254#122978>, @marmoute wrote:
  >>>>
  >>>>> Good catch, look good to me. Can we drop the `skip-blame just adding a missing r-prefix` inflight?
  >>>>
  >>>> 563dfdfd01a4 <https://phab.mercurial-scm.org/rHG563dfdfd01a4e050b5ea01796843cf2fd2d33ef9> is public, so no?
  >>>
  >>> I am confused, because this diff was not even landed. when you commented this.
  >>
  >> Note that my comment was not talking about this patch but about the commit that introduced bad regex.
  >> Anyway, I think I misunderstood your original comment because I didn't see the quotes in it, so I thought you were asking reviewers to add the missing r'' prefix in flight.
  >
  > Haa, I understand the misunderstanding now. Now that things are clearer, Can we fix the queued changeset ?
  
  Yes, I agree with removing it since this is a bug-fix (not a py3 compat thing). I'll do that.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8254/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8254

To: durin42, #hg-reviewers, marmoute, pulkit
Cc: martinvonz, marmoute, mercurial-devel

Patch

diff --git a/mercurial/revlogutils/nodemap.py b/mercurial/revlogutils/nodemap.py
--- a/mercurial/revlogutils/nodemap.py
+++ b/mercurial/revlogutils/nodemap.py
@@ -275,7 +275,7 @@ 
 
 def _other_rawdata_filepath(revlog, docket):
     prefix = revlog.nodemap_file[:-2]
-    pattern = re.compile(b"(^|/)%s-[0-9a-f]+\.nd$" % prefix)
+    pattern = re.compile(br"(^|/)%s-[0-9a-f]+\.nd$" % prefix)
     new_file_path = _rawdata_filepath(revlog, docket)
     new_file_name = revlog.opener.basename(new_file_path)
     dirpath = revlog.opener.dirname(new_file_path)