D5406: wireprotov2: unify file revision collection and linknode derivation

[indygreg (Gregory Szorc)]

This revision was automatically updated to reflect the committed changes.
Closed by commit rHG08cfa77d7288: wireprotov2: unify file revision collection and linknode derivation (authored by indygreg, committed by ).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D5406?vs=12811&id=12818

REVISION DETAIL
  https://phab.mercurial-scm.org/D5406

AFFECTED FILES
  mercurial/help/internals/wireprotocolv2.txt
  mercurial/wireprotov2server.py
  tests/test-wireproto-command-filesdata.t
  tests/test-wireproto-exchangev2.t

CHANGE DETAILS

diff --git a/tests/test-wireproto-exchangev2.t b/tests/test-wireproto-exchangev2.t
--- a/tests/test-wireproto-exchangev2.t
+++ b/tests/test-wireproto-exchangev2.t
@@ -1300,21 +1300,6 @@
 
 #if reporevlogstore
   $ hg -R client-linknode-2 debugrevlogindex dupe-file
-  abort: revlog 'dupe-file' not found
-  [255]
+     rev linkrev nodeid       p1           p2
+       0       2 2ed2a3912a0b 000000000000 000000000000
 #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 at 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]
diff --git a/tests/test-wireproto-command-filesdata.t b/tests/test-wireproto-command-filesdata.t
--- a/tests/test-wireproto-command-filesdata.t
+++ b/tests/test-wireproto-command-filesdata.t
@@ -1267,8 +1267,6 @@
     }
   ]
 
-TODO this is buggy
-
   $ sendhttpv2peer << EOF
   > command filesdata
   >     revisions eval:[{
@@ -1284,8 +1282,16 @@
   sending filesdata command
   response: gen[
     {
-      b'totalitems': 0,
-      b'totalpaths': 0
+      b'totalitems': 1,
+      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'
     }
   ]
 
diff --git a/mercurial/wireprotov2server.py b/mercurial/wireprotov2server.py
--- a/mercurial/wireprotov2server.py
+++ b/mercurial/wireprotov2server.py
@@ -1156,48 +1156,38 @@
     # changeset, it should probably be allowed to access files data for that
     # changeset.
 
-    cl = repo.changelog
-    clnode = cl.node
     outgoing = resolvenodes(repo, revisions)
     filematcher = makefilematcher(repo, pathfilter)
 
-    # Figure out what needs to be emitted.
-    changedpaths = set()
     # path -> {fnode: linknode}
     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:
         ctx = repo[node]
-        changedpaths.update(ctx.files())
-
-    changedpaths = sorted(p for p in changedpaths if filematcher(p))
+        mctx = ctx.manifestctx()
+        md = mctx.read()
 
-    # If ancestors are known, we send file revisions having a linkrev in the
-    # outgoing set of changeset revisions.
-    if haveparents:
-        outgoingclrevs = set(cl.rev(n) for n in outgoing)
-
-        for path in changedpaths:
-            try:
-                store = getfilestore(repo, proto, path)
-            except FileAccessError as e:
-                raise error.WireprotoCommandError(e.msg, e.args)
+        if haveparents:
+            checkpaths = ctx.files()
+        else:
+            checkpaths = md.keys()
 
-            for rev in store:
-                linkrev = store.linkrev(rev)
-
-                if linkrev in outgoingclrevs:
-                    fnodes[path].setdefault(store.node(rev), clnode(linkrev))
+        for path in checkpaths:
+            fnode = md[path]
 
-    # If ancestors aren't known, we walk the manifests and send all
-    # encountered file revisions.
-    else:
-        for node in outgoing:
-            mctx = repo[node].manifestctx()
+            if path in fnodes and fnode in fnodes[path]:
+                continue
 
-            for path, fnode in mctx.read().items():
-                if filematcher(path):
-                    fnodes[path].setdefault(fnode, node)
+            if not filematcher(path):
+                continue
+
+            fnodes[path].setdefault(fnode, node)
 
     yield {
         b'totalpaths': len(fnodes),
diff --git a/mercurial/help/internals/wireprotocolv2.txt b/mercurial/help/internals/wireprotocolv2.txt
--- a/mercurial/help/internals/wireprotocolv2.txt
+++ b/mercurial/help/internals/wireprotocolv2.txt
@@ -426,8 +426,10 @@
 has no file revisions data. This means that all referenced file revisions
 in the queried set of changeset revisions will be sent.
 
-TODO we'll probably want a more complicated mechanism for the client to
-specify which ancestor revisions are known.
+TODO we want a more complicated mechanism for the client to specify which
+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
 marked as introducing a file revision, since this can occur with e.g. hidden
 changesets.



To: indygreg, #hg-reviewers
Cc: mjpieters, mercurial-devel