Patchwork D1884: strip: use %d for known-int string interpolation

login
register
mail settings
Submitter phabricator
Date Jan. 18, 2018, 1:38 p.m.
Message ID <differential-rev-PHID-DREV-hqeiaoxo7b4xwrgeaqcf-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/26871/
State Superseded
Headers show

Comments

phabricator - Jan. 18, 2018, 1:38 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/strip.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 18, 2018, 1:42 p.m.
pulkit added inline comments.

INLINE COMMENTS

> strip.py:218
>              # between the working context and uctx
> -            descendantrevs = repo.revs("%s::." % uctx.rev())
> +            descendantrevs = repo.revs(b"%d::." % uctx.rev())
>              changedfiles = []

b'' is not required.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Jan. 18, 2018, 1:45 p.m.
durin42 marked an inline comment as done.
durin42 added inline comments.

INLINE COMMENTS

> pulkit wrote in strip.py:218
> b'' is not required.

It's not, but we're going to eventually want to kill the module loader hack, and so I figured since I'm dirtying the line anyway I may as well b-prefix it.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Jan. 18, 2018, 1:47 p.m.
pulkit accepted this revision.
pulkit added inline comments.

INLINE COMMENTS

> durin42 wrote in strip.py:218
> It's not, but we're going to eventually want to kill the module loader hack, and so I figured since I'm dirtying the line anyway I may as well b-prefix it.

That's a good idea. I will follow the same.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - Jan. 18, 2018, 1:53 p.m.
yuja added inline comments.

INLINE COMMENTS

> strip.py:218
>              # between the working context and uctx
> -            descendantrevs = repo.revs("%s::." % uctx.rev())
> +            descendantrevs = repo.revs(b"%d::." % uctx.rev())
>              changedfiles = []

It should be `repo.revs("%d::.", uctx.rev())` by the way.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, pulkit
Cc: yuja, pulkit, mercurial-devel
phabricator - Jan. 18, 2018, 1:55 p.m.
durin42 marked an inline comment as done.
durin42 added inline comments.

INLINE COMMENTS

> yuja wrote in strip.py:218
> It should be `repo.revs("%d::.", uctx.rev())` by the way.

I'm unclear: are you asking to remove the b-prefix? Adding it was intentional, since eventually we're going to need to rewrite everything so we can ditch the module loader hack.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, pulkit
Cc: yuja, pulkit, mercurial-devel
phabricator - Jan. 18, 2018, 2:07 p.m.
yuja added inline comments.

INLINE COMMENTS

> durin42 wrote in strip.py:218
> I'm unclear: are you asking to remove the b-prefix? Adding it was intentional, since eventually we're going to need to rewrite everything so we can ditch the module loader hack.

No. I mean it should use revset.formatspec. i.e. `revs(expr, arg)` instead of `revs(expr % arg)`.

REPOSITORY
  rHG Mercurial

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

To: durin42, #hg-reviewers, pulkit
Cc: yuja, pulkit, mercurial-devel
phabricator - Jan. 18, 2018, 3:24 p.m.
durin42 added inline comments.

INLINE COMMENTS

> yuja wrote in strip.py:218
> No. I mean it should use revset.formatspec. i.e. `revs(expr, arg)` instead of `revs(expr % arg)`.

Oh good catch, I'll insert another change to fix that in isolation.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/hgext/strip.py b/hgext/strip.py
--- a/hgext/strip.py
+++ b/hgext/strip.py
@@ -215,7 +215,7 @@ 
 
             # only reset the dirstate for files that would actually change
             # between the working context and uctx
-            descendantrevs = repo.revs("%s::." % uctx.rev())
+            descendantrevs = repo.revs(b"%d::." % uctx.rev())
             changedfiles = []
             for rev in descendantrevs:
                 # blindly reset the files, regardless of what actually changed