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

[Pierre-Yves David]

On 11/23/2016 06:39 PM, Remi Chaintron wrote:
> # HG changeset patch
> # User Remi Chaintron <remi at 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 at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

-- 
Pierre-Yves David