On Tue, May 29, 2012 at 3:07 AM, commits@source.squeak.org wrote:
Bert Freudenberg uploaded a new version of Monticello to project The Trunk: http://source.squeak.org/trunk/Monticello-bf.509.mcz
==================== Summary ====================
Name: Monticello-bf.509 Author: bf Time: 29 May 2012, 12:06:58.832 pm UUID: 02bc3aaf-6de3-4b06-8264-852ba776aeb4 Ancestors: Monticello-bf.508
Allow to specify branch name in MCFileBasedRepository>>versionNamesForPackageNamed:
=============== Diff against Monticello-bf.508 ===============
Item was changed: ----- Method: MCFileBasedRepository>>versionNamesForPackageNamed: (in category 'versions') -----
- versionNamesForPackageNamed: packageName
- versionNamesForPackageNamed: packageName
^ Array streamContents:
- [ : stream | | wantBranch |
- wantBranch := packageName includes: $..
- self allFileNamesOrCache do:
- [ : each | | mcVersionName branchName |
- [ : stream | self allFileNamesOrCache do:
- [ : each | | mcVersionName |
mcVersionName := each asMCVersionName.
- branchName := wantBranch
- ifTrue: [mcVersionName packageAndBranchName]
- ifFalse: [mcVersionName packageName].
- packageName = branchName ifTrue: [ stream nextPut: mcVersionName ] ] ]!
- mcVersionName packageName = packageName ifTrue: [ stream nextPut: mcVersionName ] ] ]!
Bert, there is a problem with this change. If you open the repository up, there is NO selected package, and this method fails if the packageName is nil. A simple fix (not claiming best) is to guard against this situation (method attached).
-Chris
(Chris Muller needed below ...)
On 31.05.2012, at 08:04, Chris Cunningham wrote:
On Tue, May 29, 2012 at 3:07 AM, commits@source.squeak.org wrote:
Bert Freudenberg uploaded a new version of Monticello to project The Trunk: http://source.squeak.org/trunk/Monticello-bf.509.mcz
==================== Summary ====================
Name: Monticello-bf.509 Author: bf Time: 29 May 2012, 12:06:58.832 pm UUID: 02bc3aaf-6de3-4b06-8264-852ba776aeb4 Ancestors: Monticello-bf.508
Allow to specify branch name in MCFileBasedRepository>>versionNamesForPackageNamed:
=============== Diff against Monticello-bf.508 ===============
Bert, there is a problem with this change. If you open the repository up, there is NO selected package,
Works for me, the same as before. Try reverting this method and see for yourself.
and this method fails if the packageName is nil.
Ah, you're right.
A simple fix (not claiming best) is to guard against this situation (method attached).
Thanks. I fixed the sender instead, since this method should never be called with a nil argument. However ...
Here's a question for cmm:
While looking at your MCFileRepositoryInspector code it seems to me that versionNames isn't even used anymore, but only allVersionNames. Indeed, when I remove the super send in MCFileRepositoryInspector>>initializeVersionNames and instead add #refreshEmphasis, everything appears to work fine. So, could the super send be removed for good?
If we could remove it, then my new nil check in MCRepositoryInspector>>initializeVersionNames wouldn't be necessary anymore ...
Also, I assume the reason for introducing allVersionNames was to avoid asking the repository on every package list click. However, wouldn't it have been simpler to do in MCRepositoryInspector, not MCFileRepositoryInspector? Right now we have a weird mix where the versionNames variable is unused etc.
- Bert -
Here's a question for cmm:
While looking at your MCFileRepositoryInspector code it seems to me that versionNames isn't even used anymore, but only allVersionNames. Indeed, when I remove the super send in MCFileRepositoryInspector>>initializeVersionNames and instead add #refreshEmphasis, everything appears to work fine. So, could the super send be removed for good?
Hi. One of the side-goals of the MCRepository upgrade was to unify the RepositoryInspectors to work generically with any MCRepository. This was initially started in Monticello-cmm.423 and completed by Monticello-cmm.425 with the removal of MCFileRepositoryInspector in its entirety.
But because FileBasedRepository's can only access the entire directory contents to do any kind of interrogation of the versions present, it became wasteful to do that under the new sustainable API, which only requires to know the versions of the *selected* package.
Therefore, an entirely new MCFileRepositoryInspector subclass was created in Monticello-cmm.427 with its cache of 'allVersionNames'. Now, it could execute its own #versionNamesForSelectedPackage without having to re-get all the directory entries. MCFileRepositoryInspector is unwanted, but tolerated solely for the improved performance.
So, let's please not inch our way toward more dependency on 'allVersionNames'. We may not have any senders of versionNames from a MCFileRepositoryInspector today but it is fair game to be called in the future since it is part of the standard API for the MCRepositoryInspector.
If we could remove it, then my new nil check in MCRepositoryInspector>>initializeVersionNames wouldn't be necessary anymore ...
We should not remove the super send, but I think your nil guard is appropriate in any case. Oh, BTW, Array empty is better than Array new because it uses a single canonicalized empty Array which can be shared throughout the system. I don't think Squeak's MC is portable to other Smalltalks as it is, so it should be ok to use its niceties.
Also, I assume the reason for introducing allVersionNames was to avoid asking the repository on every package list click. However, wouldn't it have been simpler to do in MCRepositoryInspector, not MCFileRepositoryInspector? Right now we have a weird mix where the versionNames variable is unused etc.
#allVersionNames is unsustainable and has no business anywhere but in FileBased[Repository|Inspector]'s. versionNames is absolutely used for inspectors of any other type of MCRepository than the FileBased.
- Chris
On 31.05.2012, at 20:39, Chris Muller wrote:
Here's a question for cmm:
While looking at your MCFileRepositoryInspector code it seems to me that versionNames isn't even used anymore, but only allVersionNames. Indeed, when I remove the super send in MCFileRepositoryInspector>>initializeVersionNames and instead add #refreshEmphasis, everything appears to work fine. So, could the super send be removed for good?
Hi. One of the side-goals of the MCRepository upgrade was to unify the RepositoryInspectors to work generically with any MCRepository. This was initially started in Monticello-cmm.423 and completed by Monticello-cmm.425 with the removal of MCFileRepositoryInspector in its entirety.
But because FileBasedRepository's can only access the entire directory contents to do any kind of interrogation of the versions present, it became wasteful to do that under the new sustainable API, which only requires to know the versions of the *selected* package.
Therefore, an entirely new MCFileRepositoryInspector subclass was created in Monticello-cmm.427 with its cache of 'allVersionNames'. Now, it could execute its own #versionNamesForSelectedPackage without having to re-get all the directory entries. MCFileRepositoryInspector is unwanted, but tolerated solely for the improved performance.
So, let's please not inch our way toward more dependency on 'allVersionNames'. We may not have any senders of versionNames from a MCFileRepositoryInspector today but it is fair game to be called in the future since it is part of the standard API for the MCRepositoryInspector.
If we could remove it, then my new nil check in MCRepositoryInspector>>initializeVersionNames wouldn't be necessary anymore ...
We should not remove the super send, but I think your nil guard is appropriate in any case. Oh, BTW, Array empty is better than Array new because it uses a single canonicalized empty Array which can be shared throughout the system. I don't think Squeak's MC is portable to other Smalltalks as it is, so it should be ok to use its niceties.
Also, I assume the reason for introducing allVersionNames was to avoid asking the repository on every package list click. However, wouldn't it have been simpler to do in MCRepositoryInspector, not MCFileRepositoryInspector? Right now we have a weird mix where the versionNames variable is unused etc.
#allVersionNames is unsustainable and has no business anywhere but in FileBased[Repository|Inspector]'s. versionNames is absolutely used for inspectors of any other type of MCRepository than the FileBased.
- Chris
Makes sense. Thanks!
- Bert -
squeak-dev@lists.squeakfoundation.org
-
Bert Freudenberg -
Chris Cunningham -
Chris Muller