This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5406

wireprotov2: unify file revision collection and linknode derivation
ClosedPublic

Authored by indygreg on Dec 10 2018, 2:43 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG08cfa77d7288: wireprotov2: unify file revision collection and linknode derivation
Summary

The old mechanism for choosing which file revisions to send in the
haveparents=True case was buggy in multiple ways - the most severe
of which being that file revisions were excluded when they shouldn't
have been.

This commit unifies the logic for deriving the filenodes that will
be sent by the "filesdata" command. We now consistently read files
data from manifests. The "haveparents" argument now controls whether
we iterate ctx.files() or use the full manifest to derive relevant
files.

The logic here is still woefully lacking to fully support shallow
clones. It will require an API break to fully address. This commit
should at least make the server APIs emit proper data, which is
strictly better than before.

Diff Detail

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

Event Timeline

indygreg created this revision.Dec 10 2018, 2:43 PM
Closed by commit rHG08cfa77d7288: wireprotov2: unify file revision collection and linknode derivation (authored by indygreg). · Explain WhyDec 10 2018, 4:25 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 12818

mercurial/help/internals/wireprotocolv2.txt

requested. In other words, the command may assume that all file revisions requested. In other words, the command may assume that all file revisions
for all relevant paths for ancestors of the requested changeset revisions for all relevant paths for ancestors of the requested changeset revisions
are present on the receiver. are present on the receiver.
When ``haveparents`` is false, the command MUST assume that the receiver When ``haveparents`` is false, the command MUST assume that the receiver
has no file revisions data. This means that all referenced file revisions has no file revisions data. This means that all referenced file revisions
in the queried set of changeset revisions will be sent. in the queried set of changeset revisions will be sent.
TODO we'll probably want a more complicated mechanism for the client to TODO we want a more complicated mechanism for the client to specify which
specify which ancestor revisions are known. ancestor revisions are known. This is needed so intelligent deltas can be
emitted and so updated linknodes can be sent if the client needs to adjust
its linknodes for existing file nodes to older changeset revisions.
TODO we may want to make linknodes an array so multiple changesets can be TODO we may want to make linknodes an array so multiple changesets can be
marked as introducing a file revision, since this can occur with e.g. hidden marked as introducing a file revision, since this can occur with e.g. hidden
changesets. changesets.
heads heads
----- -----
Obtain DAG heads in the repository. Obtain DAG heads in the repository.

mercurial/wireprotov2server.py

extracapabilitiesfn=filesdatacapabilities) extracapabilitiesfn=filesdatacapabilities)
def filesdata(repo, proto, haveparents, fields, pathfilter, revisions): def filesdata(repo, proto, haveparents, fields, pathfilter, revisions):
# TODO This should operate on a repo that exposes obsolete changesets. There # TODO This should operate on a repo that exposes obsolete changesets. There
# is a race between a client making a push that obsoletes a changeset and # is a race between a client making a push that obsoletes a changeset and
# another client fetching files data for that changeset. If a client has a # another client fetching files data for that changeset. If a client has a
# changeset, it should probably be allowed to access files data for that # changeset, it should probably be allowed to access files data for that
# changeset. # changeset.
cl = repo.changelog
clnode = cl.node
outgoing = resolvenodes(repo, revisions) outgoing = resolvenodes(repo, revisions)
filematcher = makefilematcher(repo, pathfilter) filematcher = makefilematcher(repo, pathfilter)
# Figure out what needs to be emitted.
changedpaths = set()
# path -> {fnode: linknode} # path -> {fnode: linknode}
fnodes = collections.defaultdict(dict) fnodes = collections.defaultdict(dict)
# We collect the set of relevant file revisions by iterating the changeset
# revisions and either walking the set of files recorded in the changeset
# or by walking the manifest at that revision. There is probably room for a
# storage-level API to request this data, as it can be expensive to compute
# and would benefit from caching or alternate storage from what revlogs
# provide.
for node in outgoing: for node in outgoing:
ctx = repo[node] ctx = repo[node]
changedpaths.update(ctx.files()) mctx = ctx.manifestctx()
md = mctx.read()
changedpaths = sorted(p for p in changedpaths if filematcher(p))
# If ancestors are known, we send file revisions having a linkrev in the
# outgoing set of changeset revisions.
if haveparents: if haveparents:
outgoingclrevs = set(cl.rev(n) for n in outgoing) checkpaths = ctx.files()
else:
for path in changedpaths: checkpaths = md.keys()
try:
store = getfilestore(repo, proto, path)
except FileAccessError as e:
raise error.WireprotoCommandError(e.msg, e.args)
for rev in store: for path in checkpaths:
linkrev = store.linkrev(rev) fnode = md[path]
if linkrev in outgoingclrevs: if path in fnodes and fnode in fnodes[path]:
fnodes[path].setdefault(store.node(rev), clnode(linkrev)) continue
# If ancestors aren't known, we walk the manifests and send all if not filematcher(path):
# encountered file revisions. continue
else:
for node in outgoing:
mctx = repo[node].manifestctx()
for path, fnode in mctx.read().items():
if filematcher(path):
fnodes[path].setdefault(fnode, node) fnodes[path].setdefault(fnode, node)
yield { yield {
b'totalpaths': len(fnodes), b'totalpaths': len(fnodes),
b'totalitems': sum(len(v) for v in fnodes.values()) b'totalitems': sum(len(v) for v in fnodes.values())
} }
for path, filenodes in sorted(fnodes.items()): for path, filenodes in sorted(fnodes.items()):
try: try:

tests/test-wireproto-command-filesdata.t

b'totalitems': 1 b'totalitems': 1
}, },
{ {
b'linknode': b'G\xfc0X\t\x11#,\xb2dg[@(\x19\xde\xdd\xf6\xc6\xf0', b'linknode': b'G\xfc0X\t\x11#,\xb2dg[@(\x19\xde\xdd\xf6\xc6\xf0',
b'node': b'.\xd2\xa3\x91*\x0b$P C\xea\xe8N\xe4\xb2y\xc1\x8b\x90\xdd' b'node': b'.\xd2\xa3\x91*\x0b$P C\xea\xe8N\xe4\xb2y\xc1\x8b\x90\xdd'
} }
] ]
TODO this is buggy
$ sendhttpv2peer << EOF $ sendhttpv2peer << EOF
> command filesdata > command filesdata
> revisions eval:[{ > revisions eval:[{
> b'type': b'changesetexplicit', > b'type': b'changesetexplicit',
> b'nodes': [ > b'nodes': [
> b'\x47\xfc\x30\x58\x09\x11\x23\x2c\xb2\x64\x67\x5b\x40\x28\x19\xde\xdd\xf6\xc6\xf0', > b'\x47\xfc\x30\x58\x09\x11\x23\x2c\xb2\x64\x67\x5b\x40\x28\x19\xde\xdd\xf6\xc6\xf0',
> ]}] > ]}]
> fields eval:[b'linknode'] > fields eval:[b'linknode']
> haveparents eval:True > haveparents eval:True
> pathfilter eval:{b'include': [b'path:dupe-file']} > pathfilter eval:{b'include': [b'path:dupe-file']}
> EOF > EOF
creating http peer for wire protocol version 2 creating http peer for wire protocol version 2
sending filesdata command sending filesdata command
response: gen[ response: gen[
{ {
b'totalitems': 0, b'totalitems': 1,
b'totalpaths': 0 b'totalpaths': 1
},
{
b'path': b'dupe-file',
b'totalitems': 1
},
{
b'linknode': b'G\xfc0X\t\x11#,\xb2dg[@(\x19\xde\xdd\xf6\xc6\xf0',
b'node': b'.\xd2\xa3\x91*\x0b$P C\xea\xe8N\xe4\xb2y\xc1\x8b\x90\xdd'
} }
] ]
$ cat error.log $ cat error.log

tests/test-wireproto-exchangev2.t

$ hg -R client-linknode-2 pull -r 639c8990d6a5 $ hg -R client-linknode-2 pull -r 639c8990d6a5
pulling from http://localhost:$HGPORT/ pulling from http://localhost:$HGPORT/
searching for changes searching for changes
new changesets 639c8990d6a5 new changesets 639c8990d6a5
(run 'hg update' to get a working copy) (run 'hg update' to get a working copy)
#if reporevlogstore #if reporevlogstore
$ hg -R client-linknode-2 debugrevlogindex dupe-file $ hg -R client-linknode-2 debugrevlogindex dupe-file
abort: revlog 'dupe-file' not found rev linkrev nodeid p1 p2
[255] 0 2 2ed2a3912a0b 000000000000 000000000000
#endif #endif
$ hg -R client-linknode-2 verify
checking changesets
checking manifests
crosschecking files in changesets and manifests
checking files
warning: revlog 'data/dupe-file.i' not in fncache!
2: empty or missing dupe-file
dupe-file@2: manifest refers to unknown revision 2ed2a3912a0b
checked 3 changesets with 2 changes to 3 files
1 warnings encountered!
hint: run "hg debugrebuildfncache" to recover from corrupt fncache
2 integrity errors encountered!
(first damaged changeset appears to be 2)
[1]