Patchwork [1,of,5,v4] revlog: merge hash checking subfunctions

login
register
mail settings
Submitter Remi Chaintron
Date Nov. 23, 2016, 5:39 p.m.
Message ID <e908dd63d485424df4c4.1479922776@iphonesleepsort.dhcp.thefacebook.com>
Download mbox | patch
Permalink /patch/17724/
State Changes Requested
Headers show

Comments

Remi Chaintron - Nov. 23, 2016, 5:39 p.m.
# HG changeset patch
# User Remi Chaintron <remi@fb.com>
# Date 1479916365 0
#      Wed Nov 23 15:52:45 2016 +0000
# Branch stable
# Node ID e908dd63d485424df4c4a4482b742d82652e2893
# Parent  24bcb76c5e6633e740f7dcec8f1ca96b06bcc536
revlog: merge hash checking subfunctions

This patch factors the behavior of both methods into 'checkhash' and returns the
text in order to allow subclasses of revlog and extensions to extend hash
computation and handle hash mismatches.
Pierre-Yves David - Nov. 24, 2016, 4:30 p.m.
On 11/23/2016 06:39 PM, Remi Chaintron wrote:
> # HG changeset patch
> # User Remi Chaintron <remi@fb.com>
> # Date 1479916365 0
> #      Wed Nov 23 15:52:45 2016 +0000
> # Branch stable
> # Node ID e908dd63d485424df4c4a4482b742d82652e2893
> # Parent  24bcb76c5e6633e740f7dcec8f1ca96b06bcc536
> revlog: merge hash checking subfunctions
>
> This patch factors the behavior of both methods into 'checkhash' and returns the
> text in order to allow subclasses of revlog and extensions to extend hash
> computation and handle hash mismatches.

Having something named "checkhash" return the text seems strange to me. 
to me, the 'check' part of the name implies that the function is read 
only//checking a state no extra logic should apply here,

If I remember our sprint discussion right, this text return have been 
introduced for censors. I can think of two ways to move forward, either 
change the 'checkhash' name to refer to this text computation part or 
changing censors to not abuse the 'check' method. What do you think?

Cheers,

> diff --git a/contrib/perf.py b/contrib/perf.py
> --- a/contrib/perf.py
> +++ b/contrib/perf.py
> @@ -861,7 +861,7 @@
>      def dohash(text):
>          if not cache:
>              r.clearcaches()
> -        r._checkhash(text, node, rev)
> +        r.checkhash(text, node, rev=rev)
>
>      def dorevision():
>          if not cache:
> diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
> --- a/mercurial/bundlerepo.py
> +++ b/mercurial/bundlerepo.py
> @@ -147,7 +147,7 @@
>              delta = self._chunk(chain.pop())
>              text = mdiff.patches(text, [delta])
>
> -        self._checkhash(text, node, rev)
> +        self.checkhash(text, node, rev=rev)
>          self._cache = (node, rev, text)
>          return text
>
> diff --git a/mercurial/filelog.py b/mercurial/filelog.py
> --- a/mercurial/filelog.py
> +++ b/mercurial/filelog.py
> @@ -104,9 +104,9 @@
>
>          return True
>
> -    def checkhash(self, text, p1, p2, node, rev=None):
> +    def checkhash(self, text, node, p1=None, p2=None, rev=None):
>          try:
> -            super(filelog, self).checkhash(text, p1, p2, node, rev=rev)
> +            super(filelog, self).checkhash(text, node, p1=p1, p2=p2, rev=rev)
>          except error.RevlogError:
>              if _censoredtext(text):
>                  raise error.CensoredNodeError(self.indexfile, node, text)
> diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -1221,9 +1221,7 @@
>              bins = bins[1:]
>
>          text = mdiff.patches(text, bins)
> -
> -        text = self._checkhash(text, node, rev)
> -
> +        self.checkhash(text, node, rev=rev)
>          self._cache = (node, rev, text)
>          return text
>
> @@ -1235,12 +1233,14 @@
>          """
>          return hash(text, p1, p2)
>
> -    def _checkhash(self, text, node, rev):
> -        p1, p2 = self.parents(node)
> -        self.checkhash(text, p1, p2, node, rev)
> -        return text
> +    def checkhash(self, text, node, p1=None, p2=None, rev=None):
> +        """Check node hash integrity.
>
> -    def checkhash(self, text, p1, p2, node, rev=None):
> +        Available as a function so that subclasses can extend hash mismatch
> +        behaviors as needed.
> +        """
> +        if p1 is None and p2 is None:
> +            p1, p2 = self.parents(node)
>          if node != self.hash(text, p1, p2):
>              revornode = rev
>              if revornode is None:
> @@ -1415,7 +1415,7 @@
>                  basetext = self.revision(self.node(baserev), _df=fh)
>                  btext[0] = mdiff.patch(basetext, delta)
>              try:
> -                self.checkhash(btext[0], p1, p2, node)
> +                self.checkhash(btext[0], node, p1=p1, p2=p2)
>                  if flags & REVIDX_ISCENSORED:
>                      raise RevlogError(_('node %s is not censored') % node)
>              except CensoredNodeError:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Rémi Chaintron - Nov. 24, 2016, 4:41 p.m.
On Thu, 24 Nov 2016 at 16:30 Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 11/23/2016 06:39 PM, Remi Chaintron wrote:
> > # HG changeset patch
> > # User Remi Chaintron <remi@fb.com>
> > # Date 1479916365 0
> > #      Wed Nov 23 15:52:45 2016 +0000
> > # Branch stable
> > # Node ID e908dd63d485424df4c4a4482b742d82652e2893
> > # Parent  24bcb76c5e6633e740f7dcec8f1ca96b06bcc536
> > revlog: merge hash checking subfunctions
> >
> > This patch factors the behavior of both methods into 'checkhash' and
> returns the
> > text in order to allow subclasses of revlog and extensions to extend hash
> > computation and handle hash mismatches.
>
> Having something named "checkhash" return the text seems strange to me.
> to me, the 'check' part of the name implies that the function is read
> only//checking a state no extra logic should apply here,
>
> If I remember our sprint discussion right, this text return have been
> introduced for censors. I can think of two ways to move forward, either
> change the 'checkhash' name to refer to this text computation part or
> changing censors to not abuse the 'check' method. What do you think?
>
>
This is a mistake on my part: Although the merge of the 'checkhash'
subfunctions has been updated to not return the text following your
feedback, I forgot to update the commit summary.


> Cheers,
>
> > diff --git a/contrib/perf.py b/contrib/perf.py
> > --- a/contrib/perf.py
> > +++ b/contrib/perf.py
> > @@ -861,7 +861,7 @@
> >      def dohash(text):
> >          if not cache:
> >              r.clearcaches()
> > -        r._checkhash(text, node, rev)
> > +        r.checkhash(text, node, rev=rev)
> >
> >      def dorevision():
> >          if not cache:
> > diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
> > --- a/mercurial/bundlerepo.py
> > +++ b/mercurial/bundlerepo.py
> > @@ -147,7 +147,7 @@
> >              delta = self._chunk(chain.pop())
> >              text = mdiff.patches(text, [delta])
> >
> > -        self._checkhash(text, node, rev)
> > +        self.checkhash(text, node, rev=rev)
> >          self._cache = (node, rev, text)
> >          return text
> >
> > diff --git a/mercurial/filelog.py b/mercurial/filelog.py
> > --- a/mercurial/filelog.py
> > +++ b/mercurial/filelog.py
> > @@ -104,9 +104,9 @@
> >
> >          return True
> >
> > -    def checkhash(self, text, p1, p2, node, rev=None):
> > +    def checkhash(self, text, node, p1=None, p2=None, rev=None):
> >          try:
> > -            super(filelog, self).checkhash(text, p1, p2, node, rev=rev)
> > +            super(filelog, self).checkhash(text, node, p1=p1, p2=p2,
> rev=rev)
> >          except error.RevlogError:
> >              if _censoredtext(text):
> >                  raise error.CensoredNodeError(self.indexfile, node,
> text)
> > diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> > --- a/mercurial/revlog.py
> > +++ b/mercurial/revlog.py
> > @@ -1221,9 +1221,7 @@
> >              bins = bins[1:]
> >
> >          text = mdiff.patches(text, bins)
> > -
> > -        text = self._checkhash(text, node, rev)
> > -
> > +        self.checkhash(text, node, rev=rev)
> >          self._cache = (node, rev, text)
> >          return text
> >
> > @@ -1235,12 +1233,14 @@
> >          """
> >          return hash(text, p1, p2)
> >
> > -    def _checkhash(self, text, node, rev):
> > -        p1, p2 = self.parents(node)
> > -        self.checkhash(text, p1, p2, node, rev)
> > -        return text
> > +    def checkhash(self, text, node, p1=None, p2=None, rev=None):
> > +        """Check node hash integrity.
> >
> > -    def checkhash(self, text, p1, p2, node, rev=None):
> > +        Available as a function so that subclasses can extend hash
> mismatch
> > +        behaviors as needed.
> > +        """
> > +        if p1 is None and p2 is None:
> > +            p1, p2 = self.parents(node)
> >          if node != self.hash(text, p1, p2):
> >              revornode = rev
> >              if revornode is None:
> > @@ -1415,7 +1415,7 @@
> >                  basetext = self.revision(self.node(baserev), _df=fh)
> >                  btext[0] = mdiff.patch(basetext, delta)
> >              try:
> > -                self.checkhash(btext[0], p1, p2, node)
> > +                self.checkhash(btext[0], node, p1=p1, p2=p2)
> >                  if flags & REVIDX_ISCENSORED:
> >                      raise RevlogError(_('node %s is not censored') %
> node)
> >              except CensoredNodeError:
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Rémi Chaintron - Nov. 28, 2016, 11:20 a.m.
Updated in local commit, should I wait for a complete review before sending
another patch?

On Thu, 24 Nov 2016 at 16:41 Rémi Chaintron <remi.chaintron@gmail.com>
wrote:

> On Thu, 24 Nov 2016 at 16:30 Pierre-Yves David <
> pierre-yves.david@ens-lyon.org> wrote:
>
>
>
> On 11/23/2016 06:39 PM, Remi Chaintron wrote:
> > # HG changeset patch
> > # User Remi Chaintron <remi@fb.com>
> > # Date 1479916365 0
> > #      Wed Nov 23 15:52:45 2016 +0000
> > # Branch stable
> > # Node ID e908dd63d485424df4c4a4482b742d82652e2893
> > # Parent  24bcb76c5e6633e740f7dcec8f1ca96b06bcc536
> > revlog: merge hash checking subfunctions
> >
> > This patch factors the behavior of both methods into 'checkhash' and
> returns the
> > text in order to allow subclasses of revlog and extensions to extend hash
> > computation and handle hash mismatches.
>
> Having something named "checkhash" return the text seems strange to me.
> to me, the 'check' part of the name implies that the function is read
> only//checking a state no extra logic should apply here,
>
> If I remember our sprint discussion right, this text return have been
> introduced for censors. I can think of two ways to move forward, either
> change the 'checkhash' name to refer to this text computation part or
> changing censors to not abuse the 'check' method. What do you think?
>
>
> This is a mistake on my part: Although the merge of the 'checkhash'
> subfunctions has been updated to not return the text following your
> feedback, I forgot to update the commit summary.
>
>
> Cheers,
>
> > diff --git a/contrib/perf.py b/contrib/perf.py
> > --- a/contrib/perf.py
> > +++ b/contrib/perf.py
> > @@ -861,7 +861,7 @@
> >      def dohash(text):
> >          if not cache:
> >              r.clearcaches()
> > -        r._checkhash(text, node, rev)
> > +        r.checkhash(text, node, rev=rev)
> >
> >      def dorevision():
> >          if not cache:
> > diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
> > --- a/mercurial/bundlerepo.py
> > +++ b/mercurial/bundlerepo.py
> > @@ -147,7 +147,7 @@
> >              delta = self._chunk(chain.pop())
> >              text = mdiff.patches(text, [delta])
> >
> > -        self._checkhash(text, node, rev)
> > +        self.checkhash(text, node, rev=rev)
> >          self._cache = (node, rev, text)
> >          return text
> >
> > diff --git a/mercurial/filelog.py b/mercurial/filelog.py
> > --- a/mercurial/filelog.py
> > +++ b/mercurial/filelog.py
> > @@ -104,9 +104,9 @@
> >
> >          return True
> >
> > -    def checkhash(self, text, p1, p2, node, rev=None):
> > +    def checkhash(self, text, node, p1=None, p2=None, rev=None):
> >          try:
> > -            super(filelog, self).checkhash(text, p1, p2, node, rev=rev)
> > +            super(filelog, self).checkhash(text, node, p1=p1, p2=p2,
> rev=rev)
> >          except error.RevlogError:
> >              if _censoredtext(text):
> >                  raise error.CensoredNodeError(self.indexfile, node,
> text)
> > diff --git a/mercurial/revlog.py b/mercurial/revlog.py
> > --- a/mercurial/revlog.py
> > +++ b/mercurial/revlog.py
> > @@ -1221,9 +1221,7 @@
> >              bins = bins[1:]
> >
> >          text = mdiff.patches(text, bins)
> > -
> > -        text = self._checkhash(text, node, rev)
> > -
> > +        self.checkhash(text, node, rev=rev)
> >          self._cache = (node, rev, text)
> >          return text
> >
> > @@ -1235,12 +1233,14 @@
> >          """
> >          return hash(text, p1, p2)
> >
> > -    def _checkhash(self, text, node, rev):
> > -        p1, p2 = self.parents(node)
> > -        self.checkhash(text, p1, p2, node, rev)
> > -        return text
> > +    def checkhash(self, text, node, p1=None, p2=None, rev=None):
> > +        """Check node hash integrity.
> >
> > -    def checkhash(self, text, p1, p2, node, rev=None):
> > +        Available as a function so that subclasses can extend hash
> mismatch
> > +        behaviors as needed.
> > +        """
> > +        if p1 is None and p2 is None:
> > +            p1, p2 = self.parents(node)
> >          if node != self.hash(text, p1, p2):
> >              revornode = rev
> >              if revornode is None:
> > @@ -1415,7 +1415,7 @@
> >                  basetext = self.revision(self.node(baserev), _df=fh)
> >                  btext[0] = mdiff.patch(basetext, delta)
> >              try:
> > -                self.checkhash(btext[0], p1, p2, node)
> > +                self.checkhash(btext[0], node, p1=p1, p2=p2)
> >                  if flags & REVIDX_ISCENSORED:
> >                      raise RevlogError(_('node %s is not censored') %
> node)
> >              except CensoredNodeError:
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
> --
> Rémi
>
Pierre-Yves David - Nov. 28, 2016, 1:35 p.m.
On 11/28/2016 12:20 PM, Rémi Chaintron wrote:
> Updated in local commit, should I wait for a complete review before
> sending another patch?

I've more feedback coming on patch 2 and patch 4. I'll post that a bit 
later today. (Also patch 5 seems to need an update of its description)

Cheers,

>
> On Thu, 24 Nov 2016 at 16:41 Rémi Chaintron <remi.chaintron@gmail.com
> <mailto:remi.chaintron@gmail.com>> wrote:
>
>     On Thu, 24 Nov 2016 at 16:30 Pierre-Yves David
>     <pierre-yves.david@ens-lyon.org
>     <mailto:pierre-yves.david@ens-lyon.org>> wrote:
>
>
>
>         On 11/23/2016 06:39 PM, Remi Chaintron wrote:
>         > # HG changeset patch
>         > # User Remi Chaintron <remi@fb.com <mailto:remi@fb.com>>
>         > # Date 1479916365 0
>         > #      Wed Nov 23 15:52:45 2016 +0000
>         > # Branch stable
>         > # Node ID e908dd63d485424df4c4a4482b742d82652e2893
>         > # Parent  24bcb76c5e6633e740f7dcec8f1ca96b06bcc536
>         > revlog: merge hash checking subfunctions
>         >
>         > This patch factors the behavior of both methods into
>         'checkhash' and returns the
>         > text in order to allow subclasses of revlog and extensions to
>         extend hash
>         > computation and handle hash mismatches.
>
>         Having something named "checkhash" return the text seems strange
>         to me.
>         to me, the 'check' part of the name implies that the function is
>         read
>         only//checking a state no extra logic should apply here,
>
>         If I remember our sprint discussion right, this text return have
>         been
>         introduced for censors. I can think of two ways to move forward,
>         either
>         change the 'checkhash' name to refer to this text computation
>         part or
>         changing censors to not abuse the 'check' method. What do you think?
>
>
>     This is a mistake on my part: Although the merge of the 'checkhash'
>     subfunctions has been updated to not return the text following your
>     feedback, I forgot to update the commit summary.
>
>
>         Cheers,
>
>         > diff --git a/contrib/perf.py b/contrib/perf.py
>         > --- a/contrib/perf.py
>         > +++ b/contrib/perf.py
>         > @@ -861,7 +861,7 @@
>         >      def dohash(text):
>         >          if not cache:
>         >              r.clearcaches()
>         > -        r._checkhash(text, node, rev)
>         > +        r.checkhash(text, node, rev=rev)
>         >
>         >      def dorevision():
>         >          if not cache:
>         > diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
>         > --- a/mercurial/bundlerepo.py
>         > +++ b/mercurial/bundlerepo.py
>         > @@ -147,7 +147,7 @@
>         >              delta = self._chunk(chain.pop())
>         >              text = mdiff.patches(text, [delta])
>         >
>         > -        self._checkhash(text, node, rev)
>         > +        self.checkhash(text, node, rev=rev)
>         >          self._cache = (node, rev, text)
>         >          return text
>         >
>         > diff --git a/mercurial/filelog.py b/mercurial/filelog.py
>         > --- a/mercurial/filelog.py
>         > +++ b/mercurial/filelog.py
>         > @@ -104,9 +104,9 @@
>         >
>         >          return True
>         >
>         > -    def checkhash(self, text, p1, p2, node, rev=None):
>         > +    def checkhash(self, text, node, p1=None, p2=None, rev=None):
>         >          try:
>         > -            super(filelog, self).checkhash(text, p1, p2,
>         node, rev=rev)
>         > +            super(filelog, self).checkhash(text, node, p1=p1,
>         p2=p2, rev=rev)
>         >          except error.RevlogError:
>         >              if _censoredtext(text):
>         >                  raise error.CensoredNodeError(self.indexfile,
>         node, text)
>         > diff --git a/mercurial/revlog.py b/mercurial/revlog.py
>         > --- a/mercurial/revlog.py
>         > +++ b/mercurial/revlog.py
>         > @@ -1221,9 +1221,7 @@
>         >              bins = bins[1:]
>         >
>         >          text = mdiff.patches(text, bins)
>         > -
>         > -        text = self._checkhash(text, node, rev)
>         > -
>         > +        self.checkhash(text, node, rev=rev)
>         >          self._cache = (node, rev, text)
>         >          return text
>         >
>         > @@ -1235,12 +1233,14 @@
>         >          """
>         >          return hash(text, p1, p2)
>         >
>         > -    def _checkhash(self, text, node, rev):
>         > -        p1, p2 = self.parents(node)
>         > -        self.checkhash(text, p1, p2, node, rev)
>         > -        return text
>         > +    def checkhash(self, text, node, p1=None, p2=None, rev=None):
>         > +        """Check node hash integrity.
>         >
>         > -    def checkhash(self, text, p1, p2, node, rev=None):
>         > +        Available as a function so that subclasses can extend
>         hash mismatch
>         > +        behaviors as needed.
>         > +        """
>         > +        if p1 is None and p2 is None:
>         > +            p1, p2 = self.parents(node)
>         >          if node != self.hash(text, p1, p2):
>         >              revornode = rev
>         >              if revornode is None:
>         > @@ -1415,7 +1415,7 @@
>         >                  basetext = self.revision(self.node(baserev),
>         _df=fh)
>         >                  btext[0] = mdiff.patch(basetext, delta)
>         >              try:
>         > -                self.checkhash(btext[0], p1, p2, node)
>         > +                self.checkhash(btext[0], node, p1=p1, p2=p2)
>         >                  if flags & REVIDX_ISCENSORED:
>         >                      raise RevlogError(_('node %s is not
>         censored') % node)
>         >              except CensoredNodeError:
>         > _______________________________________________
>         > Mercurial-devel mailing list
>         > Mercurial-devel@mercurial-scm.org
>         <mailto:Mercurial-devel@mercurial-scm.org>
>         > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>         >
>
>         --
>         Pierre-Yves David
>         _______________________________________________
>         Mercurial-devel mailing list
>         Mercurial-devel@mercurial-scm.org
>         <mailto:Mercurial-devel@mercurial-scm.org>
>         https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>     --
>     Rémi
>
> --
> Rémi

Patch

diff --git a/contrib/perf.py b/contrib/perf.py
--- a/contrib/perf.py
+++ b/contrib/perf.py
@@ -861,7 +861,7 @@ 
     def dohash(text):
         if not cache:
             r.clearcaches()
-        r._checkhash(text, node, rev)
+        r.checkhash(text, node, rev=rev)
 
     def dorevision():
         if not cache:
diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -147,7 +147,7 @@ 
             delta = self._chunk(chain.pop())
             text = mdiff.patches(text, [delta])
 
-        self._checkhash(text, node, rev)
+        self.checkhash(text, node, rev=rev)
         self._cache = (node, rev, text)
         return text
 
diff --git a/mercurial/filelog.py b/mercurial/filelog.py
--- a/mercurial/filelog.py
+++ b/mercurial/filelog.py
@@ -104,9 +104,9 @@ 
 
         return True
 
-    def checkhash(self, text, p1, p2, node, rev=None):
+    def checkhash(self, text, node, p1=None, p2=None, rev=None):
         try:
-            super(filelog, self).checkhash(text, p1, p2, node, rev=rev)
+            super(filelog, self).checkhash(text, node, p1=p1, p2=p2, rev=rev)
         except error.RevlogError:
             if _censoredtext(text):
                 raise error.CensoredNodeError(self.indexfile, node, text)
diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -1221,9 +1221,7 @@ 
             bins = bins[1:]
 
         text = mdiff.patches(text, bins)
-
-        text = self._checkhash(text, node, rev)
-
+        self.checkhash(text, node, rev=rev)
         self._cache = (node, rev, text)
         return text
 
@@ -1235,12 +1233,14 @@ 
         """
         return hash(text, p1, p2)
 
-    def _checkhash(self, text, node, rev):
-        p1, p2 = self.parents(node)
-        self.checkhash(text, p1, p2, node, rev)
-        return text
+    def checkhash(self, text, node, p1=None, p2=None, rev=None):
+        """Check node hash integrity.
 
-    def checkhash(self, text, p1, p2, node, rev=None):
+        Available as a function so that subclasses can extend hash mismatch
+        behaviors as needed.
+        """
+        if p1 is None and p2 is None:
+            p1, p2 = self.parents(node)
         if node != self.hash(text, p1, p2):
             revornode = rev
             if revornode is None:
@@ -1415,7 +1415,7 @@ 
                 basetext = self.revision(self.node(baserev), _df=fh)
                 btext[0] = mdiff.patch(basetext, delta)
             try:
-                self.checkhash(btext[0], p1, p2, node)
+                self.checkhash(btext[0], node, p1=p1, p2=p2)
                 if flags & REVIDX_ISCENSORED:
                     raise RevlogError(_('node %s is not censored') % node)
             except CensoredNodeError: