Re: [Libreoffice] Patch review

Hi there,

On Thu, 2011-01-13 at 11:38 -0500, Kohei Yoshida wrote:
> The attached patches revert the recent two commits which consist of the
> merging of OOo330-m19 changes.

        Thanks for that !

> It fixed i#115906 and i#116164, but the fix for i#116164 unfortunately
> was pretty large and invasive, plus it introduced a regression as
> reported in i#116439 of OOo bug tracker.

        Sure; i#116164 is a pretty severe, but unnusual corner case performance
regression - so we don't need a fix for it.

> It is my opinion that, since these fixes don't fit *our* blocker
> criteria, plus the change is too large to be in RC, reverting them will
> be our best option.

        Agreed.

> The 0001 is a pure git revert commit, whereas the 0002 is an application
> of a patch I generated between the pre- and post-merge commit.  Git has
> a special handling of reverting merge commits, so I decided to do the
> revert this way.

        Well; grokking git and reading your patch I must say; I get fairly
confused by what patch (the merge patch, or the cws patch) is actually
applied, and how best to revert this.

        As an example; reading the original NN patch I see:

-void ScTable::DBShowRows(SCROW nRow1, SCROW nRow2, bool bShow)
+void ScTable::DBShowRows(SCROW nRow1, SCROW nRow2, bool bShow, bool bSetFlags)
 {
+    // #i116164# IncRecalcLevel/DecRecalcLevel is in ScTable::Query
     SCROW nStartRow = nRow1;
-    IncRecalcLevel();
     InitializeNoteCaptions();
     while (nStartRow <= nRow2)
     {

        in libreoffice-3-3 as of today it reads:

void ScTable::DBShowRows(SCROW nRow1, SCROW nRow2, bool bShow, bool bSetFlags)
{
    // #i116164# IncRecalcLevel/DecRecalcLevel is in ScTable::Query
    SCROW nStartRow = nRow1;
    while (nStartRow <= nRow2)

        And yet your reversion patch -appears- not to re-instate the
IncRecalcLevel() call:

-void ScTable::DBShowRows(SCROW nRow1, SCROW nRow2, bool bShow, bool bSetFlags)
+void ScTable::DBShowRows(SCROW nRow1, SCROW nRow2, bool bShow)
 {
-    // #i116164# IncRecalcLevel/DecRecalcLevel is in ScTable::Query
     SCROW nStartRow = nRow1;
     while (nStartRow <= nRow2)
     {

        Which is scary to me :-) I'd like to discuss where the madness lies
(most likely in my method) before we push this to libreoffice-3-3-0 can
you poke me on IRC ?

        Thanks,

                Michael.;

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