Conversation
…dTransferManifest()
…rg of NewTransferQueue()
ttaylorr
left a comment
There was a problem hiding this comment.
Looks awesome, just one comment that needs your attention.
commands/commands.go
Outdated
| func newDownloadCheckQueue(options ...tq.Option) *tq.TransferQueue { | ||
| return lfs.NewDownloadCheckQueue(cfg, options...) | ||
| func newDownloadCheckQueue(manifest *tq.Manifest, remote string, options ...tq.Option) *tq.TransferQueue { | ||
| allOptions := make([]tq.Option, len(options), len(options)+1) |
There was a problem hiding this comment.
Should this be initialized with an empty length, instead of len(options)? Since you're appending below, it'll append after the zero-value elements it creates with the length argument. (From one of my PRs: 7af9281)
There was a problem hiding this comment.
Sure, better safe than sorry.
lfsapi/lfsapi.go
Outdated
| return c, nil | ||
| } | ||
|
|
||
| func (c *Client) GitEnv() env { |
There was a problem hiding this comment.
Do these need to be public? If so, I think we should also export env, since right now we're returning a private interface, so the caller has no idea what methods are exposed.
There was a problem hiding this comment.
Not really, since you pass the os and git envs to NewClient(). If transfer adapters get extracted to a separate package, I think it will need to be public. I'll unexport for now, and we can export later if necessary.
There was a problem hiding this comment.
Gah, should've looked at the code. This is needed in tq :(
| Objects []*api.ObjectResource `json:"objects"` | ||
| } | ||
|
|
||
| func (c *tqClient) Batch(remote string, bReq *batchRequest) (*batchResponse, *http.Response, error) { |
There was a problem hiding this comment.
😍 Really clean, this is awesome.
| a.workerWait.Done() | ||
| } | ||
|
|
||
| func (a *adapterBase) newHTTPRequest(method string, rel *Action) (*http.Request, error) { |
There was a problem hiding this comment.
Nice 👌, so cool that we got rid of the httputil dependency.
|
Pending a 🍏 test-suite, 👍 |
This updates the
tq.TransferQueueto uselfsapi.Clientto make batch API requests, following the pattern set in #1824.Changes:
access,operation, orremoteintq.Manifest. I initially went with the approach described in Api/tq manifest #1826, but didn't like it. Now, a*tq.Manifestis fully immutable, and can be used across multiple transfer queues for different LFS servers. This was impossible before, sinceaccesswas scoped to an LFS endpoint.tq.batchResponsenow stores thelfsapi.Endpointused for the batch request.tq.toAdapterCfg()uses that Endpoint in conjunction with the transfer queue'stq.Manifestto provide the config for adapters. This is where the concurrency will be set to1for ntlm authenticated LFS services.tq.NewTransferQueue()now requires aremoteargument, since it can't useCurrentRemotein*config.Configuration.lfsapi.Client.TODO:
api-master