Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit Feature Broken #767

Open
CoderCow opened this issue Dec 25, 2017 · 3 comments
Open

Audit Feature Broken #767

CoderCow opened this issue Dec 25, 2017 · 3 comments

Comments

@CoderCow
Copy link

The push audit feature isn't working. I'm certain this is due to the removal of the Unity Decorator Extension by 985a98e (@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
Copy link
Author

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
Copy link
Collaborator

@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
Copy link
Author

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.

@CoderCow CoderCow changed the title Auditing Feature Broken Audit Feature Broken Dec 28, 2017
CoderCow added a commit to CoderCow/Bonobo-Git-Server that referenced this issue Jan 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants