[PATCH 1 of 5 v4] revlog: merge hash checking subfunctions
Pierre-Yves David
pierre-yves.david at ens-lyon.org
Mon Nov 28 13:35:06 UTC 2016
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 at gmail.com
> <mailto:remi.chaintron at gmail.com>> wrote:
>
> On Thu, 24 Nov 2016 at 16:30 Pierre-Yves David
> <pierre-yves.david at ens-lyon.org
> <mailto: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 <mailto: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
> <mailto: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
> <mailto:Mercurial-devel at mercurial-scm.org>
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
> --
> Rémi
>
> --
> Rémi
--
Pierre-Yves David
More information about the Mercurial-devel
mailing list