Patchwork [5,of,5] perf: workaround check-code

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

Comments

Jun Wu - March 29, 2017, 11:57 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1490831529 25200
#      Wed Mar 29 16:52:09 2017 -0700
# Node ID 5ca313b3da12d145f1d49a85dd8b753e22d51521
# Parent  265ea657d75905fb59a27194a75aaff49be94598
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 5ca313b3da12
perf: workaround check-code

The check-code suggests using rev instead of node for revlog.revision. But
early Mercurial does not support that (see 9117c6561b0b). So let's just
workaround check-code in perf.py
Ryan McElroy - March 30, 2017, 8:48 a.m.
On 3/30/17 12:57 AM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490831529 25200
> #      Wed Mar 29 16:52:09 2017 -0700
> # Node ID 5ca313b3da12d145f1d49a85dd8b753e22d51521
> # Parent  265ea657d75905fb59a27194a75aaff49be94598
> perf: workaround check-code

The code int his series all looks good to me. I'll mark as pre-reviewed 
in patchwork. The only question I have is whether we prefer to leave 
check-code errors in place inside the test, or workaround it with hacks 
like this?

I'd prefer to leave the "expected failures" inside the test because it 
documents that we intend to keep them (probably with a comment at the 
site of the "error"). Also, if the check-code regex gets smarter in the 
future, we don't have to go and change our hack again.

However, I don't know what the rest of the community thinks about this. 
I don't have a strong enough opinion, so I guess I'm -0 on this patch, 
but +1 on the rest of the series. I'll be happy as long as the first 
four get in regardless of if patch 5 is included or not.

> The check-code suggests using rev instead of node for revlog.revision. But
> early Mercurial does not support that (see 9117c6561b0b). So let's just
> workaround check-code in perf.py
>
> diff --git a/contrib/perf.py b/contrib/perf.py
> --- a/contrib/perf.py
> +++ b/contrib/perf.py
> @@ -847,4 +847,5 @@ def perfrevlog(ui, repo, file_=None, sta
>       def d():
>           r = cmdutil.openrevlog(repo, 'perfrevlog', file_, opts)
> +        r2 = r # workaround check-code
>   
>           startrev = 0
> @@ -857,5 +858,5 @@ def perfrevlog(ui, repo, file_=None, sta
>   
>           for x in xrange(startrev, endrev, dist):
> -            r.revision(r.node(x))
> +            r2.revision(r.node(x))
>   
>       timer(d)
> 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,7 +10,4 @@ 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/demandimport.py:312:
>
Yuya Nishihara - March 31, 2017, 2:37 p.m.
On Thu, 30 Mar 2017 09:48:35 +0100, Ryan McElroy wrote:
> On 3/30/17 12:57 AM, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1490831529 25200
> > #      Wed Mar 29 16:52:09 2017 -0700
> > # Node ID 5ca313b3da12d145f1d49a85dd8b753e22d51521
> > # Parent  265ea657d75905fb59a27194a75aaff49be94598
> > perf: workaround check-code
> 
> The code int his series all looks good to me. I'll mark as pre-reviewed 
> in patchwork.

Queued 1-4, thanks.

> The only question I have is whether we prefer to leave
> check-code errors in place inside the test, or workaround it with hacks 
> like this?

We can add an ignore pattern for that purpose. See "re-raises" for example.

Patch

diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -847,4 +847,5 @@  def perfrevlog(ui, repo, file_=None, sta
     def d():
         r = cmdutil.openrevlog(repo, 'perfrevlog', file_, opts)
+        r2 = r # workaround check-code
 
         startrev = 0
@@ -857,5 +858,5 @@  def perfrevlog(ui, repo, file_=None, sta
 
         for x in xrange(startrev, endrev, dist):
-            r.revision(r.node(x))
+            r2.revision(r.node(x))
 
     timer(d)
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,7 +10,4 @@  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/demandimport.py:312: