Patchwork memctx: fix memctx manifest file hashes

login
register
mail settings
Submitter Durham Goode
Date Feb. 4, 2016, 1:44 a.m.
Message ID <5211c5eee88649263624.1454550281@dev8486.prn1.facebook.com>
Download mbox | patch
Permalink /patch/12969/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Durham Goode - Feb. 4, 2016, 1:44 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1454550251 28800
#      Wed Feb 03 17:44:11 2016 -0800
# Node ID 5211c5eee88649263624a82e65dc53e6f3882ba3
# Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
memctx: fix memctx manifest file hashes

When memctx is asked for a manifest, it constructs one by merging the p1
manifest, and the changes that are on top. For the changes on top, it was
previously using p1.node() as the file entries parent, which actually returns
the commit node that the p1 linkrev points at! Which is entirely incorrect.

The fix is to use p1.filenode() instead, which returns the parent file node as
desired.

I don't know how to execute this or make it have a visible effect, so I'm not
sure how to test it.  It was noticed because asking for the linkrev is an
expensive operation when using the remotefilelog extension and this was causing
performance regressions with commit.
Sean Farley - Feb. 4, 2016, 1:54 a.m.
Durham Goode <durham@fb.com> writes:

> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1454550251 28800
> #      Wed Feb 03 17:44:11 2016 -0800
> # Node ID 5211c5eee88649263624a82e65dc53e6f3882ba3
> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
> memctx: fix memctx manifest file hashes
>
> When memctx is asked for a manifest, it constructs one by merging the p1
> manifest, and the changes that are on top. For the changes on top, it was
> previously using p1.node() as the file entries parent, which actually returns
> the commit node that the p1 linkrev points at! Which is entirely incorrect.
>
> The fix is to use p1.filenode() instead, which returns the parent file node as
> desired.
>
> I don't know how to execute this or make it have a visible effect, so I'm not
> sure how to test it.  It was noticed because asking for the linkrev is an
> expensive operation when using the remotefilelog extension and this was causing
> performance regressions with commit.

Huh. This is almost certainly my fault for misunderstanding node vs
filenode. I just ran this patch against our internal memctx at Bitbucket
and it passed this change seems fine to me.
Yuya Nishihara - Feb. 5, 2016, 2:58 p.m.
On Wed, 3 Feb 2016 17:44:41 -0800, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1454550251 28800
> #      Wed Feb 03 17:44:11 2016 -0800
> # Node ID 5211c5eee88649263624a82e65dc53e6f3882ba3
> # Parent  01a5143cd25f285f8c745a92986cd7186bb32c90
> memctx: fix memctx manifest file hashes

Pushed to the clowncopter as per Sean's review, thanks.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1892,9 +1892,9 @@  class memctx(committablectx):
             p2node = nullid
             p = pctx[f].parents() # if file isn't in pctx, check p2?
             if len(p) > 0:
-                p1node = p[0].node()
+                p1node = p[0].filenode()
                 if len(p) > 1:
-                    p2node = p[1].node()
+                    p2node = p[1].filenode()
             man[f] = revlog.hash(self[f].data(), p1node, p2node)
 
         for f in self._status.added: