Patchwork [1,of,8,V3] context: refactor introrev to make the next patch easier to read

login
register
mail settings
Submitter Boris Feld
Date Oct. 3, 2018, 7:10 p.m.
Message ID <68ec0cf339c7e65ee434.1538593844@localhost.localdomain>
Download mbox | patch
Permalink /patch/35417/
State New
Headers show

Comments

Boris Feld - Oct. 3, 2018, 7:10 p.m.
# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1538554251 -7200
#      Wed Oct 03 10:10:51 2018 +0200
# Node ID 68ec0cf339c7e65ee4349f543e3024068fbbe591
# Parent  1a4c1a3cc3f5b54de7f56753c0ea8b02b4443958
# EXP-Topic copy-perf
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 68ec0cf339c7
context: refactor introrev to make the next patch easier to read

We are about to update the logic for checking if the rev is available in an
efficient manner. Refactoring introrev will make the next patch easier to
read.
via Mercurial-devel - Oct. 3, 2018, 7:23 p.m.
On Wed, Oct 3, 2018 at 12:15 PM Boris Feld <boris.feld@octobus.net> wrote:

> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1538554251 -7200
> #      Wed Oct 03 10:10:51 2018 +0200
> # Node ID 68ec0cf339c7e65ee4349f543e3024068fbbe591
> # Parent  1a4c1a3cc3f5b54de7f56753c0ea8b02b4443958
> # EXP-Topic copy-perf
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r
> 68ec0cf339c7
> context: refactor introrev to make the next patch easier to read
>
> We are about to update the logic for checking if the rev is available in an
> efficient manner. Refactoring introrev will make the next patch easier to
> read.
>
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -776,10 +776,15 @@ 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:
> -            return self.linkrev()
> -        return self._adjustlinkrev(self.rev(), inclusive=True)
> +        lazyavailable = r'_changeid' in attrs or r'_changectx' in attrs
> +        if lazyavailable:
> +            rev = self.rev()
> +            if rev == lkr:
> +                return rev
> +            else:
> +                return self._adjustlinkrev(rev, inclusive=True)
> +        else:
> +            return self.rev()
>

For this to be a pure refactoring, shouldn't this be "return
self.linkrev()"? That's what we used to return when noctx was true.


>      def introfilectx(self):
>          """Return filectx having identical contents, but pointing to the
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
via Mercurial-devel - Oct. 3, 2018, 7:24 p.m.
Oh, and thanks for splitting this patch up! I find it much easier to see
what's going on now.

On Wed, Oct 3, 2018 at 12:23 PM Martin von Zweigbergk <martinvonz@google.com>
wrote:

>
>
> On Wed, Oct 3, 2018 at 12:15 PM Boris Feld <boris.feld@octobus.net> wrote:
>
>> # HG changeset patch
>> # User Boris Feld <boris.feld@octobus.net>
>> # Date 1538554251 -7200
>> #      Wed Oct 03 10:10:51 2018 +0200
>> # Node ID 68ec0cf339c7e65ee4349f543e3024068fbbe591
>> # Parent  1a4c1a3cc3f5b54de7f56753c0ea8b02b4443958
>> # EXP-Topic copy-perf
>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r
>> 68ec0cf339c7
>> context: refactor introrev to make the next patch easier to read
>>
>> We are about to update the logic for checking if the rev is available in
>> an
>> efficient manner. Refactoring introrev will make the next patch easier to
>> read.
>>
>> diff --git a/mercurial/context.py b/mercurial/context.py
>> --- a/mercurial/context.py
>> +++ b/mercurial/context.py
>> @@ -776,10 +776,15 @@ 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:
>> -            return self.linkrev()
>> -        return self._adjustlinkrev(self.rev(), inclusive=True)
>> +        lazyavailable = r'_changeid' in attrs or r'_changectx' in attrs
>> +        if lazyavailable:
>> +            rev = self.rev()
>> +            if rev == lkr:
>> +                return rev
>> +            else:
>> +                return self._adjustlinkrev(rev, inclusive=True)
>> +        else:
>> +            return self.rev()
>>
>
> For this to be a pure refactoring, shouldn't this be "return
> self.linkrev()"? That's what we used to return when noctx was true.
>
>
>>      def introfilectx(self):
>>          """Return filectx having identical contents, but pointing to the
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>
Boris Feld - Oct. 4, 2018, 2:42 p.m.
On 03/10/2018 21:23, Martin von Zweigbergk via Mercurial-devel wrote:
>
>
> On Wed, Oct 3, 2018 at 12:15 PM Boris Feld <boris.feld@octobus.net
> <mailto:boris.feld@octobus.net>> wrote:
>
>     # HG changeset patch
>     # User Boris Feld <boris.feld@octobus.net
>     <mailto:boris.feld@octobus.net>>
>     # Date 1538554251 -7200
>     #      Wed Oct 03 10:10:51 2018 +0200
>     # Node ID 68ec0cf339c7e65ee4349f543e3024068fbbe591
>     # Parent  1a4c1a3cc3f5b54de7f56753c0ea8b02b4443958
>     # EXP-Topic copy-perf
>     # Available At https://bitbucket.org/octobus/mercurial-devel/
>     #              hg pull
>     https://bitbucket.org/octobus/mercurial-devel/ -r 68ec0cf339c7
>     context: refactor introrev to make the next patch easier to read
>
>     We are about to update the logic for checking if the rev is
>     available in an
>     efficient manner. Refactoring introrev will make the next patch
>     easier to
>     read.
>
>     diff --git a/mercurial/context.py b/mercurial/context.py
>     --- a/mercurial/context.py
>     +++ b/mercurial/context.py
>     @@ -776,10 +776,15 @@ 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:
>     -            return self.linkrev()
>     -        return self._adjustlinkrev(self.rev(), inclusive=True)
>     +        lazyavailable = r'_changeid' in attrs or r'_changectx' in
>     attrs
>     +        if lazyavailable:
>     +            rev = self.rev()
>     +            if rev == lkr:
>     +                return rev
>     +            else:
>     +                return self._adjustlinkrev(rev, inclusive=True)
>     +        else:
>     +            return self.rev()
>
>
> For this to be a pure refactoring, shouldn't this be "return
> self.linkrev()"? That's what we used to return when noctx was true.

Something that makes all these changes complicated is that
_descendantrev was previously not actually taken into account (just
slowing things down). So we fix both the slow down and the usage.

To clarify the whole thing, we've prepared a V4 that duplicated some
code to make the whole process more explicit. Let's make the whole
things clear and correct first and we'll see if things can get factored
out afterward.

>
>
>          def introfilectx(self):
>              """Return filectx having identical contents, but pointing
>     to the
>     _______________________________________________
>     Mercurial-devel mailing list
>     Mercurial-devel@mercurial-scm.org
>     <mailto:Mercurial-devel@mercurial-scm.org>
>     https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -776,10 +776,15 @@  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:
-            return self.linkrev()
-        return self._adjustlinkrev(self.rev(), inclusive=True)
+        lazyavailable = r'_changeid' in attrs or r'_changectx' in attrs
+        if lazyavailable:
+            rev = self.rev()
+            if rev == lkr:
+                return rev
+            else:
+                return self._adjustlinkrev(rev, inclusive=True)
+        else:
+            return self.rev()
 
     def introfilectx(self):
         """Return filectx having identical contents, but pointing to the