git: Fix fetching after shallow clone. Fixes #305#766
git: Fix fetching after shallow clone. Fixes #305#766AriehSchneier wants to merge 1 commit intogo-git:masterfrom
Conversation
| @@ -864,6 +865,21 @@ func getHavesFromRef( | |||
| toVisit := maxHavesToVisitPerRef | |||
| return walker.ForEach(func(c *object.Commit) error { | |||
There was a problem hiding this comment.
Another option would be just to completely ignore any errors returned from this walker.ForEach call.
pjbgf
left a comment
There was a problem hiding this comment.
The boolean is redundant, so I would use struct{} instead for a slight smaller memory profile.
8336a5d to
973deae
Compare
pjbgf
left a comment
There was a problem hiding this comment.
Had another go at it and found a few other nits below.
| haves[c.Hash] = true | ||
|
|
||
| // initialise the shallows map | ||
| if *shallows == nil { |
There was a problem hiding this comment.
Deferencing a nil pointer leads to panic.
| if *shallows == nil { | |
| if shallows == nil { |
There was a problem hiding this comment.
This is a private function that we only call in 1 location, it will also contain a value (I would have passed by reference if go had that feature but it doesn't). We need to check whether the location that this variable is pointing to is nil or not. I could change this to if shallows != nil && *shallows != nil { but IMHO that would add an extra unnecessary check.
| remoteRefs map[plumbing.Hash]bool, | ||
| s storage.Storer, | ||
| haves map[plumbing.Hash]bool, | ||
| shallows *map[plumbing.Hash]struct{}, |
There was a problem hiding this comment.
This will inevitably be passed "by reference".
| shallows *map[plumbing.Hash]struct{}, | |
| shallows map[plumbing.Hash]struct{}, |
There was a problem hiding this comment.
There was a problem hiding this comment.
PS if you test your suggested change you will find that nil will always be passed in and we will recalculate the map for every iteration of range localRefs
There was a problem hiding this comment.
You are absolutely right, thanks for sharing the link. 👍
| // initialise the shallows map | ||
| if *shallows == nil { | ||
| shallowList, _ := s.Shallow() | ||
| *shallows = make(map[plumbing.Hash]struct{}, len(shallowList)) |
There was a problem hiding this comment.
| *shallows = make(map[plumbing.Hash]struct{}, len(shallowList)) | |
| shallows = make(map[plumbing.Hash]struct{}, len(shallowList)) |
| shallowList, _ := s.Shallow() | ||
| *shallows = make(map[plumbing.Hash]struct{}, len(shallowList)) | ||
| for _, sh := range shallowList { | ||
| (*shallows)[sh] = struct{}{} |
There was a problem hiding this comment.
| (*shallows)[sh] = struct{}{} | |
| shallows[sh] = struct{}{} |
| } | ||
|
|
||
| // If this is a shallow reference don't try to iterate further | ||
| if _, ok := (*shallows)[c.Hash]; ok { |
There was a problem hiding this comment.
| if _, ok := (*shallows)[c.Hash]; ok { | |
| if _, ok := shallows[c.Hash]; ok { |
| } | ||
|
|
||
| err = getHavesFromRef(ref, remoteRefs, s, haves) | ||
| err = getHavesFromRef(ref, remoteRefs, s, haves, &shallows) |
There was a problem hiding this comment.
| err = getHavesFromRef(ref, remoteRefs, s, haves, &shallows) | |
| err = getHavesFromRef(ref, remoteRefs, s, haves, shallows) |
pjbgf
left a comment
There was a problem hiding this comment.
Had another go at it and found a few other nits below.
973deae to
254c9e4
Compare
Signed-off-by: Arieh Schneier <15041913+AriehSchneier@users.noreply.github.com>
254c9e4 to
05da5ef
Compare
|
Closing this in favour of #778 |
Also fixes #207