MetacelloLoadData>>#currentVersionInfoFor:ifAbsent: bug ?

[estebanlm]

I experienced a DNU when executing this:

MetacelloPackageSpec >> workingCopyNameFor: anMCLoader

    | vi |
    (vi := anMCLoader currentVersionInfoFor: self) == nil ifTrue: [ ^nil ].
    ^vi name

because "anArray does not understand #name" (Pharo removed #name from Object). I dug into the problem and I think is here:

currentVersionInfoFor: packageSpec ifAbsent: aBlock

MetacelloLoadData >> currentVersionInfoFor: packageSpec ifAbsent: aBlock

    ^self versionInfoMap 
        at: packageSpec file 
        ifAbsent: [ 
            self packageNameMap 
                at: packageSpec name
                ifAbsent: aBlock ]

and this is because packageNameMap is a key->array dictionary, so it will answer an array instead the version. I "fixed" the problem by adding:

MetacelloLoadData >> currentVersionInfoFor: packageSpec ifAbsent: aBlock

    ^self versionInfoMap 
        at: packageSpec file 
        ifAbsent: [ 
            self packageNameMap 
                at: packageSpec name
                ifPresent: [ :vi | vi first ]
                ifAbsent: aBlock ]

but I want to know if fix is correct.

@dalehenrich ?

[dalehenrich]

@estebanlm that looks correct ... it does make me wonder how this code ever worked:) ... But when I look at the senders of #workingCopyNameFor::

MetacelloPackageSpec>>isPackageLoaded:
MetacelloPackageSpec>>updatePackageSpec:force:using:
MetacelloToolBox>>updateVersionSpec:fullVersionSpec:updateProjects:updatePackages:visited:updated:

#isPackageLoaded: is the only method that would be used during loading ... the other two methods would only be used by tools ... and would not work without your fix ...

I would feel a bit more comfortable if we had a test case that exposed the bug, so we could avoid regressions in the future ...

[estebanlm]

yeah, I will figure out a test... I just wanted to know if I was heading right direction or I did something fundamentally wrong (still warming up on metacello stuff, you know :) )

[dalehenrich]

I appreciate your help!

[estebanlm]

actually, I have no idea how to do a test for this :) any idea?

[dalehenrich]

Well the starting point is the original error ... I like "practical" tests that are based on package-level operations rather than isolated unit tests ... so if you share a stack (no need for frame state ... just the methods) with the original error, I can pick out the api that the test should be written against ...