Migrate from GitHub to SourceForge with this tool. Check out all of SourceForge's recent improvements.
Close

#2863 Foldercompare method changed

Branch
closed-fixed
nobody
GUI (476)
7
2009-06-09
2009-06-07
Matthias
No

When detecting a big file the method was changed to quick compare. So if you have more files which are text files and smaller, they all have been compared by quick content.
This patch is changeing the method only for the single file.

Discussion

  • Matthias

    Matthias - 2009-06-07

    reset compare method to original

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-07
    • milestone: 438015 --> 438013
     
  • Kimmo Varis

    Kimmo Varis - 2009-06-07

    Any proof of this bug?

    - in FolderCmp::prepAndCompareTwoFiles() we read compare method from compare context.
    - compare method is changed only to local variable of FolderCmp::prepAndCompareTwoFiles()
    - when we call FolderCmp::prepAndCompareTwoFiles() for next files we read the compare method again from compare context

    I fail to see how the changed local variable could affect value in compare context. So this really should be per-file setting.

     
  • Matthias

    Matthias - 2009-06-07

    corret,but this context has been changed!
    Test with a foldercomper and some files. Test with debugger
    put a breakpoint after
    int nCompMethod = pCtxt->m_nCompMethod;
    change this value maually to 1 (quick content) and see what happend.
    With next call this value remains 1 as pCtxt has been changed by
    *m_pCtx->GetCompareOptions(CMP_QUICK_CONTENT) in qick content.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-07

    Oh! Now I see it. Thanks for explanation.

    This is very important bug now to fix as it really explains some of those weird-looking bugs with weird results when comparing large/big files.

    I think your fix is OK for the 2.12 branch. But for trunk we need a better solution. We must fix the way changing compare options works.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-07
    • milestone: 438013 --> 438015
    • priority: 5 --> 7
     
  • Matthias

    Matthias - 2009-06-07

    I think this ok for both.

    What is to be done, is how and at what time we pass new diff options to pCtxt.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-07

    > I think this ok for both.
    No it is not. I don't want hacks like that while we can have real fixes. Hacks only cause that nobody bothers to fix the real bugs. But since we can't add real fix for 2.12. branch the hack is ok there.

    I think we need to refactor options handling a bit. CompareContext idea has been to contain per-compare data. Compare options etc. Currently we keep only current options. But as this bug shows it can lead to wrong data to hold. I think we need to keep array of compare options in CompareContext and just pick correct set for each file. I think that was my original plan but then run out of time and ended up doing it a wrong way.

     
  • Matthias

    Matthias - 2009-06-07

    >I think we need to keep array of compare options in CompareContext
    >and just pick correct set for each file.
    No, that's not ok.
    As now we start with a setting, that must be ok for one complete folder run.
    So by F5-key we need to get new setting form options, than it's ok.
    But what happend we have some running instances of WM. Do we want to have different setting there?. I think for that we should start new item in forum.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-07

    Different instances have nothing to do with this. Do not create problems we don't have. Even different compare windows have nothing to do with this. CompareContext is created for every compare and holds compare-specific data. Compare context may perfectly well hold several instances of compare options. Only downside is wasting few bytes of memory.

     
  • Matthias

    Matthias - 2009-06-07

    No that's not what I mean.
    When we change options they are stored to registry, isn't it.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-07

    Yep, but this temporarily compare method switch is only member variable which is not stored to registry. We store to registry only options that should be remembered between instances.

     
  • Matthias

    Matthias - 2009-06-07

    So now I think we should switch to forum with the discussion. As we have more bugreports relatet to the options, settings and colors also. Will be a common threat.
    Do you start, or myself?

    Let's go on here with our bug only.
    A better solution is allways the death of a running (good) one.
    So aslong we have no better ues it for truck and bruch.

    Keep this item open till we have an other better solution.

     
  • Matthias

    Matthias - 2009-06-07

    An other possibility to solve that bug
    GetCompareOptions():
    is it a must to chamge to method there, even we just want to get the correct DiffutilsOptions or QuickCompareOptions
    so just remove the line
    m_nCompMethod = compareMethod;
    would solve that also.

     
  • Matthias

    Matthias - 2009-06-07

    change in diffcontex

     
  • Matthias

    Matthias - 2009-06-07

    GetCompareOptions() was not only getting the diffvalues also setting the compare method.
    Setting must be done by a setmethod or done as now in CreateCompareOptions().
    added new patch.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-07

    This really has nothing to do with general options, or bugs in there. Can we please focus on this particular bug, not re-doing WinMerge once again. Damn, it this is about one member variable updated and not reset back.

    I still think (not having much time to to think about this) the best solution is to create separate instances of settings for each compare method. That means we create settings data once when compare begins and we don't need to re-create it when changing method. So it is faster that way even when it means it takes few bytes more memory.

    I'm not after quick tricks to fix this. I'm after best solutions we can have. There are million of bad ways to fix this and I'm not interested in them. And why I know which is good way? Because I've designed and implemented this code from beginning.

     
  • Matthias

    Matthias - 2009-06-08

    CreateCompareOptions() is creating our settings and can set as the name says also the
    comparemethod.
    GetCompareOptions() has to return the needed options for the method.
    >Damn, it this is about one member variable updated and not reset back.
    yes, but so my opion, setting done in wrong function, thats all.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-08

    I see your point. But I fail to see that as other than quick fix. It does not solve the whole problem.

    The problem is bigger and more complex. CDiffContext::m_nCompMethod is meant to be the *main* compare method. In practice the method selected in options. It is the master variable that determines how lots of things work during compare. So it was plain error from my side to forget its role and use with compare options. It has nothing to do with them directly. Master compare mode doesn't change as compare options might change during compare.

    After really thinking about this for a while I decided the solution is we add new int m_currentCompareMethod to CDiffContext. It is the *current* effective compare method and may change with the compare options. m_currentCompareMethod can change per file but m_nCompMethod is constant during the whole compare.

     
  • Matthias

    Matthias - 2009-06-08

    I don't see a sence there. We have a temp variable in foldercomper
    prepAndCompareTwoFiles(), nCompMethod
    isn't that enough?

    what do you mean with 'per file'?

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-08

    Submitted patch
    #2803152 Fix bug that compare method was not restored to global value
    http://winmerge.org/patch/2803152
    which is the correct fix for trunk.

     
  • Kimmo Varis

    Kimmo Varis - 2009-06-09
    • milestone: 438015 --> Branch
    • status: open --> closed-fixed
     
  • Kimmo Varis

    Kimmo Varis - 2009-06-09

    Committed patch based your first patch to 2.12 branch:
    Completed: At revision: 6831

    > I don't see a sence there.
    The current code has several weak points. First weakness is that code doesn't mode the reality that we have two compare methods in use. So code gets confused when the method change happens and there is no good way to handle it. Second weakness is that the compare method is public attribute which could be changed by anybody seeing the class. Which is just invitation for more bugs. Reality is main compare method does not change during the folder compare and code must model this.

     

Log in to post a comment.