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

Sean Farley sean.michael.farley at gmail.com
Mon May 12 17:57:59 CDT 2014


Durham Goode <durham at fb.com> writes:

> 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¹

Ok, I would agree with this. I'll make sure this is what my patch series
ends up doing and resend (with the suggestions you gave).

Thanks!


More information about the Mercurial-devel mailing list