Re: Accepted vlc 0.8.6-svn20061012.debian-3 (source i386 all)
On Wed, Jan 17, 2007, Steve Langasek wrote:
> - 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.
Yes, I messed up that one. Will fix.
> - patch-documentation-0.8.6debian-0.8.6a.diff contains:
> [..]
> How is this a documentation fix?
It's not. I said on IRC last week that I would remove that chunk.
Either I lied or I forgot about it.
> - 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.)
No, it actually disables maintainer mode by default, to avoid the
problem you mention.
> - patch-missing-locks-0.8.6debian-0.8.6a.diff: is playlist_LockDelete() a
> superset of playlist_Delete()?
Yes, it's simply playlist_Delete() with proper mutex locks around it.
> - patch-sanitise-javascript-0.8.6debian-0.8.6a.diff: why is it /wrong/ to
> escape \ characters here?
Upstream tells me it's not needed, I'll remove it.
> - 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?)
It's the first chunk of the patch that's wrong. I can fix it, leave
it like that, or remove it, since it only has a visual cosmetic impact
and the important part of the patch is the last chunk (uninitialised
i_original_* variables) and I just kept the full patch to avoid later
conflicts if patches need to be applied.
> 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.
I'm sorry about that. My thought was that even if the release team
does not see these patches as relevant for testing, users or porters
may benefit from the patches being at least present in the sources. (I
also believe that unapplied patches for non-released architectures are
probably at least partly accountable for the decay of some of those
architectures).
--
Sam.
Reply to: