Item-repository allocation/locking refactor

[Milian Wolff]

On Wednesday 05 December 2012 10:11:13 Ivan Shapovalov wrote:
> Hi!
> 
> 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
> 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://github.com/intelfx/kdevplatform.git".

Cool!

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 
clear.

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 
created

- 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. 
ItemRepositoryRegistry). 

- 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 milianw.de
http://milianw.de
-------------- 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: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20121206/7af2ab21/attachment.sig>