[PATCH 1 of 8] localrepo: reverse contexts in status

[Durham Goode]

On 5/12/14, 2:52 PM, "Sean Farley" <sean.michael.farley at gmail.com> wrote:

>
>Durham Goode <durham at fb.com> writes:
>
>> On 5/6/14, 4:33 PM, Sean Farley wrote:
>>> # HG changeset patch
>>> # User Sean Farley <sean.michael.farley at 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.

K, I¹d definitely add a comment though.  Aside from that, the series LGTM
based on your responses to my other comments.

>
>[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?
>

I¹d vote for workingctx.status(parentctx).  I read foo.status(bar) as
Œdiff foo against bar¹