Patchwork [1,of,5] check-code: detect r.revision(r.node(rev))

login
register
mail settings
Submitter Jun Wu
Date March 29, 2017, 11:57 p.m.
Message ID <e28b3818efa140482be9.1490831854@x1c>
Download mbox | patch
Permalink /patch/19834/
State Accepted
Headers show

Comments

Jun Wu - March 29, 2017, 11:57 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1490831217 25200
#      Wed Mar 29 16:46:57 2017 -0700
# Node ID e28b3818efa140482be9849f7bd8dd915e25fa07
# Parent  cda83a1bfb3ac3a23cfa158c407be93755c1018e
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r e28b3818efa1
check-code: detect r.revision(r.node(rev))

revlog.revision takes either node or rev, but taking a rev is more
efficient, because converting rev to node is just a seek and read.
That's cheaper than converting node to rev, which may require O(n) walk in
revlog index for the first times, and then triggering building the radix
tree index. Even with the radix tree built, rev -> node is still faster than
node -> rev because the radix tree requires more jumps in memory.

So r.revision(r.node(rev)) should be changed to r.revision(rev). This patch
adds a check-code rule to detect that.
Yuya Nishihara - March 31, 2017, 3:11 p.m.
On Wed, 29 Mar 2017 16:57:34 -0700, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490831217 25200
> #      Wed Mar 29 16:46:57 2017 -0700
> # Node ID e28b3818efa140482be9849f7bd8dd915e25fa07
> # Parent  cda83a1bfb3ac3a23cfa158c407be93755c1018e
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r e28b3818efa1
> check-code: detect r.revision(r.node(rev))

> diff --git a/tests/test-check-code.t b/tests/test-check-code.t
> --- a/tests/test-check-code.t
> +++ b/tests/test-check-code.t
> @@ -10,5 +10,14 @@ New errors are not allowed. Warnings are
>    $ hg locate -X contrib/python-zstandard -X hgext/fsmonitor/pywatchman |
>    > sed 's-\\-/-g' | xargs "$check_code" --warnings --per-file=0 || false
> +  contrib/perf.py:859:
> +   >             r.revision(r.node(x))
> +   don't covert rev to node before passing to revision(nodeorrev)

Updated test-contrib-perf.t as well.
via Mercurial-devel - April 3, 2017, 6:25 p.m.
On Wed, Mar 29, 2017 at 4:57 PM, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490831217 25200
> #      Wed Mar 29 16:46:57 2017 -0700
> # Node ID e28b3818efa140482be9849f7bd8dd915e25fa07
> # Parent  cda83a1bfb3ac3a23cfa158c407be93755c1018e
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r e28b3818efa1
> check-code: detect r.revision(r.node(rev))
>
> revlog.revision takes either node or rev, but taking a rev is more
> efficient, because converting rev to node is just a seek and read.
> That's cheaper than converting node to rev, which may require O(n) walk in
> revlog index for the first times, and then triggering building the radix
> tree index. Even with the radix tree built, rev -> node is still faster than
> node -> rev because the radix tree requires more jumps in memory.
>
> So r.revision(r.node(rev)) should be changed to r.revision(rev). This patch
> adds a check-code rule to detect that.
>
> diff --git a/contrib/check-code.py b/contrib/check-code.py
> --- a/contrib/check-code.py
> +++ b/contrib/check-code.py
> @@ -338,4 +338,6 @@ pypats = [
>      (r'^import BaseHTTPServer', "use util.httpserver instead"),
>      (r'\.next\(\)', "don't use .next(), use next(...)"),
> +    (r'([a-z]*).revision\(\1\.node\(',
> +     "don't covert rev to node before passing to revision(nodeorrev)"),

I'll send a patch to s/covert/convert/
Jun Wu - April 3, 2017, 8:21 p.m.
Excerpts from Martin von Zweigbergk's message of 2017-04-03 11:25:23 -0700:
> I'll send a patch to s/covert/convert/

Thanks! I wonder if I can obsolete "covert" from my vim dictionary...

Patch

diff --git a/contrib/check-code.py b/contrib/check-code.py
--- a/contrib/check-code.py
+++ b/contrib/check-code.py
@@ -338,4 +338,6 @@  pypats = [
     (r'^import BaseHTTPServer', "use util.httpserver instead"),
     (r'\.next\(\)', "don't use .next(), use next(...)"),
+    (r'([a-z]*).revision\(\1\.node\(',
+     "don't covert rev to node before passing to revision(nodeorrev)"),
 
     # rules depending on implementation of repquote()
diff --git a/tests/test-check-code.t b/tests/test-check-code.t
--- a/tests/test-check-code.t
+++ b/tests/test-check-code.t
@@ -10,5 +10,14 @@  New errors are not allowed. Warnings are
   $ hg locate -X contrib/python-zstandard -X hgext/fsmonitor/pywatchman |
   > sed 's-\\-/-g' | xargs "$check_code" --warnings --per-file=0 || false
+  contrib/perf.py:859:
+   >             r.revision(r.node(x))
+   don't covert rev to node before passing to revision(nodeorrev)
   Skipping i18n/polib.py it has no-che?k-code (glob)
+  mercurial/bundlerepo.py:117:
+   >         return mdiff.textdiff(self.revision(self.node(rev1)),
+   don't covert rev to node before passing to revision(nodeorrev)
+  mercurial/bundlerepo.py:118:
+   >                               self.revision(self.node(rev2)))
+   don't covert rev to node before passing to revision(nodeorrev)
   mercurial/demandimport.py:312:
    >     if os.environ.get('HGDEMANDIMPORT') != 'disable':
@@ -37,5 +46,20 @@  New errors are not allowed. Warnings are
    >     policy = os.environ.get('HGMODULEPOLICY', policy)
    use encoding.environ instead (py3)
+  mercurial/revlog.py:441:
+   >         t = self.revision(self.node(rev))
+   don't covert rev to node before passing to revision(nodeorrev)
+  mercurial/revlog.py:1599:
+   >                 basetext = self.revision(self.node(baserev), _df=fh, raw=raw)
+   don't covert rev to node before passing to revision(nodeorrev)
+  mercurial/revlog.py:1631:
+   >                     ptext = self.revision(self.node(rev), _df=fh)
+   don't covert rev to node before passing to revision(nodeorrev)
   Skipping mercurial/statprof.py it has no-che?k-code (glob)
+  mercurial/unionrepo.py:93:
+   >         return mdiff.textdiff(self.revision(self.node(rev1)),
+   don't covert rev to node before passing to revision(nodeorrev)
+  mercurial/unionrepo.py:94:
+   >                               self.revision(self.node(rev2)))
+   don't covert rev to node before passing to revision(nodeorrev)
   [1]