Support for packed refs #2121
Support for packed refs #2121
Conversation
This PR also combines `git.*Iterator` into a `git.Repository` and make the construction of the `ObjectReader` internal to it. Now there is a single exported method that takes in the `.git` path. This also opens us up to provide the `git.Repository#URL` method soon.
|
Sorry for the poor diff, the vast majority of the code has already been reviewed by you two! Let me know if you'd like me to break this apart instead (merging first, then support packed refs). |
|
@unmultimedio see if you can get through it without @saquibmian having to split anything up |
There was a problem hiding this comment.
Seems to me like reading commits and tags should be done also in a sync.Once, and all of them should be stored in memory in the repository object?
Also, probably if you have local git history, it'd be a bit easier to separate in commits the iterators interfaces merge, vs the actual packed logic, so the diff is a bit cleaner. If it's not a big deal, it'd help quite a bit.
| // BaseBranch is the base branch of the repository. This is either configured | ||
| // via the `OpenRepositoryWithBaseBranch` option, or discovered via the remote | ||
| // named `origin`. Therefore, discovery requires that the repository is pushed | ||
| // to the remote. |
There was a problem hiding this comment.
Should we default to main in case it's neither pushed nor configured?
There was a problem hiding this comment.
Ideally we auto discover as much as we can, but in the cases where we can't, I can see a buf sync --main-branch develop to help with this.
There was a problem hiding this comment.
Should this be a flag or a config in buf.yaml? It feels to me like this is a reasonable use-case for extending the config file since we already have BSR repository related config (e.g. name) in the config.
There was a problem hiding this comment.
I think when this came up at some point, we talked about storing refspecs in the buf.yaml and it was shot down? It was before my time, but I remember reading it and now I can't find it.
I think this is not module config, this is sync config. We already somewhat ignore the name in buf.yaml in sync world (because names can change over time), which I think is step in the right direction.
Generally, the less config we have, the more we've won with buf sync.
There was a problem hiding this comment.
Fair enough -- I am onboard with this approach :)
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| if strings.HasPrefix(ref, originBranchRefPrefix) { |
There was a problem hiding this comment.
So only tags, and branches in remotes named origin will be read, why? If so pls document it (and the why) in the func godoc.
There was a problem hiding this comment.
This is documented in the godoc for ForEachBranch. As for why, it's because we only want to sync state that's been pushed, we don't want to sync state that is local. For tags I haven't found a way to determine if a tag is pushed yet.
There was a problem hiding this comment.
If we can execute git commands, we could fetch origin tags and only loop remote tags? Although, outside of this PR scope.
There was a problem hiding this comment.
Yup, but ideally I can find a way to do that without forking out to git.
|
We could definitely read unpacked branches and tags in the sync.Once as well, I mainly didn't want to add too much to this PR. I am planning on caching those as well as all Commits/trees/blobs will be cached by the object reader cache, the core algorithm itself is not worth caching as a particular commit tree will likely not be visited more than once. |
This PR also combines
git.*Iteratorinto agit.Repositoryand make the construction of theObjectReaderinternal to it. Now there is a single exported method that takes in the.gitpath.This also opens us up to provide the
git.Repository#URLmethod soon.