Item-repository allocation/locking refactor

Milian Wolff mail at
Thu Dec 6 22:44:10 UTC 2012

On Wednesday 05 December 2012 10:11:13 Ivan Shapovalov wrote:
> Hi!
> Following Milian's suggestion on,
> 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
> ItemRepositoryRegistries);
> - 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
> "git://".


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.
Milian Wolff
mail at
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <>

More information about the KDevelop-devel mailing list