[PATCH 2 of 3] move store walking from streamclone to filelog
Adrian Buehlmann
adrian at cadifra.com
Thu Jul 17 13:09:54 CDT 2008
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.
More information about the Mercurial-devel
mailing list