Re: [Libreoffice] [REVIEW-3-4] Reverted - Fix overly zealous check in Zip package consistency

Hi Thorsten,

On Fri, 2011-12-16 at 09:33 +0100, Thorsten Behrens wrote:
> I'll be putting that to -3-5 in a minute - for -3-4, unfortunately,
> 3.4.5 RC1 does not read password-protected odf documents

        It worries me that (you suggest) due to this last minute fix (that I
obviously didn't do a good job of reviewing), we didn't get to have any
real testing of Stephans huge back-port of the document signing stuff in
3.4.5 ? if so, that is really far from ideal.

        It makes me think we may want to do an RC2 / re-spin of 3.4.5 - no
doubt Petr & co. will rejoice at that ;-) thoughts ? I'd feel bad if the
first time we got that really rather substantial back-port tested in
anger was in a release we plan to leave as our best-ever-build out there
for many months without an update.

> so the previous fix needs to be reverted, and replaced with a
> cherry-pick of d0ac36dd66664e3d6953de8b3bdd79eeed8d2e70

        Ho hum; which now I read it I like even less.

+        // plain ignore bits 1 & 2 of the flag field - they are either
...
+                        || (rEntry.nFlag & ~6L) != (nFlag & ~6L)

        The code seems a pretty much documentation-free zone on the magic
meanings of these bit-flags; with things like this mess:

    // ignore bits 1 & 2 for normal deflate algo - they're purely informative
    if( nHow != 8 && nHow != 9 )
        bBroken = bBroken || rEntry.nFlag != nFlag;
    else if( (rEntry.nFlag & ~6L) != (nFlag & ~6L) )
        bBroken = true;

        Being quite normal. Surely we can do better than this - with a nice
link to the spec in a comment at the top of the file, and some #defines
that specify what the bits actually mean & so on ? Fewer comments, and
more descriptive code would be a better trade-off here I think.

        Are we even certain that this new fix is correct ? I would really
rather see this bed-down in 3.5 and get some wider testing than see it
in 3.4.5 - can't we just revert the original fix there ? Anyhow, I feel
bad for letting it slip through ... sorry for that.

        ATB,

                Michael.

-- 
michael.meeks@suse.com  <><, Pseudo Engineer, itinerant idiot