search

jakubgarfield / Bonobo-Git-Server

Bonobo Git Server for Windows is a web application you can install on your IIS and easily manage and connect to your git repositories. Go to homepage for release and more info.
http://bonobogitserver.com
MIT License
1.81k stars 603 forks source link

Audit Feature Broken #767

Open CoderCow opened 6 years ago

CoderCow commented 6 years ago

The push audit feature isn't working. I'm certain this is due to the removal of the Unity Decorator Extension by 985a98e2328ff02d8c8829dccb43883fef79b1bd (@RedX2501). This causes none of the other IGitService implementations to work because they're meant decorate each other and thus gives the auditing code no chance to be executed at all.

Apparently the decorator caused a bit of trouble before, as stated by this comment, but even though it was removed, the comment and implementation there wasn't changed.

So I wonder if it was actually intended to remove the decorator? Is there a purposed replacement for it? If not, I'd suggest to revert that commit to get the auditing functionality back.

CoderCow commented 6 years ago

Taking a closer look at the implementation of ReceivePackParser I realized that it is unusable the way it is at the moment. This is due to the fact that it uses seeking on the inStream it gets passed, which is actually an unbuffered stream (thus not seekable).

As far as I'm concerned git is able to split PACKs up into multiple parts (--max-pack-size option which is unlimited by default) but doesn't support splitting them if one single file in a commit exceeds this size (not sure about this tho). So effectively, if we put any hard limit on the PACK size we'd actually limit the max size of a file in the repository or at least force the client to change their settings to split PACKs into a size the server can consume.

So in order to stay with the unbuffered approach, the current implementation of the auditing feature including the pack parsing must be rewritten into a stream-based solution capable of handling gigabytes of PACK data without noticeable memory consumption.

I think its worth the work, considering that the auditing feature and its implementation is basically the foundation for #299 and #13.

I'm sorry if all of this has been discussed before or has already been obvious. I didn't look around very hard. Just wrote up my current view of things.

Because I really want to see #299 implemented eventually, I might try implementing the said stream-based pack parsing myself, if this sounds reasonable and nobody else is currently working on this?

willdean commented 6 years ago

@CoderCow I don't think anyone is working on anything here at the moment, so feel free to have a go.

It would be a really useful to try and get more of this code under (unit, rather than just top-level integration) test - some of the low-level Git-interface code is grotesquely inefficient as it is currently implemented, but I'm really reluctant to have a go at it without any test coverage, even just in terms of reworking it to be a more efficient version of the same thing.

When is comes to working on the pack handling, please be guided by the project's contribution guidelines and C#/.NET best practice rather than by the existing code!

CoderCow commented 6 years ago

Alright, I'll try to respect that.

My current goal will be reimplementing the parsing logic and fixing the auditing feature, redesigning / dropping some of the types it originally introduced along the way. I'll PR if I have something that's working, though I shall request a review of the new production code first before I start to implement any tests for this.