[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