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

[Sean Farley]

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™.

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