Patchwork [2,of,4] revlog: tweak wording and logic for flags validation

login
register
mail settings
Submitter Gregory Szorc
Date May 20, 2017, 3:58 a.m.
Message ID <26a43d84eb714ee6bba0.1495252738@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/20755/
State Accepted
Headers show

Comments

Gregory Szorc - May 20, 2017, 3:58 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1495249850 25200
#      Fri May 19 20:10:50 2017 -0700
# Node ID 26a43d84eb714ee6bba04f21c0264ded9215a3bd
# Parent  40748da9c7c56836f815582cf9a367097755859d
revlog: tweak wording and logic for flags validation

First, the logic around the if..elif..elif was subtly wrong
and sub-optimal because all branches would be tested as long as
the revlog was valid. This patch changes things so it behaves like
a switch statement over the revlog version.

While I was here, I also tweaked error strings to make them
consistent and to read better.
Yuya Nishihara - May 21, 2017, 12:24 p.m.
On Fri, 19 May 2017 20:58:58 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1495249850 25200
> #      Fri May 19 20:10:50 2017 -0700
> # Node ID 26a43d84eb714ee6bba04f21c0264ded9215a3bd
> # Parent  40748da9c7c56836f815582cf9a367097755859d
> revlog: tweak wording and logic for flags validation

> --- a/mercurial/revlog.py
> +++ b/mercurial/revlog.py
> @@ -326,15 +326,19 @@ class revlog(object):
>          self._generaldelta = v & FLAG_GENERALDELTA
>          flags = v & ~0xFFFF
>          fmt = v & 0xFFFF
> -        if fmt == REVLOGV0 and flags:
> -            raise RevlogError(_("index %s unknown flags %#04x for format v0")
> -                              % (self.indexfile, flags >> 16))
> -        elif fmt == REVLOGV1 and flags & ~REVLOGV1_FLAGS:
> -            raise RevlogError(_("index %s unknown flags %#04x for revlogng")
> -                              % (self.indexfile, flags >> 16))
> +        if fmt == REVLOGV0:
> +            if flags:
> +                raise RevlogError(_('unknown flags (%#04x) in version %d '
> +                                    'revlog %s') %
> +                                    (flags >> 16, fmt, self.indexfile))
> +        elif fmt == REVLOGV1:
> +            if flags & ~REVLOGV1_FLAGS:
> +                raise RevlogError(_('unknown flags (%#04x) in version %d '
> +                                    'revlog %s') %
> +                                  (flags >> 16, fmt, self.indexfile))
>          elif fmt > REVLOGV1:

Nit: maybe this could simply be "else:".

> -            raise RevlogError(_("index %s unknown format %d")
> -                              % (self.indexfile, fmt))
> +            raise RevlogError(_('unknown version (%d) in revlog %s') %
> +                              (fmt, self.indexfile))
Gregory Szorc - May 22, 2017, 1:14 a.m.
On Sun, May 21, 2017 at 5:24 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Fri, 19 May 2017 20:58:58 -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1495249850 25200
> > #      Fri May 19 20:10:50 2017 -0700
> > # Node ID 26a43d84eb714ee6bba04f21c0264ded9215a3bd
> > # Parent  40748da9c7c56836f815582cf9a367097755859d
> > revlog: tweak wording and logic for flags validation
>
> > --- a/mercurial/revlog.py
> > +++ b/mercurial/revlog.py
> > @@ -326,15 +326,19 @@ class revlog(object):
> >          self._generaldelta = v & FLAG_GENERALDELTA
> >          flags = v & ~0xFFFF
> >          fmt = v & 0xFFFF
> > -        if fmt == REVLOGV0 and flags:
> > -            raise RevlogError(_("index %s unknown flags %#04x for
> format v0")
> > -                              % (self.indexfile, flags >> 16))
> > -        elif fmt == REVLOGV1 and flags & ~REVLOGV1_FLAGS:
> > -            raise RevlogError(_("index %s unknown flags %#04x for
> revlogng")
> > -                              % (self.indexfile, flags >> 16))
> > +        if fmt == REVLOGV0:
> > +            if flags:
> > +                raise RevlogError(_('unknown flags (%#04x) in version
> %d '
> > +                                    'revlog %s') %
> > +                                    (flags >> 16, fmt, self.indexfile))
> > +        elif fmt == REVLOGV1:
> > +            if flags & ~REVLOGV1_FLAGS:
> > +                raise RevlogError(_('unknown flags (%#04x) in version
> %d '
> > +                                    'revlog %s') %
> > +                                  (flags >> 16, fmt, self.indexfile))
> >          elif fmt > REVLOGV1:
>
> Nit: maybe this could simply be "else:".
>

I agree.

Since these haven't been pushed to hg-committed, can you change in flight?


>
> > -            raise RevlogError(_("index %s unknown format %d")
> > -                              % (self.indexfile, fmt))
> > +            raise RevlogError(_('unknown version (%d) in revlog %s') %
> > +                              (fmt, self.indexfile))
>
Yuya Nishihara - May 22, 2017, 1:05 p.m.
On Sun, 21 May 2017 18:14:35 -0700, Gregory Szorc wrote:
> On Sun, May 21, 2017 at 5:24 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> > On Fri, 19 May 2017 20:58:58 -0700, Gregory Szorc wrote:
> > > # HG changeset patch
> > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > # Date 1495249850 25200
> > > #      Fri May 19 20:10:50 2017 -0700
> > > # Node ID 26a43d84eb714ee6bba04f21c0264ded9215a3bd
> > > # Parent  40748da9c7c56836f815582cf9a367097755859d
> > > revlog: tweak wording and logic for flags validation
> >
> > > --- a/mercurial/revlog.py
> > > +++ b/mercurial/revlog.py
> > > @@ -326,15 +326,19 @@ class revlog(object):
> > >          self._generaldelta = v & FLAG_GENERALDELTA
> > >          flags = v & ~0xFFFF
> > >          fmt = v & 0xFFFF
> > > -        if fmt == REVLOGV0 and flags:
> > > -            raise RevlogError(_("index %s unknown flags %#04x for
> > format v0")
> > > -                              % (self.indexfile, flags >> 16))
> > > -        elif fmt == REVLOGV1 and flags & ~REVLOGV1_FLAGS:
> > > -            raise RevlogError(_("index %s unknown flags %#04x for
> > revlogng")
> > > -                              % (self.indexfile, flags >> 16))
> > > +        if fmt == REVLOGV0:
> > > +            if flags:
> > > +                raise RevlogError(_('unknown flags (%#04x) in version
> > %d '
> > > +                                    'revlog %s') %
> > > +                                    (flags >> 16, fmt, self.indexfile))
> > > +        elif fmt == REVLOGV1:
> > > +            if flags & ~REVLOGV1_FLAGS:
> > > +                raise RevlogError(_('unknown flags (%#04x) in version
> > %d '
> > > +                                    'revlog %s') %
> > > +                                  (flags >> 16, fmt, self.indexfile))
> > >          elif fmt > REVLOGV1:
> >
> > Nit: maybe this could simply be "else:".
> >
> 
> I agree.
> 
> Since these haven't been pushed to hg-committed, can you change in flight?

Okay, updated it and queued the first three. Thanks.

Patch

diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -326,15 +326,19 @@  class revlog(object):
         self._generaldelta = v & FLAG_GENERALDELTA
         flags = v & ~0xFFFF
         fmt = v & 0xFFFF
-        if fmt == REVLOGV0 and flags:
-            raise RevlogError(_("index %s unknown flags %#04x for format v0")
-                              % (self.indexfile, flags >> 16))
-        elif fmt == REVLOGV1 and flags & ~REVLOGV1_FLAGS:
-            raise RevlogError(_("index %s unknown flags %#04x for revlogng")
-                              % (self.indexfile, flags >> 16))
+        if fmt == REVLOGV0:
+            if flags:
+                raise RevlogError(_('unknown flags (%#04x) in version %d '
+                                    'revlog %s') %
+                                    (flags >> 16, fmt, self.indexfile))
+        elif fmt == REVLOGV1:
+            if flags & ~REVLOGV1_FLAGS:
+                raise RevlogError(_('unknown flags (%#04x) in version %d '
+                                    'revlog %s') %
+                                  (flags >> 16, fmt, self.indexfile))
         elif fmt > REVLOGV1:
-            raise RevlogError(_("index %s unknown format %d")
-                              % (self.indexfile, fmt))
+            raise RevlogError(_('unknown version (%d) in revlog %s') %
+                              (fmt, self.indexfile))
 
         self.storedeltachains = True
 
diff --git a/tests/test-requires.t b/tests/test-requires.t
--- a/tests/test-requires.t
+++ b/tests/test-requires.t
@@ -5,7 +5,7 @@ 
   $ hg commit -m test
   $ rm .hg/requires
   $ hg tip
-  abort: index 00changelog.i unknown format 2!
+  abort: unknown version (2) in revlog 00changelog.i!
   [255]
   $ echo indoor-pool > .hg/requires
   $ hg tip
diff --git a/tests/test-revlog.t b/tests/test-revlog.t
--- a/tests/test-revlog.t
+++ b/tests/test-revlog.t
@@ -7,7 +7,7 @@  Flags on revlog version 0 are rejected
   ...     fh.write('\x00\x01\x00\x00')
 
   $ hg log
-  abort: index 00changelog.i unknown flags 0x01 for format v0!
+  abort: unknown flags (0x01) in version 0 revlog 00changelog.i!
   [255]
 
 Unknown flags on revlog version 1 are rejected
@@ -16,7 +16,7 @@  Unknown flags on revlog version 1 are re
   ...     fh.write('\x00\x04\x00\x01')
 
   $ hg log
-  abort: index 00changelog.i unknown flags 0x04 for revlogng!
+  abort: unknown flags (0x04) in version 1 revlog 00changelog.i!
   [255]
 
 Unknown version is rejected
@@ -25,7 +25,7 @@  Unknown version is rejected
   ...     fh.write('\x00\x00\x00\x02')
 
   $ hg log
-  abort: index 00changelog.i unknown format 2!
+  abort: unknown version (2) in revlog 00changelog.i!
   [255]
 
   $ cd ..