This is an archive of the discontinued Mercurial Phabricator instance.

nodemap: fix missing r-prefix on regular expression
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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?

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
This revision was automatically updated to reflect the committed changes.

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.

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.

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 ?

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.