-
Notifications
You must be signed in to change notification settings - Fork 325
[WIP] Use a managed HTTP(S) transport implementation #374
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
Conversation
5fee079
to
3e26c31
Compare
@carlosmn This is pretty exciting, I actually hacked on implementing pure-go transports a few months ago. Would be great to reduce the surface area of C code and potential memory leaks. We've had issues with I also explored a more radical approach and hacked together a fairly useable |
This implements a "smart subtransport" which lets us implement just the I/O portion and leave the Git Smart Protocol logic in the library.
We currently cannot ask the transport for its proxy settings, so for now we'll use the one from the environment.
This does get called when we use the libgit2 stack, but is not yet something the Go one does.
The test for now just tests that we can handle returning a `GIT_EUSER` if the caller decides to abort.
This lets us stream the data straight from the creation of the packfile, instead of having to buffer it all in memory.
header.Data = uintptr(unsafe.Pointer(data)) | ||
|
||
n, err := stream.Read(p) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per https://golang.org/pkg/io/#Reader, this should be
if err != nil { | |
if n == 0 && err != nil { |
you should also be able to add something along the lines of diff --git a/script/build-libgit2-static.sh b/script/build-libgit2-static.sh
index 680dd93..a17944a 100755
--- a/script/build-libgit2-static.sh
+++ b/script/build-libgit2-static.sh
@@ -12,6 +12,8 @@ cd "${BUILD_PATH}/build" &&
cmake -DTHREADSAFE=ON \
-DBUILD_CLAR=OFF \
-DBUILD_SHARED_LIBS=OFF \
+ -DUSE_HTTPS=OFF \
+ -DUSE_EXT_HTTP_PARSER=OFF \
-DCMAKE_C_FLAGS=-fPIC \
-DCMAKE_BUILD_TYPE="RelWithDebInfo" \
-DCMAKE_INSTALL_PREFIX="${BUILD_PATH}/install" \ |
Hi! I did a small refactoring of this change and a) split it between creating a Transport interface and implementing the HTTP/HTTPs transports with that, libgit2 can be compiled without openssh and openssl! with a bit more work, we can probably make it fully static. you can find the refactor here: https://github.com/lhchavez/git2go/commits/transports what would be the best course of action to move this change forward? |
I just landed #810, which was heavily based upon this change, with proxy support! |
This is a bit like #372 but goes further by replacing the whole HTTP stack and just leaving the logic in libgit2. This lets us use the Go HTTP code as well, instead of the one in the library.
The current state is enough to let the tests work but there is no custom proxy configuration available and we don't currently test that authentication actually works.
Proxy support depends on libgit2/libgit2#4206 or similar getting merged. I guess we'll have to add poxyproxy to our tests as well so it doesn't regress.
While this is useful by itself (by taking us completely away from OpenSSL and libcurl in libgit2), a potentially more exciting outcome might be to build on the lessons from this to build a managed ssh transport, which would eliminate the need for libssh2.