Patchwork [15,of,21,RFC] revlog: allow importing revision deltas based on a censored file tombstone

login
register
mail settings
Submitter michaeljedgar@gmail.com
Date Sept. 11, 2014, 12:26 a.m.
Message ID <1e85b2cd488fb1b13517.1410395176@adgar-macbookpro3.roam.corp.google.com>
Download mbox | patch
Permalink /patch/5790/
State Changes Requested
Headers show

Comments

michaeljedgar@gmail.com - Sept. 11, 2014, 12:26 a.m.
# HG changeset patch
# User Mike Edgar <adgar@google.com>
# Date 1409776499 14400
#      Wed Sep 03 16:34:59 2014 -0400
# Node ID 1e85b2cd488fb1b13517345b3ff425e0da7b48b9
# Parent  8adefa28d6fb5816ff9623408ef53c1d2d70f884
revlog: allow importing revision deltas based on a censored file tombstone

Whenever a changegroup is imported with a non-null delta base, the delta base
will be materialized, which could fail if that base revision is censored. This
change allows delta application against the tombstone to proceed, setting the
base text to the well-known tombstone text.
Matt Mackall - Sept. 11, 2014, 5:58 p.m.
On Wed, 2014-09-10 at 20:26 -0400, michaeljedgar@gmail.com wrote:
> # HG changeset patch
> # User Mike Edgar <adgar@google.com>
> # Date 1409776499 14400
> #      Wed Sep 03 16:34:59 2014 -0400
> # Node ID 1e85b2cd488fb1b13517345b3ff425e0da7b48b9
> # Parent  8adefa28d6fb5816ff9623408ef53c1d2d70f884
> revlog: allow importing revision deltas based on a censored file tombstone

It's great that you're thinking ahead this far, but we don't need to see
all of this in your initial patchbombs. Shorter batches are better as
reviewer attention is scarce.
michaeljedgar@gmail.com - Sept. 14, 2014, 4:11 p.m.
On Thu, Sep 11, 2014 at 1:58 PM, Matt Mackall <mpm@selenic.com> wrote:
>
> On Wed, 2014-09-10 at 20:26 -0400, michaeljedgar@gmail.com wrote:
> > # HG changeset patch
> > # User Mike Edgar <adgar@google.com>
> > # Date 1409776499 14400
> > #      Wed Sep 03 16:34:59 2014 -0400
> > # Node ID 1e85b2cd488fb1b13517345b3ff425e0da7b48b9
> > # Parent  8adefa28d6fb5816ff9623408ef53c1d2d70f884
> > revlog: allow importing revision deltas based on a censored file tombstone
>
> It's great that you're thinking ahead this far, but we don't need to see
> all of this in your initial patchbombs. Shorter batches are better as
> reviewer attention is scarce.

That's good to know, I wasn't sure how best to share all this work.

One question: what should I do when later patches justify decisions
made in earlier patches? For example, this patch and its 3 successors
require the "metadata" attribute on CensoredNodeError which you noted
as questionable in patch 3 of 21.

Should I first send out a shorter branch without that attribute, then
add the attribute when I send a branch which uses it? That certainly
makes sense for this example, given that there were 12 patches between
introduction and use.
Matt Mackall - Sept. 14, 2014, 8:16 p.m.
On Sun, 2014-09-14 at 12:11 -0400, Michael Edgar wrote:
> On Thu, Sep 11, 2014 at 1:58 PM, Matt Mackall <mpm@selenic.com> wrote:
> >
> > On Wed, 2014-09-10 at 20:26 -0400, michaeljedgar@gmail.com wrote:
> > > # HG changeset patch
> > > # User Mike Edgar <adgar@google.com>
> > > # Date 1409776499 14400
> > > #      Wed Sep 03 16:34:59 2014 -0400
> > > # Node ID 1e85b2cd488fb1b13517345b3ff425e0da7b48b9
> > > # Parent  8adefa28d6fb5816ff9623408ef53c1d2d70f884
> > > revlog: allow importing revision deltas based on a censored file tombstone
> >
> > It's great that you're thinking ahead this far, but we don't need to see
> > all of this in your initial patchbombs. Shorter batches are better as
> > reviewer attention is scarce.
> 
> That's good to know, I wasn't sure how best to share all this work.
> 
> One question: what should I do when later patches justify decisions
> made in earlier patches? For example, this patch and its 3 successors
> require the "metadata" attribute on CensoredNodeError which you noted
> as questionable in patch 3 of 21.

Forward references are fine.

"This metadata attribute will be needed later when.."

> Should I first send out a shorter branch without that attribute, then
> add the attribute when I send a branch which uses it? That certainly
> makes sense for this example, given that there were 12 patches between
> introduction and use.

Or that.

Patch

diff -r 8adefa28d6fb -r 1e85b2cd488f mercurial/revlog.py
--- a/mercurial/revlog.py	Wed Sep 03 16:34:29 2014 -0400
+++ b/mercurial/revlog.py	Wed Sep 03 16:34:59 2014 -0400
@@ -1167,7 +1167,10 @@ 
             if dfh:
                 dfh.flush()
             ifh.flush()
-            basetext = self.revision(self.node(cachedelta[0]))
+            try:
+                basetext = self.revision(self.node(cachedelta[0]))
+            except CensoredNodeError, e:
+                basetext = e.metadata
             btext[0] = mdiff.patch(basetext, cachedelta[1])
             try:
                 self.checkhash(btext[0], p1, p2, node)