[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Accepted vlc 0.8.6-svn20061012.debian-3 (source i386 all)



Hi Sam,

On Thu, Jan 11, 2007 at 10:32:10PM +0000, Sam Hocevar wrote:
>  vlc (0.8.6-svn20061012.debian-3) testing-proposed-updates; urgency=high

Reviewing this now, I have a few problems with parts of the diff that I'd
like your help clearing up.

- 020_notify.diff includes a patch to prepend DATA_PATH to
  "/usr/share/pixmaps/vlc.png".  Why?  This isn't documented in the
  changelog, and doesn't make any sense to me anyway.
- patch-documentation-0.8.6debian-0.8.6a.diff contains:

+@@ -150,7 +150,13 @@
+     char *psz_server = 0;
+     int i_result;
+
+-    if( !p_access->b_force ) return VLC_EGENERIC;
++    if( !p_access->psz_access || (
++        strncmp( p_access->psz_access, "rtsp", 4 ) &&
++        strncmp( p_access->psz_access, "pnm", 3 )  &&
++        strncmp( p_access->psz_access, "realrtsp", 8 ) ))
++    {
++            return VLC_EGENERIC;
++    }
+
+     p_access->pf_read = NULL;
+     p_access->pf_block = BlockRead;

  How is this a documentation fix?
- 000_bootstrap.diff seems to be enabling maintainer mode by default.  Why
  is this a) appropriate, and b) necessary?  Are you certain that there are
  no possibilities of timestamp skew here that would cause a FTBFS on
  autobuilders when autofoo are not installed?  (The answer here appears to
  depend on the order in which the patches are applied, which I haven't
  analyzed.)
- patch-missing-locks-0.8.6debian-0.8.6a.diff: is playlist_LockDelete() a
  superset of playlist_Delete()?
- patch-sanitise-javascript-0.8.6debian-0.8.6a.diff: why is it /wrong/ to
  escape \ characters here?
- patch-badly-initialised-data-0.8.6debian-0.8.6a.diff:

+@@ -450,8 +450,8 @@ static void default_clut_init( decoder_t
+         }
+
+         p_sys->default_clut.c_4b[i].Y = RGB_TO_Y(R,G,B);
+-        p_sys->default_clut.c_4b[i].Cr = RGB_TO_U(R,G,B);
+-        p_sys->default_clut.c_4b[i].Cb = RGB_TO_V(R,G,B);
++        p_sys->default_clut.c_4b[i].Cr = RGB_TO_V(R,G,B);
++        p_sys->default_clut.c_4b[i].Cb = RGB_TO_U(R,G,B);
+         p_sys->default_clut.c_4b[i].T = T;
+     }
+

  this reverses the initialization of the .Cr and .Cb elements, and in fact,
  disagrees with an immediately preceeding section of the same patch.  Can
  you explain?  (It does agree with an immediately /following/ section of
  the patch, so perhaps it's the first section that's wrong?)

The debdiff also includes changes specific to architectures which aren't
releasing with etch, and are apparently not planning to do their own
rebuilds of etch, and even includes added patches from unstable that have
only been disabled in the patch sequence file.  This makes for a very
time-consuming diff to review, and certainly leaves me less confident that I
haven't overlooked any regressions.  I would be much more comfortable
letting this straight into testing if this weren't the case.

Thanks,
-- 
Steve Langasek                   Give me a lever long enough and a Free OS
Debian Developer                   to set it on, and I can move the world.
vorlon@debian.org                                   http://www.debian.org/



Reply to: