[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