[PATCH 3 of 6] merge: remove uses of manifest.matches

[Durham Goode]

On 3/7/17 4:41 PM, Martin von Zweigbergk wrote:
> On Fri, Mar 3, 2017 at 11:34 AM, Durham Goode <durham at fb.com> wrote:
>> # HG changeset patch
>> # User Durham Goode <durham at fb.com>
>> # Date 1488519936 28800
>> #      Thu Mar 02 21:45:36 2017 -0800
>> # Node ID 883bb49a3b40609074d56257aab7619f0c306efc
>> # Parent  4cebdd029399cf7c3b0fff73faf1f41af0e895d1
>> merge: remove uses of manifest.matches
>>
>> This gets rid of the manifest.matches calls in merge.py in favor of the new api.
>> This is part of getting rid of manifest.matches since it is O(manifest).
>>
>> diff --git a/mercurial/merge.py b/mercurial/merge.py
>> --- a/mercurial/merge.py
>> +++ b/mercurial/merge.py
>> @@ -818,11 +818,7 @@ def manifestmerge(repo, wctx, p2, pa, br
>>          if any(wctx.sub(s).dirty() for s in wctx.substate):
>>              m1['.hgsubstate'] = modifiednodeid
>>
>> -    # Compare manifests
>> -    if matcher is not None:
>> -        m1 = m1.matches(matcher)
>> -        m2 = m2.matches(matcher)
>> -    diff = m1.diff(m2)
>> +    diff = m1.diff(m2, match=matcher)
>>
>>      actions = {}
>>      for f, ((n1, fl1), (n2, fl2)) in diff.iteritems():
>> @@ -858,7 +854,7 @@ def manifestmerge(repo, wctx, p2, pa, br
>>                  pass # we'll deal with it on m2 side
>>              elif f in movewithdir: # directory rename, move local
>>                  f2 = movewithdir[f]
>> -                if f2 in m2:
>> +                if f2 in m2 and (not matcher or matcher(f2)):
>
> Would it be easier to set matcher = match.always(...) (or however you
> create such matchers)  when matcher is None, so we don't have to check
> in 4 places?

I figured an `if None` check was faster than a match.always() check, 
especially since this is a hotpath. I could change it though.

> Also, will it be better to swap the order of the conditions to "if
> matcher(f2) and f2 in m2" to avoid loading some treemanifests if
> matcher(f2) is false? I guess we can change that later if it turns out
> to be better.

Hmm, yes.

> (For this case the lazy manifest.match() approach would have been mean
> cleaner. It's a little unfortunate that we have to remember to check
> the matcher ever time, but I don't see a better way of doing it.)

The ugliness of checking it 4 times here is because I'm retrofitting a 
piece of code that wasn't designed with this in mind and I was trying to 
avoid any semantic changes. I'd expect code that was built to expect a 
matcher would not be so ugly.

>>                      actions[f2] = ('m', (f, f2, None, True, pa.node()),
>>                                     "remote directory rename, both created")
>>                  else:
>> @@ -887,7 +883,7 @@ def manifestmerge(repo, wctx, p2, pa, br
>>                  pass # we'll deal with it on m1 side
>>              elif f in movewithdir:
>>                  f2 = movewithdir[f]
>> -                if f2 in m1:
>> +                if f2 in m1 and (not matcher or matcher(f2)):
>>                      actions[f2] = ('m', (f2, f, None, False, pa.node()),
>>                                     "local directory rename, both created")
>>                  else:
>> @@ -895,7 +891,7 @@ def manifestmerge(repo, wctx, p2, pa, br
>>                                     "local directory rename - get from " + f)
>>              elif f in copy:
>>                  f2 = copy[f]
>> -                if f2 in m2:
>> +                if f2 in m2 and (not matcher or matcher(f2)):
>>                      actions[f] = ('m', (f2, f, f2, False, pa.node()),
>>                                    "remote copied from " + f2)
>>                  else:
>> @@ -927,7 +923,7 @@ def manifestmerge(repo, wctx, p2, pa, br
>>                          # new file added in a directory that was moved
>>                          df = dirmove[d] + f[len(d):]
>>                          break
>> -                if df in m1:
>> +                if df in m1 and (not matcher or matcher(df)):
>>                      actions[df] = ('m', (df, f, f, False, pa.node()),
>>                              "local directory rename - respect move from " + f)
>>                  elif acceptremote:
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel at mercurial-scm.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=kiGdgxyWqhs0Cu0YjkuskFVobqCWr4TIOthkrkxFU5w&s=4vcpzhVAiK7FHCfzH2C5FUDNbQkM0oP5DXa9g8h-9Dk&e=