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

FUJIWARA Katsunori foozy at lares.dti.ne.jp
Thu Jun 2 14:54:23 EDT 2016


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


More information about the Mercurial-devel mailing list