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

Rémi Chaintron remi.chaintron at gmail.com
Thu Nov 24 11:41:57 EST 2016


On Thu, 24 Nov 2016 at 16:30 Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:

>
>
> 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?
>
>
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 at mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
-- 
Rémi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20161124/7c03836c/attachment.html>


More information about the Mercurial-devel mailing list