Item-repository allocation/locking refactor
mail at milianw.de
Thu Dec 6 22:44:10 UTC 2012
On Wednesday 05 December 2012 10:11:13 Ivan Shapovalov wrote:
> Following Milian's suggestion on https://git.reviewboard.kde.org/r/106945/,
> I'm putting the work on refactoring the "disk storage allocation" part of
> the item-repositories into a public repo instead of a review-request.
> The refactoring is just about
> - getting rid of locking in the item-repository directories (because we have
> rather good per-session locks);
> - getting rid of numbered subdirectories inside of per-session cache
> directories (because nobody shall use and nobody actually uses multiple
> - changing the cache path once again
> from "~/.cache/kdevduchain/$session"
> to "~/.cache/kdevduchain/$appname-$session";
> - making ItemRepositoryRegistry a true singleton;
> - finally making the cache directories deleted when a session is deleted
> from disk.
> The real reason behind all this was the last point.
> The branch is called "itemrepository" and is on
Some points I spotted so far - please don't be overwhelmed by that... But I
rather want it to merge a nicely designed refactoring which should make this
stuff future proof.
- testcore mentioned in DUChain is bad - that should not need any special
handling. Put anything you need into ICore, such as an aboutToShutdown signal
and connect to it using ICore::self() (which is then a TestCore in a unit
test). Actually there is already a aboutToShutdown, which should be used
instead of QApplication::aboutToQuit in the DUChain.
- I dislike that QUuid is used in repositoryPathForSession and
deleteRepositoryFromDisk. This should imo be an ISession* to make the coupling
Also, both of these should not be public API. Rather, you could e.g. add a
signal in ISession which notifies about the deletion of the session (from
disk) and act accordingly. I'd opt for a aboutToDeleteFromDisk() which gets
emitted in Session::deleteFromDisk before the removeDirectory. Then listen to
that signal in the registry and act upon it.
The way it stands now, I could e.g. call "delete" on a session that is
currently running in a different session and then when I close that session
magically my cache disappears - this is nothing we want. You only ever want to
delete your own session's cache.
- ItemRepoReg::self's comment is odd, remove the "and the only"
- in ::open you now do a QDir::count and then check again for a version* file
in that dir if no files exists - that makes no sense. Also you should probably
use QDirIterator::hasNext instead of counting files if all you need to figure
out is whether the dir is empty. imo you should not do such a check at all
though - an empty dir should be handled like a dir that has not yet been
- the public API for ItemRepositoryRegistry::open should take an ISession*,
otherwise I could open arbitrary locations (which we do not want)
- while you are at it, move all classes in itemrepository.h into their own
files and ensure that public API is properly pimpled (esp.
- I'm missing unit/integration tests for this stuff. Could you maybe try to
create some which ensure this new stuff works as it should?
Cheers, and many thanks for looking into this.
mail at milianw.de
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 198 bytes
Desc: This is a digitally signed message part.
More information about the KDevelop-devel