Modernise the Repository Manager

[scunz]

Okay, this is not going to become a two hour hack. It's about adjusting the RepoMan to almost 2 years of thinking about it - and that was quite a bit. Thus, we need a new logic to tackle the progress here. What I'd like to see is the following workflow:

• We declare development to be in maintenance mode. (Thus, we might actually fix things that we find there unless we already know it clashes with the changes herein - This might be worth just for a few days). So, that development is buildable and runnable.
• We declare repoman to be a feature branch. I imagine that this could work like: While @scunz is doing the basic refactoring, @antis81 might be doing tiny cleanups in the development branch (i.e. remove views like the RemoteView that are never used; basically gaining a very deep inside knowledge of what code we currently have, what is used (remove everything else)).
• During this first phase, repoman will be constantly rebased to live on top of development. Every change in repoman that is cosmetic or otherwise unrelated to the conceptional change will be upstreamed to development asap.
• The end of this Phase I will be a state where we'll have a working RepoMan, but a dysfunctional application.
• This is where Phase II jumps in. By that time we'll hopefully have identified lots of dead code and eliminated it from both branches.
• Now, we can implement all the services and adjust the (by then probably disabled) code to use them.
• Once we're on the same level in development and repoman, we can merge it and take it from there.

So, what this is going to bring, will be:

• [ ] Split RepoMan logically into a front end and a backend. While the backend will be running in a behind-the-scenes thread, the frontend will be accessible to the rest of the application. Thus, any git activity has to be done inside the RepoMan.
• [ ] Create an event system that can be used across thread boundaries.
• [x] Turn the frontend classes (the RM:Base derivates) into disposable and short-lived objects.
• [x] Turn the data classes (previously RM::Internal::BasePrivate) into reference countable dumb data containers
• [x] Remove any QObject mechanics from the frontend.
• [x] Create an abstract RM::Service and a RM::ServiceRunner (which will actually be the backend).
• [ ] Move the refresh logic into a RM::Service. This service will be the only module that is allowed to create new data objects.
• [ ] Ensure that the front end is not leaking any libGitWrap objects through its interface
• [ ] Implement the required services to accomplish the missions that we've already implemented:
  • [ ] RMDiffGenerator - A RM::Service that produces libDiffView models of diffs. libDiffViews is actually a good abstraction here already, so that we can reuse the Adaptor patterns we have in place.
  • [ ] RMHistoryGenerator - A RM::Service that produces a model suitable to display the repository's history. We currently depend very highly on GitWrap inside the history module. This is a problem. We'll have to extract that code into a real model with no data access and write 2 adaptors to it. The first adaptor will later become the bespoken RMHistoryGeneratorService. The other one would be the RMHistoryFetchDetailService. Extracting that model and the service logic from the history module is a task that can be done independent of the progress in repoman branch; i.e. when repoman has reached a minimum stable interface, one of us can branch off the repoman branch and do this.
  • [ ] RMHistoryFetchDetail - a very short running RM::Service that will query the details for a given commit and report it back into the history model.
  • [ ] RM::Services::CreateTag
  • [ ] RM::Services::CreateBranch
  • [ ] RM::Services::DeleteBranch
  • [ ] RM::Services::DeleteTag
  • [ ] RM::Services::MoveBranch
  • [ ] RM::Services::RemoteFetch
  • [ ] RM::Sercices::RemotePush

Basically, this is finally about "doing it right" and to stop beating around the bush :)

[antis81]

I just tried our install script MGV-get.sh: Works still like a charm :smile:. It just reveals a minor compile error based on RepoMan redesign.

@scunz Seems like a forgotten commit!? We really need back our build system - off-topic though :smile:.

Scanning dependencies of target ModRepository

[ 69%] Building CXX object Modules/Repository/CMakeFiles/ModRepository.dir/RepositoryModule.cpp.o
/home/nils/Projects/MacGitver-Release/MacGitver/Modules/Repository/RepositoryModule.cpp:135:26: error: 'open' is a private member of 'RM::RepoMan'
    MacGitver::repoMan().open( repo );
    ~~~~~~~~~~~~~~~~~~~~~^~~~
/home/nils/Projects/MacGitver-Release/MacGitver/Libs/libMacGitverCore/RepoMan/RepoMan.hpp:64:15: note: declared private here                                                   
        Repo* open(const Git::Repository& repo);
              ^
1 error generated.

[scunz]

I'm puzzled about that. I have the same code locally and it compiles while it definitely should not - since this is clearly an oversight of mine! Will investigate.

[scunz]

Okay, so the cause for this is: The desired workflow is:

• Install RAD-Tools
• Compile & install libHeaven
• Compile & install libGitWrap
• Compile MacGitver

Once you install one of our libraries / applications, the includes are diverted for all of them. These diverted locations are preferred over the local ones. So, be sure not to install MGV (or rm -rf the install prefix before building).

After that I can obviously reproduce this compiler error (and another one, too). Will make a fix to development...

[scunz]

Done. Should work way better now.

[scunz]

NOTE: This branch will soon require v7 of RAD-Tools :)

[antis81]

NOTE: This branch will soon require v7 of RAD-Tools :)

Thanks for the hint :smile:.

In this context I strongly recommend our MGV-get.sh script. It actually always builds from the latest version of RAD-Tools and our "development" branches. I'll commit it to the "scripts" folder (maybe rename to "mgv-build-install" or something more meaningful)? The script needs some corrections: For example the script's working directory is always set to "$HOME/Projects/MacGitver-Release" folder. In general it is the light version of our "out-of-order" Jenkins build bot (... and requires some user interaction).

[scunz]

Okay, so after playing around quite a lot with PIMPL and automaigc D-Pointers, I have found something that works just as I want it to work.

The focus here is on:

• Get it working without utilising any macros.
• Have it thread safe, yet use the best possible assembly code we can get away with
• Simple and easy to use syntax.

So, when reading the following code, one should first take a look at the essentials (from a code-user perspective). These are classes RM::Data::Base, RM::Data::Repo and the method RM::Frontend::Repo::isActive().

Everything inbetween is noise which (in the actual implementation) is hidden somewhere. The following code ought to be the minimum viable demonstration of the concept.

However, when looking at the templates in depth, one will figure that there seems to be going on a lot of mess.

One of the big advantages of this code (and a immense drawback of our previously used macro based solutions) will be the fact that we can integrate the RM::Backend::RepoLocker class within the D-Pointer encapsulation and get it executed at the right places. This would be a major problem to get right with macros...

So, enough said. Here's the code in it's entire uglyness (with a following discussion):

namespace RM {

  namespace Data {

    class Repo;

    class Base {
    public:
      const Repo* repository() const { return mRepo; }
            Repo* repository()       { return mRepo; }

      bool isActive() const          { return mIsActive; }

    private:
      bool  mIsActive : 1;
      Repo* mRepo;
    };

    class Repo : public Base {
    public:
      QMutex& mutex() const { return mMtx; }

    private:
      mutable QMutex mMtx;
    }

  }

  namespace Backend {

    class RepoLocker {
    public:
      RepoLocker(const Data::Repo* repo)
        : d(repo)
      {
        if (d) {
          d->mutex().lock();
        }
      }

      ~RepoLocker()
      {
        if (d) {
          d->mutex().unlock();
        }
      }
    };

  }

  namespace Frontend {

    class Base {
    protected:
      Data::Base* mData;

      enum LockingMode { Locked, NotLocked };
      template<typename T, LockingMode M = Locked>
      class DPtrT;

    private:
      template<LockingMode M> class Locker;
    };

    template<typename T, Base::LockingMode M>
    struct Locker
    {
      constexpr Locker(const T*) {}
    };

    template<typename T>
    struct Locker<Base::Locked>
      : private Backend::RepoLocker
    {
      Locker(T* t)
        : Backend::RepoLocker(t ? t->repository() : nullptr)
      {}
    };

    template<typename T, Base::LockingMode LOCKED>
    class Base::DPtrT
    {
    public:
      typedef typename T::DPtrType DPtrType;

      DPtrT(T* that)
          : d(static_cast<DPtrType*>(that->mData)), l(d)
      {}

      const DPtrType* operator->() const      { return d; }
      DPtrType* operator->()                  { return d; }
      operator const DPtrType*() const        { return d; }
      operator DPtrType*()                    { return d; }

    private:
      DPtrType* d;
      Base::Locker<typename T::DPtrType, LOCKED> l;
    };

    template<typename T, Base::LockingMode LOCKED>
    class Base::DPtrT<const T, LOCKED>
    {
    public:
      typedef typename T::DPtrType DPtrType;

      DPtrT(const T* that)
          : d(static_cast<const DPtrType*>(that->mData)), l(d)
      {}

      const DPtrType* operator->() const      { return d; }
      operator const DPtrType*() const        { return d; }

    private:
      const DPtrType* d;
      Base::Locker<const typename T::DPtrType, LOCKED> l;
    };

    class Repo : public Base {
    public:
      typedef Data::Repo DPtrType;

    public:
      bool isActive() const;
    };

    // In CPP File, actually:
    bool Repo::isActive() const {
      DPtrT<const Repo> d(this);
      return d & d->isActive();
    }

  }

}

So, messy, ya?

The point is, that it should be as easy as possible to use these templates.

DPtrT<const Foo> d(this); // if this is a const-method on a Foo object
DPtrT<      Foo> d(this); // if this is a non-const-method on a Foo object

DPtrT<const Foo, NotLocked> d(this); // If we don't require a locked mutex...

And finally, here's a annotated version of what clang (with -O3) makes out of the Repo::isActive method (omitting the meta data that does not actually produce any assembly code):

; On input to this method the pointer to the RM::Frontend::Repo object is stored
; in RDI register. The resut of this method is expected in the lowest bit of RAX

__ZNK2RM8Frontend4Repo8isActiveEv:      ; RM::Frontend::Repo::isActive()

        pushq   %rbp                    ; Safe Frame-Pointer
        movq    %rsp, %rbp              ; Load new Frame-Pointer
        pushq   %r14                    ; This code needs the registers RBX and R14
        pushq   %rbx                    ; so safe them
        movq    8(%rdi), %rbx           ; Get the value stored at RDI+8 into RBX [That is
                                        ; "this->mData"]
        testq   %rbx, %rbx              ; Test for nullptr
        je      LBB6_1                  ; If yes, get out of here
                                        ; (Note that we have not yet locked anything!)
        movq    16(%rbx), %r14          ; Get the value stored at [RBX+16] into R14
                                        ; [That is "this->mData->mRepo"]
        testq   %r14, %r14              ; Test for NULL
        je      LBB6_3                  ; If yes, jump to the branch where we do
                                        ; NOT lock the mutex
        addq    $88, %r14               ; R14 = R14 + 130 (130 is the offest of mMtx in my local
                                        ; code), so this _one_ instruction is actually the
                                        ; mRepo->mutex() call inside RepoLocker
        movq    %r14, %rdi              ; Safe pointer to this->mData->mRepo->mMtx
        callq   __ZN6QMutex4lockEv      ; into RDI and call QMutex::lock
        movb    40(%rbx), %bl           ; Get the _byte_ at RBX+40 into BL; isolate the
        andb    $8, %bl                 ; 4th bit of that byte and move this bit into
        shrb    $3, %bl                 ; position zero (40 is the index of mIsActive in my local
                                        ; code and it is there stored inside the 4th bit).
        movq    %r14, %rdi              ; Move mMtx into RDI again (the lock() call above would
                                        ; have been allowed to destroy the content of RDI)
        callq   __ZN6QMutex6unlockEv    ; Then call QMutex::unlock()
        jmp     LBB6_5                  ; and finally get out of here.

; We get here, in case this->mData is nullptr. We want to return false, thus:
LBB6_1: xorl    %ebx, %ebx              ; zero out the RBX register
        jmp     LBB6_5                  ; and get out of here

; This code is reached, if this->mData->mRepo is nullptr. (i.e. We do not belong to
; a repository, thus need not lock that repository):
LBB6_3: movb    40(%rbx), %bl           ; Same as above: Load 40th byte of this->mData into
        andb    $8, %bl                 ; BL register and isolate the 4th bit then
        shrb    $3, %bl                 ; shift it to bit 0.
        ; fall through

; We get here for cleanup in all 3 different code paths; while the result is in BL

LBB6_5: movb    %bl, %al                ; ABI says results have to be in RAX, so move BL to AL
        popq    %rbx                    ; Restore the volatile registers that we have used:
        popq    %r14                    ; RBX and R14
        popq    %rbp                    ; Restore the Frame-Pointer for the debugger or Core-
        retq                            ; Dumper - and finally: Go home...

So, quite frankly, I'd probably not be able to produce much better assembly code, if I had written this manually. Of course, the compiler cannot know that in case of RM::Frontend::Repo it is always the case that this->mData->mRepo == this... But well, I think this is the best code we could get out of that - and I'm quite suprised that the above weird C++ code actually results in such nice assembly code. Great Kudo's to the CLang devs... :)

[antis81]

Kudos to you mate :smile: :+1:! This is quite impressive and I don't think it looks messy at all.

Did you consider using QMutexLocker? This could reduce the "messyness". Other than that I'm totally fine with this concept.

[scunz]

Here's an update to the repoman branch. Nothing compiles yet, but I've rebase the shit out of that "temporary code" commit (knowing that there's still a lot inside it and that I have another one of that magnitude sitting around here locally but it doesn't apply anymore).

At times I had split that one commit into more than 120 different patches, most of which I've already collapsed again.

Now, going to get this compile again; bit by bit...

[antis81]

Now, going to get this compile again; bit by bit...

Do you think you can get this done first? As of now, we cannot proceed with the libActivities stuff and rebasing comes close to impossible.

[scunz]

Sorry, I've a huge pile of things that are distracting me right now and I've not even had some spare time over the weekend (since I spent it at the customer's office). I actually switched to my mail client right now to write a mail (shouting for some help with stuff) But I'm not sure to whom to address that in the first place...

If it helps, you might take the commits unique to the activities branch, cherrypick them to development and code against that. There's no hard dependency (yet) on repoman. You'd have to fake the event sending.

I might even say that using std::thread() you should be able to setup a background thread that does the cloning and hence develop a clone service without all the repoman stuff in place yet.

[antis81]

Ok. I'll see what I can do. Additionally, there's enough other stuff to do like completing the new website.