D4088: changegroup: move revision maps to cgpacker

[indygreg (Gregory Szorc)]

indygreg added subscribers: spectral, martinvonz, durin42.
indygreg added inline comments.

INLINE COMMENTS

> changegroup.py:599-600
> +if self._nextclrevtolocalrev:
> +self.clrevtolocalrev = self._nextclrevtolocalrev
> +self._nextclrevtolocalrev.clear()
>  self._changelogdone = True

There are 2 bugs here.

One is dropping the `_` prefix from `clrevtolocalrev`.

The other is aliasing the variables then calling `.clear()`, which has the 
effect of undoing both and effectively nerfing `self._nextclrevtolocalrev`.

Surprisingly no tests failed as a result of these bugs. So I'm not sure what's 
going on.

Maybe @spectral, @martinvonz, or @durin42 know what's up.

REPOSITORY
  rHG Mercurial

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

To: indygreg, #hg-reviewers
Cc: durin42, martinvonz, spectral, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D4088: changegroup: move revision maps to cgpacker

[indygreg (Gregory Szorc)]

This revision was automatically updated to reflect the committed changes.
Closed by commit rHG0548f696795b: changegroup: move revision maps to cgpacker 
(authored by indygreg, committed by ).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D4088?vs=9850=9964

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

AFFECTED FILES
  mercurial/changegroup.py

CHANGE DETAILS

diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
--- a/mercurial/changegroup.py
+++ b/mercurial/changegroup.py
@@ -583,12 +583,21 @@
 # controlled via arguments to group() that influence behavior.
 self._changelogdone = False
 
+# Maps CL revs to per-revlog revisions. Cleared in close() at
+# the end of each group.
+self._clrevtolocalrev = {}
+self._nextclrevtolocalrev = {}
+
+# Maps changelog nodes to changelog revs. Filled in once
+# during changelog stage and then left unmodified.
+self._clnodetorev = {}
+
 def _close(self):
 # Ellipses serving mode.
-getattr(self, '_clrev_to_localrev', {}).clear()
-if getattr(self, '_next_clrev_to_localrev', {}):
-self._clrev_to_localrev = self._next_clrev_to_localrev
-del self._next_clrev_to_localrev
+self._clrevtolocalrev.clear()
+if self._nextclrevtolocalrev:
+self.clrevtolocalrev = self._nextclrevtolocalrev
+self._nextclrevtolocalrev.clear()
 self._changelogdone = True
 
 return closechunk()
@@ -615,8 +624,8 @@
 # order that they're introduced in dramatis personae by the
 # changelog, so what we do is we sort the non-changelog histories
 # by the order in which they are used by the changelog.
-if util.safehasattr(self, '_full_nodes') and self._clnode_to_rev:
-key = lambda n: self._clnode_to_rev[lookup(n)]
+if util.safehasattr(self, '_full_nodes') and self._clnodetorev:
+key = lambda n: self._clnodetorev[lookup(n)]
 return [store.rev(n) for n in sorted(nodelist, key=key)]
 
 # for generaldelta revlogs, we linearize the revs; this will both be
@@ -740,8 +749,8 @@
 # manifest revnum to look up for this cl revnum. (Part of
 # mapping changelog ellipsis parents to manifest ellipsis
 # parents)
-self._next_clrev_to_localrev.setdefault(cl.rev(x),
-mfrevlog.rev(n))
+self._nextclrevtolocalrev.setdefault(cl.rev(x),
+ mfrevlog.rev(n))
 # We can't trust the changed files list in the changeset if the
 # client requested a shallow clone.
 if self._isshallow:
@@ -918,7 +927,7 @@
 for c in commonctxs:
 try:
 fnode = c.filenode(fname)
-self._clrev_to_localrev[c.rev()] = flog.rev(fnode)
+self._clrevtolocalrev[c.rev()] = flog.rev(fnode)
 except error.ManifestLookupError:
 pass
 links = oldlinknodes(flog, fname)
@@ -1063,12 +1072,12 @@
 # build up some mapping information that's useful later. See
 # the local() nested function below.
 if not self._changelogdone:
-self._clnode_to_rev[linknode] = rev
+self._clnodetorev[linknode] = rev
 linkrev = rev
-self._clrev_to_localrev[linkrev] = rev
+self._clrevtolocalrev[linkrev] = rev
 else:
-linkrev = self._clnode_to_rev[linknode]
-self._clrev_to_localrev[linkrev] = rev
+linkrev = self._clnodetorev[linknode]
+self._clrevtolocalrev[linkrev] = rev
 
 # This is a node to send in full, because the changeset it
 # corresponds to was a full changeset.
@@ -1100,9 +1109,9 @@
 # need to store some extra mapping information so that
 # our contained ellipsis nodes will be able to resolve
 # their parents.
-if clrev not in self._clrev_to_localrev:
+if clrev not in self._clrevtolocalrev:
 clnode = store.node(clrev)
-self._clnode_to_rev[clnode] = clrev
+self._clnodetorev[clnode] = clrev
 return clrev
 
 # Walk the ellipsis-ized changelog breadth-first looking for a
@@ -1119,8 +1128,8 @@
 while walk:
 p = walk[0]
 walk = walk[1:]
-if p in self._clrev_to_localrev:
-return self._clrev_to_localrev[p]
+if p in self._clrevtolocalrev:
+return self._clrevtolocalrev[p]
 elif p in self._full_nodes:
 

D4088: changegroup: move revision maps to cgpacker

[indygreg (Gregory Szorc)]

indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  And remove the underscores so the variables conform to our
  naming convention.
  
  The logic in _close() should be the only thing warranting scrutiny
  during review.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/changegroup.py

CHANGE DETAILS

diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
--- a/mercurial/changegroup.py
+++ b/mercurial/changegroup.py
@@ -586,12 +586,21 @@
 # controlled via arguments to group() that influence behavior.
 self._changelogdone = False
 
+# Maps CL revs to per-revlog revisions. Cleared in close() at
+# the end of each group.
+self._clrevtolocalrev = {}
+self._nextclrevtolocalrev = {}
+
+# Maps changelog nodes to changelog revs. Filled in once
+# during changelog stage and then left unmodified.
+self._clnodetorev = {}
+
 def _close(self):
 # Ellipses serving mode.
-getattr(self, '_clrev_to_localrev', {}).clear()
-if getattr(self, '_next_clrev_to_localrev', {}):
-self._clrev_to_localrev = self._next_clrev_to_localrev
-del self._next_clrev_to_localrev
+self._clrevtolocalrev.clear()
+if self._nextclrevtolocalrev:
+self.clrevtolocalrev = self._nextclrevtolocalrev
+self._nextclrevtolocalrev.clear()
 self._changelogdone = True
 
 return closechunk()
@@ -618,8 +627,8 @@
 # order that they're introduced in dramatis personae by the
 # changelog, so what we do is we sort the non-changelog histories
 # by the order in which they are used by the changelog.
-if util.safehasattr(self, '_full_nodes') and self._clnode_to_rev:
-key = lambda n: self._clnode_to_rev[lookup(n)]
+if util.safehasattr(self, '_full_nodes') and self._clnodetorev:
+key = lambda n: self._clnodetorev[lookup(n)]
 return [store.rev(n) for n in sorted(nodelist, key=key)]
 
 # for generaldelta revlogs, we linearize the revs; this will both be
@@ -743,8 +752,8 @@
 # manifest revnum to look up for this cl revnum. (Part of
 # mapping changelog ellipsis parents to manifest ellipsis
 # parents)
-self._next_clrev_to_localrev.setdefault(cl.rev(x),
-mfrevlog.rev(n))
+self._nextclrevtolocalrev.setdefault(cl.rev(x),
+ mfrevlog.rev(n))
 # We can't trust the changed files list in the changeset if the
 # client requested a shallow clone.
 if self._isshallow:
@@ -921,7 +930,7 @@
 for c in commonctxs:
 try:
 fnode = c.filenode(fname)
-self._clrev_to_localrev[c.rev()] = flog.rev(fnode)
+self._clrevtolocalrev[c.rev()] = flog.rev(fnode)
 except error.ManifestLookupError:
 pass
 links = oldlinknodes(flog, fname)
@@ -1066,12 +1075,12 @@
 # build up some mapping information that's useful later. See
 # the local() nested function below.
 if not self._changelogdone:
-self._clnode_to_rev[linknode] = rev
+self._clnodetorev[linknode] = rev
 linkrev = rev
-self._clrev_to_localrev[linkrev] = rev
+self._clrevtolocalrev[linkrev] = rev
 else:
-linkrev = self._clnode_to_rev[linknode]
-self._clrev_to_localrev[linkrev] = rev
+linkrev = self._clnodetorev[linknode]
+self._clrevtolocalrev[linkrev] = rev
 
 # This is a node to send in full, because the changeset it
 # corresponds to was a full changeset.
@@ -1103,9 +1112,9 @@
 # need to store some extra mapping information so that
 # our contained ellipsis nodes will be able to resolve
 # their parents.
-if clrev not in self._clrev_to_localrev:
+if clrev not in self._clrevtolocalrev:
 clnode = store.node(clrev)
-self._clnode_to_rev[clnode] = clrev
+self._clnodetorev[clnode] = clrev
 return clrev
 
 # Walk the ellipsis-ized changelog breadth-first looking for a
@@ -1122,8 +1131,8 @@
 while walk:
 p = walk[0]
 walk = walk[1:]
-if p in self._clrev_to_localrev:
-return self._clrev_to_localrev[p]
+if p in self._clrevtolocalrev:
+return self._clrevtolocalrev[p]
 elif p in