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

Advertise multi ack capabilities #200

Closed
wants to merge 6 commits into from

Conversation

phillebaba
Copy link

This PR hopes to fix a long standing issue for go-git when communicating with servers that require multi-ack capability. The biggest source of that problem right now is Azure DevOps. Meaning that any project that uses go-git currently can't support it.

This first PR does not fully implement multi-ack but instead advertises the capability and then does the normal transaction. My hope is to get multi ack fully implemented in a later PR, but this is a first step to enable projects to work with Azure DevOps.

I had to make a small change to how ssh connections are closed as Azure DevOps will close the connection after sending all data, instead of waiting for the client to close the connection.

Fixes #64 #157

The PR depends on @bashims PR #111, so it would be great if we could get that merged before to make sure he gets credit for his work.

@phillebaba
Copy link
Author

I dont really understand the failing test. @mcuadros do you have some insight in why it is failing only on macos?

@phillebaba phillebaba changed the title Add multi ack support Advertise multi ack capabilities Nov 2, 2020
@mcuadros
Copy link
Member

mcuadros commented Nov 3, 2020

The test is just a flaky one, sorry for that.

About the PR, I really don't understand how it is going to work; if you anounce the capabilities, nothing is going to solve the problem since it's not implemented. So I rather wait to have the full functionallity before merge it.

@phillebaba
Copy link
Author

Ah good, as I was a bit confused if it was something I was doing.

So let me start off by saying that full support is the end goal, but I do not know how long it would take for me to get it done.
I have been following the steps suggested by @smola src-d/go-git#335 (comment) to get this first feature done.

My understanding of multi_ack and correct me if I am wrong, is that it is an optimization when the client sends its haves/wants to avoid having the client walk back the full branch. This is done by the server responding with a continue when the client has walked far enough back for the server to have the data. So if the client does not listen to these responses all its doing is doing more processing and sending more data than is actually needed, it is just extra work. As long as we can parse the data format returned by the server we should be fine.
https://git-scm.com/docs/protocol-capabilities#_multi_ack

As the capabilities are determined by the server we will not advertise multi_ack unless the server can support it.

@djoyahoy
Copy link

djoyahoy commented Nov 3, 2020

I want to chime in here and say that Azure DevOps is very important to our users and even partial support would be useful to users of kpack.

@mcuadros
Copy link
Member

mcuadros commented Nov 4, 2020

The problem is that Azure DevOps, only supports multi_ack, I suppose Azure has developed his own git library and they didn't implement the classic protocol. So if you announce the support of multi_ack, then go-git is going to fail elsewhere. The only way to make it work is by implementing multi_ack support.

Multi_ack requires a refactor of the protocol and some other packages, which is a middium task that will require quite amount of hours and knowledge, who know this better is @smola

@djoyahoy version November 2019 this project has lost his unique supporter, so many of the contributors are out of time to work on this, like my case. If big corporations are using this library will be great if they can support it.

@phillebaba
Copy link
Author

I will close this PR now as we have added support for both libgit2 and go-git in the flux project for those who requires multi_ack capabilities.

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

Successfully merging this pull request may close these issues.

Git Clone does not work with Azure DevOps repos
3 participants