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

login
register
mail settings
Submitter Durham Goode
Date March 3, 2017, 7:34 p.m.
Message ID <883bb49a3b40609074d5.1488569658@dev111.prn1.facebook.com>
Download mbox | patch
Permalink /patch/18903/
State Accepted
Delegated to: Martin von Zweigbergk
Headers show

Comments

Durham Goode - March 3, 2017, 7:34 p.m.
# HG changeset patch
# User Durham Goode <durham@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).
via Mercurial-devel - March 8, 2017, 12:41 a.m.
On Fri, Mar 3, 2017 at 11:34 AM, Durham Goode <durham@fb.com> wrote:
> # HG changeset patch
> # User Durham Goode <durham@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?

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.

(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.)

>                      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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Durham Goode - March 8, 2017, 12:47 a.m.
On 3/7/17 4:41 PM, Martin von Zweigbergk wrote:
> On Fri, Mar 3, 2017 at 11:34 AM, Durham Goode <durham@fb.com> wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@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@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=

Patch

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)):
                     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: