Item-repository allocation/locking refactor

[Ivan Shapovalov]

On 06 December 2012 23:44:10 Milian Wolff wrote:
> 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.

Please, review the next iteration. It's on the same branch, rebased against 
current master (non-fast-forward, of course).

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

Done using ICore::shutdownCompleted().

> 
> - I dislike that QUuid is used in repositoryPathForSession and
> deleteRepositoryFromDisk. This should imo be an ISession* to make the
> coupling clear.

Done.

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


Not done: in some situations like the session picker dialog nothing is created 
yet, so there is no place to emit signals from. Also, it seems like unneeded 
complexity to me: deleteRepositoryFromDisk() is properly guarded, taking 
session locks and active session into account.

> 
> - ItemRepoReg::self's comment is odd, remove the "and the only"

Done.

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

Done, refactored the whole logic.

> 
> - the public API for ItemRepositoryRegistry::open should take an ISession*,
> otherwise I could open arbitrary locations (which we do not want)

Not done - maybe we'll need to open arbitrary locations someday. If you 
insist, I'd better merge the function into constructor as there will be no 
possible use-cases for it: why one would need to open a cache for session A 
using the directory of session B?

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

Done. Only ItemRepositoryRegistry is pimpled - other classes are templated.

> 
> - 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?

Created tests for:
- automatic deletion of itemrepository directory of temporary sessions
- manual deletion of itemrepository directory of the running session

The "offline deletion" (delete the directory belonging to a different session) 
can't be tested due to impossibility to create more than one instance of Core 
from a single launch.

Thanks,
Ivan.

> 
> Cheers, and many thanks for looking into this.