[PATCH 3 of 7] context: fix introrev to avoid computation as initially intended

[Boris FELD]

On 01/10/2018 18:43, Martin von Zweigbergk via Mercurial-devel wrote:
>
>
> On Mon, Oct 1, 2018 at 9:11 AM Boris FELD <boris.feld at octobus.net 
> <mailto:boris.feld at octobus.net>> wrote:
>
>     On 10/09/2018 18:21, Martin von Zweigbergk via Mercurial-devel wrote:
>>
>>
>>     On Fri, Sep 7, 2018 at 8:09 AM Boris Feld <boris.feld at octobus.net
>>     <mailto:boris.feld at octobus.net>> wrote:
>>
>>         # HG changeset patch
>>         # User Boris Feld <boris.feld at octobus.net
>>         <mailto:boris.feld at octobus.net>>
>>         # Date 1536254177 14400
>>         #      Thu Sep 06 13:16:17 2018 -0400
>>         # Node ID a4c3eb6c1a36cbbf64fa8930b173154b2e77ef2b
>>         # Parent 9a18509c522deeb62a7b244dcf4c7b79a8dc1132
>>         # EXP-Topic copy-perf
>>         # Available At https://bitbucket.org/octobus/mercurial-devel/
>>         #              hg pull
>>         https://bitbucket.org/octobus/mercurial-devel/ -r a4c3eb6c1a36
>>         context: fix introrev to avoid computation as initially intended
>>
>>         diff --git a/mercurial/context.py b/mercurial/context.py
>>         --- a/mercurial/context.py
>>         +++ b/mercurial/context.py
>>         @@ -829,6 +829,23 @@ class basefilectx(object):
>>                      # result is crash somewhere else at to some point.
>>                  return lkr
>>
>>         +    def _lazyrev(self):
>>
>>
>>     We usually try to separate refactoring (like extracting a method)
>>     from functional (or performance-related) changes. Could you do
>>     that with this patch or does it not make sense for some reason?
>     In this case, the two changes are a bit too interleaved to be
>     easily split in two. We can't do the special casing until we have
>     the new method and the new method can't have any caller without
>     changing the conditionals to include the special casing.
>
>
> Maybe I missed something, but it looks like _lazyrev() returns either 
> "None" or "self.rev()", even at the end of the series. Did I get that 
> right? Will that change later? If not, it seems like you could instead 
> extract a method that calculates what's currently called "noctx". Even 
> if that's going to change, it might make it easier to understand this 
> patch if you split out a patch that made the structure here more 
> similar to your goal, something like:
>
>
> @@ -837,9 +837,13 @@ class basefilectx(object):
>          lkr = self.linkrev()
>          attrs = vars(self)
>          noctx = not (r'_changeid' in attrs or r'_changectx' in attrs)
> -        if noctx or self.rev() == lkr:
> +        if noctx:
>              return self.linkrev()
> -        return self._adjustlinkrev(self.rev(), inclusive=True)
> +        else:
> +            if self.rev() == lkr:
> +                return self.linkrev()
> +            else:
> +                return self._adjustlinkrev(self.rev(), inclusive=True)
Yes, you are right, we can split the changeset in two.

I don't feel that it would help the readability of the series but I'm 
not the reviewer. Do you think it would help you review the patch?
>
>>
>>
>>     _______________________________________________
>>     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
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20181002/7aa767af/attachment.html>