Patchwork [16,of,19] workingctx: use inheritance for _generatestatus while keeping the fastpath

login
register
mail settings
Submitter Sean Farley
Date May 15, 2014, 9:16 p.m.
Message ID <5e4e31f7dca07c7883f0.1400188594@laptop.local>
Download mbox | patch
Permalink /patch/4775/
State Changes Requested
Headers show

Comments

Sean Farley - May 15, 2014, 9:16 p.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley@gmail.com>
# Date 1398346484 18000
#      Thu Apr 24 08:34:44 2014 -0500
# Node ID 5e4e31f7dca07c7883f01e339e1365c882e5d213
# Parent  dde09eaa8280d8138c76c8e74eae388f30ffce40
workingctx: use inheritance for _generatestatus while keeping the fastpath
Pierre-Yves David - May 16, 2014, 12:20 a.m.
On 05/15/2014 02:16 PM, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1398346484 18000
> #      Thu Apr 24 08:34:44 2014 -0500
> # Node ID 5e4e31f7dca07c7883f01e339e1365c882e5d213
> # Parent  dde09eaa8280d8138c76c8e74eae388f30ffce40
> workingctx: use inheritance for _generatestatus while keeping the fastpath

Can you be a bit more verbose on this fastpath business?


>
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -1300,10 +1300,23 @@ class workingctx(committablectx):
>               if fixup and listclean:
>                   clean += fixup
>
>           return [modified, added, removed, deleted, unknown, ignored, clean]
>
> +    def _generatestatus(self, other, s, match, listignored, listclean,
> +                        listunknown):
> +        """generate a status with respect to another context
> +
> +        This includes logic for maintaining the fast path of status when
> +        comparing against its parent.
> +        """
> +        if other != self._repo['.']:

This check looks strange to me. It seems to no include parent2 in case 
of merge. apparently the old code had the same bias. What is happening 
if we use both parent here?

> +            s = super(workingctx, self)._generatestatus(other, s, match,
> +                                                        listignored, listclean,
> +                                                        listunknown)
> +        return s
> +
Sean Farley - May 16, 2014, 1:05 a.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 05/15/2014 02:16 PM, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1398346484 18000
>> #      Thu Apr 24 08:34:44 2014 -0500
>> # Node ID 5e4e31f7dca07c7883f01e339e1365c882e5d213
>> # Parent  dde09eaa8280d8138c76c8e74eae388f30ffce40
>> workingctx: use inheritance for _generatestatus while keeping the fastpath
>
> Can you be a bit more verbose on this fastpath business?

Sure. There is only one fast path in status: comparing the working
directory with its first parent. For that, we need a few things to
happen fast:

1) check the dirstate
2) generate a (pseudo)manifest
3) filter out suspect/bad symlinks (only for win32)

(1) and (2) check test for the fast path situation (workingctx +
repo['.']) and then, if so, copy the manifest from the parent and
replace it with modified, removed, etc. files.

>> diff --git a/mercurial/context.py b/mercurial/context.py
>> --- a/mercurial/context.py
>> +++ b/mercurial/context.py
>> @@ -1300,10 +1300,23 @@ class workingctx(committablectx):
>>               if fixup and listclean:
>>                   clean += fixup
>>
>>           return [modified, added, removed, deleted, unknown, ignored, clean]
>>
>> +    def _generatestatus(self, other, s, match, listignored, listclean,
>> +                        listunknown):
>> +        """generate a status with respect to another context
>> +
>> +        This includes logic for maintaining the fast path of status when
>> +        comparing against its parent.
>> +        """
>> +        if other != self._repo['.']:
>
> This check looks strange to me. It seems to no include parent2 in case 
> of merge. apparently the old code had the same bias. What is happening 
> if we use both parent here?

Hmmm, ok, that's a good point. I will need someone else's help here to
explain what should be done.
Pierre-Yves David - May 16, 2014, 4:04 a.m.
On 05/15/2014 06:05 PM, Sean Farley wrote:
>
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>
>> On 05/15/2014 02:16 PM, Sean Farley wrote:
>>> # HG changeset patch
>>> # User Sean Farley <sean.michael.farley@gmail.com>
>>> # Date 1398346484 18000
>>> #      Thu Apr 24 08:34:44 2014 -0500
>>> # Node ID 5e4e31f7dca07c7883f01e339e1365c882e5d213
>>> # Parent  dde09eaa8280d8138c76c8e74eae388f30ffce40
>>> workingctx: use inheritance for _generatestatus while keeping the fastpath
>>
>> Can you be a bit more verbose on this fastpath business?
>
> Sure. There is only one fast path in status: comparing the working
> directory with its first parent. For that, we need a few things to
> happen fast:
>
> 1) check the dirstate
> 2) generate a (pseudo)manifest
> 3) filter out suspect/bad symlinks (only for win32)
>
> (1) and (2) check test for the fast path situation (workingctx +
> repo['.']) and then, if so, copy the manifest from the parent and
> replace it with modified, removed, etc. files.
>
>>> diff --git a/mercurial/context.py b/mercurial/context.py
>>> --- a/mercurial/context.py
>>> +++ b/mercurial/context.py
>>> @@ -1300,10 +1300,23 @@ class workingctx(committablectx):
>>>                if fixup and listclean:
>>>                    clean += fixup
>>>
>>>            return [modified, added, removed, deleted, unknown, ignored, clean]
>>>
>>> +    def _generatestatus(self, other, s, match, listignored, listclean,
>>> +                        listunknown):
>>> +        """generate a status with respect to another context
>>> +
>>> +        This includes logic for maintaining the fast path of status when
>>> +        comparing against its parent.
>>> +        """
>>> +        if other != self._repo['.']:
>>
>> This check looks strange to me. It seems to no include parent2 in case
>> of merge. apparently the old code had the same bias. What is happening
>> if we use both parent here?
>
> Hmmm, ok, that's a good point. I will need someone else's help here to
> explain what should be done.

For now you are moving code around, so the main option is still to leave 
this as his an push forward your main goal.

However moving code is alway a good time to read code. I'm not sure what 
is going on here. And as dead Chtulhu is dreaming in his house of at 
R'lyeh, we can usually not ask him what was its initial intend here.

So press forward with this series but keep a note about this somewhere.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1300,10 +1300,23 @@  class workingctx(committablectx):
             if fixup and listclean:
                 clean += fixup
 
         return [modified, added, removed, deleted, unknown, ignored, clean]
 
+    def _generatestatus(self, other, s, match, listignored, listclean,
+                        listunknown):
+        """generate a status with respect to another context
+
+        This includes logic for maintaining the fast path of status when
+        comparing against its parent.
+        """
+        if other != self._repo['.']:
+            s = super(workingctx, self)._generatestatus(other, s, match,
+                                                        listignored, listclean,
+                                                        listunknown)
+        return s
+
     def status(self, ignored=False, clean=False, unknown=False, match=None):
         """Explicit status query
         Unless this method is used to query the working copy status, the
         _status property will implicitly read the status using its default
         arguments."""
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1546,15 +1546,12 @@  class localrepository(object):
                     self.ui.warn('%s: %s\n' % (self.dirstate.pathto(f), msg))
             match.bad = bad
 
         r = [[], [], [], [], [], [], []]
         r = ctx2._prestatus(ctx1, r, match, listignored, listclean, listunknown)
-
-        if not parentworking:
-            r = ctx2._generatestatus(ctx1, s, match, listignored, listclean,
-                                     listunknown)
-
+        r = ctx2._generatestatus(ctx1, r, match, listignored, listclean,
+                                 listunknown)
         r = ctx2._poststatus(ctx1, r, match, listignored, listclean,
                              listunknown)
 
         if reversed:
             r[1], r[2] = r[2], r[1]