[PATCH 01 of 16 V3] largefiles: move basestore._openstore into new module to remove cycle

Piotr Listkiewicz piotr.listkiewicz at gmail.com
Thu Jun 2 15:12:30 EDT 2016


>
> - as discussed in previous thread, name of new module "store" seems
>   a little strange
>   "store" + something ("factory" or so ?) or "lfstore" seem better,
>   because mercurial/store.py already exists and this "store" has no
>   relation to mercurial/store.py.


Right, storefactory seems fine to me

- is combination of "copy from basestore" + "delete about basestore"
>   suitable for factoring code path out in Mercurial source ?
>   I know that this works well with Mercurial, and that this will make
>   following "_openstore()" history across factoring out easier. But
>   I'm not sure whether this is suitable for history of Mercurial
>   itself.


I did it because one of the reviewers requested it, I am open to change it

- was this series posted to devel-ml ?
>   (not directly related to this patch itself)
>   I can't find any article by this "largefiles: move
>   basestore._openstore into new module to remove cycle" subject in
>   devel-ml archive and my inbox. I found only V2 of this series.
>   Please forget this, if this email with citation was intentionally
>   posted as a kind of RFC or so.


I my mail client sent folder series was marked as sent, but i didnt find it
in devel mailing list archives so i was not sure. It seems like I have to
resend it


2016-06-02 20:54 GMT+02:00 FUJIWARA Katsunori <foozy at lares.dti.ne.jp>:

> At Wed, 1 Jun 2016 23:24:10 +0200,
> Piotr Listkiewicz wrote:
> >
> > How about this solution to the problem, any opinions?
>
> I think that moving _openstore() from basestore into new module itself
> is good solution.
>
> And some nitpicking.
>
> - as discussed in previous thread, name of new module "store" seems
>   a little strange
>
>   "store" + something ("factory" or so ?) or "lfstore" seem better,
>   because mercurial/store.py already exists and this "store" has no
>   relation to mercurial/store.py.
>
> - is combination of "copy from basestore" + "delete about basestore"
>   suitable for factoring code path out in Mercurial source ?
>
>   I know that this works well with Mercurial, and that this will make
>   following "_openstore()" history across factoring out easier. But
>   I'm not sure whether this is suitable for history of Mercurial
>   itself.
>
>   I want opinion of core members about this way.
>
> - was this series posted to devel-ml ?
>   (not directly related to this patch itself)
>
>   I can't find any article by this "largefiles: move
>   basestore._openstore into new module to remove cycle" subject in
>   devel-ml archive and my inbox. I found only V2 of this series.
>
>   Please forget this, if this email with citation was intentionally
>   posted as a kind of RFC or so.
>
>
> > 2016-05-20 10:09 GMT+02:00 liscju <piotr.listkiewicz at gmail.com>:
> >
> > > # HG changeset patch
> > > # User Martin von Zweigbergk <martinvonz at google.com>
> > > # Date 1463505236 25200
> > > #      Tue May 17 10:13:56 2016 -0700
> > > # Node ID 7f2f418a9d39cd3beacb5f7c82c3a9c1d47ffe22
> > > # Parent  7bcfb9090c86d99b4eeb4e02cdda7ca3cb3373d9
> > > largefiles: move basestore._openstore into new module to remove cycle
> > >
> > > diff --git a/hgext/largefiles/basestore.py
> b/hgext/largefiles/basestore.py
> > > --- a/hgext/largefiles/basestore.py
> > > +++ b/hgext/largefiles/basestore.py
> > > @@ -8,9 +8,7 @@
> > >
> > >  '''base class for store implementations and store-related utility
> code'''
> > >
> > > -import re
> > > -
> > > -from mercurial import util, node, hg, error
> > > +from mercurial import util, node
> > >  from mercurial.i18n import _
> > >
> > >  import lfutil
> > > @@ -164,63 +162,3 @@ class basestore(object):
> > >          Returns _true_ if any problems are found!
> > >          '''
> > >          raise NotImplementedError('abstract method')
> > > -
> > > -import localstore, wirestore
> > > -
> > > -_storeprovider = {
> > > -    'file':  [localstore.localstore],
> > > -    'http':  [wirestore.wirestore],
> > > -    'https': [wirestore.wirestore],
> > > -    'ssh': [wirestore.wirestore],
> > > -    }
> > > -
> > > -_scheme_re = re.compile(r'^([a-zA-Z0-9+-.]+)://')
> > > -
> > > -# During clone this function is passed the src's ui object
> > > -# but it needs the dest's ui object so it can read out of
> > > -# the config file. Use repo.ui instead.
> > > -def _openstore(repo, remote=None, put=False):
> > > -    ui = repo.ui
> > > -
> > > -    if not remote:
> > > -        lfpullsource = getattr(repo, 'lfpullsource', None)
> > > -        if lfpullsource:
> > > -            path = ui.expandpath(lfpullsource)
> > > -        elif put:
> > > -            path = ui.expandpath('default-push', 'default')
> > > -        else:
> > > -            path = ui.expandpath('default')
> > > -
> > > -        # ui.expandpath() leaves 'default-push' and 'default' alone if
> > > -        # they cannot be expanded: fallback to the empty string,
> > > -        # meaning the current directory.
> > > -        if path == 'default-push' or path == 'default':
> > > -            path = ''
> > > -            remote = repo
> > > -        else:
> > > -            path, _branches = hg.parseurl(path)
> > > -            remote = hg.peer(repo, {}, path)
> > > -
> > > -    # The path could be a scheme so use Mercurial's normal
> functionality
> > > -    # to resolve the scheme to a repository and use its path
> > > -    path = util.safehasattr(remote, 'url') and remote.url() or
> remote.path
> > > -
> > > -    match = _scheme_re.match(path)
> > > -    if not match:                       # regular filesystem path
> > > -        scheme = 'file'
> > > -    else:
> > > -        scheme = match.group(1)
> > > -
> > > -    try:
> > > -        storeproviders = _storeprovider[scheme]
> > > -    except KeyError:
> > > -        raise error.Abort(_('unsupported URL scheme %r') % scheme)
> > > -
> > > -    for classobj in storeproviders:
> > > -        try:
> > > -            return classobj(ui, repo, remote)
> > > -        except lfutil.storeprotonotcapable:
> > > -            pass
> > > -
> > > -    raise error.Abort(_('%s does not appear to be a largefile store')
> %
> > > -                     util.hidepassword(path))
> > > diff --git a/hgext/largefiles/lfcommands.py
> > > b/hgext/largefiles/lfcommands.py
> > > --- a/hgext/largefiles/lfcommands.py
> > > +++ b/hgext/largefiles/lfcommands.py
> > > @@ -20,7 +20,7 @@ from hgext.convert import convcmd
> > >  from hgext.convert import filemap
> > >
> > >  import lfutil
> > > -import basestore
> > > +import store as storemod
> > >
> > >  # -- Commands
> ----------------------------------------------------------
> > >
> > > @@ -337,7 +337,7 @@ def uploadlfiles(ui, rsrc, rdst, files):
> > >      if not files:
> > >          return
> > >
> > > -    store = basestore._openstore(rsrc, rdst, put=True)
> > > +    store = storemod._openstore(rsrc, rdst, put=True)
> > >
> > >      at = 0
> > >      ui.debug("sending statlfile command for %d largefiles\n" %
> len(files))
> > > @@ -368,7 +368,7 @@ def verifylfiles(ui, repo, all=False, co
> > >      else:
> > >          revs = ['.']
> > >
> > > -    store = basestore._openstore(repo)
> > > +    store = storemod._openstore(repo)
> > >      return store.verify(revs, contents=contents)
> > >
> > >  def cachelfiles(ui, repo, node, filelist=None):
> > > @@ -394,7 +394,7 @@ def cachelfiles(ui, repo, node, filelist
> > >              toget.append((lfile, expectedhash))
> > >
> > >      if toget:
> > > -        store = basestore._openstore(repo)
> > > +        store = storemod._openstore(repo)
> > >          ret = store.get(toget)
> > >          return ret
> > >
> > > diff --git a/hgext/largefiles/overrides.py
> b/hgext/largefiles/overrides.py
> > > --- a/hgext/largefiles/overrides.py
> > > +++ b/hgext/largefiles/overrides.py
> > > @@ -17,7 +17,7 @@ from mercurial.i18n import _
> > >
> > >  import lfutil
> > >  import lfcommands
> > > -import basestore
> > > +import store as storemod
> > >
> > >  # -- Utility functions: commonly/repeatedly needed functionality
> > > ---------------
> > >
> > > @@ -1109,7 +1109,7 @@ def _getoutgoings(repo, other, missing,
> > >              lfhashes.add(lfhash)
> > >      lfutil.getlfilestoupload(repo, missing, dedup)
> > >      if lfhashes:
> > > -        lfexists = basestore._openstore(repo, other).exists(lfhashes)
> > > +        lfexists = storemod._openstore(repo, other).exists(lfhashes)
> > >          for fn, lfhash in knowns:
> > >              if not lfexists[lfhash]: # lfhash doesn't exist on "other"
> > >                  addfunc(fn, lfhash)
> > > @@ -1338,7 +1338,7 @@ def overridecat(orig, ui, repo, file1, *
> > >          else:
> > >              hash = lfutil.readstandin(repo, lf, ctx.rev())
> > >              if not lfutil.inusercache(repo.ui, hash):
> > > -                store = basestore._openstore(repo)
> > > +                store = storemod._openstore(repo)
> > >                  success, missing = store.get([(lf, hash)])
> > >                  if len(success) != 1:
> > >                      raise error.Abort(
> > > diff --git a/hgext/largefiles/basestore.py b/hgext/largefiles/store.py
> > > copy from hgext/largefiles/basestore.py
> > > copy to hgext/largefiles/store.py
> > > --- a/hgext/largefiles/basestore.py
> > > +++ b/hgext/largefiles/store.py
> > > @@ -1,180 +1,23 @@
> > > -# Copyright 2009-2010 Gregory P. Ward
> > > -# Copyright 2009-2010 Intelerad Medical Systems Incorporated
> > > -# Copyright 2010-2011 Fog Creek Software
> > > -# Copyright 2010-2011 Unity Technologies
> > > -#
> > >  # This software may be used and distributed according to the terms of
> the
> > >  # GNU General Public License version 2 or any later version.
> > >
> > > -'''base class for store implementations and store-related utility
> code'''
> > > +from __future__ import absolute_import
> > >
> > >  import re
> > >
> > > -from mercurial import util, node, hg, error
> > >  from mercurial.i18n import _
> > >
> > > -import lfutil
> > > -
> > > -class StoreError(Exception):
> > > -    '''Raised when there is a problem getting files from or putting
> > > -    files to a central store.'''
> > > -    def __init__(self, filename, hash, url, detail):
> > > -        self.filename = filename
> > > -        self.hash = hash
> > > -        self.url = url
> > > -        self.detail = detail
> > > -
> > > -    def longmessage(self):
> > > -        return (_("error getting id %s from url %s for file %s:
> %s\n") %
> > > -                 (self.hash, util.hidepassword(self.url),
> self.filename,
> > > -                  self.detail))
> > > -
> > > -    def __str__(self):
> > > -        return "%s: %s" % (util.hidepassword(self.url), self.detail)
> > > -
> > > -class basestore(object):
> > > -    def __init__(self, ui, repo, url):
> > > -        self.ui = ui
> > > -        self.repo = repo
> > > -        self.url = url
> > > -
> > > -    def put(self, source, hash):
> > > -        '''Put source file into the store so it can be retrieved by
> > > hash.'''
> > > -        raise NotImplementedError('abstract method')
> > > -
> > > -    def exists(self, hashes):
> > > -        '''Check to see if the store contains the given hashes. Given
> an
> > > -        iterable of hashes it returns a mapping from hash to bool.'''
> > > -        raise NotImplementedError('abstract method')
> > > -
> > > -    def get(self, files):
> > > -        '''Get the specified largefiles from the store and write to
> local
> > > -        files under repo.root.  files is a list of (filename, hash)
> > > -        tuples.  Return (success, missing), lists of files
> successfully
> > > -        downloaded and those not found in the store.  success is a
> list
> > > -        of (filename, hash) tuples; missing is a list of filenames
> that
> > > -        we could not get.  (The detailed error message will already
> have
> > > -        been presented to the user, so missing is just supplied as a
> > > -        summary.)'''
> > > -        success = []
> > > -        missing = []
> > > -        ui = self.ui
> > > -
> > > -        at = 0
> > > -        available = self.exists(set(hash for (_filename, hash) in
> files))
> > > -        for filename, hash in files:
> > > -            ui.progress(_('getting largefiles'), at, unit=_('files'),
> > > -                total=len(files))
> > > -            at += 1
> > > -            ui.note(_('getting %s:%s\n') % (filename, hash))
> > > -
> > > -            if not available.get(hash):
> > > -                ui.warn(_('%s: largefile %s not available from %s\n')
> > > -                        % (filename, hash,
> util.hidepassword(self.url)))
> > > -                missing.append(filename)
> > > -                continue
> > > -
> > > -            if self._gethash(filename, hash):
> > > -                success.append((filename, hash))
> > > -            else:
> > > -                missing.append(filename)
> > > -
> > > -        ui.progress(_('getting largefiles'), None)
> > > -        return (success, missing)
> > > -
> > > -    def _gethash(self, filename, hash):
> > > -        """Get file with the provided hash and store it in the local
> > > repo's
> > > -        store and in the usercache.
> > > -        filename is for informational messages only.
> > > -        """
> > > -        util.makedirs(lfutil.storepath(self.repo, ''))
> > > -        storefilename = lfutil.storepath(self.repo, hash)
> > > -
> > > -        tmpname = storefilename + '.tmp'
> > > -        tmpfile = util.atomictempfile(tmpname,
> > > -
> > > createmode=self.repo.store.createmode)
> > > +from mercurial import (
> > > +    error,
> > > +    hg,
> > > +    util,
> > > +)
> > >
> > > -        try:
> > > -            gothash = self._getfile(tmpfile, filename, hash)
> > > -        except StoreError as err:
> > > -            self.ui.warn(err.longmessage())
> > > -            gothash = ""
> > > -        tmpfile.close()
> > > -
> > > -        if gothash != hash:
> > > -            if gothash != "":
> > > -                self.ui.warn(_('%s: data corruption (expected %s, got
> > > %s)\n')
> > > -                             % (filename, hash, gothash))
> > > -            util.unlink(tmpname)
> > > -            return False
> > > -
> > > -        util.rename(tmpname, storefilename)
> > > -        lfutil.linktousercache(self.repo, hash)
> > > -        return True
> > > -
> > > -    def verify(self, revs, contents=False):
> > > -        '''Verify the existence (and, optionally, contents) of every
> big
> > > -        file revision referenced by every changeset in revs.
> > > -        Return 0 if all is well, non-zero on any errors.'''
> > > -
> > > -        self.ui.status(_('searching %d changesets for largefiles\n') %
> > > -                       len(revs))
> > > -        verified = set()                # set of (filename, filenode)
> > > tuples
> > > -        filestocheck = []               # list of (cset, filename,
> > > expectedhash)
> > > -        for rev in revs:
> > > -            cctx = self.repo[rev]
> > > -            cset = "%d:%s" % (cctx.rev(), node.short(cctx.node()))
> > > -
> > > -            for standin in cctx:
> > > -                filename = lfutil.splitstandin(standin)
> > > -                if filename:
> > > -                    fctx = cctx[standin]
> > > -                    key = (filename, fctx.filenode())
> > > -                    if key not in verified:
> > > -                        verified.add(key)
> > > -                        expectedhash = fctx.data()[0:40]
> > > -                        filestocheck.append((cset, filename,
> > > expectedhash))
> > > -
> > > -        failed = self._verifyfiles(contents, filestocheck)
> > > -
> > > -        numrevs = len(verified)
> > > -        numlfiles = len(set([fname for (fname, fnode) in verified]))
> > > -        if contents:
> > > -            self.ui.status(
> > > -                _('verified contents of %d revisions of %d
> largefiles\n')
> > > -                % (numrevs, numlfiles))
> > > -        else:
> > > -            self.ui.status(
> > > -                _('verified existence of %d revisions of %d
> largefiles\n')
> > > -                % (numrevs, numlfiles))
> > > -        return int(failed)
> > > -
> > > -    def _getfile(self, tmpfile, filename, hash):
> > > -        '''Fetch one revision of one file from the store and write it
> > > -        to tmpfile.  Compute the hash of the file on-the-fly as it
> > > -        downloads and return the hash.  Close tmpfile.  Raise
> > > -        StoreError if unable to download the file (e.g. it does not
> > > -        exist in the store).'''
> > > -        raise NotImplementedError('abstract method')
> > > -
> > > -    def _verifyfiles(self, contents, filestocheck):
> > > -        '''Perform the actual verification of files in the store.
> > > -        'contents' controls verification of content hash.
> > > -        'filestocheck' is list of files to check.
> > > -        Returns _true_ if any problems are found!
> > > -        '''
> > > -        raise NotImplementedError('abstract method')
> > > -
> > > -import localstore, wirestore
> > > -
> > > -_storeprovider = {
> > > -    'file':  [localstore.localstore],
> > > -    'http':  [wirestore.wirestore],
> > > -    'https': [wirestore.wirestore],
> > > -    'ssh': [wirestore.wirestore],
> > > -    }
> > > -
> > > -_scheme_re = re.compile(r'^([a-zA-Z0-9+-.]+)://')
> > > +from . import (
> > > +    lfutil,
> > > +    localstore,
> > > +    wirestore,
> > > +)
> > >
> > >  # During clone this function is passed the src's ui object
> > >  # but it needs the dest's ui object so it can read out of
> > > @@ -224,3 +67,12 @@ def _openstore(repo, remote=None, put=Fa
> > >
> > >      raise error.Abort(_('%s does not appear to be a largefile store')
> %
> > >                       util.hidepassword(path))
> > > +
> > > +_storeprovider = {
> > > +    'file':  [localstore.localstore],
> > > +    'http':  [wirestore.wirestore],
> > > +    'https': [wirestore.wirestore],
> > > +    'ssh': [wirestore.wirestore],
> > > +    }
> > > +
> > > +_scheme_re = re.compile(r'^([a-zA-Z0-9+-.]+)://')
> > > diff --git a/tests/test-check-module-imports.t
> > > b/tests/test-check-module-imports.t
> > > --- a/tests/test-check-module-imports.t
> > > +++ b/tests/test-check-module-imports.t
> > > @@ -162,5 +162,3 @@ outputs, which should be fixed later.
> > >    > -X tests/test-hgweb-no-request-uri.t \
> > >    > -X tests/test-hgweb-non-interactive.t \
> > >    > | sed 's-\\-/-g' | python "$import_checker" -
> > > -  Import cycle: hgext.largefiles.basestore ->
> hgext.largefiles.localstore
> > > -> hgext.largefiles.basestore
> > > -  [1]
> > >
>
> ----------------------------------------------------------------------
> [FUJIWARA Katsunori]                             foozy at lares.dti.ne.jp
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160602/505ee929/attachment.html>


More information about the Mercurial-devel mailing list