Patchwork [1,of,2] dicthelpers.diff: compare against default for missing values

login
register
mail settings
Submitter Siddharth Agarwal
Date April 10, 2013, 6:21 p.m.
Message ID <645bee3e5241dd659dae.1365618118@sid0x220>
Download mbox | patch
Permalink /patch/1269/
State Superseded, archived
Headers show

Comments

Siddharth Agarwal - April 10, 2013, 6:21 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1365616160 25200
#      Wed Apr 10 10:49:20 2013 -0700
# Node ID 645bee3e5241dd659dae1c9bcc68f208484d6eb0
# Parent  99a410e82b35e71d5f6121a9b9bfc11103678e83
dicthelpers.diff: compare against default for missing values

This is not only a bit faster, but also aligns with caller's expectations
better since we can legitimately have manifestdict's _flags set to '' instead
of unset.

hg perfmergecalculate -r .
before: ! wall 0.139582 comb 0.140000 user 0.140000 sys 0.000000 (best of 59)
after:  ! wall 0.126154 comb 0.120000 user 0.120000 sys 0.000000 (best of 74)

hg perfmergecalculate -r .^
before: ! wall 0.236333 comb 0.240000 user 0.240000 sys 0.000000 (best of 36)
after:  ! wall 0.212265 comb 0.210000 user 0.210000 sys 0.000000 (best of 45)
Bryan O'Sullivan - April 10, 2013, 6:29 p.m.
On Wed, Apr 10, 2013 at 11:21 AM, Siddharth Agarwal <sid0@fb.com> wrote:

> dicthelpers.diff: compare against default for missing values
>
> This is not only a bit faster, but also aligns with caller's expectations
> better since we can legitimately have manifestdict's _flags set to ''
> instead
> of unset.
>

This is all well and good, but it's now looking like a special-purpose
function that does exactly what manifestmerge needs, rather than a function
that belongs in a notionally general purpose module?
Siddharth Agarwal - April 10, 2013, 6:36 p.m.
On 04/10/2013 11:29 AM, Bryan O'Sullivan wrote:
>
> On Wed, Apr 10, 2013 at 11:21 AM, Siddharth Agarwal <sid0@fb.com 
> <mailto:sid0@fb.com>> wrote:
>
>     dicthelpers.diff: compare against default for missing values
>
>     This is not only a bit faster, but also aligns with caller's
>     expectations
>     better since we can legitimately have manifestdict's _flags set to
>     '' instead
>     of unset.
>
>
> This is all well and good, but it's now looking like a special-purpose 
> function that does exactly what manifestmerge needs, rather than a 
> function that belongs in a notionally general purpose module?

I think that even in the general case this behaviour is better than the 
old one. It prevents returning values like ('', ''), which callers 
wouldn't be able to distinguish from ('', not present) or (not present, 
'') anyway.
Bryan O'Sullivan - April 10, 2013, 6:44 p.m.
On Wed, Apr 10, 2013 at 11:36 AM, Siddharth Agarwal <sid0@fb.com> wrote:

> I think that even in the general case this behaviour is better than the
> old one. It prevents returning values like ('', ''), which callers wouldn't
> be able to distinguish from ('', not present) or (not present, '') anyway.
>
>
Hmm, okay.

Patch

diff -r 99a410e82b35 -r 645bee3e5241 mercurial/dicthelpers.py
--- a/mercurial/dicthelpers.py	Sat Apr 06 19:50:03 2013 -0700
+++ b/mercurial/dicthelpers.py	Wed Apr 10 10:49:20 2013 -0700
@@ -11,23 +11,23 @@  def diff(d1, d2, default=None):
     This includes keys that are present in one dict but not the other, and
     keys whose values are different. The return value is a dict with values
     being pairs of values from d1 and d2 respectively, and missing values
-    represented as default.'''
+    treated as default, so if a value is missing from one dict and the same as
+    default in the other, it will not be returned.'''
     res = {}
     if d1 is d2:
         # same dict, so diff is empty
         return res
 
     for k1, v1 in d1.iteritems():
-        if k1 in d2:
-            v2 = d2[k1]
-            if v1 != v2:
-                res[k1] = (v1, v2)
-        else:
-            res[k1] = (v1, default)
+        v2 = d2.get(k1, default)
+        if v1 != v2:
+            res[k1] = (v1, v2)
 
     for k2 in d2:
         if k2 not in d1:
-            res[k2] = (default, d2[k2])
+            v2 = d2[k2]
+            if v2 != default:
+                res[k2] = (default, v2)
 
     return res
 
diff -r 99a410e82b35 -r 645bee3e5241 tests/test-dicthelpers.py
--- a/tests/test-dicthelpers.py	Sat Apr 06 19:50:03 2013 -0700
+++ b/tests/test-dicthelpers.py	Wed Apr 10 10:49:20 2013 -0700
@@ -49,5 +49,11 @@  class testdicthelpers(unittest.TestCase)
                                              'c': ('baz', 456),
                                              'd': (456, 'quux')})
 
+        # check that we compare against default
+        self.assertEqual(diff(d1, d2, 'baz'), {'a': ('foo', 'foo2'),
+                                               'd': ('baz', 'quux')})
+        self.assertEqual(diff(d1, d2, 'quux'), {'a': ('foo', 'foo2'),
+                                                'c': ('baz', 'quux')})
+
 if __name__ == '__main__':
     silenttestrunner.main(__name__)