( )⚙ D1304 treemanifest: allow hybrid repos to make treeonly commits

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1304

treemanifest: allow hybrid repos to make treeonly commits
ClosedPublic

Authored by durham on Nov 2 2017, 7:27 PM.
Tags
None
Subscribers
quark

Details

Reviewers
quark
Group Reviewers
Restricted Project
Commits
rFBHGX6776e7a6e931: treemanifest: allow hybrid repos to make treeonly commits
Summary

If a hybrid repo pulls in a treeonly commit from a treeonly client, it
previously couldn't commit on top of it because it tried to read the flat
manifest. This patch makes it possible for the hybrid repo to make a treeonly
commit if it is committing on top of a treeonly commit (i.e. where the manifest
only exists in the tree store, not in the flat manifest revlog).

This makes it easier for multiple types of repositories to interact, and to flip
back and forth between treeonly and non-treeonly as we migrate.

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

durham created this revision.Nov 2 2017, 7:27 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 2 2017, 7:27 PM
quark added a subscriber: quark.Nov 6 2017, 4:13 PM
quark added inline comments.
fastmanifest/implementation.py
1114

Does this mean each add has to re-lookup the path from starting from p1? If so, it's worth a comment here saying performance could be improved here.

quark accepted this revision.Nov 6 2017, 4:14 PM
This revision is now accepted and ready to land.Nov 6 2017, 4:14 PM
durham added inline comments.Nov 7 2017, 11:58 AM
fastmanifest/implementation.py
1114

I'm not sure what you mean? Re-lookup what path? You mean the delta chain? The caller is responsible for producing delta chains, so in this case we aren't doing any deltas at all.

quark added inline comments.Nov 7 2017, 5:43 PM
fastmanifest/implementation.py
1114

I thought inserting multiple paths like:

/a/b/c/d/1
/a/b/c/d/2

may require a/b/c/d lookups multiple times. Ideally it's:

x = lookup("/a/b/c/d")
x.add("1")
x.add("2")

But the underlying store may be smart enough so it handles this transparently. So I think it's fine to not change high-level code here.

Closed by commit rFBHGX6776e7a6e931: treemanifest: allow hybrid repos to make treeonly commits (authored by durham). · Explain WhyNov 8 2017, 6:28 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 3351

fastmanifest/implementation.py

self.ui.debug("[FM] wrote manifest %s\n" % (hexnode,)) self.ui.debug("[FM] wrote manifest %s\n" % (hexnode,))
else: else:
# If neither cache could help, fallback to the normal add # If neither cache could help, fallback to the normal add
node = orig(*args, **kwargs) node = orig(*args, **kwargs)
return node return node
def ctxwrite(self, orig, mfctx, transaction, link, p1, p2, added, removed): def ctxwrite(self, orig, mfctx, transaction, link, p1, p2, added, removed):
mfl = mfctx._manifestlog
treeenabled = self.ui.configbool("fastmanifest", "usetree")
if (supportsctree and treeenabled and
p1 not in mfl._revlog.nodemap and
not mfl.datastore.getmissing([('', p1)])):
# If p1 is not in the flat manifest but is in the tree store, then
# this is a commit on top of a tree only commit and we should then
# produce a treeonly commit.
node = None
else:
node = orig(mfctx, transaction, link, p1, p2, added, removed) node = orig(mfctx, transaction, link, p1, p2, added, removed)
treeenabled = self.ui.configbool("fastmanifest", "usetree")
if supportsctree and treeenabled: if supportsctree and treeenabled:
mfl = mfctx._manifestlog
datastore = mfl.datastore datastore = mfl.datastore
opener = mfctx._revlog().opener opener = mfctx._revlog().opener
m = mfctx._manifestdict m = mfctx._manifestdict
# hybridmanifest requires you provide either a value or loadflat. # hybridmanifest requires you provide either a value or loadflat.
# Let's give it a dummy value, since we know we'll only be calling # Let's give it a dummy value, since we know we'll only be calling
# _treemanifest() # _treemanifest()
def abort(tr): def abort(tr):
tr.treedatapack.abort() tr.treedatapack.abort()
tr.treehistpack.abort() tr.treehistpack.abort()
transaction.addfinalize('treepack', finalize) transaction.addfinalize('treepack', finalize)
transaction.addabort('treepack', abort) transaction.addabort('treepack', abort)
transaction.addpending('treepack', writepending) transaction.addpending('treepack', writepending)
# If the manifest was already committed as a flat manifest, use
# its node.
if node is not None:
dpack = treemanifest.InterceptedMutableDataPack( dpack = treemanifest.InterceptedMutableDataPack(
transaction.treedatapack, transaction.treedatapack,
node, p1) node, p1)
hpack = treemanifest.InterceptedMutableHistoryPack( hpack = treemanifest.InterceptedMutableHistoryPack(
transaction.treehistpack, node, p1) transaction.treehistpack, node, p1)
else:
dpack = transaction.treedatapack
hpack = transaction.treehistpack
newtreeiter = newtree.finalize(tree) newtreeiter = newtree.finalize(tree)
for nname, nnode, ntext, np1text, np1, np2 in newtreeiter: for nname, nnode, ntext, np1text, np1, np2 in newtreeiter:
# If the node wasn't set by a flat manifest, use the tree
# root node.
if node is None and nname == '':
node = nnode
# Not using deltas, since there aren't any other trees in # Not using deltas, since there aren't any other trees in
# this pack it could delta against. # this pack it could delta against.
dpack.add(nname, nnode, revlog.nullid, ntext) dpack.add(nname, nnode, revlog.nullid, ntext)
quarkUnsubmitted
Not Done

Does this mean each add has to re-lookup the path from starting from p1? If so, it's worth a comment here saying performance could be improved here.

quark: Does this mean each `add` has to re-lookup the path from starting from `p1`? If so, it's worth…
durhamAuthorUnsubmitted
Not Done

I'm not sure what you mean? Re-lookup what path? You mean the delta chain? The caller is responsible for producing delta chains, so in this case we aren't doing any deltas at all.

durham: I'm not sure what you mean? Re-lookup what path? You mean the delta chain? The caller is…
quarkUnsubmitted
Not Done

I thought inserting multiple paths like:

/a/b/c/d/1
/a/b/c/d/2

may require a/b/c/d lookups multiple times. Ideally it's:

x = lookup("/a/b/c/d")
x.add("1")
x.add("2")

But the underlying store may be smart enough so it handles this transparently. So I think it's fine to not change high-level code here.

quark: I thought inserting multiple paths like: /a/b/c/d/1 /a/b/c/d/2 may require `a/b/c/d`…
hpack.add(nname, nnode, np1, np2, revlog.nullid, '') hpack.add(nname, nnode, np1, np2, revlog.nullid, '')
treemanifestcache.getinstance(opener, treemanifestcache.getinstance(opener,
self.ui)[node] = newtree self.ui)[node] = newtree
return node return node
def _silent_debug(*args, **kwargs): def _silent_debug(*args, **kwargs):
"""Replacement for ui.debug that silently swallows the arguments. """Replacement for ui.debug that silently swallows the arguments.
Typically enabled when running the mercurial test suite by setting: Typically enabled when running the mercurial test suite by setting:
--extra-config-opt=fastmanifest.silent=True""" --extra-config-opt=fastmanifest.silent=True"""

tests/test-treemanifest-treeonly.t

saved backup bundle to $TESTTMP/client/.hg/strip-backup/38a88da6315b-320da9c1-histedit.hg (glob) saved backup bundle to $TESTTMP/client/.hg/strip-backup/38a88da6315b-320da9c1-histedit.hg (glob)
$ hg log -l 2 -G -T '{desc}' $ hg log -l 2 -G -T '{desc}'
@ hybrid flat+tree commit @ hybrid flat+tree commit
| |
o add y o add y
| |
~ ~
Test turning treeonly off and making sure we can still commit on top of treeonly
commits
$ echo >> subdir/x
$ hg debugindex -m --config treemanifest.treeonly=False | tail -1
2 102 51 -1 2 0427baa4e948 85b359fdb09e 000000000000
$ hg commit -m 'treeonly from hybrid repo' --config treemanifest.treeonly=False
$ hg log -r . -T '{desc}\n' --stat
treeonly from hybrid repo
subdir/x | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
$ hg log -r . -T '{desc}\n' --stat --config treemanifest.treeonly=False
treeonly from hybrid repo
subdir/x | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
$ hg debugindex -m --config treemanifest.treeonly=False | tail -1
2 102 51 -1 2 0427baa4e948 85b359fdb09e 000000000000
$ hg strip -r . --config fbamend.safestrip=False
1 files updated, 0 files merged, 0 files removed, 0 files unresolved
saved backup bundle to $TESTTMP/client/.hg/strip-backup/87da9865954c-3cfa5389-backup.hg (glob)
Test peer-to-peer push/pull of tree only commits Test peer-to-peer push/pull of tree only commits
$ cd .. $ cd ..
$ clearcache $ clearcache
$ hgcloneshallow ssh://user@dummy/master client2 -q --config treemanifest.treeonly=True --config extensions.treemanifest= $ hgcloneshallow ssh://user@dummy/master client2 -q --config treemanifest.treeonly=True --config extensions.treemanifest=
2 trees fetched over * (glob) 2 trees fetched over * (glob)
1 trees fetched over * (glob) 1 trees fetched over * (glob)
1 files fetched over 1 fetches - (1 misses, 0.00% hit ratio) over * (glob) 1 files fetched over 1 fetches - (1 misses, 0.00% hit ratio) over * (glob)
$ cd client2 $ cd client2