Patchwork obscache: use _readmarkers() from core with correct signature

login
register
mail settings
Submitter via Mercurial-devel
Date Aug. 22, 2017, 8:05 p.m.
Message ID <efc4b69aecfb3b407ca3.1503432309@martinvonz.svl.corp.google.com>
Download mbox | patch
Permalink /patch/23214/
State Superseded
Headers show

Comments

via Mercurial-devel - Aug. 22, 2017, 8:05 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1503424759 25200
#      Tue Aug 22 10:59:19 2017 -0700
# Node ID efc4b69aecfb3b407ca362cff318044445e9672c
# Parent  95470e817c00b03fcf99e486412cc7d7f0116681
obscache: use _readmarkers() from core with correct signature

The copied _readmarkers() went stale with Mercurial core commit
5d3ba4395288 (obsstore: let read marker API take a range of offsets,
2017-06-04). At the same time, the Mercurial core version of the
function gained the desired offset argument, so we can now use that
function.
Boris Feld - Aug. 23, 2017, 8:36 a.m.
The patch seems fine. Could you move the _readmarkers function
definition inside the if/else, flake8 and pyflakes are complaining:

+  hgext3rd/evolve/obscache.py:133:5: F811 redefinition of unused
'_readmarkers' from line 115

How did you detect it? Did you call _readmarkers from another
extension? Should we add an Evolve test to not be taken off-guard the
next time?

On Tue, 2017-08-22 at 13:05 -0700, Martin von Zweigbergk via Mercurial-
devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1503424759 25200
> #      Tue Aug 22 10:59:19 2017 -0700
> # Node ID efc4b69aecfb3b407ca362cff318044445e9672c
> # Parent  95470e817c00b03fcf99e486412cc7d7f0116681
> obscache: use _readmarkers() from core with correct signature
> 
> The copied _readmarkers() went stale with Mercurial core commit
> 5d3ba4395288 (obsstore: let read marker API take a range of offsets,
> 2017-06-04). At the same time, the Mercurial core version of the
> function gained the desired offset argument, so we can now use that
> function.
> 
> diff --git a/hgext3rd/evolve/obscache.py
> b/hgext3rd/evolve/obscache.py
> --- a/hgext3rd/evolve/obscache.py
> +++ b/hgext3rd/evolve/obscache.py
> @@ -127,6 +127,11 @@
>                            % diskversion)
>      return diskversion, obsolete.formats[diskversion][0](data, off)
>  
> +if obsolete._readmarkers.__code__.co_argcount > 1:
> +    # hg-4.3+ has the "offset" parameter, and _fm?readmarkers also
> have an
> +    # extra "stop" parameter
> +    _readmarkers = obsolete._readmarkers
> +
>  def markersfrom(obsstore, byteoffset, firstmarker):
>      if not firstmarker:
>          return list(obsstore)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - Aug. 23, 2017, 1:06 p.m.
On Aug 23, 2017 1:36 AM, "Boris Feld" <boris.feld@octobus.net> wrote:

The patch seems fine. Could you move the _readmarkers function
definition inside the if/else, flake8 and pyflakes are complaining:

+  hgext3rd/evolve/obscache.py:133:5: F811 redefinition of unused
'_readmarkers' from line 115


Ok, will do.


How did you detect it? Did you call _readmarkers from another
extension? Should we add an Evolve test to not be taken off-guard the
next time?


A user saw it crash. It's called from obscache.py, but only in rare cases,
it seems.


On Tue, 2017-08-22 at 13:05 -0700, Martin von Zweigbergk via Mercurial-
devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1503424759 25200
> #      Tue Aug 22 10:59:19 2017 -0700
> # Node ID efc4b69aecfb3b407ca362cff318044445e9672c
> # Parent  95470e817c00b03fcf99e486412cc7d7f0116681
> obscache: use _readmarkers() from core with correct signature
>
> The copied _readmarkers() went stale with Mercurial core commit
> 5d3ba4395288 (obsstore: let read marker API take a range of offsets,
> 2017-06-04). At the same time, the Mercurial core version of the
> function gained the desired offset argument, so we can now use that
> function.
>
> diff --git a/hgext3rd/evolve/obscache.py
> b/hgext3rd/evolve/obscache.py
> --- a/hgext3rd/evolve/obscache.py
> +++ b/hgext3rd/evolve/obscache.py
> @@ -127,6 +127,11 @@
>                            % diskversion)
>      return diskversion, obsolete.formats[diskversion][0](data, off)
>
> +if obsolete._readmarkers.__code__.co_argcount > 1:
> +    # hg-4.3+ has the "offset" parameter, and _fm?readmarkers also
> have an
> +    # extra "stop" parameter
> +    _readmarkers = obsolete._readmarkers
> +
>  def markersfrom(obsstore, byteoffset, firstmarker):
>      if not firstmarker:
>          return list(obsstore)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Boris Feld - Aug. 23, 2017, 2:06 p.m.
On Wed, 2017-08-23 at 06:06 -0700, Martin von Zweigbergk wrote:
> On Aug 23, 2017 1:36 AM, "Boris Feld" <boris.feld@octobus.net> wrote:
> The patch seems fine. Could you move the _readmarkers function
> 
> definition inside the if/else, flake8 and pyflakes are complaining:
> 
> 
> 
> +  hgext3rd/evolve/obscache.py:133:5: F811 redefinition of unused
> 
> '_readmarkers' from line 115
> 
> 
> Ok, will do.
> 
> 
> 
> 
> How did you detect it? Did you call _readmarkers from another
> 
> extension? Should we add an Evolve test to not be taken off-guard the
> 
> next time?
> 
> 
> A user saw it crash. It's called from obscache.py, but only in rare
> cases, it seems.

You are right, I just ran the tests with coverage and we don't cover
this case, we will try add a test for it.
> 
> 
> On Tue, 2017-08-22 at 13:05 -0700, Martin von Zweigbergk via
> Mercurial-
> 
> devel wrote:
> 
> > # HG changeset patch
> 
> > # User Martin von Zweigbergk <martinvonz@google.com>
> 
> > # Date 1503424759 25200
> 
> > #      Tue Aug 22 10:59:19 2017 -0700
> 
> > # Node ID efc4b69aecfb3b407ca362cff318044445e9672c
> 
> > # Parent  95470e817c00b03fcf99e486412cc7d7f0116681
> 
> > obscache: use _readmarkers() from core with correct signature
> 
> >
> 
> > The copied _readmarkers() went stale with Mercurial core commit
> 
> > 5d3ba4395288 (obsstore: let read marker API take a range of
> offsets,
> 
> > 2017-06-04). At the same time, the Mercurial core version of the
> 
> > function gained the desired offset argument, so we can now use that
> 
> > function.
> 
> >
> 
> > diff --git a/hgext3rd/evolve/obscache.py
> 
> > b/hgext3rd/evolve/obscache.py
> 
> > --- a/hgext3rd/evolve/obscache.py
> 
> > +++ b/hgext3rd/evolve/obscache.py
> 
> > @@ -127,6 +127,11 @@
> 
> >                            % diskversion)
> 
> >      return diskversion, obsolete.formats[diskversion][0](data,
> off)
> 
> >  
> 
> > +if obsolete._readmarkers.__code__.co_argcount > 1:
> 
> > +    # hg-4.3+ has the "offset" parameter, and _fm?readmarkers also
> 
> > have an
> 
> > +    # extra "stop" parameter
> 
> > +    _readmarkers = obsolete._readmarkers
> 
> > +
> 
> >  def markersfrom(obsstore, byteoffset, firstmarker):
> 
> >      if not firstmarker:
> 
> >          return list(obsstore)
> 
> > _______________________________________________
> 
> > Mercurial-devel mailing list
> 
> > Mercurial-devel@mercurial-scm.org
> 
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> 
> 
>

Patch

diff --git a/hgext3rd/evolve/obscache.py b/hgext3rd/evolve/obscache.py
--- a/hgext3rd/evolve/obscache.py
+++ b/hgext3rd/evolve/obscache.py
@@ -127,6 +127,11 @@ 
                           % diskversion)
     return diskversion, obsolete.formats[diskversion][0](data, off)
 
+if obsolete._readmarkers.__code__.co_argcount > 1:
+    # hg-4.3+ has the "offset" parameter, and _fm?readmarkers also have an
+    # extra "stop" parameter
+    _readmarkers = obsolete._readmarkers
+
 def markersfrom(obsstore, byteoffset, firstmarker):
     if not firstmarker:
         return list(obsstore)