This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1325

remotefilelog-datapack: refactor createPack method to include pack directory
ClosedPublic

Authored by singhsrb on Nov 6 2017, 6:35 PM.
Tags
None
Subscribers
durham

Details

Reviewers
durham
Group Reviewers
Restricted Project
Commits
rFBHGX1c2a208458c0: remotefilelog-datapack: refactor createPack method to include pack directory
Summary

createPack had no option to specify the pack directory because of
which it can only create one pack in a directory. This restriction was in place
because we only test the datapack and not the datapackstore during these tests.
This commit makes the method more generic and includes the option to specify
the directory for creating the packs. This would allow for the datapackstore to
be tested while reusing most of the current logic.

Test Plan

Ran all the tests.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

singhsrb created this revision.Nov 6 2017, 6:35 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 6 2017, 6:35 PM
durham requested changes to this revision.Nov 7 2017, 1:29 PM
durham added a subscriber: durham.
durham added inline comments.
tests/remotefilelog-datapack.py
60

The normal pattern here would be to have createPack take an optional packDir argument. If that argument is None (the default) then we use makeTempDir. That way we don't have a plethora of functions for the variations on the same action. And we don't have to update any of the existing callers.

This revision now requires changes to proceed.Nov 7 2017, 1:29 PM
singhsrb added inline comments.Nov 7 2017, 1:52 PM
tests/remotefilelog-datapack.py
60

I actually did that and then did this for the following reasons:

  1. I wanted to have the packdir as the first argument. So, that would have meant refactoring the existing code anyways.
  2. After that change, createPack was doing too much for me.

I can change it to an optional argument.

singhsrb updated this revision to Diff 3321.Nov 7 2017, 2:10 PM
singhsrb marked an inline comment as done.Nov 7 2017, 8:24 PM
durham accepted this revision.Nov 7 2017, 8:28 PM
This revision is now accepted and ready to land.Nov 7 2017, 8:28 PM
Closed by commit rFBHGX1c2a208458c0: remotefilelog-datapack: refactor createPack method to include pack directory (authored by singhsrb). · Explain WhyNov 8 2017, 6:28 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mtests/remotefilelog-datapack.py (6 lines)

Diff 3354

tests/remotefilelog-datapack.py

return tempdir return tempdir
def getHash(self, content): def getHash(self, content):
return hashlib.sha1(content).digest() return hashlib.sha1(content).digest()
def getFakeHash(self): def getFakeHash(self):
return ''.join(chr(random.randint(0, 255)) for _ in range(20)) return ''.join(chr(random.randint(0, 255)) for _ in range(20))
def createPack(self, revisions=None, version=0): def createPack(self, revisions=None, packdir=None, version=0):
if revisions is None: if revisions is None:
revisions = [("filename", self.getFakeHash(), nullid, "content")] revisions = [("filename", self.getFakeHash(), nullid, "content")]
if packdir is None:
durhamUnsubmitted
Done

The normal pattern here would be to have createPack take an optional packDir argument. If that argument is None (the default) then we use makeTempDir. That way we don't have a plethora of functions for the variations on the same action. And we don't have to update any of the existing callers.

durham: The normal pattern here would be to have createPack take an optional packDir argument. If that…
singhsrbAuthorUnsubmitted
Not Done

I actually did that and then did this for the following reasons:

  1. I wanted to have the packdir as the first argument. So, that would have meant refactoring the existing code anyways.
  2. After that change, createPack was doing too much for me.

I can change it to an optional argument.

singhsrb: I actually did that and then did this for the following reasons: # I wanted to have the…
packdir = self.makeTempDir() packdir = self.makeTempDir()
packer = mutabledatapack(mercurial.ui.ui(), packdir, version=version) packer = mutabledatapack(mercurial.ui.ui(), packdir, version=version)
for args in revisions: for args in revisions:
filename, node, base, content = args[0:4] filename, node, base, content = args[0:4]
# meta is optional # meta is optional
meta = None meta = None
if len(args) > 4: if len(args) > 4:
meta = args[4] meta = args[4]