Patchwork treemanifest: fix bad argument order to treemanifestctx

login
register
mail settings
Submitter via Mercurial-devel
Date Oct. 17, 2016, 11:13 p.m.
Message ID <b36a81cd4015b9742d1f.1476746036@martinvonz.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/17163/
State Accepted
Headers show

Comments

via Mercurial-devel - Oct. 17, 2016, 11:13 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1476745932 25200
#      Mon Oct 17 16:12:12 2016 -0700
# Node ID b36a81cd4015b9742d1fbb0d5f94207e7a400cdb
# Parent  8a864844d5a0c34bdb24d2e098a0cd339e32e020
treemanifest: fix bad argument order to treemanifestctx

Found by running tests with _treeinmem (both of them) modified to be
True.
Durham Goode - Oct. 17, 2016, 11:34 p.m.
On 10/17/16, 4:13 PM, "Martin von Zweigbergk" <martinvonz@google.com> wrote:

># HG changeset patch

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

># Date 1476745932 25200

>#      Mon Oct 17 16:12:12 2016 -0700

># Node ID b36a81cd4015b9742d1fbb0d5f94207e7a400cdb

># Parent  8a864844d5a0c34bdb24d2e098a0cd339e32e020

>treemanifest: fix bad argument order to treemanifestctx

>

>Found by running tests with _treeinmem (both of them) modified to be

>True.

>

>diff -r 8a864844d5a0 -r b36a81cd4015 mercurial/manifest.py

>--- a/mercurial/manifest.py	Wed Oct 12 21:33:45 2016 +0200

>+++ b/mercurial/manifest.py	Mon Oct 17 16:12:12 2016 -0700

>@@ -1386,7 +1386,7 @@

>         # Need to perform a slow delta

>         revlog = self._revlog

>         r0 = revlog.deltaparent(revlog.rev(self._node))

>-        m0 = treemanifestctx(revlog, revlog.node(r0), dir=self._dir).read()

>+        m0 = treemanifestctx(revlog, self._dir, revlog.node(r0)).read()

>         m1 = self.read()

>         md = treemanifest(dir=self._dir)

>         for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():


Looks good to me.  What would be required to get some _treeinmem code coverage in the tests?
via Mercurial-devel - Oct. 17, 2016, 11:39 p.m.
On Mon, Oct 17, 2016 at 4:34 PM Durham Goode <durham@fb.com> wrote:

> On 10/17/16, 4:13 PM, "Martin von Zweigbergk" <martinvonz@google.com>
> wrote:
>
> ># HG changeset patch
> ># User Martin von Zweigbergk <martinvonz@google.com>
> ># Date 1476745932 25200
> >#      Mon Oct 17 16:12:12 2016 -0700
> ># Node ID b36a81cd4015b9742d1fbb0d5f94207e7a400cdb
> ># Parent  8a864844d5a0c34bdb24d2e098a0cd339e32e020
> >treemanifest: fix bad argument order to treemanifestctx
> >
> >Found by running tests with _treeinmem (both of them) modified to be
> >True.
> >
> >diff -r 8a864844d5a0 -r b36a81cd4015 mercurial/manifest.py
> >--- a/mercurial/manifest.py    Wed Oct 12 21:33:45 2016 +0200
> >+++ b/mercurial/manifest.py    Mon Oct 17 16:12:12 2016 -0700
> >@@ -1386,7 +1386,7 @@
> >         # Need to perform a slow delta
> >         revlog = self._revlog
> >         r0 = revlog.deltaparent(revlog.rev(self._node))
> >-        m0 = treemanifestctx(revlog, revlog.node(r0),
> dir=self._dir).read()
> >+        m0 = treemanifestctx(revlog, self._dir, revlog.node(r0)).read()
> >         m1 = self.read()
> >         md = treemanifest(dir=self._dir)
> >         for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():
>
> Looks good to me.  What would be required to get some _treeinmem code
> coverage in the tests?
>
>
Twice the runtime? :-) But seriously, I suspect that's reason enough that
we don't want it enabled by default. And I don't know what subset would be
useful to run it on either.

But it was much less broken than I expected, actually. This was all that
was needed. I expected it to be broken before your series even started, but
it wasn't. There is one failing test case (test-rebase-newancestor.t) that
I have not understood why it's failing. It scares me a bit that I still
don't know why it's failing.
Durham Goode - Oct. 17, 2016, 11:42 p.m.
From: Martin von Zweigbergk <martinvonz@google.com>

Date: Monday, October 17, 2016 at 4:39 PM
To: Durham Goode <durham@fb.com>, "mercurial-devel@mercurial-scm.org" <mercurial-devel@mercurial-scm.org>
Subject: Re: [PATCH] treemanifest: fix bad argument order to treemanifestctx


On Mon, Oct 17, 2016 at 4:34 PM Durham Goode <durham@fb.com<mailto:durham@fb.com>> wrote:
On 10/17/16, 4:13 PM, "Martin von Zweigbergk" <martinvonz@google.com<mailto:martinvonz@google.com>> wrote:

># HG changeset patch

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

># Date 1476745932 25200

>#      Mon Oct 17 16:12:12 2016 -0700

># Node ID b36a81cd4015b9742d1fbb0d5f94207e7a400cdb

># Parent  8a864844d5a0c34bdb24d2e098a0cd339e32e020

>treemanifest: fix bad argument order to treemanifestctx

>

>Found by running tests with _treeinmem (both of them) modified to be

>True.

>

>diff -r 8a864844d5a0 -r b36a81cd4015 mercurial/manifest.py

>--- a/mercurial/manifest.py    Wed Oct 12 21:33:45 2016 +0200

>+++ b/mercurial/manifest.py    Mon Oct 17 16:12:12 2016 -0700

>@@ -1386,7 +1386,7 @@

>         # Need to perform a slow delta

>         revlog = self._revlog

>         r0 = revlog.deltaparent(revlog.rev(self._node))

>-        m0 = treemanifestctx(revlog, revlog.node(r0), dir=self._dir).read()

>+        m0 = treemanifestctx(revlog, self._dir, revlog.node(r0)).read()

>         m1 = self.read()

>         md = treemanifest(dir=self._dir)

>         for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():


Looks good to me.  What would be required to get some _treeinmem code coverage in the tests?

Twice the runtime? :-) But seriously, I suspect that's reason enough that we don't want it enabled by default. And I don't know what subset would be useful to run it on either.

But it was much less broken than I expected, actually. This was all that was needed. I expected it to be broken before your series even started, but it wasn't. There is one failing test case (test-rebase-newancestor.t) that I have not understood why it's failing. It scares me a bit that I still don't know why it's failing.

Just doing one run of test-manifest.t with it on would probably be enough to catch most issues.  What’s the exact command you used to run the tests?  So I can run it myself before sending more patches.
via Mercurial-devel - Oct. 17, 2016, 11:45 p.m.
On Mon, Oct 17, 2016 at 4:42 PM Durham Goode <durham@fb.com> wrote:





*From: *Martin von Zweigbergk <martinvonz@google.com>
*Date: *Monday, October 17, 2016 at 4:39 PM
*To: *Durham Goode <durham@fb.com>, "mercurial-devel@mercurial-scm.org" <
mercurial-devel@mercurial-scm.org>
*Subject: *Re: [PATCH] treemanifest: fix bad argument order to
treemanifestctx





On Mon, Oct 17, 2016 at 4:34 PM Durham Goode <durham@fb.com> wrote:

On 10/17/16, 4:13 PM, "Martin von Zweigbergk" <martinvonz@google.com> wrote:

># HG changeset patch
># User Martin von Zweigbergk <martinvonz@google.com>
># Date 1476745932 25200
>#      Mon Oct 17 16:12:12 2016 -0700
># Node ID b36a81cd4015b9742d1fbb0d5f94207e7a400cdb
># Parent  8a864844d5a0c34bdb24d2e098a0cd339e32e020
>treemanifest: fix bad argument order to treemanifestctx
>
>Found by running tests with _treeinmem (both of them) modified to be
>True.
>
>diff -r 8a864844d5a0 -r b36a81cd4015 mercurial/manifest.py
>--- a/mercurial/manifest.py    Wed Oct 12 21:33:45 2016 +0200
>+++ b/mercurial/manifest.py    Mon Oct 17 16:12:12 2016 -0700
>@@ -1386,7 +1386,7 @@
>         # Need to perform a slow delta
>         revlog = self._revlog
>         r0 = revlog.deltaparent(revlog.rev(self._node))
>-        m0 = treemanifestctx(revlog, revlog.node(r0),
dir=self._dir).read()
>+        m0 = treemanifestctx(revlog, self._dir, revlog.node(r0)).read()
>         m1 = self.read()
>         md = treemanifest(dir=self._dir)
>         for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():

Looks good to me.  What would be required to get some _treeinmem code
coverage in the tests?



Twice the runtime? :-) But seriously, I suspect that's reason enough that
we don't want it enabled by default. And I don't know what subset would be
useful to run it on either.



But it was much less broken than I expected, actually. This was all that
was needed. I expected it to be broken before your series even started, but
it wasn't. There is one failing test case (test-rebase-newancestor.t) that
I have not understood why it's failing. It scares me a bit that I still
don't know why it's failing.



Just doing one run of test-manifest.t with it on would probably be enough
to catch most issues.  What’s the exact command you used to run the tests?
So I can run it myself before sending more patches.


 sed -i -e 's/_treeinmem =.*/_treeinmem = True/g' mercurial/manifest.py
Augie Fackler - Oct. 17, 2016, 11:47 p.m.
On Mon, Oct 17, 2016 at 11:39:51PM +0000, Martin von Zweigbergk via Mercurial-devel wrote:
> On Mon, Oct 17, 2016 at 4:34 PM Durham Goode <durham@fb.com> wrote:
>
> > On 10/17/16, 4:13 PM, "Martin von Zweigbergk" <martinvonz@google.com>
> > wrote:
> >
> > ># HG changeset patch
> > ># User Martin von Zweigbergk <martinvonz@google.com>
> > ># Date 1476745932 25200
> > >#      Mon Oct 17 16:12:12 2016 -0700
> > ># Node ID b36a81cd4015b9742d1fbb0d5f94207e7a400cdb
> > ># Parent  8a864844d5a0c34bdb24d2e098a0cd339e32e020
> > >treemanifest: fix bad argument order to treemanifestctx
> > >
> > >Found by running tests with _treeinmem (both of them) modified to be
> > >True.
> > >
> > >diff -r 8a864844d5a0 -r b36a81cd4015 mercurial/manifest.py
> > >--- a/mercurial/manifest.py    Wed Oct 12 21:33:45 2016 +0200
> > >+++ b/mercurial/manifest.py    Mon Oct 17 16:12:12 2016 -0700
> > >@@ -1386,7 +1386,7 @@
> > >         # Need to perform a slow delta
> > >         revlog = self._revlog
> > >         r0 = revlog.deltaparent(revlog.rev(self._node))
> > >-        m0 = treemanifestctx(revlog, revlog.node(r0),
> > dir=self._dir).read()
> > >+        m0 = treemanifestctx(revlog, self._dir, revlog.node(r0)).read()
> > >         m1 = self.read()
> > >         md = treemanifest(dir=self._dir)
> > >         for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():
> >
> > Looks good to me.  What would be required to get some _treeinmem code
> > coverage in the tests?

LGTM as well - I can't get the patch to apply - maybe some damage in
flight. Please feel encouraged to just push it to committed and let us
know here that you've done that.
via Mercurial-devel - Oct. 17, 2016, 11:51 p.m.
On Mon, Oct 17, 2016 at 4:47 PM Augie Fackler <raf@durin42.com> wrote:

> On Mon, Oct 17, 2016 at 11:39:51PM +0000, Martin von Zweigbergk via
> Mercurial-devel wrote:
> > On Mon, Oct 17, 2016 at 4:34 PM Durham Goode <durham@fb.com> wrote:
> >
> > > On 10/17/16, 4:13 PM, "Martin von Zweigbergk" <martinvonz@google.com>
> > > wrote:
> > >
> > > ># HG changeset patch
> > > ># User Martin von Zweigbergk <martinvonz@google.com>
> > > ># Date 1476745932 25200
> > > >#      Mon Oct 17 16:12:12 2016 -0700
> > > ># Node ID b36a81cd4015b9742d1fbb0d5f94207e7a400cdb
> > > ># Parent  8a864844d5a0c34bdb24d2e098a0cd339e32e020
> > > >treemanifest: fix bad argument order to treemanifestctx
> > > >
> > > >Found by running tests with _treeinmem (both of them) modified to be
> > > >True.
> > > >
> > > >diff -r 8a864844d5a0 -r b36a81cd4015 mercurial/manifest.py
> > > >--- a/mercurial/manifest.py    Wed Oct 12 21:33:45 2016 +0200
> > > >+++ b/mercurial/manifest.py    Mon Oct 17 16:12:12 2016 -0700
> > > >@@ -1386,7 +1386,7 @@
> > > >         # Need to perform a slow delta
> > > >         revlog = self._revlog
> > > >         r0 = revlog.deltaparent(revlog.rev(self._node))
> > > >-        m0 = treemanifestctx(revlog, revlog.node(r0),
> > > dir=self._dir).read()
> > > >+        m0 = treemanifestctx(revlog, self._dir,
> revlog.node(r0)).read()
> > > >         m1 = self.read()
> > > >         md = treemanifest(dir=self._dir)
> > > >         for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():
> > >
> > > Looks good to me.  What would be required to get some _treeinmem code
> > > coverage in the tests?
>
> LGTM as well - I can't get the patch to apply - maybe some damage in
> flight. Please feel encouraged to just push it to committed and let us
> know here that you've done that.
>

Done.

Patch

diff -r 8a864844d5a0 -r b36a81cd4015 mercurial/manifest.py
--- a/mercurial/manifest.py	Wed Oct 12 21:33:45 2016 +0200
+++ b/mercurial/manifest.py	Mon Oct 17 16:12:12 2016 -0700
@@ -1386,7 +1386,7 @@ 
         # Need to perform a slow delta
         revlog = self._revlog
         r0 = revlog.deltaparent(revlog.rev(self._node))
-        m0 = treemanifestctx(revlog, revlog.node(r0), dir=self._dir).read()
+        m0 = treemanifestctx(revlog, self._dir, revlog.node(r0)).read()
         m1 = self.read()
         md = treemanifest(dir=self._dir)
         for f, ((n0, fl0), (n1, fl1)) in m0.diff(m1).iteritems():