Review Request: Cleanup "~/.cache/kdevduchain/<session>" on session remove (+refactor allocateRepository()).

[Milian Wolff]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106945/#review22608
-----------------------------------------------------------


While it's cool that you looked into this, I'm afraid I have to reject the patch in its current form. The code is pretty fragile and this just shows in apparently simple changes like this one which might introduce subtle bugs...

I'll maybe look into this in more depth this week or so and fix it properly.


language/duchain/repositories/itemrepository.cpp
<http://git.reviewboard.kde.org/r/106945/#comment17284>

    why did you remove the asserts?



language/duchain/repositories/itemrepository.cpp
<http://git.reviewboard.kde.org/r/106945/#comment17285>

    this unrelated cleanup should go into a separate commit and review.
    
    Also, what exactly do you try to achieve by refactoring the code? Personally I think the whole mess should be refactored quite more drastically, considering that a session is required. Since the session has its own locking mechanism (which is much better than what we have here) I think we should just assume that when we have a session uuid, we can blindly allocate the DUchain cache for that uuid.
    
    This will require a change to the cache path though as it does not contain the app name so far.
    
    If you *really* want to work on this, then fine but in a separate commit.



shell/session.cpp
<http://git.reviewboard.kde.org/r/106945/#comment17286>

    this is broken, as the DUChain is still "alive" at this point in case of a temporary session.
    
    what we need is a delete function in the DUChain, which either "marks" the DUChain for deletion when we enter aboutToQuit, or directly removes the folder if possible. Then again, the latter requires proper API to ensure we do not delete a running session.


- Milian Wolff


On Oct. 29, 2012, 2:19 p.m., Ivan Shapovalov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106945/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2012, 2:19 p.m.)
> 
> 
> Review request for KDevelop and Milian Wolff.
> 
> 
> Description
> -------
> 
> 1. Clean (rmdir) the per-session duchain store on session removal (particularly useful in unit-tests and their temporary sessions).
> 2. Partially refactor allocateRepository() (separate cache path compution into a different function, fix formatting, handle lockfiles properly)
> 
> Don't know if I've done that properly, but I wanted to avoid mentioning ".cache/kdevduchain" in two unrelated places.
> 
> 
> Diffs
> -----
> 
>   language/duchain/repositories/itemrepository.h 392847c 
>   language/duchain/repositories/itemrepository.cpp 8d559cf 
>   shell/session.cpp a59d70e 
> 
> Diff: http://git.reviewboard.kde.org/r/106945/diff/
> 
> 
> Testing
> -------
> 
> Existing unit-tests and manual testing.
> 
> 
> Thanks,
> 
> Ivan Shapovalov
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20121126/4f27833e/attachment.html>