Open CoderCow opened 6 years ago
I have quickly looked through it and tested it with a few small commits plus a ~1 GB commit and it seems to work fine :)
Generally it looks very well written. So +1 for some unit test.
In addition I used git 2.15.0.windows.1 as my local client.
Is it possible to add more handlers or is only one supporrted as is? for me it looks as only one is supported, but at places you write as if multiple handlers could be added?
Generally it looks very well written.
Thank you sir.
Is it possible to add more handlers or is only one supporrted as is?
Yep, if you wanted to add more handlers you'd just decorate one another - pretty much the same way as the IGitService
implementations do it. This gives a lot of control over the order and when (if even) each handler decides to call the next one.
Alright, added some unit tests for the important parts of ReceivePackInspectStream
and actually fixed two bugs. The stream is now tested against every possible vital push scenario I could think of (using an up-to-date version of Git).
Considering the already existing msys integration tests, which are using different Git client versions and perform some basic pushes, I'd call this ready for merge now.
This could be a good starting point thinking about the usefulness of implementing webhooks.
if it had so good reviews i only con wonder why it was denied still after 2 years...
The new implementation is slimmer and easier to grab and shouldn't cause any noticeable performance overhead at all (which the old implementation did very much). The way I designed it should make implementing features like #13 and #299 a breeze, at least regarding the Git stuff. It can also be extended to introduce things like per-branch permissions in the future, if desired.
Compared to the current state of the codebase, the only critical thing this implementation adds is the ReceivePackInspectStream class, because an instance of it is installed in the input stream for every push operation. It could cause pushes to fail. There's no way but to test it throughly against different Git clients and push scenarios before we can call this production ready. That said, nothing else added by this PR can be considered critical - the worst thing which could happen is that some commits may not have notes added to them when audit is enabled...
Note that I've removed the durability / recovery feature completely as it didn't really make sense to me. If the Git process fails, the new handler which adds the commit notes will never be invoked. If the Git process succeeds but adding notes to the commits fails for some reason, then we should try to fix the root of that problem instead of temporarily saving request data to disk and trying to add the notes over and over again until it works. I don't see why it was implemented like that.
I've removed these configuration settings:
Please let me know what you think about the new implementation. If it looks alright, I'll go ahead and write some tests, especially for the ReceivePackInspectStream class.