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

[Adrian Buehlmann]

On 17.07.2008 20:09, Adrian Buehlmann wrote:
> On 17.07.2008 18:24, Matt Mackall wrote:
>> 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.
> 
> If I knew how, I would try doing so.
> 
>>> +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.
> 
> This is nearly verbatim moved from streamclone. I wouldn't have
> expected to rewrite streamclone.
> 
>>> +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".
> 
> Could you please tell me how to do it instead?
> 
>>> +class _store:
>> If you're going to use a class, you should use inheritance to derive the
>> variations of your class. 
> 
> I'm lost here. I think I did use inheritence? _store is a base
> 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.
> 
> I did use inheritance. fnlogstore (patch 3) is derived from _store.
> 
>>> +    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.
> 
> What should be in 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.
> 
> This code was moved nearly verbatim from streamclone.py. Of course
> I have no problem with renaming anything.
> 
>>> +    def storefiles(self, getsizes):
>>> +        return (False, [], 0)
>> Is this actually necessary? We could just test whether the repo is
>> local, no?
> 
> I was not sure here. I think it *is* already tested that the repo
> is not local. I will try to see if this can be removed.
> 
> Some general comments:
> 
> If I still happen to have the energy to continue trying.
> Currently, I'm a bit tired.
> 
> I just couldn't believe that the aux and filename length problem can't
> be solved. I still think it should be solvable. But maybe not by me.
> 
>>From a functional standpoint, the patch series works fine.
^
My error, I have inserted a '>' there. That sentence was written
by me not Matt. Apologies.

> Maybe it would be easier to write the code yourself Matt?
> After all, it can't be that hard for you, no?