[PATCH 2 of 3] move store walking from streamclone to filelog

[Matt Mackall]

On Thu, 2008-07-17 at 16:23 +0200, Adrian Buehlmann wrote:
> # HG changeset patch
> # User Adrian Buehlmann <adrian at cadifra.com>
> # Date 1216299442 -7200
> # Node ID d29d8f832d09a8570a25a68c964baf8572454b75
> # Parent  64d51f7facf9804d3b004f4eae7844ae7a1937ca
> move store walking from streamclone to filelog
> 
> * add method localrepo.localrepository.storefiles
> * add function filelog.store
> * add class filelog._store

This could all be a lot simpler.

> +def _dirwalk(path, recurse, strip_count, decodefn):
> +    '''yields (*.d or *.i filename, size)'''
> +    for e, kind, st in osutil.listdir(path, stat=True):
> +        pe = os.path.join(path, e)
> +        if kind == stat.S_IFDIR:
> +            if recurse:
> +                for x in _dirwalk(pe, True, strip_count, decodefn):
> +                    yield x
> +        else:
> +            if kind != stat.S_IFREG or len(e) < 2:
> +                continue
> +            sfx = e[-2:]
> +            if sfx in ('.d', '.i'):
> +                f = decodefn(util.pconvert(pe[strip_count:]))
> +                yield (f, st.st_size)

This function is too smart. Decoding, stripping and filtering can all be
done at a higher level.

> +def store(requirements, spath, createmode):
> +    efn = encodefn(requirements)
> +    if 'store' not in requirements:
> +        return _store(efn, lambda x: x, spath, createmode)
> +    else:
> +        return _store(efn, decodefilename, spath, createmode)

Layering violation: filelog's not supposed to know about "requirements".

> +class _store:

If you're going to use a class, you should use inheritance to derive the
variations of your class. 

> +    def __init__(self, encodefn, decodefn, spath, createmode):
> +        self.spath = spath
> +        self.encodefn = encodefn
> +        self.__decodefn = decodefn
> +        so = util.opener(self.spath)
> +        so.createmode = createmode
> +        self.opener = encodedopener(so, self.encodefn)

For instance, if you used inheritance, you could simply use a normal
opener here.

> +    def _walkfiledata(self):
> +        strip_count = len(self.spath) + len(os.sep)
> +        dpath = os.path.join(self.spath, 'data')
> +        for x in _dirwalk(dpath, True, strip_count, self.__decodefn):
> +            yield x
> +
> +    def walk(self):
> +        '''yields: direncoded filename, size of file'''
> +        # yield file data first
> +        for x in self._walkfiledata():
> +            yield x
> +        # yield manifest before changelog

Layering violation: filelog.py is only concerned with what lives inside
data/. Perhaps we want a store.py.

> +    def storefiles(self):
> +        '''get all *.i and *.d files in the store
> +
> +        Returns (ok, list of (filename, size), total_bytes)'''
> +
> +        # get consistent snapshot of repo, lock during scan
> +        repolock = None

We usually just call this lock.
 
> +    def storefiles(self, getsizes):
> +        return (False, [], 0)

Is this actually necessary? We could just test whether the repo is
local, no?

-- 
Mathematics is the supreme nostalgia of our time.