Review Request 118481: Basic integration of Bazaar VCS

[Sven Brauch]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118481/#review59021
-----------------------------------------------------------


Looks quite nice, thanks! I agree with kevin though, unit tests would be really nice -- I don't know of anyone active in KDevelop development who regularily uses bzr, so breakage might go unnoticed for a while without them.


plugins/bazaar/CMakeLists.txt
<https://git.reviewboard.kde.org/r/118481/#comment41064>

    that area is git support already, no?



plugins/bazaar/src/bzrannotatejob.cpp
<https://git.reviewboard.kde.org/r/118481/#comment41065>

    How many lines of output are there typically? Is it really the best performing option to return to the event loop after parsing each single line?
    
    Or do I misunderstand this?



plugins/bazaar/src/bzrannotatejob.cpp
<https://git.reviewboard.kde.org/r/118481/#comment41066>

    .startswith("revno: ") maybe, also below?



plugins/bazaar/src/copyjob.h
<https://git.reviewboard.kde.org/r/118481/#comment41067>

    maybe prefix this with BZR_, nothing is more annoying to debug than duplicate header guards ;)



plugins/bazaar/src/diffjob.h
<https://git.reviewboard.kde.org/r/118481/#comment41068>

    also here re. unique header guard name



plugins/bazaar/src/utilities.h
<https://git.reviewboard.kde.org/r/118481/#comment41069>

    also here, header guard name -- esp. important for such a generic thing ;)



plugins/bazaar/src/utilities.cpp
<https://git.reviewboard.kde.org/r/118481/#comment41070>

    kWarning()



plugins/bazaar/src/utilities.cpp
<https://git.reviewboard.kde.org/r/118481/#comment41071>

    This function, or at least something which looks quite similar, also exists in bzrannotatejob.cpp, can one copy be dropped or can the two be merged?



plugins/bazaar/src/utilities.cpp
<https://git.reviewboard.kde.org/r/118481/#comment41072>

    To me it looks like bazaar output might be localized. Are you sure this will work on all systems? Can you force the language to be english somehow?


- Sven Brauch


On June 2, 2014, 6:51 p.m., Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118481/
> -----------------------------------------------------------
> 
> (Updated June 2, 2014, 6:51 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> This patch adds basic integration of Bazaar VCS.
> 
> It provides implementation of KDevelop::IDistributedVersionControl interface as a plugin.
> It works by forwarding (user) requests to bzr executable (which must be in PATH) and parsing (text output) result from spawned bzr process.
> 
> 
> Diffs
> -----
> 
>   plugins/CMakeLists.txt e0ccbc1 
>   plugins/bazaar/CMakeLists.txt PRE-CREATION 
>   plugins/bazaar/bazaar.kdev4 PRE-CREATION 
>   plugins/bazaar/src/CMakeLists.txt PRE-CREATION 
>   plugins/bazaar/src/bazaarplugin.h PRE-CREATION 
>   plugins/bazaar/src/bazaarplugin.cpp PRE-CREATION 
>   plugins/bazaar/src/bzrannotatejob.h PRE-CREATION 
>   plugins/bazaar/src/bzrannotatejob.cpp PRE-CREATION 
>   plugins/bazaar/src/copyjob.h PRE-CREATION 
>   plugins/bazaar/src/copyjob.cpp PRE-CREATION 
>   plugins/bazaar/src/diffjob.h PRE-CREATION 
>   plugins/bazaar/src/diffjob.cpp PRE-CREATION 
>   plugins/bazaar/src/kdevbazaar.desktop.cmake PRE-CREATION 
>   plugins/bazaar/src/utilities.h PRE-CREATION 
>   plugins/bazaar/src/utilities.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/118481/diff/
> 
> 
> Testing
> -------
> 
> Only manual testing
> 
> 
> Thanks,
> 
> Maciej Poleski
> 
>

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