Patchwork manifest: don't store None in fulltextcache

login
register
mail settings
Submitter via Mercurial-devel
Date Oct. 18, 2016, 5:58 a.m.
Message ID <d2c313417026d76cb195.1476770296@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/17166/
State Accepted
Headers show

Comments

via Mercurial-devel - Oct. 18, 2016, 5:58 a.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1476769882 25200
#      Mon Oct 17 22:51:22 2016 -0700
# Node ID d2c313417026d76cb19534277df3f3a8b6b22425
# Parent  87a7c0d403ff29dcae2a41e0516c75bbd9f6a5a8
manifest: don't store None in fulltextcache

When we read a value from fulltextcache, we expect it to be an array,
so we should not store None in it. Found while working on narrowhg.
Pierre-Yves David - Oct. 18, 2016, 12:20 p.m.
On 10/18/2016 07:58 AM, Martin von Zweigbergk via Mercurial-devel wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1476769882 25200
> #      Mon Oct 17 22:51:22 2016 -0700
> # Node ID d2c313417026d76cb19534277df3f3a8b6b22425
> # Parent  87a7c0d403ff29dcae2a41e0516c75bbd9f6a5a8
> manifest: don't store None in fulltextcache

Apparently it was still the october 17th when you mailed that ;-)

I've pushed it.
Durham Goode - Oct. 18, 2016, 8:09 p.m.
On 10/17/16, 10:58 PM, "Martin von Zweigbergk" <martinvonz@google.com> wrote:

># HG changeset patch

># User Martin von Zweigbergk <martinvonz@google.com>

># Date 1476769882 25200

>#      Mon Oct 17 22:51:22 2016 -0700

># Node ID d2c313417026d76cb19534277df3f3a8b6b22425

># Parent  87a7c0d403ff29dcae2a41e0516c75bbd9f6a5a8

>manifest: don't store None in fulltextcache

>

>When we read a value from fulltextcache, we expect it to be an array,

>so we should not store None in it. Found while working on narrowhg.

>

>diff -r 87a7c0d403ff -r d2c313417026 mercurial/manifest.py

>--- a/mercurial/manifest.py	Tue Oct 18 02:09:08 2016 +0200

>+++ b/mercurial/manifest.py	Mon Oct 17 22:51:22 2016 -0700

>@@ -1210,7 +1210,8 @@

>                 n = self.addrevision(text, transaction, link, p1, p2)

>                 arraytext = array.array('c', text)

> 

>-        self.fulltextcache[n] = arraytext

>+        if arraytext is not None:

>+            self.fulltextcache[n] = arraytext

> 

>         return n

> 

>@@ -1506,7 +1507,8 @@

>             m = self._newmanifest(text)

>             arraytext = array.array('c', text)

>         self._mancache[node] = m

>-        self.fulltextcache[node] = arraytext

>+        if arraytext is not None:

>+            self.fulltextcache[node] = arraytext

>         return m

> 

>     def readshallow(self, node):


LGTM.  Is there a tree code path that hits this that is not covered by tests?
via Mercurial-devel - Oct. 18, 2016, 8:41 p.m.
On Tue, Oct 18, 2016 at 1:09 PM Durham Goode <durham@fb.com> wrote:

>
>
> On 10/17/16, 10:58 PM, "Martin von Zweigbergk" <martinvonz@google.com>
> wrote:
>
> ># HG changeset patch
> ># User Martin von Zweigbergk <martinvonz@google.com>
> ># Date 1476769882 25200
> >#      Mon Oct 17 22:51:22 2016 -0700
> ># Node ID d2c313417026d76cb19534277df3f3a8b6b22425
> ># Parent  87a7c0d403ff29dcae2a41e0516c75bbd9f6a5a8
> >manifest: don't store None in fulltextcache
> >
> >When we read a value from fulltextcache, we expect it to be an array,
> >so we should not store None in it. Found while working on narrowhg.
> >
> >diff -r 87a7c0d403ff -r d2c313417026 mercurial/manifest.py
> >--- a/mercurial/manifest.py    Tue Oct 18 02:09:08 2016 +0200
> >+++ b/mercurial/manifest.py    Mon Oct 17 22:51:22 2016 -0700
> >@@ -1210,7 +1210,8 @@
> >                 n = self.addrevision(text, transaction, link, p1, p2)
> >                 arraytext = array.array('c', text)
> >
> >-        self.fulltextcache[n] = arraytext
> >+        if arraytext is not None:
> >+            self.fulltextcache[n] = arraytext
> >
> >         return n
> >
> >@@ -1506,7 +1507,8 @@
> >             m = self._newmanifest(text)
> >             arraytext = array.array('c', text)
> >         self._mancache[node] = m
> >-        self.fulltextcache[node] = arraytext
> >+        if arraytext is not None:
> >+            self.fulltextcache[node] = arraytext
> >         return m
> >
> >     def readshallow(self, node):
>
> LGTM.  Is there a tree code path that hits this that is not covered by
> tests?
>
>
Yes, what our tests in narrowhg triggered was a line in bundlemanifest:
self.fulltextcache[node].tostring(). The test case was trying to pull from
a bundle. We do have a test case for that in test-treemanifest.t. I don't
know what the difference is. Perhaps something means it gets cached in our
narrowhg test case, but not in the test-treemanifest.t one.

Patch

diff -r 87a7c0d403ff -r d2c313417026 mercurial/manifest.py
--- a/mercurial/manifest.py	Tue Oct 18 02:09:08 2016 +0200
+++ b/mercurial/manifest.py	Mon Oct 17 22:51:22 2016 -0700
@@ -1210,7 +1210,8 @@ 
                 n = self.addrevision(text, transaction, link, p1, p2)
                 arraytext = array.array('c', text)
 
-        self.fulltextcache[n] = arraytext
+        if arraytext is not None:
+            self.fulltextcache[n] = arraytext
 
         return n
 
@@ -1506,7 +1507,8 @@ 
             m = self._newmanifest(text)
             arraytext = array.array('c', text)
         self._mancache[node] = m
-        self.fulltextcache[node] = arraytext
+        if arraytext is not None:
+            self.fulltextcache[node] = arraytext
         return m
 
     def readshallow(self, node):