Patchwork [V2] localrepo: reverse contexts in status

login
register
mail settings
Submitter Sean Farley
Date May 16, 2014, 1:09 a.m.
Message ID <b96074a56e0d75b39476.1400202562@laptop.local>
Download mbox | patch
Permalink /patch/4780/
State Accepted
Headers show

Comments

Sean Farley - May 16, 2014, 1:09 a.m.
# HG changeset patch
# User Sean Farley <sean.michael.farley@gmail.com>
# Date 1397684088 18000
#      Wed Apr 16 16:34:48 2014 -0500
# Node ID b96074a56e0d75b39476e37d91395505b1e01d2f
# Parent  8844e50911049ffb10c3693b6cfea7e2ea4440c7
localrepo: reverse contexts in status

This is a slight tweak to how localrepo.status calculates what files have
changed. By forcing a changectx to be first operator and anything not a
changectx to be the second operator, we can later exploit this to allow
refactoring the status operation as a method of a context object.

Furthermore, this change will speed up 'hg diff --reverse' when used with the
working directory because the code will now hit a fast path without needing to
calculate an unneeded second manifest.
Pierre-Yves David - May 16, 2014, 4:27 a.m.
On 05/15/2014 06:09 PM, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean.michael.farley@gmail.com>
> # Date 1397684088 18000
> #      Wed Apr 16 16:34:48 2014 -0500
> # Node ID b96074a56e0d75b39476e37d91395505b1e01d2f
> # Parent  8844e50911049ffb10c3693b6cfea7e2ea4440c7
> localrepo: reverse contexts in status
>
> This is a slight tweak to how localrepo.status calculates what files have
> changed. By forcing a changectx to be first operator and anything not a
> changectx to be the second operator, we can later exploit this to allow
> refactoring the status operation as a method of a context object.
>
> Furthermore, this change will speed up 'hg diff --reverse' when used with the
> working directory because the code will now hit a fast path without needing to
> calculate an unneeded second manifest.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1521,10 +1521,28 @@ class localrepository(object):
>               return mf
>
>           ctx1 = self[node1]
>           ctx2 = self[node2]
>
> +        # This next code block is, admittedly, fragile logic that tests for
> +        # reversing the contexts and wouldn't need to exist if it weren't for
> +        # the fast (and common) code path of comparing the working directory
> +        # with its first parent.
> +        #
> +        # What we're aiming for here is the ability to call:
> +        #
> +        # workingctx.status(parentctx)
> +        #
> +        # If we always built the manifest for each context and compared those,
> +        # then we'd be done. But the special case of the above call means we
> +        # just copy the manifest of the parent.
> +        reversed = False
> +        if (not isinstance(ctx1, context.changectx)
> +            and isinstance(ctx2, context.changectx)):

Not a super fan of isinstance. Do we have any other alternative?
(no is a valid answer)

> +            reversed = True
> +            ctx1, ctx2 = ctx2, ctx1
> +
>           working = ctx2.rev() is None
>           parentworking = working and ctx1 == self['.']
>           match = match or matchmod.always(self.root, self.getcwd())
>           listignored, listclean, listunknown = ignored, clean, unknown
>
> @@ -1579,10 +1597,13 @@ class localrepository(object):
>               removed = mf1.keys()
>
>           if working:
>               modified = ctx2._filtersuspectsymlink(modified)
>
> +        if reversed:
> +            added, removed = removed, added
> +

I see that the deleted/unknown thing disapeared, I assume it did not 
made much sense in the first place?
Sean Farley - May 16, 2014, 6:16 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 05/15/2014 06:09 PM, Sean Farley wrote:
>> # HG changeset patch
>> # User Sean Farley <sean.michael.farley@gmail.com>
>> # Date 1397684088 18000
>> #      Wed Apr 16 16:34:48 2014 -0500
>> # Node ID b96074a56e0d75b39476e37d91395505b1e01d2f
>> # Parent  8844e50911049ffb10c3693b6cfea7e2ea4440c7
>> localrepo: reverse contexts in status
>>
>> This is a slight tweak to how localrepo.status calculates what files have
>> changed. By forcing a changectx to be first operator and anything not a
>> changectx to be the second operator, we can later exploit this to allow
>> refactoring the status operation as a method of a context object.
>>
>> Furthermore, this change will speed up 'hg diff --reverse' when used with the
>> working directory because the code will now hit a fast path without needing to
>> calculate an unneeded second manifest.
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>> --- a/mercurial/localrepo.py
>> +++ b/mercurial/localrepo.py
>> @@ -1521,10 +1521,28 @@ class localrepository(object):
>>               return mf
>>
>>           ctx1 = self[node1]
>>           ctx2 = self[node2]
>>
>> +        # This next code block is, admittedly, fragile logic that tests for
>> +        # reversing the contexts and wouldn't need to exist if it weren't for
>> +        # the fast (and common) code path of comparing the working directory
>> +        # with its first parent.
>> +        #
>> +        # What we're aiming for here is the ability to call:
>> +        #
>> +        # workingctx.status(parentctx)
>> +        #
>> +        # If we always built the manifest for each context and compared those,
>> +        # then we'd be done. But the special case of the above call means we
>> +        # just copy the manifest of the parent.
>> +        reversed = False
>> +        if (not isinstance(ctx1, context.changectx)
>> +            and isinstance(ctx2, context.changectx)):
>
> Not a super fan of isinstance. Do we have any other alternative?
> (no is a valid answer)

This is just a stop gap until we can figure out something
better. Especially when memctx is thrown into the mix. 'isinstance' will
keep things working even when memctx will soon inherit from
committablectx. After that we could discuss how to approach it (just
like we could discuss the pre/post status methods at that time).

>> +            reversed = True
>> +            ctx1, ctx2 = ctx2, ctx1
>> +
>>           working = ctx2.rev() is None
>>           parentworking = working and ctx1 == self['.']
>>           match = match or matchmod.always(self.root, self.getcwd())
>>           listignored, listclean, listunknown = ignored, clean, unknown
>>
>> @@ -1579,10 +1597,13 @@ class localrepository(object):
>>               removed = mf1.keys()
>>
>>           if working:
>>               modified = ctx2._filtersuspectsymlink(modified)
>>
>> +        if reversed:
>> +            added, removed = removed, added
>> +
>
> I see that the deleted/unknown thing disapeared, I assume it did not 
> made much sense in the first place?

I found a counterexample for switching deleted/unknown so I removed
that. If we could define this all in terms of operators and rings,
that'd be great :-)
Pierre-Yves David - May 16, 2014, 8:29 p.m.
On 05/16/2014 11:16 AM, Sean Farley wrote:
>
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>
>> On 05/15/2014 06:09 PM, Sean Farley wrote:
>>> # HG changeset patch
>>> # User Sean Farley <sean.michael.farley@gmail.com>
>>> # Date 1397684088 18000
>>> #      Wed Apr 16 16:34:48 2014 -0500
>>> # Node ID b96074a56e0d75b39476e37d91395505b1e01d2f
>>> # Parent  8844e50911049ffb10c3693b6cfea7e2ea4440c7
>>> localrepo: reverse contexts in status

Explanation looks good. I pushed that to the clowncopter

(Hey, patchbot, pushed == queued)

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1521,10 +1521,28 @@  class localrepository(object):
             return mf
 
         ctx1 = self[node1]
         ctx2 = self[node2]
 
+        # This next code block is, admittedly, fragile logic that tests for
+        # reversing the contexts and wouldn't need to exist if it weren't for
+        # the fast (and common) code path of comparing the working directory
+        # with its first parent.
+        #
+        # What we're aiming for here is the ability to call:
+        #
+        # workingctx.status(parentctx)
+        #
+        # If we always built the manifest for each context and compared those,
+        # then we'd be done. But the special case of the above call means we
+        # just copy the manifest of the parent.
+        reversed = False
+        if (not isinstance(ctx1, context.changectx)
+            and isinstance(ctx2, context.changectx)):
+            reversed = True
+            ctx1, ctx2 = ctx2, ctx1
+
         working = ctx2.rev() is None
         parentworking = working and ctx1 == self['.']
         match = match or matchmod.always(self.root, self.getcwd())
         listignored, listclean, listunknown = ignored, clean, unknown
 
@@ -1579,10 +1597,13 @@  class localrepository(object):
             removed = mf1.keys()
 
         if working:
             modified = ctx2._filtersuspectsymlink(modified)
 
+        if reversed:
+            added, removed = removed, added
+
         r = modified, added, removed, deleted, unknown, ignored, clean
 
         if listsubrepos:
             for subpath, sub in scmutil.itersubrepos(ctx1, ctx2):
                 if working: