remote: add the last 100 commits for each ref in haves list #610
Conversation
strib
force-pushed
the
strib/gh-master-list-extra-haves
branch
from
e1b9f7b to
c337614
Compare
Codecov Report
@@ Coverage Diff @@
## master #610 +/- ##
=========================================
- Coverage 78.28% 77.68% -0.6%
=========================================
Files 130 130
Lines 10160 10197 +37
=========================================
- Hits 7954 7922 -32
- Misses 1351 1428 +77
+ Partials 855 847 -8
Continue to review full report at Codecov.
|
|
By the way, this is what all my recent PRs having been working towards: https://keybase.io/blog/encrypted-git-for-everyone (shout-out to this project in that post). Thanks everyone here for the library and your hard work! |
There was a problem hiding this comment.
One thing to note is that this may not be temporary code at all. We may want to preserve this type of behavior when communicating with any server that does not support the multi_ack or multi_ack_detailed capabilities.
On closer inspection of the git documentation for the pack protocol I get the impression that without multi_ack it might be expected that the client send have lines for ALL commit hashes that it can't assume the server has. Maybe we want to update this code to do that, rather than just up to 100 descendants for each client branch?
remote.go
Outdated
| // Build a map of all the remote references, to avoid loading too | ||
| // many parent commits for references we know don't need to be | ||
| // transferred. | ||
| stopEarlyRefs := map[plumbing.Hash]bool{} |
There was a problem hiding this comment.
Maybe we can rename this to remoteRefs and rename the function parameter to remoteRefStorer?
remote.go
Outdated
| return nil, err | ||
| } | ||
| err = iter.ForEach(func(ref *plumbing.Reference) error { | ||
| stopEarlyRefs[ref.Hash()] = true |
There was a problem hiding this comment.
Do we need to do something special here to confirm the ref isn't symbolic?
remote.go
Outdated
| // upload pack request, include up to 100 commits from the | ||
| // history of each ref. | ||
| walker := object.NewCommitPreorderIter(commit, haves, nil) | ||
| const maxToVisit = 100 |
There was a problem hiding this comment.
since it's a constant, what do you think about moving this to the top level?
If the local ref is not an ancestor of the remote ref being fetched, then when we send an UploadPack request with that local ref as one of the Haves, the remote will not recognize it, and will think we are asking for the entire history of the repo, even if there's a common ancestor. To do this right, we need to support the multi-ack protocol so we can negotiate a common commit. That's hard though; this is a quick fix just to include the previous 100 commits for each local ref in the Haves list, and hope that one of them is the common commit.
strib
force-pushed
the
strib/gh-master-list-extra-haves
branch
from
c337614 to
39853aa
Compare
|
Thanks everyone for taking a look. I made all the suggested changes, and factored out most of the logic into two new helper functions that can be dropped if desired when multi-ack is ready. I made a top-level constant that can be set to 0, if we decide we want it to send ALL haves that the remote might not know about. Please take another look, thanks! |
I found that a
Remote.Fetchcall, where the repo's branch is ahead of the remote, causes the remote to send the entire history of the branch being fetched, because no common ancestor is negotiated. This represents a quick and dirty fix until multi-ack is implemented. Understandable if you don't want to merge half-measures, thought I'd submit it just in case!===========
If the local ref is not an ancestor of the remote ref being fetched, then when we send an UploadPack request with that local ref as one of the Haves, the remote will not recognize it, and will think we are asking for the entire history of the repo, even if there's a common ancestor.
To do this right, we need to support the multi-ack protocol so we can negotiate a common commit. That's hard though; this is a quick fix just to include the previous 100 commits for each local ref in the Haves list, and hope that one of them is the common commit.