This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8254

nodemap: fix missing r-prefix on regular expression
ClosedPublic

Authored by durin42 on Mar 6 2020, 2:04 PM.

Details

Reviewers
marmoute
pulkit
Group Reviewers
hg-reviewers
Commits
rHG6aee0647e026: nodemap: fix missing r-prefix on regular expression
rHG6d4f6c2f7f18: nodemap: fix missing r-prefix on regular expression
rHG50b570f04abb: nodemap: fix missing r-prefix on regular expression
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

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

durin42 created this revision.Mar 6 2020, 2:04 PM
marmoute accepted this revision.Mar 7 2020, 4:01 AM
marmoute added a subscriber: marmoute.

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

martinvonz added a subscriber: martinvonz.Mar 7 2020, 11:05 AM
In D8254#122978, @marmoute wrote:

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

563dfdfd01a4 is public, so no?

pulkit accepted this revision.Mar 9 2020, 4:50 AM
This revision is now accepted and ready to land.Mar 9 2020, 4:50 AM
durin42 added a commit: rHG50b570f04abb: nodemap: fix missing r-prefix on regular expression.Mar 9 2020, 6:03 AM
Closed by commit rHG50b570f04abb: nodemap: fix missing r-prefix on regular expression (authored by durin42). · Explain Why
This revision was automatically updated to reflect the committed changes.
marmoute added a comment.Mar 9 2020, 6:17 AM
In D8254#122981, @martinvonz wrote:
In D8254#122978, @marmoute wrote:

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

563dfdfd01a4 is public, so no?

I am confused, because this diff was not even landed. when you commented this.

martinvonz added a comment.Mar 9 2020, 9:57 AM
In D8254#123109, @marmoute wrote:
In D8254#122981, @martinvonz wrote:
In D8254#122978, @marmoute wrote:

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

563dfdfd01a4 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.

marmoute added a comment.Mar 9 2020, 10:08 AM
In D8254#123110, @martinvonz wrote:
In D8254#123109, @marmoute wrote:
In D8254#122981, @martinvonz wrote:
In D8254#122978, @marmoute wrote:

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

563dfdfd01a4 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 ?

martinvonz added a comment.Mar 9 2020, 11:23 AM
In D8254#123111, @marmoute wrote:
In D8254#123110, @martinvonz wrote:
In D8254#123109, @marmoute wrote:
In D8254#122981, @martinvonz wrote:
In D8254#122978, @marmoute wrote:

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

563dfdfd01a4 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.

Revision Contents
Changeset List

PathPackages
Mmercurial/revlogutils/nodemap.py (2 lines)

Diff 20569

mercurial/revlogutils/nodemap.py

def _rawdata_filepath(revlog, docket): def _rawdata_filepath(revlog, docket):
"""The (vfs relative) nodemap's rawdata file for a given uid""" """The (vfs relative) nodemap's rawdata file for a given uid"""
prefix = revlog.nodemap_file[:-2] prefix = revlog.nodemap_file[:-2]
return b"%s-%s.nd" % (prefix, docket.uid) return b"%s-%s.nd" % (prefix, docket.uid)
def _other_rawdata_filepath(revlog, docket): def _other_rawdata_filepath(revlog, docket):
prefix = revlog.nodemap_file[:-2] 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_path = _rawdata_filepath(revlog, docket)
new_file_name = revlog.opener.basename(new_file_path) new_file_name = revlog.opener.basename(new_file_path)
dirpath = revlog.opener.dirname(new_file_path) dirpath = revlog.opener.dirname(new_file_path)
others = [] others = []
for f in revlog.opener.listdir(dirpath): for f in revlog.opener.listdir(dirpath):
if pattern.match(f) and f != new_file_name: if pattern.match(f) and f != new_file_name:
others.append(f) others.append(f)
return others return others