[PATCH 01 of 16 V3] largefiles: move basestore._openstore into new module to remove cycle
FUJIWARA Katsunori
foozy at lares.dti.ne.jp
Fri Jun 3 04:37:06 EDT 2016
At Thu, 2 Jun 2016 21:12:30 +0200,
Piotr Listkiewicz wrote:
>
> [1 <text/plain; UTF-8 (7bit)>]
> >
> > - 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
I see.
> - 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
I sometimes receive bounce mail saying "hey, your mail was blocked,
because many spam were sent via that SMTP server" from public spam
filter for patch series, even though manually replying mails are
accepted :-<
If you use SMTP server of not major domain or ISP, would you confirm
such bounce mail in your inbox ?
> 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
> >
> [2 <text/html; UTF-8 (quoted-printable)>]
>
----------------------------------------------------------------------
[FUJIWARA Katsunori] foozy at lares.dti.ne.jp
More information about the Mercurial-devel
mailing list