Conversation
alecthomas
left a comment
alecthomas
left a comment
There was a problem hiding this comment.
Overall LGTM if it works! :)
| var ( | ||
| sharedManager *Manager | ||
| sharedManagerMu sync.RWMutex | ||
| ) | ||
|
|
||
| func SetShared(m *Manager) { | ||
| sharedManagerMu.Lock() | ||
| defer sharedManagerMu.Unlock() | ||
| sharedManager = m | ||
| } | ||
|
|
||
| func GetShared() *Manager { | ||
| sharedManagerMu.RLock() | ||
| defer sharedManagerMu.RUnlock() | ||
| return sharedManager | ||
| } |
There was a problem hiding this comment.
I can't tell why this exists. We should be constructing a single Manager in main, then passing it everywhere, so AFAICT we shouldn't need this?
There was a problem hiding this comment.
Because of where things load and where things need to be constructed, this gets more complicated than you might think. I'll address this as a separate PR because I think it might be easier to illustrate the problem and then we can work out what to do.
| var output []byte | ||
| var err error | ||
|
|
||
| repo.WithReadLock(func() { |
There was a problem hiding this comment.
Could also use generics here, so you can return values:
func WithRepoReadLock[R any](repo *Repository, apply func() (R, error)) (value R, err error) {
repo.WithReadLock(func() { value, err = apply() })
return
}Usage:
output, err := gitclone.WithRepoReadLock(repo, func() ([]byte, error) {
cmd := exec.CommandContext(ctx, "git", "-C", repo.Path(), "log", "-1", "--format=%cI", ref)
return cmd.CombinedOutput()
})There was a problem hiding this comment.
Slightly clearer, but eh, your call
There was a problem hiding this comment.
I might clean this up in a separate PR. Just because I think it's going to touch some additional code in the gitclone package.
Co-authored-by: Alec Thomas <aat@block.xyz>
…m/block/sfptc into johnm/gomod-private-attempt-three
2 participants
What?
Re-implements #77, which adds support for private gomod repositories.
Why?
So that we are able to cache private go modules.
Tests?
Includes new unit tests. Additionally some manual testing has been performed...