[v6-exp] Consolidate in-flight changes #1166
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
go-git does not support concurrency. In some cases, read concurrency may lead to repositories becoming corrupted or actions to yield invalid results (e.g. not finding an existing ref). This commit is an initial attempt to start supporting read concurrency. Some initial use cases that seem to be enabled by these changes: - repository.CommitObjects().Foreach() - repository.Tags().Foreach() - commit.IsAncestor() The mutex objects that were introduced must be expanded other parts of the code base affected, and some performance off-setting is planned to take place. Both will be tackled in follow-up commits. Signed-off-by: Paulo Gomes <[email protected]>
The implementation of the v2 wire protocol will require that the protocol can be set and shared amongst different parts of the code base. This change introduces the config support for managing protocol.version but more importantly introduces the Version type in plumbing/protocol. Signed-off-by: Paulo Gomes <[email protected]>
The BoundOS further aligns go-git with the upstream behaviour, removing suprises that generally catch go-git users off-guard. For more information refer to go-billy's docs: https://github.com/go-git/go-billy/blob/69f6dc8f11964f3aafa2a7a0be6db030ad43ecf4/osfs/os_bound.go#L32-L42 Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Member
aymanbagabas
left a comment
aymanbagabas
left a comment
There was a problem hiding this comment.
LGTM, some minor design comments
config/config.go
Outdated
Comment on lines
122
to
124
| // using the specified protocol version. If the server does not | ||
| // support it, communication falls back to version 0. If unset, | ||
| // the default is 2. Supported versions: |
Member
There was a problem hiding this comment.
Suggested change
| // using the specified protocol version. If the server does not | |
| // support it, communication falls back to version 0. If unset, | |
| // the default is 2. Supported versions: | |
| // using the specified protocol version. If the server does not | |
| // support it, communication falls back to version 0. If unset, | |
| // the default version will be used. Supported versions: |
Member
There was a problem hiding this comment.
Support depends on the implementation of both client and server.
config/config.go
Outdated
| // should be marshalled or not. | ||
| // Note that this does not need to align with the default protocol | ||
| // version from plumbing/protocol. | ||
| DefaultProtocolVersion = protocol.Unknown |
Member
There was a problem hiding this comment.
Suggested change
| DefaultProtocolVersion = protocol.Unknown | |
| DefaultProtocolVersion = protocol.V0 // go-git only supports V0 for the moment |
Comment on lines
+126
to
+129
| // 0 - the original wire protocol. | ||
| // 1 - the original wire protocol with the addition of a | ||
| // version string in the initial response from the server. | ||
| // 2 - Wire protocol version 2. |
Member
There was a problem hiding this comment.
Fwiw, go-git only supports V0 as of now
plumbing/protocol/version.go
Outdated
| type Version int | ||
|
|
||
| const ( | ||
| Unknown Version = -1 |
Member
There was a problem hiding this comment.
Suggested change
| Unknown Version = -1 | |
| Undefined Version = -1 |
Perhaps Undefined signifies the intention better
|
|
||
| // Override http(s) default protocol to use our custom client | ||
| client.InstallProtocol("https", githttp.NewClient(customClient)) | ||
| transport.Register("https", githttp.NewClient(customClient)) |
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Signed-off-by: Paulo Gomes <[email protected]>
Member
|
@pjbgf Thinking more about |
Member
Author
|
@aymanbagabas good point. I think we are better positioned to review the mechanics around this during the implementation of newer protocol versions, so we can test it E2E. I am aiming to work on this soonish - just trying to get other minor changes on v6-exp out of the way first. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A few changes aimed at the
v6-expbranch were floating around different branches. This PR consolidates those changes so that those other branches can be deleted.Changes of note:
osfs.BoundOSthe defaultosfs(fixes Symlinks pointing outside repository are mangled with PlainOpen #1155).