Patchwork [1,of,8] localrepo: reverse contexts in status

login
register
mail settings
Submitter Sean Farley
Date May 6, 2014, 11:33 p.m.
Message ID <c8586e9a821d8abbc884.1399419198@laptop.local>
Download mbox | patch
Permalink /patch/4646/
State Superseded
Commit 242637139efbbdde8b070482acd9f1900d81d7ea
Headers show

Comments

Sean Farley - May 6, 2014, 11:33 p.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 c8586e9a821d8abbc88438f2e78aa564e0b5e87a
# Parent  0768cda8b5799dc803dc0ee27a832cd64e05f28a
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.
Durham Goode - May 10, 2014, 1:43 a.m.
On 5/6/14, 4:33 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 c8586e9a821d8abbc88438f2e78aa564e0b5e87a
> # Parent  0768cda8b5799dc803dc0ee27a832cd64e05f28a
> 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
> @@ -1518,10 +1518,17 @@ class localrepository(object):
>               return mf
>   
>           ctx1 = self[node1]
>           ctx2 = self[node2]
>   
> +        # check if contexts are sent in reversed
> +        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
>   
> @@ -1620,10 +1627,14 @@ class localrepository(object):
>                                         ' "%s"\n' % f)
>                           continue
>                   sane.append(f)
>               modified = sane
>   
> +        if reversed:
> +            added, removed = removed, added
> +            deleted, unknown = unknown, deleted
> +
>           r = modified, added, removed, deleted, unknown, ignored, clean
>   
>
This is pretty subtle.  Probably warrants in-code comments.

It also seems a little fragile (what if other people introduce other 
things that need to be reversed).  Will it still be here once all your 
patches are landed?
Sean Farley - May 12, 2014, 9:52 p.m.
Durham Goode <durham@fb.com> writes:

> On 5/6/14, 4:33 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 c8586e9a821d8abbc88438f2e78aa564e0b5e87a
>> # Parent  0768cda8b5799dc803dc0ee27a832cd64e05f28a
>> 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
>> @@ -1518,10 +1518,17 @@ class localrepository(object):
>>               return mf
>>   
>>           ctx1 = self[node1]
>>           ctx2 = self[node2]
>>   
>> +        # check if contexts are sent in reversed
>> +        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
>>   
>> @@ -1620,10 +1627,14 @@ class localrepository(object):
>>                                         ' "%s"\n' % f)
>>                           continue
>>                   sane.append(f)
>>               modified = sane
>>   
>> +        if reversed:
>> +            added, removed = removed, added
>> +            deleted, unknown = unknown, deleted
>> +
>>           r = modified, added, removed, deleted, unknown, ignored, clean
>>   
>>
> This is pretty subtle.  Probably warrants in-code comments.
>
> It also seems a little fragile (what if other people introduce other 
> things that need to be reversed).  Will it still be here once all your 
> patches are landed?

This, admittedly, fragile logic 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:

ctx1.status(ctx2)

If we always built the manifest for each context and compared those
things, then we'd be done. But the special case of:

workingctx.status(parentctx) [1]

means we just copy the manifest of the parent. I don't like this code
block either but am open to suggestions of how to put status into the
base context class and have all the children Do The Right Thing™.

[1] - Or should it be parentctx.status(wctx)? This will be an important
decision that others might have opinions on. For example, should
memctx.status(workingctx) mean the differences from memctx to workingctx
or workingctx to memctx?

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1518,10 +1518,17 @@  class localrepository(object):
             return mf
 
         ctx1 = self[node1]
         ctx2 = self[node2]
 
+        # check if contexts are sent in reversed
+        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
 
@@ -1620,10 +1627,14 @@  class localrepository(object):
                                       ' "%s"\n' % f)
                         continue
                 sane.append(f)
             modified = sane
 
+        if reversed:
+            added, removed = removed, added
+            deleted, unknown = unknown, deleted
+
         r = modified, added, removed, deleted, unknown, ignored, clean
 
         if listsubrepos:
             for subpath, sub in scmutil.itersubrepos(ctx1, ctx2):
                 if working: