The Inbox: Monticello-ct.732.mcz

A new version of Monticello was added to project The Inbox:
http://source.squeak.org/inbox/Monticello-ct.732.mcz

==================== Summary ====================

Name: Monticello-ct.732
Author: ct
Time: 16 November 2020, 3:59:45.315045 pm
UUID: 49586c09-c5d0-3444-a45c-3ccaf04fcc21
Ancestors: Monticello-mt.730

Fixes a defect in MCPackageLoader when installing a snapshot that contains both an addition and a removal of a code object (e.g. a method). In this case, the addition should be installed but the removal be withdrawn (until today, the definition was first added but then removed again).

While this situation does not occur in everyday Monticello use, it can indeed occur when using a MCVersionLoader to load multiple versions each representing a different packages at once and any method or class definition has a different package affiliation in this set of versions relative to the working copies of the packages.

Concretely, this constellation arises when the effect of loading this set of versions should be expected to move the relevant definition from one package into another one. For the actuating failure, see https://github.com/Metacello/metacello/issues/524#issuecomment-723644601.

Thanks to Tom Beckmann (TB) for the implementation proposal! I have extended it to match class and trait definitions as well. Please review. :-)

=============== Diff against Monticello-mt.730 ===============

Item was added:
+ ----- Method: MCClassDefinition>>isOrganizationMember (in category 'testing') -----
+ isOrganizationMember
+
+ ^ true!

Item was added:
+ ----- Method: MCClassTraitDefinition>>isOrganizationMember (in category 'testing') -----
+ isOrganizationMember
+
+ ^ true!

Item was added:
+ ----- Method: MCDefinition>>isOrganizationMember (in category 'testing') -----
+ isOrganizationMember
+
+ ^ false!

Item was added:
+ ----- Method: MCMethodDefinition>>isOrganizationMember (in category 'testing') -----
+ isOrganizationMember
+
+ ^ true!

Item was changed:
  ----- Method: MCPackageLoader>>forgetSuperfluousMethodRemovals (in category 'private') -----
  forgetSuperfluousMethodRemovals
  |  removedClasses |
+ removedClasses := removals select: #isClassDefinition thenCollect: #actualClass.
- removedClasses := (removals select: #isClassDefinition) collect: #actualClass.
  removedClasses addAll: (removedClasses collect: #class).
+ removals := removals reject: [:removal |
+ removal isMethodDefinition and: [removedClasses includes: removal actualClass]].
+ removals := removals reject: [:removal |
+ removal isOrganizationMember and: [
+ additions anySatisfy: [:addition |
+ addition isOrganizationMember and: [addition description = removal description]]]].!
- removals := removals reject: [:e | e isMethodDefinition and: [removedClasses includes: e actualClass]]!

Hi,

Am Mo., 16. Nov. 2020 um 16:00 Uhr schrieb <[hidden email]>:
>
> +       removals := removals reject: [:removal |
> +               removal isOrganizationMember and: [
> +                       additions anySatisfy: [:addition |
> +                               addition isOrganizationMember and: [addition description = removal description]]]].!

How about using MCDefinition>>#isRevisionOf: instead of a description
= b description? Saves people like me from wondering why description
is used here (and what it contains anyway).

In domain terms, why is it significant here that the definitions are
members of an organization? Why does this procedure not apply to
organization changes or script changes, for example?

Instead of allocating another Collection instance, you could also use
an OrderedCollection for the removals and use removeAllSuchThat:, or
put all the conditions in just one reject block.

> Thanks to Tom Beckmann (TB)

Finally, doesn't Tom use tobe as his author initials?

Kind regards,
Jakob

Hi,

Am Mo., 16. Nov. 2020 um 16:00 Uhr schrieb <[hidden email]>:
>
> +       removals := removals reject: [:removal |
> +               removal isOrganizationMember and: [
> +                       additions anySatisfy: [:addition |
> +                               addition isOrganizationMember and: [addition description = removal description]]]].!

How about using MCDefinition>>#isRevisionOf: instead of a description
= b description? Saves people like me from wondering why description
is used here (and what it contains anyway).

In domain terms, why is it significant here that the definitions are
members of an organization? Why does this procedure not apply to
organization changes or script changes, for example?

Instead of allocating another Collection instance, you could also use
an OrderedCollection for the removals and use removeAllSuchThat:, or
put all the conditions in just one reject block.

> Thanks to Tom Beckmann (TB)

Finally, doesn't Tom use tobe as his author initials?

Kind regards,
Jakob

Am Mo., 16. Nov. 2020 um 18:22 Uhr schrieb Thiede, Christoph
<[hidden email]>:
>
> However, since you're commenting this, I'm asking myself two questions:
>
> Is it a valid operation to remove an entire organization definition under any circumstances?

taH pagh taHbe (To be, or not to be)... [1]
There is always an organization as long as there is a package.
Removing the package organization is only valid when you unload
(remove) the whole package. According to
MCOrganizationDefinition>>#unload it would only remove empty
categories anyway.

[1] https://en.wikipedia.org/wiki/The_Klingon_Hamlet

> Couldn't we instead modify the #definition implementation of MCOrganizationDefinition and insert the containing system category name at this place? Semantically, this would look better for me, but unfortunately, MCOrganizationDefinition does not have a relevant instance variable ...
>

Did you mean #description instead? In absence of a packageName
variable, it would have to find the common prefix of the elements in
its categories variable. Though for cross-package loads, it must be
made sure that it still behaves correctly
(GoferLoad>>#updateCategories).

I suggest to check or resolve that later and keep the current
behavior, to not block your current contribution.

> > In domain terms, why is it significant here that the definitions are members of an organization? Why does this procedure not apply to organization changes or script changes, for example?
>
> Fair question. #description is only unique for a definition that is part of an organization. Given a set of two MCVersions one of which would add and the second one would delete an MCOrganizationDefinition for separate packages, by the current implementation these organization definitions would be revisions of each other and thus would be wiped up in #forgetSuperfluousMethodRemovals.
>

Instead for now, is there a more meaningful selector than
isOrganizationMember given that this property just corresponds to
another one that you are really looking for?
canDistinguish[SomethingLikeInstancesOrTheOppositeOfRevisionsOfAnother],
canDistinguishRevisionsFromOtherObjects,
canDistinguishRevisionsAcrossPackages?

Or override isRevisionOf:, but then it would be inconsistent with
behavior that relies on description directly for the time being (e. g.
hash --> Dictionary overwriting).

Am Mo., 16. Nov. 2020 um 18:22 Uhr schrieb Thiede, Christoph
<[hidden email]>:
>
> However, since you're commenting this, I'm asking myself two questions:
>
> Is it a valid operation to remove an entire organization definition under any circumstances?

taH pagh taHbe (To be, or not to be)... [1]
There is always an organization as long as there is a package.
Removing the package organization is only valid when you unload
(remove) the whole package. According to
MCOrganizationDefinition>>#unload it would only remove empty
categories anyway.

[1] https://en.wikipedia.org/wiki/The_Klingon_Hamlet

> Couldn't we instead modify the #definition implementation of MCOrganizationDefinition and insert the containing system category name at this place? Semantically, this would look better for me, but unfortunately, MCOrganizationDefinition does not have a relevant instance variable ...
>

Did you mean #description instead? In absence of a packageName
variable, it would have to find the common prefix of the elements in
its categories variable. Though for cross-package loads, it must be
made sure that it still behaves correctly
(GoferLoad>>#updateCategories).

I suggest to check or resolve that later and keep the current
behavior, to not block your current contribution.

> > In domain terms, why is it significant here that the definitions are members of an organization? Why does this procedure not apply to organization changes or script changes, for example?
>
> Fair question. #description is only unique for a definition that is part of an organization. Given a set of two MCVersions one of which would add and the second one would delete an MCOrganizationDefinition for separate packages, by the current implementation these organization definitions would be revisions of each other and thus would be wiped up in #forgetSuperfluousMethodRemovals.
>

Instead for now, is there a more meaningful selector than
isOrganizationMember given that this property just corresponds to
another one that you are really looking for?
canDistinguish[SomethingLikeInstancesOrTheOppositeOfRevisionsOfAnother],
canDistinguishRevisionsFromOtherObjects,
canDistinguishRevisionsAcrossPackages?

Or override isRevisionOf:, but then it would be inconsistent with
behavior that relies on description directly for the time being (e. g.
hash --> Dictionary overwriting).

Am Fr., 20. Nov. 2020 um 13:23 Uhr schrieb Thiede, Christoph
<[hidden email]>:
>
> > Instead for now, is there a more meaningful selector than isOrganizationMember given that this property just corresponds to another one that you are really looking for? canDistinguish[SomethingLikeInstancesOrTheOppositeOfRevisionsOfAnother], canDistinguishRevisionsFromOtherObjects, canDistinguishRevisionsAcrossPackages?
>
>
> Hm, quite long names. :-) How about #hasUniqueDescription? #isDescription[Globally]Unique? Something like that? :-)
>

I was also tinkering with "unique", but the problem is that the
descriptions are not unique. Different definitions of the same method
have equal descriptions, that's the point. Description itself is a
rather abstract term here. It is more like the key for an index (and
it is used as such in dictionaries).

Thinking out loud to inspire others to find a better name: As of now,
the objects defined by these MCDefinitions exist at most once (= are
unique) in an Environment at any point in time. To say the same for
MCOrganizationDefinition and MCScriptDefinition, as of now, you would
in addition have to give the package name. For those kinds of
definitions that you want to skip here, you say the definitions cannot
be distinguished by their description if you combine the patches of
multiple packages. Well, actually that's not entirely true:
MCScriptDefinition has the packageName in its description, so you can
manage scripts of multiple packages in the same patch and they are
immune to your consolidation procedure -- but unlike classes, traits,
and methods, you cannot say that this script is still the same if you
move it to another package. For the identity of classes, traits, and
methods, the package is irrelevant (unlike in other languages) and
therefore the removals and additions across packages can be
consolidated. The package is just a filter superimposed on these
objects, whereas each script is associated with a package directly.
MCOrganizationDefinition is not officially associated with a package,
but its instances usually contain only the categories of just a single
package. And conversely, packages are delimited/defined/identified by
categories.

canMoveBetweenPackages?

Later you might change some of the premises, such as: either add the
packageName to MCOrganizationDefinition's description, or consolidate
MCOrganizationDefinitions if you do a cross-package load so it does
contain categories from multiple packages. Then you could also
consolidate the operations like for methods (although with a different
algorithm in the second approach).

Or you stick with your isOrganizationMember, but promise to refactor
very soon, so it does not stick around. ;-)