Patchwork test-verify: add a testcase where the file has magic meta header

login
register
mail settings
Submitter Jun Wu
Date March 29, 2017, 8:45 p.m.
Message ID <e9fda3b8614a8b701bd4.1490820327@x1c>
Download mbox | patch
Permalink /patch/19830/
State Changes Requested
Headers show

Comments

Jun Wu - March 29, 2017, 8:45 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1490820077 25200
#      Wed Mar 29 13:41:17 2017 -0700
# Node ID e9fda3b8614a8b701bd48041afa8b709e1227f27
# Parent  cda83a1bfb3ac3a23cfa158c407be93755c1018e
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r e9fda3b8614a
test-verify: add a testcase where the file has magic meta header

We use a magic header "\1\n" to store metadata like renames. See filelog.py.
The patch adds tests about files with the special header.
Ryan McElroy - March 30, 2017, 9:02 a.m.
On 3/29/17 9:45 PM, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1490820077 25200
> #      Wed Mar 29 13:41:17 2017 -0700
> # Node ID e9fda3b8614a8b701bd48041afa8b709e1227f27
> # Parent  cda83a1bfb3ac3a23cfa158c407be93755c1018e
> test-verify: add a testcase where the file has magic meta header
>
> We use a magic header "\1\n" to store metadata like renames. See filelog.py.
> The patch adds tests about files with the special header.

Sure, but why? Just to prevent regressions of handling files with this 
data that "looks like" metadata in them? What's the motivation for 
adding this test? Just future safety or did you run into something?

I'm not against it -- adding this test seems like a good idea, but I'd 
love more context on what led you to write it.

>
> diff --git a/tests/test-verify.t b/tests/test-verify.t
> --- a/tests/test-verify.t
> +++ b/tests/test-verify.t
> @@ -318,2 +318,18 @@ test revlog format 0
>     1 files, 1 changesets, 1 total revisions
>     $ cd ..
> +
> +Files with the meta header (see comment in filelog.size)
> +
> +  $ cat > $TESTTMP/writemeta.py <<EOF
> +  > import sys
> +  > with open(sys.argv[1], 'wb') as f:
> +  >     f.write(b'\x01\n\x01\n%s' % sys.argv[2])
> +  > EOF
> +
> +  $ hg init c
> +  $ cd c
> +  $ $PYTHON $TESTTMP/writemeta.py a ''
> +  $ $PYTHON $TESTTMP/writemeta.py b 'b'
> +  $ hg ci -Aqm meta a b
> +  $ hg verify -q
> +  $ cd ..
>
Yuya Nishihara - March 31, 2017, 1:41 p.m.
On Thu, 30 Mar 2017 10:02:34 +0100, Ryan McElroy wrote:
> On 3/29/17 9:45 PM, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1490820077 25200
> > #      Wed Mar 29 13:41:17 2017 -0700
> > # Node ID e9fda3b8614a8b701bd48041afa8b709e1227f27
> > # Parent  cda83a1bfb3ac3a23cfa158c407be93755c1018e
> > test-verify: add a testcase where the file has magic meta header
> >
> > We use a magic header "\1\n" to store metadata like renames. See filelog.py.
> > The patch adds tests about files with the special header.
> 
> Sure, but why? Just to prevent regressions of handling files with this 
> data that "looks like" metadata in them? What's the motivation for 
> adding this test? Just future safety or did you run into something?
> 
> I'm not against it -- adding this test seems like a good idea, but I'd 
> love more context on what led you to write it.

I remember I ran into a weird bug in production and fixed as 012b285cf643,
so I'm sure this test will avoid future bugs caused by '\1\n'.

Jun, can you resend it with more context, perhaps in your verify/rawsize
series? (There seem multiple threads related to this, but I don't follow
all of them.)

Patch

diff --git a/tests/test-verify.t b/tests/test-verify.t
--- a/tests/test-verify.t
+++ b/tests/test-verify.t
@@ -318,2 +318,18 @@  test revlog format 0
   1 files, 1 changesets, 1 total revisions
   $ cd ..
+
+Files with the meta header (see comment in filelog.size)
+
+  $ cat > $TESTTMP/writemeta.py <<EOF
+  > import sys
+  > with open(sys.argv[1], 'wb') as f:
+  >     f.write(b'\x01\n\x01\n%s' % sys.argv[2])
+  > EOF
+
+  $ hg init c
+  $ cd c
+  $ $PYTHON $TESTTMP/writemeta.py a ''
+  $ $PYTHON $TESTTMP/writemeta.py b 'b'
+  $ hg ci -Aqm meta a b
+  $ hg verify -q
+  $ cd ..