From 40786709a2098c663d10cf982939f874403bbc72 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Tue, 3 Jan 2017 15:23:37 -0700 Subject: [PATCH 01/24] commands: make the tq.Manifest an explicit argument --- commands/command_fetch.go | 2 +- commands/command_prune.go | 2 +- commands/command_pull.go | 2 +- commands/commands.go | 27 +++++++++++++++------------ commands/uploader.go | 6 ++++-- 5 files changed, 22 insertions(+), 17 deletions(-) diff --git a/commands/command_fetch.go b/commands/command_fetch.go index 05404fba..98edba52 100644 --- a/commands/command_fetch.go +++ b/commands/command_fetch.go @@ -279,7 +279,7 @@ func fetchAndReportToChan(allpointers []*lfs.WrappedPointer, filter *filepathfil } ready, pointers, meter := readyAndMissingPointers(allpointers, filter) - q := newDownloadQueue(tq.WithProgress(meter)) + q := newDownloadQueue(TransferManifest(), tq.WithProgress(meter)) if out != nil { // If we already have it, or it won't be fetched diff --git a/commands/command_prune.go b/commands/command_prune.go index b1314c73..a8c0d371 100644 --- a/commands/command_prune.go +++ b/commands/command_prune.go @@ -122,7 +122,7 @@ func prune(fetchPruneConfig config.FetchPruneConfig, verifyRemote, dryRun, verbo if verifyRemote { cfg.CurrentRemote = fetchPruneConfig.PruneRemoteName // build queue now, no estimates or progress output - verifyQueue = newDownloadCheckQueue() + verifyQueue = newDownloadCheckQueue(TransferManifest()) verifiedObjects = tools.NewStringSetWithCapacity(len(localObjects) / 2) // this channel is filled with oids for which Check() succeeded & Transfer() was called diff --git a/commands/command_pull.go b/commands/command_pull.go index 6bd6e97c..8effc07b 100644 --- a/commands/command_pull.go +++ b/commands/command_pull.go @@ -47,7 +47,7 @@ func pull(filter *filepathfilter.Filter) { pointers := newPointerMap() meter := progress.NewMeter(progress.WithOSEnv(cfg.Os)) singleCheckout := newSingleCheckout() - q := newDownloadQueue(tq.WithProgress(meter)) + q := newDownloadQueue(singleCheckout.manifest, tq.WithProgress(meter)) gitscanner := lfs.NewGitScanner(func(p *lfs.WrappedPointer, err error) { if err != nil { LoggedError(err, "Scanner error") diff --git a/commands/commands.go b/commands/commands.go index 365822d3..54421e6a 100644 --- a/commands/commands.go +++ b/commands/commands.go @@ -38,6 +38,12 @@ var ( excludeArg string ) +// TransferManifest builds a tq.Manifest from the commands package global +// cfg var. +func TransferManifest() *tq.Manifest { + return lfs.TransferManifest(cfg) +} + func newAPIClient() *lfsapi.Client { c, err := lfsapi.NewClient(cfg.Os, cfg.Git) if err != nil { @@ -59,25 +65,22 @@ func newLockClient(remote string) *locking.Client { return lockClient } -// TransferManifest builds a tq.Manifest from the commands package global -// cfg var. -func TransferManifest() *tq.Manifest { - return lfs.TransferManifest(cfg) -} - // newDownloadCheckQueue builds a checking queue, checks that objects are there but doesn't download -func newDownloadCheckQueue(options ...tq.Option) *tq.TransferQueue { - return lfs.NewDownloadCheckQueue(cfg, options...) +func newDownloadCheckQueue(manifest *tq.Manifest, options ...tq.Option) *tq.TransferQueue { + allOptions := make([]tq.Option, len(options), len(options)+1) + allOptions = append(allOptions, options...) + allOptions = append(allOptions, tq.DryRun(true)) + return newDownloadQueue(manifest, allOptions...) } // newDownloadQueue builds a DownloadQueue, allowing concurrent downloads. -func newDownloadQueue(options ...tq.Option) *tq.TransferQueue { - return lfs.NewDownloadQueue(cfg, options...) +func newDownloadQueue(manifest *tq.Manifest, options ...tq.Option) *tq.TransferQueue { + return tq.NewTransferQueue(tq.Download, manifest, options...) } // newUploadQueue builds an UploadQueue, allowing `workers` concurrent uploads. -func newUploadQueue(options ...tq.Option) *tq.TransferQueue { - return lfs.NewUploadQueue(cfg, options...) +func newUploadQueue(manifest *tq.Manifest, options ...tq.Option) *tq.TransferQueue { + return tq.NewTransferQueue(tq.Upload, manifest, options...) } func buildFilepathFilter(config *config.Configuration, includeArg, excludeArg *string) *filepathfilter.Filter { diff --git a/commands/uploader.go b/commands/uploader.go index 6634b7cf..609e0d70 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -13,6 +13,7 @@ var uploadMissingErr = "%s does not exist in .git/lfs/objects. Tried %s, which m type uploadContext struct { DryRun bool + manifest *tq.Manifest uploadedOids tools.StringSet } @@ -20,6 +21,7 @@ func newUploadContext(dryRun bool) *uploadContext { return &uploadContext{ DryRun: dryRun, uploadedOids: tools.NewStringSet(), + manifest: TransferManifest(), } } @@ -75,7 +77,7 @@ func (c *uploadContext) prepareUpload(unfiltered []*lfs.WrappedPointer) (*tq.Tra // build the TransferQueue, automatically skipping any missing objects that // the server already has. - uploadQueue := newUploadQueue(tq.WithProgress(meter), tq.DryRun(c.DryRun)) + uploadQueue := newUploadQueue(c.manifest, tq.WithProgress(meter), tq.DryRun(c.DryRun)) for _, p := range missingLocalObjects { if c.HasUploaded(p.Oid) { // if the server already has this object, call Skip() on @@ -99,7 +101,7 @@ func (c *uploadContext) checkMissing(missing []*lfs.WrappedPointer, missingSize return } - checkQueue := newDownloadCheckQueue() + checkQueue := newDownloadCheckQueue(c.manifest) transferCh := checkQueue.Watch() done := make(chan int) From 4ab463585cbf9c22a2b1080a740da83e4b481d3a Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Tue, 3 Jan 2017 15:23:53 -0700 Subject: [PATCH 02/24] lfs: remove useless tq.NewTransferQueue wrappers --- lfs/download_queue.go | 19 ------------------- lfs/upload_queue.go | 11 ----------- test/git-lfs-test-server-api/main.go | 3 ++- 3 files changed, 2 insertions(+), 31 deletions(-) delete mode 100644 lfs/download_queue.go delete mode 100644 lfs/upload_queue.go diff --git a/lfs/download_queue.go b/lfs/download_queue.go deleted file mode 100644 index 7e4b7ae2..00000000 --- a/lfs/download_queue.go +++ /dev/null @@ -1,19 +0,0 @@ -package lfs - -import ( - "github.com/git-lfs/git-lfs/config" - "github.com/git-lfs/git-lfs/tq" -) - -// NewDownloadCheckQueue builds a checking queue, checks that objects are there but doesn't download -func NewDownloadCheckQueue(cfg *config.Configuration, options ...tq.Option) *tq.TransferQueue { - allOptions := make([]tq.Option, len(options), len(options)+1) - allOptions = append(allOptions, options...) - allOptions = append(allOptions, tq.DryRun(true)) - return NewDownloadQueue(cfg, allOptions...) -} - -// NewDownloadQueue builds a DownloadQueue, allowing concurrent downloads. -func NewDownloadQueue(cfg *config.Configuration, options ...tq.Option) *tq.TransferQueue { - return tq.NewTransferQueue(tq.Download, TransferManifest(cfg), options...) -} diff --git a/lfs/upload_queue.go b/lfs/upload_queue.go deleted file mode 100644 index f8df98c3..00000000 --- a/lfs/upload_queue.go +++ /dev/null @@ -1,11 +0,0 @@ -package lfs - -import ( - "github.com/git-lfs/git-lfs/config" - "github.com/git-lfs/git-lfs/tq" -) - -// NewUploadQueue builds an UploadQueue, allowing `workers` concurrent uploads. -func NewUploadQueue(cfg *config.Configuration, options ...tq.Option) *tq.TransferQueue { - return tq.NewTransferQueue(tq.Upload, TransferManifest(cfg), options...) -} diff --git a/test/git-lfs-test-server-api/main.go b/test/git-lfs-test-server-api/main.go index f6cf26de..ffb1f63e 100644 --- a/test/git-lfs-test-server-api/main.go +++ b/test/git-lfs-test-server-api/main.go @@ -161,7 +161,8 @@ func buildTestData() (oidsExist, oidsMissing []TestObject, err error) { outputs := repo.AddCommits([]*test.CommitInput{&commit}) // now upload - uploadQueue := lfs.NewUploadQueue(config.Config, tq.WithProgress(meter)) + manifest := lfs.TransferManifest(config.Config) + uploadQueue := tq.NewTransferQueue(tq.Upload, manifest, tq.WithProgress(meter)) for _, f := range outputs[0].Files { oidsExist = append(oidsExist, TestObject{Oid: f.Oid, Size: f.Size}) From 7878bc35835d9a8c95bb4b4f197a399cc26c9921 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Tue, 3 Jan 2017 15:48:30 -0700 Subject: [PATCH 03/24] tq: add NewManifestWithClient constructor --- lfsapi/lfsapi.go | 8 ++++++++ test/git-lfs-test-server-api/main.go | 9 +++++++-- tq/manifest.go | 14 +++++++++++++- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go index 73ae2e96..c4e04855 100644 --- a/lfsapi/lfsapi.go +++ b/lfsapi/lfsapi.go @@ -90,6 +90,14 @@ func NewClient(osEnv env, gitEnv env) (*Client, error) { return c, nil } +func (c *Client) GitEnv() env { + return c.gitEnv +} + +func (c *Client) OSEnv() env { + return c.osEnv +} + func IsDecodeTypeError(err error) bool { _, ok := err.(*decodeTypeError) return ok diff --git a/test/git-lfs-test-server-api/main.go b/test/git-lfs-test-server-api/main.go index ffb1f63e..d8162d58 100644 --- a/test/git-lfs-test-server-api/main.go +++ b/test/git-lfs-test-server-api/main.go @@ -49,7 +49,6 @@ func main() { } func testServerApi(cmd *cobra.Command, args []string) { - if (len(apiUrl) == 0 && len(cloneUrl) == 0) || (len(apiUrl) != 0 && len(cloneUrl) != 0) { exit("Must supply either --url or --clone (and not both)") @@ -138,6 +137,12 @@ func (*testDataCallback) Errorf(format string, args ...interface{}) { } func buildTestData() (oidsExist, oidsMissing []TestObject, err error) { + cfg := config.Config + apiClient, err := lfsapi.NewClient(cfg.Os, cfg.Git) + if err != nil { + return nil, nil, err + } + const oidCount = 50 oidsExist = make([]TestObject, 0, oidCount) oidsMissing = make([]TestObject, 0, oidCount) @@ -161,7 +166,7 @@ func buildTestData() (oidsExist, oidsMissing []TestObject, err error) { outputs := repo.AddCommits([]*test.CommitInput{&commit}) // now upload - manifest := lfs.TransferManifest(config.Config) + manifest := tq.NewManifestWithClient(apiClient, "upload", cfg.CurrentRemote) uploadQueue := tq.NewTransferQueue(tq.Upload, manifest, tq.WithProgress(meter)) for _, f := range outputs[0].Files { oidsExist = append(oidsExist, TestObject{Oid: f.Oid, Size: f.Size}) diff --git a/tq/manifest.go b/tq/manifest.go index 173134d8..6f39630f 100644 --- a/tq/manifest.go +++ b/tq/manifest.go @@ -3,6 +3,7 @@ package tq import ( "sync" + "github.com/git-lfs/git-lfs/lfsapi" "github.com/rubyist/tracerx" ) @@ -32,7 +33,18 @@ func (m *Manifest) ConcurrentTransfers() int { } func NewManifest() *Manifest { - return NewManifestWithGitEnv("", nil) + cli, err := lfsapi.NewClient(nil, nil) + if err != nil { + tracerx.Printf("unable to init tq.Manifest: %s", err) + return nil + } + + return NewManifestWithClient(cli, "", "") +} + +func NewManifestWithClient(apiClient *lfsapi.Client, operation, remote string) *Manifest { + e := apiClient.Endpoints.Endpoint(operation, remote) + return NewManifestWithGitEnv(string(apiClient.Endpoints.AccessFor(e.Url)), apiClient.GitEnv()) } func NewManifestWithGitEnv(access string, git Env) *Manifest { From 3719fece121c1939129abd834d92ba214adec4fe Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Tue, 3 Jan 2017 16:01:45 -0700 Subject: [PATCH 04/24] lfs: remove TransferManifest() --- commands/commands.go | 8 ++++++- lfs/lfs.go | 5 ---- lfs/manifest_test.go | 56 +++++++++++++++++++++++--------------------- 3 files changed, 36 insertions(+), 33 deletions(-) diff --git a/commands/commands.go b/commands/commands.go index 54421e6a..3a6ed310 100644 --- a/commands/commands.go +++ b/commands/commands.go @@ -41,7 +41,13 @@ var ( // TransferManifest builds a tq.Manifest from the commands package global // cfg var. func TransferManifest() *tq.Manifest { - return lfs.TransferManifest(cfg) + return buildTransferManifest("download", cfg.CurrentRemote) +} + +// buildTransferManifest builds a tq.Manifest from the global os and git +// environments. +func buildTransferManifest(operation, remote string) *tq.Manifest { + return tq.NewManifestWithClient(newAPIClient(), operation, remote) } func newAPIClient() *lfsapi.Client { diff --git a/lfs/lfs.go b/lfs/lfs.go index 8c3c421a..ee11c0ef 100644 --- a/lfs/lfs.go +++ b/lfs/lfs.go @@ -118,11 +118,6 @@ func Environ(cfg *config.Configuration, manifest *tq.Manifest) []string { return env } -// TransferManifest builds a tq.Manifest using the given cfg. -func TransferManifest(cfg *config.Configuration) *tq.Manifest { - return tq.NewManifestWithGitEnv(string(cfg.Access("download")), cfg.Git) -} - func InRepo() bool { return config.LocalGitDir != "" } diff --git a/lfs/manifest_test.go b/lfs/manifest_test.go index 2372c08d..7a60dd47 100644 --- a/lfs/manifest_test.go +++ b/lfs/manifest_test.go @@ -3,48 +3,50 @@ package lfs import ( "testing" - "github.com/git-lfs/git-lfs/config" + "github.com/git-lfs/git-lfs/lfsapi" + "github.com/git-lfs/git-lfs/tq" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestManifestIsConfigurable(t *testing.T) { - cfg := config.NewFrom(config.Values{ - Git: map[string]string{ - "lfs.transfer.maxretries": "3", - }, - }) - m := TransferManifest(cfg) + cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.transfer.maxretries": "3", + })) + require.Nil(t, err) + + m := tq.NewManifestWithClient(cli, "", "") assert.Equal(t, 3, m.MaxRetries()) } func TestManifestChecksNTLM(t *testing.T) { - cfg := config.NewFrom(config.Values{ - Git: map[string]string{ - "lfs.url": "http://foo", - "lfs.http://foo.access": "ntlm", - "lfs.concurrenttransfers": "3", - }, - }) - m := TransferManifest(cfg) + cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.url": "http://foo", + "lfs.http://foo.access": "ntlm", + "lfs.concurrenttransfers": "3", + })) + require.Nil(t, err) + + m := tq.NewManifestWithClient(cli, "", "") assert.Equal(t, 1, m.MaxRetries()) } func TestManifestClampsValidValues(t *testing.T) { - cfg := config.NewFrom(config.Values{ - Git: map[string]string{ - "lfs.transfer.maxretries": "-1", - }, - }) - m := TransferManifest(cfg) + cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.transfer.maxretries": "-1", + })) + require.Nil(t, err) + + m := tq.NewManifestWithClient(cli, "", "") assert.Equal(t, 1, m.MaxRetries()) } func TestManifestIgnoresNonInts(t *testing.T) { - cfg := config.NewFrom(config.Values{ - Git: map[string]string{ - "lfs.transfer.maxretries": "not_an_int", - }, - }) - m := TransferManifest(cfg) + cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.transfer.maxretries": "not_an_int", + })) + require.Nil(t, err) + + m := tq.NewManifestWithClient(cli, "", "") assert.Equal(t, 1, m.MaxRetries()) } From 7302143a01bf431aacb394c4fe36ffa92cfb150b Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Tue, 3 Jan 2017 16:07:57 -0700 Subject: [PATCH 05/24] commands: remove magical TransferManifest() in favor of explicit buildTransferManifest() --- commands/command_env.go | 2 +- commands/command_fetch.go | 3 ++- commands/command_prune.go | 5 ++--- commands/command_smudge.go | 3 ++- commands/commands.go | 14 +++++++------- commands/pull.go | 2 +- commands/uploader.go | 2 +- 7 files changed, 16 insertions(+), 15 deletions(-) diff --git a/commands/command_env.go b/commands/command_env.go index 4cb30f6c..52f495d6 100644 --- a/commands/command_env.go +++ b/commands/command_env.go @@ -35,7 +35,7 @@ func envCommand(cmd *cobra.Command, args []string) { } } - for _, env := range lfs.Environ(cfg, TransferManifest()) { + for _, env := range lfs.Environ(cfg, defaultTransferManifest()) { Print(env) } diff --git a/commands/command_fetch.go b/commands/command_fetch.go index 98edba52..82a0c885 100644 --- a/commands/command_fetch.go +++ b/commands/command_fetch.go @@ -279,7 +279,8 @@ func fetchAndReportToChan(allpointers []*lfs.WrappedPointer, filter *filepathfil } ready, pointers, meter := readyAndMissingPointers(allpointers, filter) - q := newDownloadQueue(TransferManifest(), tq.WithProgress(meter)) + manifest := buildTransferManifest("download", cfg.CurrentRemote) + q := newDownloadQueue(manifest, tq.WithProgress(meter)) if out != nil { // If we already have it, or it won't be fetched diff --git a/commands/command_prune.go b/commands/command_prune.go index a8c0d371..f0c61dee 100644 --- a/commands/command_prune.go +++ b/commands/command_prune.go @@ -120,9 +120,8 @@ func prune(fetchPruneConfig config.FetchPruneConfig, verifyRemote, dryRun, verbo var verifywait sync.WaitGroup if verifyRemote { - cfg.CurrentRemote = fetchPruneConfig.PruneRemoteName - // build queue now, no estimates or progress output - verifyQueue = newDownloadCheckQueue(TransferManifest()) + manifest := buildTransferManifest("download", fetchPruneConfig.PruneRemoteName) + verifyQueue = newDownloadCheckQueue(manifest) verifiedObjects = tools.NewStringSetWithCapacity(len(localObjects) / 2) // this channel is filled with oids for which Check() succeeded & Transfer() was called diff --git a/commands/command_smudge.go b/commands/command_smudge.go index de10bc94..74d5adfc 100644 --- a/commands/command_smudge.go +++ b/commands/command_smudge.go @@ -40,7 +40,8 @@ func smudge(to io.Writer, ptr *lfs.Pointer, filename string, skip bool, filter * download = filter.Allows(filename) } - err = ptr.Smudge(to, filename, download, TransferManifest(), cb) + manifest := buildTransferManifest("download", cfg.CurrentRemote) + err = ptr.Smudge(to, filename, download, manifest, cb) if file != nil { file.Close() } diff --git a/commands/commands.go b/commands/commands.go index 3a6ed310..8f9d45af 100644 --- a/commands/commands.go +++ b/commands/commands.go @@ -38,18 +38,18 @@ var ( excludeArg string ) -// TransferManifest builds a tq.Manifest from the commands package global -// cfg var. -func TransferManifest() *tq.Manifest { - return buildTransferManifest("download", cfg.CurrentRemote) -} - // buildTransferManifest builds a tq.Manifest from the global os and git // environments. func buildTransferManifest(operation, remote string) *tq.Manifest { return tq.NewManifestWithClient(newAPIClient(), operation, remote) } +// defaultTransferManifest builds a tq.Manifest from the commands package global +// cfg var. +func defaultTransferManifest() *tq.Manifest { + return buildTransferManifest("download", cfg.CurrentRemote) +} + func newAPIClient() *lfsapi.Client { c, err := lfsapi.NewClient(cfg.Os, cfg.Git) if err != nil { @@ -352,7 +352,7 @@ func logPanicToWriter(w io.Writer, loggedError error) { fmt.Fprintln(w, "\nENV:") // log the environment - for _, env := range lfs.Environ(cfg, TransferManifest()) { + for _, env := range lfs.Environ(cfg, defaultTransferManifest()) { fmt.Fprintln(w, env) } } diff --git a/commands/pull.go b/commands/pull.go index ce764982..6590e602 100644 --- a/commands/pull.go +++ b/commands/pull.go @@ -26,7 +26,7 @@ func newSingleCheckout() *singleCheckout { return &singleCheckout{ gitIndexer: &gitIndexer{}, pathConverter: pathConverter, - manifest: TransferManifest(), + manifest: buildTransferManifest("download", cfg.CurrentRemote), } } diff --git a/commands/uploader.go b/commands/uploader.go index 609e0d70..54c93f03 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -21,7 +21,7 @@ func newUploadContext(dryRun bool) *uploadContext { return &uploadContext{ DryRun: dryRun, uploadedOids: tools.NewStringSet(), - manifest: TransferManifest(), + manifest: buildTransferManifest("upload", cfg.CurrentRemote), } } From 29ab015945551b6c10dd4293ecd0fe5144e6cfc1 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 08:56:09 -0700 Subject: [PATCH 06/24] commands: rely on cfg.CurrentRemote less --- commands/command_checkout.go | 2 +- commands/command_clone.go | 8 +++++--- commands/command_pre_push.go | 5 ++--- commands/command_pull.go | 12 +++++++----- commands/command_push.go | 7 +++---- commands/pull.go | 4 ++-- commands/uploader.go | 13 ++++++++----- 7 files changed, 28 insertions(+), 23 deletions(-) diff --git a/commands/command_checkout.go b/commands/command_checkout.go index 9b9a31bb..26170288 100644 --- a/commands/command_checkout.go +++ b/commands/command_checkout.go @@ -17,7 +17,7 @@ func checkoutCommand(cmd *cobra.Command, args []string) { var totalBytes int64 meter := progress.NewMeter(progress.WithOSEnv(cfg.Os)) - singleCheckout := newSingleCheckout() + singleCheckout := newSingleCheckout("") chgitscanner := lfs.NewGitScanner(func(p *lfs.WrappedPointer, err error) { if err != nil { LoggedError(err, "Scanner error") diff --git a/commands/command_clone.go b/commands/command_clone.go index cc5b7dd3..72a9664d 100644 --- a/commands/command_clone.go +++ b/commands/command_clone.go @@ -62,19 +62,21 @@ func cloneCommand(cmd *cobra.Command, args []string) { // Now just call pull with default args // Support --origin option to clone + var remote string if len(cloneFlags.Origin) > 0 { - cfg.CurrentRemote = cloneFlags.Origin + remote = cloneFlags.Origin } else { - cfg.CurrentRemote = "origin" + remote = "origin" } includeArg, excludeArg := getIncludeExcludeArgs(cmd) filter := buildFilepathFilter(cfg, includeArg, excludeArg) if cloneFlags.NoCheckout || cloneFlags.Bare { // If --no-checkout or --bare then we shouldn't check out, just fetch instead + cfg.CurrentRemote = remote fetchRef("HEAD", filter) } else { - pull(filter) + pull(remote, filter) err := postCloneSubmodules(args) if err != nil { Exit("Error performing 'git lfs pull' for submodules: %v", err) diff --git a/commands/command_pre_push.go b/commands/command_pre_push.go index 69a06b7a..7aad50a4 100644 --- a/commands/command_pre_push.go +++ b/commands/command_pre_push.go @@ -52,11 +52,10 @@ func prePushCommand(cmd *cobra.Command, args []string) { Exit("Invalid remote name %q", args[0]) } - cfg.CurrentRemote = args[0] - ctx := newUploadContext(prePushDryRun) + ctx := newUploadContext(args[0], prePushDryRun) gitscanner := lfs.NewGitScanner(nil) - if err := gitscanner.RemoteForPush(cfg.CurrentRemote); err != nil { + if err := gitscanner.RemoteForPush(ctx.Remote); err != nil { ExitWithError(err) } diff --git a/commands/command_pull.go b/commands/command_pull.go index 8effc07b..0506e588 100644 --- a/commands/command_pull.go +++ b/commands/command_pull.go @@ -18,27 +18,29 @@ func pullCommand(cmd *cobra.Command, args []string) { requireGitVersion() requireInRepo() + var remote string if len(args) > 0 { // Remote is first arg if err := git.ValidateRemote(args[0]); err != nil { Panic(err, fmt.Sprintf("Invalid remote name '%v'", args[0])) } - cfg.CurrentRemote = args[0] + remote = args[0] } else { // Actively find the default remote, don't just assume origin defaultRemote, err := git.DefaultRemote() if err != nil { Panic(err, "No default remote") } - cfg.CurrentRemote = defaultRemote + remote = defaultRemote } includeArg, excludeArg := getIncludeExcludeArgs(cmd) filter := buildFilepathFilter(cfg, includeArg, excludeArg) - pull(filter) + pull(remote, filter) } -func pull(filter *filepathfilter.Filter) { +func pull(remote string, filter *filepathfilter.Filter) { + cfg.CurrentRemote = remote ref, err := git.CurrentRef() if err != nil { Panic(err, "Could not pull") @@ -46,7 +48,7 @@ func pull(filter *filepathfilter.Filter) { pointers := newPointerMap() meter := progress.NewMeter(progress.WithOSEnv(cfg.Os)) - singleCheckout := newSingleCheckout() + singleCheckout := newSingleCheckout(remote) q := newDownloadQueue(singleCheckout.manifest, tq.WithProgress(meter)) gitscanner := lfs.NewGitScanner(func(p *lfs.WrappedPointer, err error) { if err != nil { diff --git a/commands/command_push.go b/commands/command_push.go index 9b51ed24..16e7d522 100644 --- a/commands/command_push.go +++ b/commands/command_push.go @@ -20,10 +20,10 @@ var ( ) func uploadsBetweenRefAndRemote(ctx *uploadContext, refnames []string) { - tracerx.Printf("Upload refs %v to remote %v", refnames, cfg.CurrentRemote) + tracerx.Printf("Upload refs %v to remote %v", refnames, ctx.Remote) gitscanner := lfs.NewGitScanner(nil) - if err := gitscanner.RemoteForPush(cfg.CurrentRemote); err != nil { + if err := gitscanner.RemoteForPush(ctx.Remote); err != nil { ExitWithError(err) } defer gitscanner.Close() @@ -128,8 +128,7 @@ func pushCommand(cmd *cobra.Command, args []string) { Exit("Invalid remote name %q", args[0]) } - cfg.CurrentRemote = args[0] - ctx := newUploadContext(pushDryRun) + ctx := newUploadContext(args[0], pushDryRun) if pushObjectIDs { if len(args) < 2 { diff --git a/commands/pull.go b/commands/pull.go index 6590e602..9c545691 100644 --- a/commands/pull.go +++ b/commands/pull.go @@ -15,7 +15,7 @@ import ( // Handles the process of checking out a single file, and updating the git // index. -func newSingleCheckout() *singleCheckout { +func newSingleCheckout(remote string) *singleCheckout { // Get a converter from repo-relative to cwd-relative // Since writing data & calling git update-index must be relative to cwd pathConverter, err := lfs.NewRepoToCurrentPathConverter() @@ -26,7 +26,7 @@ func newSingleCheckout() *singleCheckout { return &singleCheckout{ gitIndexer: &gitIndexer{}, pathConverter: pathConverter, - manifest: buildTransferManifest("download", cfg.CurrentRemote), + manifest: buildTransferManifest("download", remote), } } diff --git a/commands/uploader.go b/commands/uploader.go index 54c93f03..947d9649 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -12,16 +12,19 @@ import ( var uploadMissingErr = "%s does not exist in .git/lfs/objects. Tried %s, which matches %s." type uploadContext struct { + Remote string DryRun bool - manifest *tq.Manifest + Manifest *tq.Manifest uploadedOids tools.StringSet } -func newUploadContext(dryRun bool) *uploadContext { +func newUploadContext(remote string, dryRun bool) *uploadContext { + cfg.CurrentRemote = remote return &uploadContext{ + Remote: remote, + Manifest: buildTransferManifest("upload", remote), DryRun: dryRun, uploadedOids: tools.NewStringSet(), - manifest: buildTransferManifest("upload", cfg.CurrentRemote), } } @@ -77,7 +80,7 @@ func (c *uploadContext) prepareUpload(unfiltered []*lfs.WrappedPointer) (*tq.Tra // build the TransferQueue, automatically skipping any missing objects that // the server already has. - uploadQueue := newUploadQueue(c.manifest, tq.WithProgress(meter), tq.DryRun(c.DryRun)) + uploadQueue := newUploadQueue(c.Manifest, tq.WithProgress(meter), tq.DryRun(c.DryRun)) for _, p := range missingLocalObjects { if c.HasUploaded(p.Oid) { // if the server already has this object, call Skip() on @@ -101,7 +104,7 @@ func (c *uploadContext) checkMissing(missing []*lfs.WrappedPointer, missingSize return } - checkQueue := newDownloadCheckQueue(c.manifest) + checkQueue := newDownloadCheckQueue(c.Manifest) transferCh := checkQueue.Watch() done := make(chan int) From 0cdad9b658584cf96a1c80d550a480e4a6a75332 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 09:05:29 -0700 Subject: [PATCH 07/24] tq: remove tq.NewManifestWithGitEnv --- tq/custom_test.go | 63 ++++++++++++++++++++++----------------------- tq/manifest.go | 10 +++---- tq/transfer_test.go | 14 +++++----- 3 files changed, 42 insertions(+), 45 deletions(-) diff --git a/tq/custom_test.go b/tq/custom_test.go index d17adf68..9b35b407 100644 --- a/tq/custom_test.go +++ b/tq/custom_test.go @@ -3,17 +3,19 @@ package tq import ( "testing" - "github.com/git-lfs/git-lfs/config" + "github.com/git-lfs/git-lfs/lfsapi" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestCustomTransferBasicConfig(t *testing.T) { path := "/path/to/binary" - cfg := config.NewFrom(config.Values{ - Git: map[string]string{"lfs.customtransfer.testsimple.path": path}, - }) + cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.customtransfer.testsimple.path": path, + })) + require.Nil(t, err) - m := NewManifestWithGitEnv("", cfg.Git) + m := NewManifestWithClient(cli, "", "") u := m.NewUploadAdapter("testsimple") assert.NotNil(t, u, "Upload adapter should be present") cu, _ := u.(*customAdapter) @@ -34,16 +36,15 @@ func TestCustomTransferBasicConfig(t *testing.T) { func TestCustomTransferDownloadConfig(t *testing.T) { path := "/path/to/binary" args := "-c 1 --whatever" - cfg := config.NewFrom(config.Values{ - Git: map[string]string{ - "lfs.customtransfer.testdownload.path": path, - "lfs.customtransfer.testdownload.args": args, - "lfs.customtransfer.testdownload.concurrent": "false", - "lfs.customtransfer.testdownload.direction": "download", - }, - }) + cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.customtransfer.testdownload.path": path, + "lfs.customtransfer.testdownload.args": args, + "lfs.customtransfer.testdownload.concurrent": "false", + "lfs.customtransfer.testdownload.direction": "download", + })) + require.Nil(t, err) - m := NewManifestWithGitEnv("", cfg.Git) + m := NewManifestWithClient(cli, "", "") u := m.NewUploadAdapter("testdownload") assert.NotNil(t, u, "Upload adapter should always be created") cu, _ := u.(*customAdapter) @@ -61,16 +62,15 @@ func TestCustomTransferDownloadConfig(t *testing.T) { func TestCustomTransferUploadConfig(t *testing.T) { path := "/path/to/binary" args := "-c 1 --whatever" - cfg := config.NewFrom(config.Values{ - Git: map[string]string{ - "lfs.customtransfer.testupload.path": path, - "lfs.customtransfer.testupload.args": args, - "lfs.customtransfer.testupload.concurrent": "false", - "lfs.customtransfer.testupload.direction": "upload", - }, - }) + cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.customtransfer.testupload.path": path, + "lfs.customtransfer.testupload.args": args, + "lfs.customtransfer.testupload.concurrent": "false", + "lfs.customtransfer.testupload.direction": "upload", + })) + require.Nil(t, err) - m := NewManifestWithGitEnv("", cfg.Git) + m := NewManifestWithClient(cli, "", "") d := m.NewDownloadAdapter("testupload") assert.NotNil(t, d, "Download adapter should always be created") cd, _ := d.(*customAdapter) @@ -88,16 +88,15 @@ func TestCustomTransferUploadConfig(t *testing.T) { func TestCustomTransferBothConfig(t *testing.T) { path := "/path/to/binary" args := "-c 1 --whatever --yeah" - cfg := config.NewFrom(config.Values{ - Git: map[string]string{ - "lfs.customtransfer.testboth.path": path, - "lfs.customtransfer.testboth.args": args, - "lfs.customtransfer.testboth.concurrent": "yes", - "lfs.customtransfer.testboth.direction": "both", - }, - }) + cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.customtransfer.testboth.path": path, + "lfs.customtransfer.testboth.args": args, + "lfs.customtransfer.testboth.concurrent": "yes", + "lfs.customtransfer.testboth.direction": "both", + })) + require.Nil(t, err) - m := NewManifestWithGitEnv("", cfg.Git) + m := NewManifestWithClient(cli, "", "") d := m.NewDownloadAdapter("testboth") assert.NotNil(t, d, "Download adapter should be present") cd, _ := d.(*customAdapter) diff --git a/tq/manifest.go b/tq/manifest.go index 6f39630f..443bc016 100644 --- a/tq/manifest.go +++ b/tq/manifest.go @@ -43,18 +43,13 @@ func NewManifest() *Manifest { } func NewManifestWithClient(apiClient *lfsapi.Client, operation, remote string) *Manifest { - e := apiClient.Endpoints.Endpoint(operation, remote) - return NewManifestWithGitEnv(string(apiClient.Endpoints.AccessFor(e.Url)), apiClient.GitEnv()) -} - -func NewManifestWithGitEnv(access string, git Env) *Manifest { m := &Manifest{ downloadAdapterFuncs: make(map[string]NewAdapterFunc), uploadAdapterFuncs: make(map[string]NewAdapterFunc), } var tusAllowed bool - if git != nil { + if git := apiClient.GitEnv(); git != nil { if v := git.Int("lfs.transfer.maxretries", 0); v > 0 { m.maxRetries = v } @@ -70,7 +65,8 @@ func NewManifestWithGitEnv(access string, git Env) *Manifest { m.maxRetries = defaultMaxRetries } - if access == "ntlm" { + e := apiClient.Endpoints.Endpoint(operation, remote) + if apiClient.Endpoints.AccessFor(e.Url) == lfsapi.NTLMAccess { m.concurrentTransfers = 1 } else if m.concurrentTransfers < 1 { m.concurrentTransfers = defaultConcurrentTransfers diff --git a/tq/transfer_test.go b/tq/transfer_test.go index 96640146..764b0016 100644 --- a/tq/transfer_test.go +++ b/tq/transfer_test.go @@ -3,9 +3,9 @@ package tq import ( "testing" - "github.com/git-lfs/git-lfs/config" - + "github.com/git-lfs/git-lfs/lfsapi" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) type testAdapter struct { @@ -118,10 +118,12 @@ func testAdapterRegAndOverride(t *testing.T) { } func testAdapterRegButBasicOnly(t *testing.T) { - cfg := config.NewFrom(config.Values{ - Git: map[string]string{"lfs.basictransfersonly": "yes"}, - }) - m := NewManifestWithGitEnv("", cfg.Git) + cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.basictransfersonly": "yes", + })) + require.Nil(t, err) + + m := NewManifestWithClient(cli, "", "") assert := assert.New(t) From f52de2fd89061316f9f40fccf001e1a21d3d7a9e Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 09:33:40 -0700 Subject: [PATCH 08/24] tq: implement batch api using lfsapi.Client --- locking/api.go | 4 +- tq/api.go | 47 ++++++++++++++++++++ tq/api_test.go | 118 +++++++++++++++++++++++++++++++++++++++++++++++++ tq/manifest.go | 6 +++ 4 files changed, 173 insertions(+), 2 deletions(-) create mode 100644 tq/api.go create mode 100644 tq/api_test.go diff --git a/locking/api.go b/locking/api.go index 4acea890..20dbde2c 100644 --- a/locking/api.go +++ b/locking/api.go @@ -62,14 +62,14 @@ func (c *lockClient) Lock(remote string, lockReq *lockRequest) (*lockResponse, * } lockRes := &lockResponse{} - err = lfsapi.DecodeJSON(res, lockRes) - return lockRes, res, err + return lockRes, res, lfsapi.DecodeJSON(res, lockRes) } // UnlockRequest encapsulates the data sent in an API request to remove a lock. type unlockRequest struct { // Id is the Id of the lock that the user wishes to unlock. Id string `json:"id"` + // Force determines whether or not the lock should be "forcibly" // unlocked; that is to say whether or not a given individual should be // able to break a different individual's lock. diff --git a/tq/api.go b/tq/api.go new file mode 100644 index 00000000..636da1a7 --- /dev/null +++ b/tq/api.go @@ -0,0 +1,47 @@ +package tq + +import ( + "net/http" + + "github.com/git-lfs/git-lfs/api" + "github.com/git-lfs/git-lfs/lfsapi" +) + +type tqClient struct { + *lfsapi.Client +} + +type batchRequest struct { + Operation string `json:"operation"` + Objects []*api.ObjectResource `json:"objects"` + TransferAdapterNames []string `json:"transfers,omitempty"` +} + +type batchResponse struct { + TransferAdapterName string `json:"transfer"` + Objects []*api.ObjectResource `json:"objects"` +} + +func (c *tqClient) Batch(remote string, bReq *batchRequest) (*batchResponse, *http.Response, error) { + bRes := &batchResponse{} + if len(bReq.Objects) == 0 { + return bRes, nil, nil + } + + if len(bReq.TransferAdapterNames) == 1 && bReq.TransferAdapterNames[0] == "basic" { + bReq.TransferAdapterNames = nil + } + + e := c.Endpoints.Endpoint(bReq.Operation, remote) + req, err := c.NewRequest("POST", e, "objects/batch", bReq) + if err != nil { + return nil, nil, err + } + + res, err := c.DoWithAuth(remote, req) + if err != nil { + return nil, nil, err + } + + return bRes, res, lfsapi.DecodeJSON(res, bRes) +} diff --git a/tq/api_test.go b/tq/api_test.go new file mode 100644 index 00000000..bd6b5272 --- /dev/null +++ b/tq/api_test.go @@ -0,0 +1,118 @@ +package tq + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/git-lfs/git-lfs/api" + "github.com/git-lfs/git-lfs/lfsapi" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestAPIBatch(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/objects/batch" { + w.WriteHeader(404) + return + } + + assert.Equal(t, "POST", r.Method) + + bReq := &batchRequest{} + err := json.NewDecoder(r.Body).Decode(bReq) + r.Body.Close() + assert.Nil(t, err) + assert.EqualValues(t, []string{"basic", "whatev"}, bReq.TransferAdapterNames) + if assert.Equal(t, 1, len(bReq.Objects)) { + assert.Equal(t, "a", bReq.Objects[0].Oid) + } + + w.Header().Set("Content-Type", "application/json") + err = json.NewEncoder(w).Encode(&batchResponse{ + TransferAdapterName: "basic", + Objects: bReq.Objects, + }) + })) + defer srv.Close() + + c, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.url": srv.URL + "/api", + })) + require.Nil(t, err) + + tqc := &tqClient{Client: c} + bReq := &batchRequest{ + TransferAdapterNames: []string{"basic", "whatev"}, + Objects: []*api.ObjectResource{ + &api.ObjectResource{Oid: "a", Size: 1}, + }, + } + bRes, res, err := tqc.Batch("remote", bReq) + require.Nil(t, err) + assert.Equal(t, 200, res.StatusCode) + assert.Equal(t, "basic", bRes.TransferAdapterName) + if assert.Equal(t, 1, len(bRes.Objects)) { + assert.Equal(t, "a", bRes.Objects[0].Oid) + } +} + +func TestAPIBatchOnlyBasic(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/objects/batch" { + w.WriteHeader(404) + return + } + + assert.Equal(t, "POST", r.Method) + + bReq := &batchRequest{} + err := json.NewDecoder(r.Body).Decode(bReq) + r.Body.Close() + assert.Nil(t, err) + assert.Equal(t, 0, len(bReq.TransferAdapterNames)) + if assert.Equal(t, 1, len(bReq.Objects)) { + assert.Equal(t, "a", bReq.Objects[0].Oid) + } + + w.Header().Set("Content-Type", "application/json") + err = json.NewEncoder(w).Encode(&batchResponse{ + TransferAdapterName: "basic", + }) + })) + defer srv.Close() + + c, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + "lfs.url": srv.URL + "/api", + })) + require.Nil(t, err) + + tqc := &tqClient{Client: c} + bReq := &batchRequest{ + TransferAdapterNames: []string{"basic"}, + Objects: []*api.ObjectResource{ + &api.ObjectResource{Oid: "a", Size: 1}, + }, + } + bRes, res, err := tqc.Batch("remote", bReq) + require.Nil(t, err) + assert.Equal(t, 200, res.StatusCode) + assert.Equal(t, "basic", bRes.TransferAdapterName) +} + +func TestAPIBatchEmptyObjects(t *testing.T) { + c, err := lfsapi.NewClient(nil, nil) + require.Nil(t, err) + + tqc := &tqClient{Client: c} + bReq := &batchRequest{ + TransferAdapterNames: []string{"basic", "whatev"}, + } + bRes, res, err := tqc.Batch("remote", bReq) + require.Nil(t, err) + assert.Nil(t, res) + assert.Equal(t, "", bRes.TransferAdapterName) + assert.Equal(t, 0, len(bRes.Objects)) +} diff --git a/tq/manifest.go b/tq/manifest.go index 443bc016..8fca4177 100644 --- a/tq/manifest.go +++ b/tq/manifest.go @@ -21,9 +21,14 @@ type Manifest struct { tusTransfersAllowed bool downloadAdapterFuncs map[string]NewAdapterFunc uploadAdapterFuncs map[string]NewAdapterFunc + apiClient *lfsapi.Client mu sync.Mutex } +func (m *Manifest) APIClient() *lfsapi.Client { + return m.apiClient +} + func (m *Manifest) MaxRetries() int { return m.maxRetries } @@ -44,6 +49,7 @@ func NewManifest() *Manifest { func NewManifestWithClient(apiClient *lfsapi.Client, operation, remote string) *Manifest { m := &Manifest{ + apiClient: apiClient, downloadAdapterFuncs: make(map[string]NewAdapterFunc), uploadAdapterFuncs: make(map[string]NewAdapterFunc), } From faeb7f00cb371ebbdacd3cd4161e55670e89457f Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 10:11:16 -0700 Subject: [PATCH 09/24] tq: use lfsapi.Client to make batch api requests --- test/test-batch-transfer.sh | 1 - tq/api.go | 18 ++++++++++++++++-- tq/manifest.go | 2 ++ tq/transfer_queue.go | 29 ++++++++++++++++++----------- 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/test/test-batch-transfer.sh b/test/test-batch-transfer.sh index 52d1455a..1046e5b6 100755 --- a/test/test-batch-transfer.sh +++ b/test/test-batch-transfer.sh @@ -100,4 +100,3 @@ begin_test "batch transfers occur in reverse order by size" [ "$pos_large" -lt "$pos_small" ] ) end_test - diff --git a/tq/api.go b/tq/api.go index 636da1a7..92dc3cec 100644 --- a/tq/api.go +++ b/tq/api.go @@ -2,9 +2,12 @@ package tq import ( "net/http" + "strings" "github.com/git-lfs/git-lfs/api" + "github.com/git-lfs/git-lfs/errors" "github.com/git-lfs/git-lfs/lfsapi" + "github.com/rubyist/tracerx" ) type tqClient struct { @@ -35,12 +38,23 @@ func (c *tqClient) Batch(remote string, bReq *batchRequest) (*batchResponse, *ht e := c.Endpoints.Endpoint(bReq.Operation, remote) req, err := c.NewRequest("POST", e, "objects/batch", bReq) if err != nil { - return nil, nil, err + return nil, nil, errors.Wrap(err, "batch request") } + tracerx.Printf("api: batch %d files", len(bReq.Objects)) + res, err := c.DoWithAuth(remote, req) if err != nil { - return nil, nil, err + tracerx.Printf("api error: %s", err) + return nil, nil, errors.Wrap(err, "batch response") + } + c.LogResponse("lfs.batch", res) + + if res.StatusCode != 200 { + return nil, res, errors.Errorf("Invalid status for %s %s: %d", + req.Method, + strings.SplitN(req.URL.String(), "?", 2)[0], + res.StatusCode) } return bRes, res, lfsapi.DecodeJSON(res, bRes) diff --git a/tq/manifest.go b/tq/manifest.go index 8fca4177..2b2544f1 100644 --- a/tq/manifest.go +++ b/tq/manifest.go @@ -19,6 +19,7 @@ type Manifest struct { concurrentTransfers int basicTransfersOnly bool tusTransfersAllowed bool + remote string downloadAdapterFuncs map[string]NewAdapterFunc uploadAdapterFuncs map[string]NewAdapterFunc apiClient *lfsapi.Client @@ -50,6 +51,7 @@ func NewManifest() *Manifest { func NewManifestWithClient(apiClient *lfsapi.Client, operation, remote string) *Manifest { m := &Manifest{ apiClient: apiClient, + remote: remote, downloadAdapterFuncs: make(map[string]NewAdapterFunc), uploadAdapterFuncs: make(map[string]NewAdapterFunc), } diff --git a/tq/transfer_queue.go b/tq/transfer_queue.go index febd9f38..a9567d87 100644 --- a/tq/transfer_queue.go +++ b/tq/transfer_queue.go @@ -5,7 +5,6 @@ import ( "sync" "github.com/git-lfs/git-lfs/api" - "github.com/git-lfs/git-lfs/config" "github.com/git-lfs/git-lfs/errors" "github.com/git-lfs/git-lfs/progress" "github.com/rubyist/tracerx" @@ -94,6 +93,8 @@ func (b batch) Swap(i, j int) { b[i], b[j] = b[j], b[i] } // adapters, and dealing with progress, errors and retries. type TransferQueue struct { direction Direction + client *tqClient + remote string adapter Adapter adapterInProgress bool adapterInitMutex sync.Mutex @@ -150,6 +151,8 @@ func WithBufferDepth(depth int) Option { func NewTransferQueue(dir Direction, manifest *Manifest, options ...Option) *TransferQueue { q := &TransferQueue{ direction: dir, + client: &tqClient{Client: manifest.APIClient()}, + remote: manifest.remote, errorc: make(chan error), transfers: make(map[string]*objectTuple), trMutex: &sync.Mutex{}, @@ -281,16 +284,16 @@ func (q *TransferQueue) collectBatches() { // enqueueAndCollectRetriesFor blocks until the entire Batch "batch" has been // processed. func (q *TransferQueue) enqueueAndCollectRetriesFor(batch batch) (batch, error) { - cfg := config.Config - next := q.makeBatch() - transferAdapterNames := q.manifest.GetAdapterNames(q.direction) - tracerx.Printf("tq: sending batch of size %d", len(batch)) - objs, adapterName, err := api.Batch( - cfg, batch.ApiObjects(), q.transferKind(), transferAdapterNames, - ) + bReq := &batchRequest{ + Operation: q.transferKind(), + Objects: batch.ApiObjects(), + TransferAdapterNames: q.manifest.GetAdapterNames(q.direction), + } + + bRes, _, err := q.client.Batch(q.remote, bReq) if err != nil { // If there was an error making the batch API call, mark all of // the objects for retry, and return them along with the error @@ -309,12 +312,16 @@ func (q *TransferQueue) enqueueAndCollectRetriesFor(batch batch) (batch, error) return next, err } - q.useAdapter(adapterName) + if len(bRes.Objects) == 0 { + return next, nil + } + + q.useAdapter(bRes.TransferAdapterName) q.startProgress.Do(q.meter.Start) - toTransfer := make([]*Transfer, 0, len(objs)) + toTransfer := make([]*Transfer, 0, len(bRes.Objects)) - for _, o := range objs { + for _, o := range bRes.Objects { if o.Error != nil { q.errorc <- errors.Wrapf(o.Error, "[%v] %v", o.Oid, o.Error.Message) q.Skip(o.Size) From d874b99f79ca139e05ce76b1e67ac56026ca3c8e Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 10:20:40 -0700 Subject: [PATCH 10/24] tq: parse response before checking response code for test-batch-error-handling.sh --- tq/api.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tq/api.go b/tq/api.go index 92dc3cec..772c0e49 100644 --- a/tq/api.go +++ b/tq/api.go @@ -50,6 +50,10 @@ func (c *tqClient) Batch(remote string, bReq *batchRequest) (*batchResponse, *ht } c.LogResponse("lfs.batch", res) + if err := lfsapi.DecodeJSON(res, bRes); err != nil { + return bRes, res, errors.Wrap(err, "batch response") + } + if res.StatusCode != 200 { return nil, res, errors.Errorf("Invalid status for %s %s: %d", req.Method, @@ -57,5 +61,5 @@ func (c *tqClient) Batch(remote string, bReq *batchRequest) (*batchResponse, *ht res.StatusCode) } - return bRes, res, lfsapi.DecodeJSON(res, bRes) + return bRes, res, nil } From 2b20d510cc793d7f54da5fa3d3182f21d46d7cdc Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 14:46:30 -0700 Subject: [PATCH 11/24] tq: remove remote/operation/access from tq.Manifest, make remote an arg of NewTransferQueue() --- commands/command_checkout.go | 2 +- commands/command_env.go | 2 +- commands/command_fetch.go | 3 +-- commands/command_prune.go | 3 +-- commands/command_pull.go | 4 ++-- commands/command_smudge.go | 3 +-- commands/commands.go | 24 +++++++++--------------- commands/pull.go | 4 ++-- commands/uploader.go | 6 +++--- lfs/manifest_test.go | 8 ++++---- lfs/pointer_smudge.go | 2 +- test/git-lfs-test-server-api/main.go | 4 ++-- tq/api.go | 5 +++-- tq/custom_test.go | 8 ++++---- tq/manifest.go | 10 +++------- tq/transfer.go | 15 +++++++++++++++ tq/transfer_queue.go | 24 +++++++++++++++++------- tq/transfer_test.go | 2 +- 18 files changed, 71 insertions(+), 58 deletions(-) diff --git a/commands/command_checkout.go b/commands/command_checkout.go index 26170288..9b9a31bb 100644 --- a/commands/command_checkout.go +++ b/commands/command_checkout.go @@ -17,7 +17,7 @@ func checkoutCommand(cmd *cobra.Command, args []string) { var totalBytes int64 meter := progress.NewMeter(progress.WithOSEnv(cfg.Os)) - singleCheckout := newSingleCheckout("") + singleCheckout := newSingleCheckout() chgitscanner := lfs.NewGitScanner(func(p *lfs.WrappedPointer, err error) { if err != nil { LoggedError(err, "Scanner error") diff --git a/commands/command_env.go b/commands/command_env.go index 52f495d6..070a4dc4 100644 --- a/commands/command_env.go +++ b/commands/command_env.go @@ -35,7 +35,7 @@ func envCommand(cmd *cobra.Command, args []string) { } } - for _, env := range lfs.Environ(cfg, defaultTransferManifest()) { + for _, env := range lfs.Environ(cfg, buildTransferManifest()) { Print(env) } diff --git a/commands/command_fetch.go b/commands/command_fetch.go index 82a0c885..580229a9 100644 --- a/commands/command_fetch.go +++ b/commands/command_fetch.go @@ -279,8 +279,7 @@ func fetchAndReportToChan(allpointers []*lfs.WrappedPointer, filter *filepathfil } ready, pointers, meter := readyAndMissingPointers(allpointers, filter) - manifest := buildTransferManifest("download", cfg.CurrentRemote) - q := newDownloadQueue(manifest, tq.WithProgress(meter)) + q := newDownloadQueue(buildTransferManifest(), cfg.CurrentRemote, tq.WithProgress(meter)) if out != nil { // If we already have it, or it won't be fetched diff --git a/commands/command_prune.go b/commands/command_prune.go index f0c61dee..41b8df1a 100644 --- a/commands/command_prune.go +++ b/commands/command_prune.go @@ -120,8 +120,7 @@ func prune(fetchPruneConfig config.FetchPruneConfig, verifyRemote, dryRun, verbo var verifywait sync.WaitGroup if verifyRemote { - manifest := buildTransferManifest("download", fetchPruneConfig.PruneRemoteName) - verifyQueue = newDownloadCheckQueue(manifest) + verifyQueue = newDownloadCheckQueue(buildTransferManifest(), fetchPruneConfig.PruneRemoteName) verifiedObjects = tools.NewStringSetWithCapacity(len(localObjects) / 2) // this channel is filled with oids for which Check() succeeded & Transfer() was called diff --git a/commands/command_pull.go b/commands/command_pull.go index 0506e588..b4a212ea 100644 --- a/commands/command_pull.go +++ b/commands/command_pull.go @@ -48,8 +48,8 @@ func pull(remote string, filter *filepathfilter.Filter) { pointers := newPointerMap() meter := progress.NewMeter(progress.WithOSEnv(cfg.Os)) - singleCheckout := newSingleCheckout(remote) - q := newDownloadQueue(singleCheckout.manifest, tq.WithProgress(meter)) + singleCheckout := newSingleCheckout() + q := newDownloadQueue(singleCheckout.manifest, remote, tq.WithProgress(meter)) gitscanner := lfs.NewGitScanner(func(p *lfs.WrappedPointer, err error) { if err != nil { LoggedError(err, "Scanner error") diff --git a/commands/command_smudge.go b/commands/command_smudge.go index 74d5adfc..d1248ed4 100644 --- a/commands/command_smudge.go +++ b/commands/command_smudge.go @@ -40,8 +40,7 @@ func smudge(to io.Writer, ptr *lfs.Pointer, filename string, skip bool, filter * download = filter.Allows(filename) } - manifest := buildTransferManifest("download", cfg.CurrentRemote) - err = ptr.Smudge(to, filename, download, manifest, cb) + err = ptr.Smudge(to, filename, download, buildTransferManifest(), cb) if file != nil { file.Close() } diff --git a/commands/commands.go b/commands/commands.go index 8f9d45af..bdc1e382 100644 --- a/commands/commands.go +++ b/commands/commands.go @@ -40,14 +40,8 @@ var ( // buildTransferManifest builds a tq.Manifest from the global os and git // environments. -func buildTransferManifest(operation, remote string) *tq.Manifest { - return tq.NewManifestWithClient(newAPIClient(), operation, remote) -} - -// defaultTransferManifest builds a tq.Manifest from the commands package global -// cfg var. -func defaultTransferManifest() *tq.Manifest { - return buildTransferManifest("download", cfg.CurrentRemote) +func buildTransferManifest() *tq.Manifest { + return tq.NewManifestWithClient(newAPIClient()) } func newAPIClient() *lfsapi.Client { @@ -72,21 +66,21 @@ func newLockClient(remote string) *locking.Client { } // newDownloadCheckQueue builds a checking queue, checks that objects are there but doesn't download -func newDownloadCheckQueue(manifest *tq.Manifest, options ...tq.Option) *tq.TransferQueue { +func newDownloadCheckQueue(manifest *tq.Manifest, remote string, options ...tq.Option) *tq.TransferQueue { allOptions := make([]tq.Option, len(options), len(options)+1) allOptions = append(allOptions, options...) allOptions = append(allOptions, tq.DryRun(true)) - return newDownloadQueue(manifest, allOptions...) + return newDownloadQueue(manifest, remote, allOptions...) } // newDownloadQueue builds a DownloadQueue, allowing concurrent downloads. -func newDownloadQueue(manifest *tq.Manifest, options ...tq.Option) *tq.TransferQueue { - return tq.NewTransferQueue(tq.Download, manifest, options...) +func newDownloadQueue(manifest *tq.Manifest, remote string, options ...tq.Option) *tq.TransferQueue { + return tq.NewTransferQueue(tq.Download, manifest, remote, options...) } // newUploadQueue builds an UploadQueue, allowing `workers` concurrent uploads. -func newUploadQueue(manifest *tq.Manifest, options ...tq.Option) *tq.TransferQueue { - return tq.NewTransferQueue(tq.Upload, manifest, options...) +func newUploadQueue(manifest *tq.Manifest, remote string, options ...tq.Option) *tq.TransferQueue { + return tq.NewTransferQueue(tq.Upload, manifest, remote, options...) } func buildFilepathFilter(config *config.Configuration, includeArg, excludeArg *string) *filepathfilter.Filter { @@ -352,7 +346,7 @@ func logPanicToWriter(w io.Writer, loggedError error) { fmt.Fprintln(w, "\nENV:") // log the environment - for _, env := range lfs.Environ(cfg, defaultTransferManifest()) { + for _, env := range lfs.Environ(cfg, buildTransferManifest()) { fmt.Fprintln(w, env) } } diff --git a/commands/pull.go b/commands/pull.go index 9c545691..42ab0ec2 100644 --- a/commands/pull.go +++ b/commands/pull.go @@ -15,7 +15,7 @@ import ( // Handles the process of checking out a single file, and updating the git // index. -func newSingleCheckout(remote string) *singleCheckout { +func newSingleCheckout() *singleCheckout { // Get a converter from repo-relative to cwd-relative // Since writing data & calling git update-index must be relative to cwd pathConverter, err := lfs.NewRepoToCurrentPathConverter() @@ -26,7 +26,7 @@ func newSingleCheckout(remote string) *singleCheckout { return &singleCheckout{ gitIndexer: &gitIndexer{}, pathConverter: pathConverter, - manifest: buildTransferManifest("download", remote), + manifest: buildTransferManifest(), } } diff --git a/commands/uploader.go b/commands/uploader.go index 947d9649..0ac22d51 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -22,7 +22,7 @@ func newUploadContext(remote string, dryRun bool) *uploadContext { cfg.CurrentRemote = remote return &uploadContext{ Remote: remote, - Manifest: buildTransferManifest("upload", remote), + Manifest: buildTransferManifest(), DryRun: dryRun, uploadedOids: tools.NewStringSet(), } @@ -80,7 +80,7 @@ func (c *uploadContext) prepareUpload(unfiltered []*lfs.WrappedPointer) (*tq.Tra // build the TransferQueue, automatically skipping any missing objects that // the server already has. - uploadQueue := newUploadQueue(c.Manifest, tq.WithProgress(meter), tq.DryRun(c.DryRun)) + uploadQueue := newUploadQueue(c.Manifest, c.Remote, tq.WithProgress(meter), tq.DryRun(c.DryRun)) for _, p := range missingLocalObjects { if c.HasUploaded(p.Oid) { // if the server already has this object, call Skip() on @@ -104,7 +104,7 @@ func (c *uploadContext) checkMissing(missing []*lfs.WrappedPointer, missingSize return } - checkQueue := newDownloadCheckQueue(c.Manifest) + checkQueue := newDownloadCheckQueue(c.Manifest, c.Remote) transferCh := checkQueue.Watch() done := make(chan int) diff --git a/lfs/manifest_test.go b/lfs/manifest_test.go index 7a60dd47..ad93dfdb 100644 --- a/lfs/manifest_test.go +++ b/lfs/manifest_test.go @@ -15,7 +15,7 @@ func TestManifestIsConfigurable(t *testing.T) { })) require.Nil(t, err) - m := tq.NewManifestWithClient(cli, "", "") + m := tq.NewManifestWithClient(cli) assert.Equal(t, 3, m.MaxRetries()) } @@ -27,7 +27,7 @@ func TestManifestChecksNTLM(t *testing.T) { })) require.Nil(t, err) - m := tq.NewManifestWithClient(cli, "", "") + m := tq.NewManifestWithClient(cli) assert.Equal(t, 1, m.MaxRetries()) } @@ -37,7 +37,7 @@ func TestManifestClampsValidValues(t *testing.T) { })) require.Nil(t, err) - m := tq.NewManifestWithClient(cli, "", "") + m := tq.NewManifestWithClient(cli) assert.Equal(t, 1, m.MaxRetries()) } @@ -47,6 +47,6 @@ func TestManifestIgnoresNonInts(t *testing.T) { })) require.Nil(t, err) - m := tq.NewManifestWithClient(cli, "", "") + m := tq.NewManifestWithClient(cli) assert.Equal(t, 1, m.MaxRetries()) } diff --git a/lfs/pointer_smudge.go b/lfs/pointer_smudge.go index df556396..1173383d 100644 --- a/lfs/pointer_smudge.go +++ b/lfs/pointer_smudge.go @@ -74,7 +74,7 @@ func PointerSmudge(writer io.Writer, ptr *Pointer, workingfile string, download func downloadFile(writer io.Writer, ptr *Pointer, workingfile, mediafile string, manifest *tq.Manifest, cb progress.CopyCallback) error { fmt.Fprintf(os.Stderr, "Downloading %s (%s)\n", workingfile, pb.FormatBytes(ptr.Size)) - q := tq.NewTransferQueue(tq.Download, manifest) + q := tq.NewTransferQueue(tq.Download, manifest, "") q.Add(filepath.Base(workingfile), mediafile, ptr.Oid, ptr.Size) q.Wait() diff --git a/test/git-lfs-test-server-api/main.go b/test/git-lfs-test-server-api/main.go index d8162d58..b232d73b 100644 --- a/test/git-lfs-test-server-api/main.go +++ b/test/git-lfs-test-server-api/main.go @@ -166,8 +166,8 @@ func buildTestData() (oidsExist, oidsMissing []TestObject, err error) { outputs := repo.AddCommits([]*test.CommitInput{&commit}) // now upload - manifest := tq.NewManifestWithClient(apiClient, "upload", cfg.CurrentRemote) - uploadQueue := tq.NewTransferQueue(tq.Upload, manifest, tq.WithProgress(meter)) + manifest := tq.NewManifestWithClient(apiClient) + uploadQueue := tq.NewTransferQueue(tq.Upload, manifest, "", tq.WithProgress(meter)) for _, f := range outputs[0].Files { oidsExist = append(oidsExist, TestObject{Oid: f.Oid, Size: f.Size}) diff --git a/tq/api.go b/tq/api.go index 772c0e49..c94ab250 100644 --- a/tq/api.go +++ b/tq/api.go @@ -21,6 +21,7 @@ type batchRequest struct { } type batchResponse struct { + Endpoint lfsapi.Endpoint TransferAdapterName string `json:"transfer"` Objects []*api.ObjectResource `json:"objects"` } @@ -35,8 +36,8 @@ func (c *tqClient) Batch(remote string, bReq *batchRequest) (*batchResponse, *ht bReq.TransferAdapterNames = nil } - e := c.Endpoints.Endpoint(bReq.Operation, remote) - req, err := c.NewRequest("POST", e, "objects/batch", bReq) + bRes.Endpoint = c.Endpoints.Endpoint(bReq.Operation, remote) + req, err := c.NewRequest("POST", bRes.Endpoint, "objects/batch", bReq) if err != nil { return nil, nil, errors.Wrap(err, "batch request") } diff --git a/tq/custom_test.go b/tq/custom_test.go index 9b35b407..8c90febf 100644 --- a/tq/custom_test.go +++ b/tq/custom_test.go @@ -15,7 +15,7 @@ func TestCustomTransferBasicConfig(t *testing.T) { })) require.Nil(t, err) - m := NewManifestWithClient(cli, "", "") + m := NewManifestWithClient(cli) u := m.NewUploadAdapter("testsimple") assert.NotNil(t, u, "Upload adapter should be present") cu, _ := u.(*customAdapter) @@ -44,7 +44,7 @@ func TestCustomTransferDownloadConfig(t *testing.T) { })) require.Nil(t, err) - m := NewManifestWithClient(cli, "", "") + m := NewManifestWithClient(cli) u := m.NewUploadAdapter("testdownload") assert.NotNil(t, u, "Upload adapter should always be created") cu, _ := u.(*customAdapter) @@ -70,7 +70,7 @@ func TestCustomTransferUploadConfig(t *testing.T) { })) require.Nil(t, err) - m := NewManifestWithClient(cli, "", "") + m := NewManifestWithClient(cli) d := m.NewDownloadAdapter("testupload") assert.NotNil(t, d, "Download adapter should always be created") cd, _ := d.(*customAdapter) @@ -96,7 +96,7 @@ func TestCustomTransferBothConfig(t *testing.T) { })) require.Nil(t, err) - m := NewManifestWithClient(cli, "", "") + m := NewManifestWithClient(cli) d := m.NewDownloadAdapter("testboth") assert.NotNil(t, d, "Download adapter should be present") cd, _ := d.(*customAdapter) diff --git a/tq/manifest.go b/tq/manifest.go index 2b2544f1..3f99903d 100644 --- a/tq/manifest.go +++ b/tq/manifest.go @@ -45,13 +45,12 @@ func NewManifest() *Manifest { return nil } - return NewManifestWithClient(cli, "", "") + return NewManifestWithClient(cli) } -func NewManifestWithClient(apiClient *lfsapi.Client, operation, remote string) *Manifest { +func NewManifestWithClient(apiClient *lfsapi.Client) *Manifest { m := &Manifest{ apiClient: apiClient, - remote: remote, downloadAdapterFuncs: make(map[string]NewAdapterFunc), uploadAdapterFuncs: make(map[string]NewAdapterFunc), } @@ -73,10 +72,7 @@ func NewManifestWithClient(apiClient *lfsapi.Client, operation, remote string) * m.maxRetries = defaultMaxRetries } - e := apiClient.Endpoints.Endpoint(operation, remote) - if apiClient.Endpoints.AccessFor(e.Url) == lfsapi.NTLMAccess { - m.concurrentTransfers = 1 - } else if m.concurrentTransfers < 1 { + if m.concurrentTransfers < 1 { m.concurrentTransfers = defaultConcurrentTransfers } diff --git a/tq/transfer.go b/tq/transfer.go index 84135f43..d381dbf6 100644 --- a/tq/transfer.go +++ b/tq/transfer.go @@ -8,6 +8,7 @@ import ( "github.com/git-lfs/git-lfs/api" "github.com/git-lfs/git-lfs/errors" + "github.com/git-lfs/git-lfs/lfsapi" ) type Direction int @@ -157,9 +158,23 @@ type NewAdapterFunc func(name string, dir Direction) Adapter type ProgressCallback func(name string, totalSize, readSoFar int64, readSinceLast int) error type AdapterConfig interface { + APIClient() *lfsapi.Client ConcurrentTransfers() int } +type adapterConfig struct { + apiClient *lfsapi.Client + concurrentTransfers int +} + +func (c *adapterConfig) ConcurrentTransfers() int { + return c.concurrentTransfers +} + +func (c *adapterConfig) APIClient() *lfsapi.Client { + return c.apiClient +} + // Adapter is implemented by types which can upload and/or download LFS // file content to a remote store. Each Adapter accepts one or more requests // which it may schedule and parallelise in whatever way it chooses, clients of diff --git a/tq/transfer_queue.go b/tq/transfer_queue.go index a9567d87..85254052 100644 --- a/tq/transfer_queue.go +++ b/tq/transfer_queue.go @@ -6,6 +6,7 @@ import ( "github.com/git-lfs/git-lfs/api" "github.com/git-lfs/git-lfs/errors" + "github.com/git-lfs/git-lfs/lfsapi" "github.com/git-lfs/git-lfs/progress" "github.com/rubyist/tracerx" ) @@ -148,11 +149,11 @@ func WithBufferDepth(depth int) Option { } // NewTransferQueue builds a TransferQueue, direction and underlying mechanism determined by adapter -func NewTransferQueue(dir Direction, manifest *Manifest, options ...Option) *TransferQueue { +func NewTransferQueue(dir Direction, manifest *Manifest, remote string, options ...Option) *TransferQueue { q := &TransferQueue{ direction: dir, client: &tqClient{Client: manifest.APIClient()}, - remote: manifest.remote, + remote: remote, errorc: make(chan error), transfers: make(map[string]*objectTuple), trMutex: &sync.Mutex{}, @@ -369,7 +370,7 @@ func (q *TransferQueue) enqueueAndCollectRetriesFor(batch batch) (batch, error) } } - retries := q.addToAdapter(toTransfer) + retries := q.addToAdapter(bRes.Endpoint, toTransfer) for t := range retries { q.rc.Increment(t.Oid) count := q.rc.CountFor(t.Oid) @@ -392,10 +393,10 @@ func (q *TransferQueue) makeBatch() batch { return make(batch, 0, q.batchSize) } // closed. // // addToAdapter returns immediately, and does not block. -func (q *TransferQueue) addToAdapter(pending []*Transfer) <-chan *objectTuple { +func (q *TransferQueue) addToAdapter(e lfsapi.Endpoint, pending []*Transfer) <-chan *objectTuple { retries := make(chan *objectTuple, len(pending)) - if err := q.ensureAdapterBegun(); err != nil { + if err := q.ensureAdapterBegun(e); err != nil { close(retries) q.errorc <- err @@ -522,7 +523,7 @@ func (q *TransferQueue) transferKind() string { } } -func (q *TransferQueue) ensureAdapterBegun() error { +func (q *TransferQueue) ensureAdapterBegun(e lfsapi.Endpoint) error { q.adapterInitMutex.Lock() defer q.adapterInitMutex.Unlock() @@ -537,7 +538,7 @@ func (q *TransferQueue) ensureAdapterBegun() error { } tracerx.Printf("tq: starting transfer adapter %q", q.adapter.Name()) - err := q.adapter.Begin(q.manifest, cb) + err := q.adapter.Begin(toAdapterCfg(q.manifest, e), cb) if err != nil { return err } @@ -546,6 +547,15 @@ func (q *TransferQueue) ensureAdapterBegun() error { return nil } +func toAdapterCfg(m *Manifest, e lfsapi.Endpoint) AdapterConfig { + apiClient := m.APIClient() + concurrency := m.ConcurrentTransfers() + if apiClient.Endpoints.AccessFor(e.Url) == lfsapi.NTLMAccess { + concurrency = 1 + } + return &adapterConfig{concurrentTransfers: concurrency, apiClient: apiClient} +} + // Wait waits for the queue to finish processing all transfers. Once Wait is // called, Add will no longer add transfers to the queue. Any failed // transfers will be automatically retried once. diff --git a/tq/transfer_test.go b/tq/transfer_test.go index 764b0016..6a8c4641 100644 --- a/tq/transfer_test.go +++ b/tq/transfer_test.go @@ -123,7 +123,7 @@ func testAdapterRegButBasicOnly(t *testing.T) { })) require.Nil(t, err) - m := NewManifestWithClient(cli, "", "") + m := NewManifestWithClient(cli) assert := assert.New(t) From 8db9cdbdce81c7cd32c19aa8c91dede228114ea6 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 14:50:54 -0700 Subject: [PATCH 12/24] lfs,tq: move manifest test to tq pkg --- {lfs => tq}/manifest_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) rename {lfs => tq}/manifest_test.go (85%) diff --git a/lfs/manifest_test.go b/tq/manifest_test.go similarity index 85% rename from lfs/manifest_test.go rename to tq/manifest_test.go index ad93dfdb..63d1129a 100644 --- a/lfs/manifest_test.go +++ b/tq/manifest_test.go @@ -1,10 +1,9 @@ -package lfs +package tq import ( "testing" "github.com/git-lfs/git-lfs/lfsapi" - "github.com/git-lfs/git-lfs/tq" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -15,7 +14,7 @@ func TestManifestIsConfigurable(t *testing.T) { })) require.Nil(t, err) - m := tq.NewManifestWithClient(cli) + m := NewManifestWithClient(cli) assert.Equal(t, 3, m.MaxRetries()) } @@ -27,7 +26,7 @@ func TestManifestChecksNTLM(t *testing.T) { })) require.Nil(t, err) - m := tq.NewManifestWithClient(cli) + m := NewManifestWithClient(cli) assert.Equal(t, 1, m.MaxRetries()) } @@ -37,7 +36,7 @@ func TestManifestClampsValidValues(t *testing.T) { })) require.Nil(t, err) - m := tq.NewManifestWithClient(cli) + m := NewManifestWithClient(cli) assert.Equal(t, 1, m.MaxRetries()) } @@ -47,6 +46,6 @@ func TestManifestIgnoresNonInts(t *testing.T) { })) require.Nil(t, err) - m := tq.NewManifestWithClient(cli) + m := NewManifestWithClient(cli) assert.Equal(t, 1, m.MaxRetries()) } From b718ace960402b8e1f43e124c1734a3f5f7097e0 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 14:54:48 -0700 Subject: [PATCH 13/24] tq: remove 'remote' property from Manifest --- tq/manifest.go | 1 - 1 file changed, 1 deletion(-) diff --git a/tq/manifest.go b/tq/manifest.go index 3f99903d..dd36d549 100644 --- a/tq/manifest.go +++ b/tq/manifest.go @@ -19,7 +19,6 @@ type Manifest struct { concurrentTransfers int basicTransfersOnly bool tusTransfersAllowed bool - remote string downloadAdapterFuncs map[string]NewAdapterFunc uploadAdapterFuncs map[string]NewAdapterFunc apiClient *lfsapi.Client From a064192173e16153b5647591d3a35ccaeb4368c3 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 15:08:42 -0700 Subject: [PATCH 14/24] tq: custom adapter passes new *adapterConfig to Begin --- tq/custom.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tq/custom.go b/tq/custom.go index 7a31680f..80c12d65 100644 --- a/tq/custom.go +++ b/tq/custom.go @@ -111,7 +111,7 @@ func (a *customAdapter) Begin(cfg AdapterConfig, cb ProgressCallback) error { } // If config says not to launch multiple processes, downgrade incoming value - newCfg := &Manifest{concurrentTransfers: 1} + newCfg := &adapterConfig{concurrentTransfers: 1, apiClient: cfg.APIClient()} return a.adapterBase.Begin(newCfg, cb) } From 2db8239a916f370cc6f4e6f222dba808028599f0 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 15:18:23 -0700 Subject: [PATCH 15/24] tq: use struct embedding instead of recreating an adapterConfig --- tq/custom.go | 11 +++++++++-- tq/transfer_queue.go | 14 +++++++++----- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/tq/custom.go b/tq/custom.go index 80c12d65..9c0a1ad8 100644 --- a/tq/custom.go +++ b/tq/custom.go @@ -111,8 +111,7 @@ func (a *customAdapter) Begin(cfg AdapterConfig, cb ProgressCallback) error { } // If config says not to launch multiple processes, downgrade incoming value - newCfg := &adapterConfig{concurrentTransfers: 1, apiClient: cfg.APIClient()} - return a.adapterBase.Begin(newCfg, cb) + return a.adapterBase.Begin(&customAdapterConfig{AdapterConfig: cfg}, cb) } func (a *customAdapter) ClearTempStorage() error { @@ -376,3 +375,11 @@ func configureCustomAdapters(git Env, m *Manifest) { } } } + +type customAdapterConfig struct { + AdapterConfig +} + +func (c *customAdapterConfig) ConcurrentTransfers() int { + return 1 +} diff --git a/tq/transfer_queue.go b/tq/transfer_queue.go index 85254052..7e2f947b 100644 --- a/tq/transfer_queue.go +++ b/tq/transfer_queue.go @@ -538,7 +538,7 @@ func (q *TransferQueue) ensureAdapterBegun(e lfsapi.Endpoint) error { } tracerx.Printf("tq: starting transfer adapter %q", q.adapter.Name()) - err := q.adapter.Begin(toAdapterCfg(q.manifest, e), cb) + err := q.adapter.Begin(q.toAdapterCfg(e), cb) if err != nil { return err } @@ -547,13 +547,17 @@ func (q *TransferQueue) ensureAdapterBegun(e lfsapi.Endpoint) error { return nil } -func toAdapterCfg(m *Manifest, e lfsapi.Endpoint) AdapterConfig { - apiClient := m.APIClient() - concurrency := m.ConcurrentTransfers() +func (q *TransferQueue) toAdapterCfg(e lfsapi.Endpoint) AdapterConfig { + apiClient := q.manifest.APIClient() + concurrency := q.manifest.ConcurrentTransfers() if apiClient.Endpoints.AccessFor(e.Url) == lfsapi.NTLMAccess { concurrency = 1 } - return &adapterConfig{concurrentTransfers: concurrency, apiClient: apiClient} + + return &adapterConfig{ + concurrentTransfers: concurrency, + apiClient: apiClient, + } } // Wait waits for the queue to finish processing all transfers. Once Wait is From 3d72a371e8177d4422d997f9b4c1aa31af0dc667 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 15:22:31 -0700 Subject: [PATCH 16/24] tq: teach basic adapter to use lfsapi.Client --- tq/adapterbase.go | 6 +++++- tq/basic_download.go | 21 +++++++++++++++------ tq/transfer.go | 6 ++++++ tq/transfer_queue.go | 1 + 4 files changed, 27 insertions(+), 7 deletions(-) diff --git a/tq/adapterbase.go b/tq/adapterbase.go index 85262e51..06210769 100644 --- a/tq/adapterbase.go +++ b/tq/adapterbase.go @@ -4,6 +4,7 @@ import ( "fmt" "sync" + "github.com/git-lfs/git-lfs/lfsapi" "github.com/rubyist/tracerx" ) @@ -14,6 +15,8 @@ type adapterBase struct { name string direction Direction transferImpl transferImplementation + apiClient *lfsapi.Client + remote string jobChan chan *job cb ProgressCallback // WaitGroup to sync the completion of all workers @@ -61,6 +64,8 @@ func (a *adapterBase) Direction() Direction { } func (a *adapterBase) Begin(cfg AdapterConfig, cb ProgressCallback) error { + a.apiClient = cfg.APIClient() + a.remote = cfg.Remote() a.cb = cb a.jobChan = make(chan *job, 100) maxConcurrency := cfg.ConcurrentTransfers() @@ -123,7 +128,6 @@ func (a *adapterBase) End() { // worker function, many of these run per adapter func (a *adapterBase) worker(workerNum int, ctx interface{}) { - tracerx.Printf("xfer: adapter %q worker %d starting", a.Name(), workerNum) waitForAuth := workerNum > 0 signalAuthOnResponse := workerNum == 0 diff --git a/tq/basic_download.go b/tq/basic_download.go index 766bca93..c4f5793c 100644 --- a/tq/basic_download.go +++ b/tq/basic_download.go @@ -4,14 +4,13 @@ import ( "fmt" "hash" "io" + "net/http" "os" "path/filepath" "regexp" "strconv" - "github.com/git-lfs/git-lfs/config" "github.com/git-lfs/git-lfs/errors" - "github.com/git-lfs/git-lfs/httputil" "github.com/git-lfs/git-lfs/localstorage" "github.com/git-lfs/git-lfs/tools" "github.com/rubyist/tracerx" @@ -44,7 +43,6 @@ func (a *basicDownloadAdapter) WorkerEnding(workerNum int, ctx interface{}) { } func (a *basicDownloadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb ProgressCallback, authOkFunc func()) error { - f, fromByte, hashSoFar, err := a.checkResumeDownload(t) if err != nil { return err @@ -97,11 +95,15 @@ func (a *basicDownloadAdapter) download(t *Transfer, cb ProgressCallback, authOk // return errors.New("Object not found on the server.") } - req, err := httputil.NewHttpRequest("GET", rel.Href, rel.Header) + req, err := http.NewRequest("GET", rel.Href, nil) if err != nil { return err } + for key, value := range rel.Header { + req.Header.Set(key, value) + } + if fromByte > 0 { if dlFile == nil || hash == nil { return fmt.Errorf("Cannot restart %v from %d without a file & hash", t.Oid, fromByte) @@ -110,7 +112,13 @@ func (a *basicDownloadAdapter) download(t *Transfer, cb ProgressCallback, authOk req.Header.Set("Range", fmt.Sprintf("bytes=%d-%d", fromByte, t.Size-1)) } - res, err := httputil.DoHttpRequest(config.Config, req, !t.Authenticated) + var res *http.Response + if t.Authenticated { + res, err = a.apiClient.Do(req) + } else { + res, err = a.apiClient.DoWithAuth(a.remote, req) + } + if err != nil { // Special-case status code 416 () - fall back if fromByte > 0 && dlFile != nil && res.StatusCode == 416 { @@ -121,7 +129,8 @@ func (a *basicDownloadAdapter) download(t *Transfer, cb ProgressCallback, authOk } return errors.NewRetriableError(err) } - httputil.LogTransfer(config.Config, "lfs.data.download", res) + + a.apiClient.LogResponse("lfs.data.download", res) defer res.Body.Close() // Range request must return 206 & content range to confirm diff --git a/tq/transfer.go b/tq/transfer.go index d381dbf6..d083d20c 100644 --- a/tq/transfer.go +++ b/tq/transfer.go @@ -160,11 +160,13 @@ type ProgressCallback func(name string, totalSize, readSoFar int64, readSinceLas type AdapterConfig interface { APIClient() *lfsapi.Client ConcurrentTransfers() int + Remote() string } type adapterConfig struct { apiClient *lfsapi.Client concurrentTransfers int + remote string } func (c *adapterConfig) ConcurrentTransfers() int { @@ -175,6 +177,10 @@ func (c *adapterConfig) APIClient() *lfsapi.Client { return c.apiClient } +func (c *adapterConfig) Remote() string { + return c.remote +} + // Adapter is implemented by types which can upload and/or download LFS // file content to a remote store. Each Adapter accepts one or more requests // which it may schedule and parallelise in whatever way it chooses, clients of diff --git a/tq/transfer_queue.go b/tq/transfer_queue.go index 7e2f947b..780e4f74 100644 --- a/tq/transfer_queue.go +++ b/tq/transfer_queue.go @@ -557,6 +557,7 @@ func (q *TransferQueue) toAdapterCfg(e lfsapi.Endpoint) AdapterConfig { return &adapterConfig{ concurrentTransfers: concurrency, apiClient: apiClient, + remote: q.remote, } } From a8dc86bccb005d16af044fcb914ec221443112aa Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 15:27:07 -0700 Subject: [PATCH 17/24] tq: teach basic upload adapter to use lfsapi.Client --- tq/basic_upload.go | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/tq/basic_upload.go b/tq/basic_upload.go index d03b9cea..e318f390 100644 --- a/tq/basic_upload.go +++ b/tq/basic_upload.go @@ -3,14 +3,13 @@ package tq import ( "io" "io/ioutil" + "net/http" "os" "path/filepath" "strconv" + "strings" - "github.com/git-lfs/git-lfs/config" "github.com/git-lfs/git-lfs/errors" - "github.com/git-lfs/git-lfs/httputil" - "github.com/git-lfs/git-lfs/lfsapi" "github.com/git-lfs/git-lfs/progress" ) @@ -50,11 +49,15 @@ func (a *basicUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb Progres // return fmt.Errorf("No upload action for this object.") } - req, err := httputil.NewHttpRequest("PUT", rel.Href, rel.Header) + req, err := http.NewRequest("PUT", rel.Href, nil) if err != nil { return err } + for key, value := range rel.Header { + req.Header.Set(key, value) + } + if len(req.Header.Get("Content-Type")) == 0 { req.Header.Set("Content-Type", "application/octet-stream") } @@ -97,11 +100,17 @@ func (a *basicUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb Progres req.Body = ioutil.NopCloser(reader) - res, err := httputil.DoHttpRequest(config.Config, req, !t.Authenticated) + var res *http.Response + if t.Authenticated { + res, err = a.apiClient.Do(req) + } else { + res, err = a.apiClient.DoWithAuth(a.remote, req) + } if err != nil { return errors.NewRetriableError(err) } - httputil.LogTransfer(config.Config, "lfs.data.upload", res) + + a.apiClient.LogResponse("lfs.data.upload", res) // A status code of 403 likely means that an authentication token for the // upload has expired. This can be safely retried. @@ -111,14 +120,17 @@ func (a *basicUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb Progres } if res.StatusCode > 299 { - return errors.Wrapf(nil, "Invalid status for %s: %d", httputil.TraceHttpReq(req), res.StatusCode) + return errors.Wrapf(nil, "Invalid status for %s %s: %d", + req.Method, + strings.SplitN(req.URL.String(), "?", 2)[0], + res.StatusCode, + ) } io.Copy(ioutil.Discard, res.Body) res.Body.Close() - cli := &lfsapi.Client{} - return verifyUpload(cli, t) + return verifyUpload(a.apiClient, t) } // startCallbackReader is a reader wrapper which calls a function as soon as the From 1401e0e39cf433b5b809b780094c746e31d44c7b Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 15:33:31 -0700 Subject: [PATCH 18/24] tq: teach thus adapter to use lfsapi.Client --- tq/tus_upload.go | 41 +++++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/tq/tus_upload.go b/tq/tus_upload.go index 0cc17c9e..3224a5b6 100644 --- a/tq/tus_upload.go +++ b/tq/tus_upload.go @@ -4,12 +4,12 @@ import ( "fmt" "io" "io/ioutil" + "net/http" "os" "strconv" + "strings" - "github.com/git-lfs/git-lfs/config" "github.com/git-lfs/git-lfs/errors" - "github.com/git-lfs/git-lfs/httputil" "github.com/git-lfs/git-lfs/lfsapi" "github.com/git-lfs/git-lfs/progress" "github.com/rubyist/tracerx" @@ -49,12 +49,23 @@ func (a *tusUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb ProgressC // 1. Send HEAD request to determine upload start point // Request must include Tus-Resumable header (version) tracerx.Printf("xfer: sending tus.io HEAD request for %q", t.Oid) - req, err := httputil.NewHttpRequest("HEAD", rel.Href, rel.Header) + req, err := http.NewRequest("HEAD", rel.Href, nil) if err != nil { return err } + + for key, value := range rel.Header { + req.Header.Set(key, value) + } + req.Header.Set("Tus-Resumable", TusVersion) - res, err := httputil.DoHttpRequest(config.Config, req, false) + + var res *http.Response + if t.Authenticated { + res, err = a.apiClient.Do(req) + } else { + res, err = a.apiClient.DoWithAuth(a.remote, req) + } if err != nil { return errors.NewRetriableError(err) } @@ -101,10 +112,15 @@ func (a *tusUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb ProgressC // Response may include Upload-Expires header in which case check not passed tracerx.Printf("xfer: sending tus.io PATCH request for %q", t.Oid) - req, err = httputil.NewHttpRequest("PATCH", rel.Href, rel.Header) + req, err = http.NewRequest("PATCH", rel.Href, nil) if err != nil { return err } + + for key, value := range rel.Header { + req.Header.Set(key, value) + } + req.Header.Set("Tus-Resumable", TusVersion) req.Header.Set("Upload-Offset", strconv.FormatInt(offset, 10)) req.Header.Set("Content-Type", "application/offset+octet-stream") @@ -135,11 +151,16 @@ func (a *tusUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb ProgressC req.Body = ioutil.NopCloser(reader) - res, err = httputil.DoHttpRequest(config.Config, req, false) + if t.Authenticated { + res, err = a.apiClient.Do(req) + } else { + res, err = a.apiClient.DoWithAuth(a.remote, req) + } if err != nil { return errors.NewRetriableError(err) } - httputil.LogTransfer(config.Config, "lfs.data.upload", res) + + a.apiClient.LogResponse("lfs.data.upload", res) // A status code of 403 likely means that an authentication token for the // upload has expired. This can be safely retried. @@ -149,7 +170,11 @@ func (a *tusUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb ProgressC } if res.StatusCode > 299 { - return errors.Wrapf(nil, "Invalid status for %s: %d", httputil.TraceHttpReq(req), res.StatusCode) + return errors.Wrapf(nil, "Invalid status for %s %s: %d", + req.Method, + strings.SplitN(req.URL.String(), "?", 2)[0], + res.StatusCode, + ) } io.Copy(ioutil.Discard, res.Body) From d960b1441e7d84982fbcc28e9cbce43410a794de Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 15:43:29 -0700 Subject: [PATCH 19/24] tq: reduce duplication with http helpers in adapters --- tq/adapterbase.go | 24 ++++++++++++++++++++++-- tq/basic_download.go | 15 ++------------- tq/basic_upload.go | 14 ++------------ tq/tus_upload.go | 26 ++++---------------------- 4 files changed, 30 insertions(+), 49 deletions(-) diff --git a/tq/adapterbase.go b/tq/adapterbase.go index 06210769..e1bf9fba 100644 --- a/tq/adapterbase.go +++ b/tq/adapterbase.go @@ -2,6 +2,7 @@ package tq import ( "fmt" + "net/http" "sync" "github.com/git-lfs/git-lfs/lfsapi" @@ -50,8 +51,7 @@ func newAdapterBase(name string, dir Direction, ti transferImplementation) *adap name: name, direction: dir, transferImpl: ti, - - jobWait: new(sync.WaitGroup), + jobWait: new(sync.WaitGroup), } } @@ -176,6 +176,26 @@ func (a *adapterBase) worker(workerNum int, ctx interface{}) { a.workerWait.Done() } +func (a *adapterBase) newHTTPRequest(method string, rel *Action) (*http.Request, error) { + req, err := http.NewRequest(method, rel.Href, nil) + if err != nil { + return nil, err + } + + for key, value := range rel.Header { + req.Header.Set(key, value) + } + + return req, nil +} + +func (a *adapterBase) doHTTP(t *Transfer, req *http.Request) (*http.Response, error) { + if t.Authenticated { + return a.apiClient.Do(req) + } + return a.apiClient.DoWithAuth(a.remote, req) +} + func advanceCallbackProgress(cb ProgressCallback, t *Transfer, numBytes int64) { if cb != nil { // Must split into max int sizes since read count is int diff --git a/tq/basic_download.go b/tq/basic_download.go index c4f5793c..b77b3298 100644 --- a/tq/basic_download.go +++ b/tq/basic_download.go @@ -4,7 +4,6 @@ import ( "fmt" "hash" "io" - "net/http" "os" "path/filepath" "regexp" @@ -95,15 +94,11 @@ func (a *basicDownloadAdapter) download(t *Transfer, cb ProgressCallback, authOk // return errors.New("Object not found on the server.") } - req, err := http.NewRequest("GET", rel.Href, nil) + req, err := a.newHTTPRequest("GET", rel) if err != nil { return err } - for key, value := range rel.Header { - req.Header.Set(key, value) - } - if fromByte > 0 { if dlFile == nil || hash == nil { return fmt.Errorf("Cannot restart %v from %d without a file & hash", t.Oid, fromByte) @@ -112,13 +107,7 @@ func (a *basicDownloadAdapter) download(t *Transfer, cb ProgressCallback, authOk req.Header.Set("Range", fmt.Sprintf("bytes=%d-%d", fromByte, t.Size-1)) } - var res *http.Response - if t.Authenticated { - res, err = a.apiClient.Do(req) - } else { - res, err = a.apiClient.DoWithAuth(a.remote, req) - } - + res, err := a.doHTTP(t, req) if err != nil { // Special-case status code 416 () - fall back if fromByte > 0 && dlFile != nil && res.StatusCode == 416 { diff --git a/tq/basic_upload.go b/tq/basic_upload.go index e318f390..b1cb9fb7 100644 --- a/tq/basic_upload.go +++ b/tq/basic_upload.go @@ -3,7 +3,6 @@ package tq import ( "io" "io/ioutil" - "net/http" "os" "path/filepath" "strconv" @@ -49,15 +48,11 @@ func (a *basicUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb Progres // return fmt.Errorf("No upload action for this object.") } - req, err := http.NewRequest("PUT", rel.Href, nil) + req, err := a.newHTTPRequest("PUT", rel) if err != nil { return err } - for key, value := range rel.Header { - req.Header.Set(key, value) - } - if len(req.Header.Get("Content-Type")) == 0 { req.Header.Set("Content-Type", "application/octet-stream") } @@ -100,12 +95,7 @@ func (a *basicUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb Progres req.Body = ioutil.NopCloser(reader) - var res *http.Response - if t.Authenticated { - res, err = a.apiClient.Do(req) - } else { - res, err = a.apiClient.DoWithAuth(a.remote, req) - } + res, err := a.doHTTP(t, req) if err != nil { return errors.NewRetriableError(err) } diff --git a/tq/tus_upload.go b/tq/tus_upload.go index 3224a5b6..9722fc24 100644 --- a/tq/tus_upload.go +++ b/tq/tus_upload.go @@ -4,7 +4,6 @@ import ( "fmt" "io" "io/ioutil" - "net/http" "os" "strconv" "strings" @@ -49,23 +48,14 @@ func (a *tusUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb ProgressC // 1. Send HEAD request to determine upload start point // Request must include Tus-Resumable header (version) tracerx.Printf("xfer: sending tus.io HEAD request for %q", t.Oid) - req, err := http.NewRequest("HEAD", rel.Href, nil) + req, err := a.newHTTPRequest("HEAD", rel) if err != nil { return err } - for key, value := range rel.Header { - req.Header.Set(key, value) - } - req.Header.Set("Tus-Resumable", TusVersion) - var res *http.Response - if t.Authenticated { - res, err = a.apiClient.Do(req) - } else { - res, err = a.apiClient.DoWithAuth(a.remote, req) - } + res, err := a.doHTTP(t, req) if err != nil { return errors.NewRetriableError(err) } @@ -112,15 +102,11 @@ func (a *tusUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb ProgressC // Response may include Upload-Expires header in which case check not passed tracerx.Printf("xfer: sending tus.io PATCH request for %q", t.Oid) - req, err = http.NewRequest("PATCH", rel.Href, nil) + req, err = a.newHTTPRequest("PATCH", rel) if err != nil { return err } - for key, value := range rel.Header { - req.Header.Set(key, value) - } - req.Header.Set("Tus-Resumable", TusVersion) req.Header.Set("Upload-Offset", strconv.FormatInt(offset, 10)) req.Header.Set("Content-Type", "application/offset+octet-stream") @@ -151,11 +137,7 @@ func (a *tusUploadAdapter) DoTransfer(ctx interface{}, t *Transfer, cb ProgressC req.Body = ioutil.NopCloser(reader) - if t.Authenticated { - res, err = a.apiClient.Do(req) - } else { - res, err = a.apiClient.DoWithAuth(a.remote, req) - } + res, err = a.doHTTP(t, req) if err != nil { return errors.NewRetriableError(err) } From fecdc9e745f224432be213354c4624f227ea1b03 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 16:10:30 -0700 Subject: [PATCH 20/24] commands: keep a global lfsapi.Client and tq.Manifest for a single lfs command --- commands/command_env.go | 2 +- commands/command_fetch.go | 2 +- commands/command_prune.go | 2 +- commands/command_smudge.go | 2 +- commands/commands.go | 40 ++++++++++++++++++++++++++++---------- commands/pull.go | 2 +- commands/uploader.go | 2 +- 7 files changed, 36 insertions(+), 16 deletions(-) diff --git a/commands/command_env.go b/commands/command_env.go index 070a4dc4..20202e63 100644 --- a/commands/command_env.go +++ b/commands/command_env.go @@ -35,7 +35,7 @@ func envCommand(cmd *cobra.Command, args []string) { } } - for _, env := range lfs.Environ(cfg, buildTransferManifest()) { + for _, env := range lfs.Environ(cfg, getTransferManifest()) { Print(env) } diff --git a/commands/command_fetch.go b/commands/command_fetch.go index 580229a9..0baa357e 100644 --- a/commands/command_fetch.go +++ b/commands/command_fetch.go @@ -279,7 +279,7 @@ func fetchAndReportToChan(allpointers []*lfs.WrappedPointer, filter *filepathfil } ready, pointers, meter := readyAndMissingPointers(allpointers, filter) - q := newDownloadQueue(buildTransferManifest(), cfg.CurrentRemote, tq.WithProgress(meter)) + q := newDownloadQueue(getTransferManifest(), cfg.CurrentRemote, tq.WithProgress(meter)) if out != nil { // If we already have it, or it won't be fetched diff --git a/commands/command_prune.go b/commands/command_prune.go index 41b8df1a..90927fe1 100644 --- a/commands/command_prune.go +++ b/commands/command_prune.go @@ -120,7 +120,7 @@ func prune(fetchPruneConfig config.FetchPruneConfig, verifyRemote, dryRun, verbo var verifywait sync.WaitGroup if verifyRemote { - verifyQueue = newDownloadCheckQueue(buildTransferManifest(), fetchPruneConfig.PruneRemoteName) + verifyQueue = newDownloadCheckQueue(getTransferManifest(), fetchPruneConfig.PruneRemoteName) verifiedObjects = tools.NewStringSetWithCapacity(len(localObjects) / 2) // this channel is filled with oids for which Check() succeeded & Transfer() was called diff --git a/commands/command_smudge.go b/commands/command_smudge.go index d1248ed4..83d695c2 100644 --- a/commands/command_smudge.go +++ b/commands/command_smudge.go @@ -40,7 +40,7 @@ func smudge(to io.Writer, ptr *lfs.Pointer, filename string, skip bool, filter * download = filter.Allows(filename) } - err = ptr.Smudge(to, filename, download, buildTransferManifest(), cb) + err = ptr.Smudge(to, filename, download, getTransferManifest(), cb) if file != nil { file.Close() } diff --git a/commands/commands.go b/commands/commands.go index bdc1e382..125d5693 100644 --- a/commands/commands.go +++ b/commands/commands.go @@ -9,6 +9,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync" "time" "github.com/git-lfs/git-lfs/config" @@ -34,26 +35,45 @@ var ( ManPages = make(map[string]string, 20) cfg = config.Config + tqManifest *tq.Manifest + apiClient *lfsapi.Client + global sync.Mutex + includeArg string excludeArg string ) -// buildTransferManifest builds a tq.Manifest from the global os and git +// getTransferManifest builds a tq.Manifest from the global os and git // environments. -func buildTransferManifest() *tq.Manifest { - return tq.NewManifestWithClient(newAPIClient()) +func getTransferManifest() *tq.Manifest { + c := getAPIClient() + + global.Lock() + defer global.Unlock() + + if tqManifest == nil { + tqManifest = tq.NewManifestWithClient(c) + } + + return tqManifest } -func newAPIClient() *lfsapi.Client { - c, err := lfsapi.NewClient(cfg.Os, cfg.Git) - if err != nil { - ExitWithError(err) +func getAPIClient() *lfsapi.Client { + global.Lock() + defer global.Unlock() + + if apiClient == nil { + c, err := lfsapi.NewClient(cfg.Os, cfg.Git) + if err != nil { + ExitWithError(err) + } + apiClient = c } - return c + return apiClient } func newLockClient(remote string) *locking.Client { - lockClient, err := locking.NewClient(remote, newAPIClient()) + lockClient, err := locking.NewClient(remote, getAPIClient()) if err == nil { err = lockClient.SetupFileCache(filepath.Join(config.LocalGitStorageDir, "lfs")) } @@ -346,7 +366,7 @@ func logPanicToWriter(w io.Writer, loggedError error) { fmt.Fprintln(w, "\nENV:") // log the environment - for _, env := range lfs.Environ(cfg, buildTransferManifest()) { + for _, env := range lfs.Environ(cfg, getTransferManifest()) { fmt.Fprintln(w, env) } } diff --git a/commands/pull.go b/commands/pull.go index 42ab0ec2..0d1e6286 100644 --- a/commands/pull.go +++ b/commands/pull.go @@ -26,7 +26,7 @@ func newSingleCheckout() *singleCheckout { return &singleCheckout{ gitIndexer: &gitIndexer{}, pathConverter: pathConverter, - manifest: buildTransferManifest(), + manifest: getTransferManifest(), } } diff --git a/commands/uploader.go b/commands/uploader.go index 0ac22d51..4f704e60 100644 --- a/commands/uploader.go +++ b/commands/uploader.go @@ -22,7 +22,7 @@ func newUploadContext(remote string, dryRun bool) *uploadContext { cfg.CurrentRemote = remote return &uploadContext{ Remote: remote, - Manifest: buildTransferManifest(), + Manifest: getTransferManifest(), DryRun: dryRun, uploadedOids: tools.NewStringSet(), } From c9a03bb3bcacbcdf7ed913bc40e8dcdd1ccfd787 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Wed, 4 Jan 2017 16:10:53 -0700 Subject: [PATCH 21/24] commands: remove last httputil usage --- commands/command_version.go | 4 ++-- commands/run.go | 31 +++++++++++++++++++++++++++++-- test/test-custom-transfers.sh | 3 ++- 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/commands/command_version.go b/commands/command_version.go index 62bece48..bde27cd1 100644 --- a/commands/command_version.go +++ b/commands/command_version.go @@ -1,7 +1,7 @@ package commands import ( - "github.com/git-lfs/git-lfs/httputil" + "github.com/git-lfs/git-lfs/lfsapi" "github.com/spf13/cobra" ) @@ -10,7 +10,7 @@ var ( ) func versionCommand(cmd *cobra.Command, args []string) { - Print(httputil.UserAgent) + Print(lfsapi.UserAgent) if lovesComics { Print("Nothing may see Gah Lak Tus and survive!") diff --git a/commands/run.go b/commands/run.go index 45c9026f..de603f56 100644 --- a/commands/run.go +++ b/commands/run.go @@ -3,11 +3,13 @@ package commands import ( "fmt" "os" + "path/filepath" "strings" "sync" + "time" "github.com/git-lfs/git-lfs/config" - "github.com/git-lfs/git-lfs/httputil" + "github.com/git-lfs/git-lfs/lfsapi" "github.com/git-lfs/git-lfs/localstorage" "github.com/spf13/cobra" ) @@ -64,7 +66,7 @@ func Run() { } root.Execute() - httputil.LogHttpStats(cfg) + logHTTPStats(getAPIClient()) } func gitlfsCommand(cmd *cobra.Command, args []string) { @@ -103,3 +105,28 @@ func printHelp(commandName string) { fmt.Fprintf(os.Stderr, "Sorry, no usage text found for %q\n", commandName) } } + +func logHTTPStats(c *lfsapi.Client) { + if !c.LoggingStats { + return + } + + file, err := statsLogFile() + if err != nil { + fmt.Fprintf(os.Stderr, "Error logging http stats: %s\n", err) + return + } + + defer file.Close() + c.LogStats(file) +} + +func statsLogFile() (*os.File, error) { + logBase := filepath.Join(config.LocalLogDir, "http") + if err := os.MkdirAll(logBase, 0755); err != nil { + return nil, err + } + + logFile := fmt.Sprintf("http-%d.log", time.Now().Unix()) + return os.Create(filepath.Join(logBase, logFile)) +} diff --git a/test/test-custom-transfers.sh b/test/test-custom-transfers.sh index e1ebd079..63b97317 100755 --- a/test/test-custom-transfers.sh +++ b/test/test-custom-transfers.sh @@ -102,7 +102,8 @@ begin_test "custom-transfer-upload-download" grep "xfer: started custom adapter process" fetchcustom.log grep "xfer\[lfstest-customadapter\]:" fetchcustom.log grep "11 of 11 files" fetchcustom.log - [ `find .git/lfs/objects -type f | wc -l` = 11 ] + objectlist=`find .git/lfs/objects -type f` + [ "$(echo "$objectlist" | wc -l)" -eq 11 ] ) end_test From 61ec20e6292a7eccab9b01c5ed3386d5c72114e7 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 6 Jan 2017 11:31:07 -0700 Subject: [PATCH 22/24] commands: initialize with capacity, not len /cc 7af92813912bccf1f144b87a59fdedf28cfd62b6 --- commands/commands.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commands/commands.go b/commands/commands.go index 125d5693..5ec56e4e 100644 --- a/commands/commands.go +++ b/commands/commands.go @@ -87,7 +87,7 @@ func newLockClient(remote string) *locking.Client { // newDownloadCheckQueue builds a checking queue, checks that objects are there but doesn't download func newDownloadCheckQueue(manifest *tq.Manifest, remote string, options ...tq.Option) *tq.TransferQueue { - allOptions := make([]tq.Option, len(options), len(options)+1) + allOptions := make([]tq.Option, 0, len(options)+1) allOptions = append(allOptions, options...) allOptions = append(allOptions, tq.DryRun(true)) return newDownloadQueue(manifest, remote, allOptions...) From c4e7c1af5c782bb6cb65666d3018986473e700f4 Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 6 Jan 2017 11:34:43 -0700 Subject: [PATCH 23/24] lfsapi: rename Env export to TestEnv --- lfsapi/auth_test.go | 6 ++-- lfsapi/certs_test.go | 16 +++++------ lfsapi/client.go | 4 +-- lfsapi/client_test.go | 16 +++++------ lfsapi/endpoint_finder_test.go | 50 +++++++++++++++++----------------- lfsapi/lfsapi.go | 18 ++++++------ lfsapi/proxy_test.go | 14 +++++----- locking/api_test.go | 6 ++-- locking/locks_test.go | 2 +- tq/api_test.go | 4 +-- tq/custom_test.go | 8 +++--- tq/manifest_test.go | 8 +++--- tq/transfer_test.go | 2 +- 13 files changed, 77 insertions(+), 77 deletions(-) diff --git a/lfsapi/auth_test.go b/lfsapi/auth_test.go index 01938816..a32db43a 100644 --- a/lfsapi/auth_test.go +++ b/lfsapi/auth_test.go @@ -74,7 +74,7 @@ func TestDoWithAuthApprove(t *testing.T) { creds := newMockCredentialHelper() c := &Client{ Credentials: creds, - Endpoints: NewEndpointFinder(Env(map[string]string{ + Endpoints: NewEndpointFinder(TestEnv(map[string]string{ "lfs.url": srv.URL + "/repo/lfs", })), } @@ -144,7 +144,7 @@ func TestDoWithAuthReject(t *testing.T) { c := &Client{ Credentials: creds, - Endpoints: NewEndpointFinder(Env(map[string]string{ + Endpoints: NewEndpointFinder(TestEnv(map[string]string{ "lfs.url": srv.URL, })), } @@ -467,7 +467,7 @@ func TestGetCreds(t *testing.T) { req.Header.Set(key, value) } - ef := NewEndpointFinder(Env(test.Config)) + ef := NewEndpointFinder(TestEnv(test.Config)) endpoint, access, creds, credsURL, err := getCreds(credHelper, netrcFinder, ef, test.Remote, req) if !assert.Nil(t, err) { continue diff --git a/lfsapi/certs_test.go b/lfsapi/certs_test.go index c74fcb6a..a1e8c421 100644 --- a/lfsapi/certs_test.go +++ b/lfsapi/certs_test.go @@ -60,7 +60,7 @@ func TestCertFromSSLCAInfoConfig(t *testing.T) { // Test http..sslcainfo for _, hostName := range sslCAInfoConfigHostNames { hostKey := fmt.Sprintf("http.https://%v.sslcainfo", hostName) - c, err := NewClient(nil, Env(map[string]string{ + c, err := NewClient(nil, TestEnv(map[string]string{ hostKey: tempfile.Name(), })) assert.Nil(t, err) @@ -82,7 +82,7 @@ func TestCertFromSSLCAInfoConfig(t *testing.T) { } // Test http.sslcainfo - c, err := NewClient(nil, Env(map[string]string{ + c, err := NewClient(nil, TestEnv(map[string]string{ "http.sslcainfo": tempfile.Name(), })) assert.Nil(t, err) @@ -103,7 +103,7 @@ func TestCertFromSSLCAInfoEnv(t *testing.T) { assert.Nil(t, err, "Error writing temp cert file") tempfile.Close() - c, err := NewClient(Env(map[string]string{ + c, err := NewClient(TestEnv(map[string]string{ "GIT_SSL_CAINFO": tempfile.Name(), }), nil) assert.Nil(t, err) @@ -123,7 +123,7 @@ func TestCertFromSSLCAPathConfig(t *testing.T) { err = ioutil.WriteFile(filepath.Join(tempdir, "cert1.pem"), []byte(testCert), 0644) assert.Nil(t, err, "Error creating cert file") - c, err := NewClient(nil, Env(map[string]string{ + c, err := NewClient(nil, TestEnv(map[string]string{ "http.sslcapath": tempdir, })) @@ -144,7 +144,7 @@ func TestCertFromSSLCAPathEnv(t *testing.T) { err = ioutil.WriteFile(filepath.Join(tempdir, "cert1.pem"), []byte(testCert), 0644) assert.Nil(t, err, "Error creating cert file") - c, err := NewClient(Env(map[string]string{ + c, err := NewClient(TestEnv(map[string]string{ "GIT_SSL_CAPATH": tempdir, }), nil) assert.Nil(t, err) @@ -164,7 +164,7 @@ func TestCertVerifyDisabledGlobalEnv(t *testing.T) { assert.False(t, tr.TLSClientConfig.InsecureSkipVerify) } - c, err := NewClient(Env(map[string]string{ + c, err := NewClient(TestEnv(map[string]string{ "GIT_SSL_NO_VERIFY": "1", }), nil) @@ -185,7 +185,7 @@ func TestCertVerifyDisabledGlobalConfig(t *testing.T) { assert.False(t, tr.TLSClientConfig.InsecureSkipVerify) } - c, err := NewClient(nil, Env(map[string]string{ + c, err := NewClient(nil, TestEnv(map[string]string{ "http.sslverify": "false", })) assert.Nil(t, err) @@ -211,7 +211,7 @@ func TestCertVerifyDisabledHostConfig(t *testing.T) { assert.False(t, tr.TLSClientConfig.InsecureSkipVerify) } - c, err := NewClient(nil, Env(map[string]string{ + c, err := NewClient(nil, TestEnv(map[string]string{ "http.https://specifichost.com/.sslverify": "false", })) assert.Nil(t, err) diff --git a/lfsapi/client.go b/lfsapi/client.go index ef5c8e3b..72c604f3 100644 --- a/lfsapi/client.go +++ b/lfsapi/client.go @@ -99,11 +99,11 @@ func (c *Client) httpClient(host string) *http.Client { defer c.clientMu.Unlock() if c.gitEnv == nil { - c.gitEnv = make(Env) + c.gitEnv = make(TestEnv) } if c.osEnv == nil { - c.osEnv = make(Env) + c.osEnv = make(TestEnv) } if c.hostClients == nil { diff --git a/lfsapi/client_test.go b/lfsapi/client_test.go index bcf354b8..4afe3ceb 100644 --- a/lfsapi/client_test.go +++ b/lfsapi/client_test.go @@ -99,7 +99,7 @@ func TestClientRedirect(t *testing.T) { } func TestNewClient(t *testing.T) { - c, err := NewClient(Env(map[string]string{}), Env(map[string]string{ + c, err := NewClient(TestEnv(map[string]string{}), TestEnv(map[string]string{ "lfs.dialtimeout": "151", "lfs.keepalive": "152", "lfs.tlstimeout": "153", @@ -119,7 +119,7 @@ func TestNewClientWithGitSSLVerify(t *testing.T) { assert.False(t, c.SkipSSLVerify) for _, value := range []string{"true", "1", "t"} { - c, err = NewClient(Env(map[string]string{}), Env(map[string]string{ + c, err = NewClient(TestEnv(map[string]string{}), TestEnv(map[string]string{ "http.sslverify": value, })) t.Logf("http.sslverify: %q", value) @@ -128,7 +128,7 @@ func TestNewClientWithGitSSLVerify(t *testing.T) { } for _, value := range []string{"false", "0", "f"} { - c, err = NewClient(Env(map[string]string{}), Env(map[string]string{ + c, err = NewClient(TestEnv(map[string]string{}), TestEnv(map[string]string{ "http.sslverify": value, })) t.Logf("http.sslverify: %q", value) @@ -143,18 +143,18 @@ func TestNewClientWithOSSSLVerify(t *testing.T) { assert.False(t, c.SkipSSLVerify) for _, value := range []string{"false", "0", "f"} { - c, err = NewClient(Env(map[string]string{ + c, err = NewClient(TestEnv(map[string]string{ "GIT_SSL_NO_VERIFY": value, - }), Env(map[string]string{})) + }), TestEnv(map[string]string{})) t.Logf("GIT_SSL_NO_VERIFY: %q", value) assert.Nil(t, err) assert.False(t, c.SkipSSLVerify) } for _, value := range []string{"true", "1", "t"} { - c, err = NewClient(Env(map[string]string{ + c, err = NewClient(TestEnv(map[string]string{ "GIT_SSL_NO_VERIFY": value, - }), Env(map[string]string{})) + }), TestEnv(map[string]string{})) t.Logf("GIT_SSL_NO_VERIFY: %q", value) assert.Nil(t, err) assert.True(t, c.SkipSSLVerify) @@ -170,7 +170,7 @@ func TestNewRequest(t *testing.T) { } for _, test := range tests { - c, err := NewClient(nil, Env(map[string]string{ + c, err := NewClient(nil, TestEnv(map[string]string{ "lfs.url": test[0], })) require.Nil(t, err) diff --git a/lfsapi/endpoint_finder_test.go b/lfsapi/endpoint_finder_test.go index d3b73f2c..29ed0c2f 100644 --- a/lfsapi/endpoint_finder_test.go +++ b/lfsapi/endpoint_finder_test.go @@ -7,7 +7,7 @@ import ( ) func TestEndpointDefaultsToOrigin(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.lfsurl": "abc", })) @@ -18,7 +18,7 @@ func TestEndpointDefaultsToOrigin(t *testing.T) { } func TestEndpointOverridesOrigin(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "lfs.url": "abc", "remote.origin.lfsurl": "def", })) @@ -30,7 +30,7 @@ func TestEndpointOverridesOrigin(t *testing.T) { } func TestEndpointNoOverrideDefaultRemote(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.lfsurl": "abc", "remote.other.lfsurl": "def", })) @@ -42,7 +42,7 @@ func TestEndpointNoOverrideDefaultRemote(t *testing.T) { } func TestEndpointUseAlternateRemote(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.lfsurl": "abc", "remote.other.lfsurl": "def", })) @@ -54,7 +54,7 @@ func TestEndpointUseAlternateRemote(t *testing.T) { } func TestEndpointAddsLfsSuffix(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "https://example.com/foo/bar", })) @@ -65,7 +65,7 @@ func TestEndpointAddsLfsSuffix(t *testing.T) { } func TestBareEndpointAddsLfsSuffix(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "https://example.com/foo/bar.git", })) @@ -76,7 +76,7 @@ func TestBareEndpointAddsLfsSuffix(t *testing.T) { } func TestEndpointSeparateClonePushUrl(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "https://example.com/foo/bar.git", "remote.origin.pushurl": "https://readwrite.com/foo/bar.git", })) @@ -93,7 +93,7 @@ func TestEndpointSeparateClonePushUrl(t *testing.T) { } func TestEndpointOverriddenSeparateClonePushLfsUrl(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "https://example.com/foo/bar.git", "remote.origin.pushurl": "https://readwrite.com/foo/bar.git", "remote.origin.lfsurl": "https://examplelfs.com/foo/bar", @@ -112,7 +112,7 @@ func TestEndpointOverriddenSeparateClonePushLfsUrl(t *testing.T) { } func TestEndpointGlobalSeparateLfsPush(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "lfs.url": "https://readonly.com/foo/bar", "lfs.pushurl": "https://write.com/foo/bar", })) @@ -129,7 +129,7 @@ func TestEndpointGlobalSeparateLfsPush(t *testing.T) { } func TestSSHEndpointOverridden(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "git@example.com:foo/bar", "remote.origin.lfsurl": "lfs", })) @@ -142,7 +142,7 @@ func TestSSHEndpointOverridden(t *testing.T) { } func TestSSHEndpointAddsLfsSuffix(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "ssh://git@example.com/foo/bar", })) @@ -154,7 +154,7 @@ func TestSSHEndpointAddsLfsSuffix(t *testing.T) { } func TestSSHCustomPortEndpointAddsLfsSuffix(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "ssh://git@example.com:9000/foo/bar", })) @@ -166,7 +166,7 @@ func TestSSHCustomPortEndpointAddsLfsSuffix(t *testing.T) { } func TestBareSSHEndpointAddsLfsSuffix(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "git@example.com:foo/bar.git", })) @@ -178,7 +178,7 @@ func TestBareSSHEndpointAddsLfsSuffix(t *testing.T) { } func TestSSHEndpointFromGlobalLfsUrl(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "lfs.url": "git@example.com:foo/bar.git", })) @@ -190,7 +190,7 @@ func TestSSHEndpointFromGlobalLfsUrl(t *testing.T) { } func TestHTTPEndpointAddsLfsSuffix(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "http://example.com/foo/bar", })) @@ -202,7 +202,7 @@ func TestHTTPEndpointAddsLfsSuffix(t *testing.T) { } func TestBareHTTPEndpointAddsLfsSuffix(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "http://example.com/foo/bar.git", })) @@ -214,7 +214,7 @@ func TestBareHTTPEndpointAddsLfsSuffix(t *testing.T) { } func TestGitEndpointAddsLfsSuffix(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "git://example.com/foo/bar", })) @@ -226,7 +226,7 @@ func TestGitEndpointAddsLfsSuffix(t *testing.T) { } func TestGitEndpointAddsLfsSuffixWithCustomProtocol(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "git://example.com/foo/bar", "lfs.gitprotocol": "http", })) @@ -239,7 +239,7 @@ func TestGitEndpointAddsLfsSuffixWithCustomProtocol(t *testing.T) { } func TestBareGitEndpointAddsLfsSuffix(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "remote.origin.url": "git://example.com/foo/bar.git", })) @@ -266,7 +266,7 @@ func TestAccessConfig(t *testing.T) { } for value, expected := range tests { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "lfs.url": "http://example.com", "lfs.http://example.com.access": value, "lfs.https://example.com.access": "bad", @@ -285,7 +285,7 @@ func TestAccessConfig(t *testing.T) { // Test again but with separate push url for value, expected := range tests { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "lfs.url": "http://example.com", "lfs.pushurl": "http://examplepush.com", "lfs.http://example.com.access": value, @@ -312,7 +312,7 @@ func TestAccessAbsentConfig(t *testing.T) { } func TestSetAccess(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{})) + finder := NewEndpointFinder(TestEnv(map[string]string{})) assert.Equal(t, NoneAccess, finder.AccessFor("http://example.com")) finder.SetAccess("http://example.com", NTLMAccess) @@ -320,7 +320,7 @@ func TestSetAccess(t *testing.T) { } func TestChangeAccess(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "lfs.http://example.com.access": "basic", })) @@ -330,7 +330,7 @@ func TestChangeAccess(t *testing.T) { } func TestDeleteAccessWithNone(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "lfs.http://example.com.access": "basic", })) @@ -340,7 +340,7 @@ func TestDeleteAccessWithNone(t *testing.T) { } func TestDeleteAccessWithEmptyString(t *testing.T) { - finder := NewEndpointFinder(Env(map[string]string{ + finder := NewEndpointFinder(TestEnv(map[string]string{ "lfs.http://example.com.access": "basic", })) diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go index c4e04855..fc3c64c3 100644 --- a/lfsapi/lfsapi.go +++ b/lfsapi/lfsapi.go @@ -52,11 +52,11 @@ type Client struct { func NewClient(osEnv env, gitEnv env) (*Client, error) { if osEnv == nil { - osEnv = make(Env) + osEnv = make(TestEnv) } if gitEnv == nil { - gitEnv = make(Env) + gitEnv = make(TestEnv) } netrc, err := ParseNetrc(osEnv) @@ -136,16 +136,16 @@ type env interface { All() map[string]string } -// basic config.Environment implementation. Only used in tests, or as a zero -// value to NewClient(). -type Env map[string]string +// TestEnv is a basic config.Environment implementation. Only used in tests, or +// as a zero value to NewClient(). +type TestEnv map[string]string -func (e Env) Get(key string) (string, bool) { +func (e TestEnv) Get(key string) (string, bool) { v, ok := e[key] return v, ok } -func (e Env) Int(key string, def int) (val int) { +func (e TestEnv) Int(key string, def int) (val int) { s, _ := e.Get(key) if len(s) == 0 { return def @@ -159,7 +159,7 @@ func (e Env) Int(key string, def int) (val int) { return i } -func (e Env) Bool(key string, def bool) (val bool) { +func (e TestEnv) Bool(key string, def bool) (val bool) { s, _ := e.Get(key) if len(s) == 0 { return def @@ -175,6 +175,6 @@ func (e Env) Bool(key string, def bool) (val bool) { } } -func (e Env) All() map[string]string { +func (e TestEnv) All() map[string]string { return e } diff --git a/lfsapi/proxy_test.go b/lfsapi/proxy_test.go index 3e6c2600..5c8fc2c0 100644 --- a/lfsapi/proxy_test.go +++ b/lfsapi/proxy_test.go @@ -9,9 +9,9 @@ import ( ) func TestProxyFromGitConfig(t *testing.T) { - c, err := NewClient(Env(map[string]string{ + c, err := NewClient(TestEnv(map[string]string{ "HTTPS_PROXY": "https://proxy-from-env:8080", - }), Env(map[string]string{ + }), TestEnv(map[string]string{ "http.proxy": "https://proxy-from-git-config:8080", })) require.Nil(t, err) @@ -25,9 +25,9 @@ func TestProxyFromGitConfig(t *testing.T) { } func TestHttpProxyFromGitConfig(t *testing.T) { - c, err := NewClient(Env(map[string]string{ + c, err := NewClient(TestEnv(map[string]string{ "HTTPS_PROXY": "https://proxy-from-env:8080", - }), Env(map[string]string{ + }), TestEnv(map[string]string{ "http.proxy": "http://proxy-from-git-config:8080", })) require.Nil(t, err) @@ -41,7 +41,7 @@ func TestHttpProxyFromGitConfig(t *testing.T) { } func TestProxyFromEnvironment(t *testing.T) { - c, err := NewClient(Env(map[string]string{ + c, err := NewClient(TestEnv(map[string]string{ "HTTPS_PROXY": "https://proxy-from-env:8080", }), nil) require.Nil(t, err) @@ -66,9 +66,9 @@ func TestProxyIsNil(t *testing.T) { } func TestProxyNoProxy(t *testing.T) { - c, err := NewClient(Env(map[string]string{ + c, err := NewClient(TestEnv(map[string]string{ "NO_PROXY": "some-host", - }), Env(map[string]string{ + }), TestEnv(map[string]string{ "http.proxy": "https://proxy-from-git-config:8080", })) require.Nil(t, err) diff --git a/locking/api_test.go b/locking/api_test.go index 54331cb9..44f6aa7f 100644 --- a/locking/api_test.go +++ b/locking/api_test.go @@ -39,7 +39,7 @@ func TestAPILock(t *testing.T) { })) defer srv.Close() - c, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + c, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.url": srv.URL + "/api", })) require.Nil(t, err) @@ -81,7 +81,7 @@ func TestAPIUnlock(t *testing.T) { })) defer srv.Close() - c, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + c, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.url": srv.URL + "/api", })) require.Nil(t, err) @@ -121,7 +121,7 @@ func TestAPISearch(t *testing.T) { })) defer srv.Close() - c, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + c, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.url": srv.URL + "/api", })) require.Nil(t, err) diff --git a/locking/locks_test.go b/locking/locks_test.go index 52439e36..da9f5fc6 100644 --- a/locking/locks_test.go +++ b/locking/locks_test.go @@ -46,7 +46,7 @@ func TestRefreshCache(t *testing.T) { srv.Close() }() - lfsclient, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + lfsclient, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.url": srv.URL + "/api", "user.name": "Fred", "user.email": "fred@bloggs.com", diff --git a/tq/api_test.go b/tq/api_test.go index bd6b5272..1259c0ed 100644 --- a/tq/api_test.go +++ b/tq/api_test.go @@ -38,7 +38,7 @@ func TestAPIBatch(t *testing.T) { })) defer srv.Close() - c, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + c, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.url": srv.URL + "/api", })) require.Nil(t, err) @@ -84,7 +84,7 @@ func TestAPIBatchOnlyBasic(t *testing.T) { })) defer srv.Close() - c, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + c, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.url": srv.URL + "/api", })) require.Nil(t, err) diff --git a/tq/custom_test.go b/tq/custom_test.go index 8c90febf..f07bcaa8 100644 --- a/tq/custom_test.go +++ b/tq/custom_test.go @@ -10,7 +10,7 @@ import ( func TestCustomTransferBasicConfig(t *testing.T) { path := "/path/to/binary" - cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + cli, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.customtransfer.testsimple.path": path, })) require.Nil(t, err) @@ -36,7 +36,7 @@ func TestCustomTransferBasicConfig(t *testing.T) { func TestCustomTransferDownloadConfig(t *testing.T) { path := "/path/to/binary" args := "-c 1 --whatever" - cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + cli, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.customtransfer.testdownload.path": path, "lfs.customtransfer.testdownload.args": args, "lfs.customtransfer.testdownload.concurrent": "false", @@ -62,7 +62,7 @@ func TestCustomTransferDownloadConfig(t *testing.T) { func TestCustomTransferUploadConfig(t *testing.T) { path := "/path/to/binary" args := "-c 1 --whatever" - cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + cli, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.customtransfer.testupload.path": path, "lfs.customtransfer.testupload.args": args, "lfs.customtransfer.testupload.concurrent": "false", @@ -88,7 +88,7 @@ func TestCustomTransferUploadConfig(t *testing.T) { func TestCustomTransferBothConfig(t *testing.T) { path := "/path/to/binary" args := "-c 1 --whatever --yeah" - cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + cli, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.customtransfer.testboth.path": path, "lfs.customtransfer.testboth.args": args, "lfs.customtransfer.testboth.concurrent": "yes", diff --git a/tq/manifest_test.go b/tq/manifest_test.go index 63d1129a..94ce3f81 100644 --- a/tq/manifest_test.go +++ b/tq/manifest_test.go @@ -9,7 +9,7 @@ import ( ) func TestManifestIsConfigurable(t *testing.T) { - cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + cli, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.transfer.maxretries": "3", })) require.Nil(t, err) @@ -19,7 +19,7 @@ func TestManifestIsConfigurable(t *testing.T) { } func TestManifestChecksNTLM(t *testing.T) { - cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + cli, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.url": "http://foo", "lfs.http://foo.access": "ntlm", "lfs.concurrenttransfers": "3", @@ -31,7 +31,7 @@ func TestManifestChecksNTLM(t *testing.T) { } func TestManifestClampsValidValues(t *testing.T) { - cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + cli, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.transfer.maxretries": "-1", })) require.Nil(t, err) @@ -41,7 +41,7 @@ func TestManifestClampsValidValues(t *testing.T) { } func TestManifestIgnoresNonInts(t *testing.T) { - cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + cli, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.transfer.maxretries": "not_an_int", })) require.Nil(t, err) diff --git a/tq/transfer_test.go b/tq/transfer_test.go index 6a8c4641..c13a1ad5 100644 --- a/tq/transfer_test.go +++ b/tq/transfer_test.go @@ -118,7 +118,7 @@ func testAdapterRegAndOverride(t *testing.T) { } func testAdapterRegButBasicOnly(t *testing.T) { - cli, err := lfsapi.NewClient(nil, lfsapi.Env(map[string]string{ + cli, err := lfsapi.NewClient(nil, lfsapi.TestEnv(map[string]string{ "lfs.basictransfersonly": "yes", })) require.Nil(t, err) From f7afd4f8b5c2348d42d2aa53bd7d0dbd16d3c6ee Mon Sep 17 00:00:00 2001 From: risk danger olson Date: Fri, 6 Jan 2017 11:37:31 -0700 Subject: [PATCH 24/24] lfsapi: Export Env interface since `(*Client) GitEnv()` needs to be public --- lfsapi/certs.go | 2 +- lfsapi/endpoint_finder.go | 8 ++++---- lfsapi/lfsapi.go | 14 ++++++++------ lfsapi/netrc.go | 2 +- lfsapi/proxy.go | 2 +- 5 files changed, 15 insertions(+), 13 deletions(-) diff --git a/lfsapi/certs.go b/lfsapi/certs.go index 5bb3b71a..a7e9ef3f 100644 --- a/lfsapi/certs.go +++ b/lfsapi/certs.go @@ -35,7 +35,7 @@ func getRootCAsForHost(c *Client, host string) *x509.CertPool { return appendRootCAsForHostFromPlatform(pool, host) } -func appendRootCAsForHostFromGitconfig(osEnv env, gitEnv env, pool *x509.CertPool, host string) *x509.CertPool { +func appendRootCAsForHostFromGitconfig(osEnv Env, gitEnv Env, pool *x509.CertPool, host string) *x509.CertPool { // Accumulate certs from all these locations: // GIT_SSL_CAINFO first diff --git a/lfsapi/endpoint_finder.go b/lfsapi/endpoint_finder.go index 37fe039a..1022f763 100644 --- a/lfsapi/endpoint_finder.go +++ b/lfsapi/endpoint_finder.go @@ -36,7 +36,7 @@ type EndpointFinder interface { } type endpointGitFinder struct { - git env + git Env gitProtocol string aliasMu sync.Mutex @@ -46,7 +46,7 @@ type endpointGitFinder struct { urlAccess map[string]Access } -func NewEndpointFinder(git env) EndpointFinder { +func NewEndpointFinder(git Env) EndpointFinder { e := &endpointGitFinder{ gitProtocol: "https", aliases: make(map[string]string), @@ -229,7 +229,7 @@ func urlWithoutAuth(rawurl string) string { return u.String() } -func fetchGitAccess(git env, key string) Access { +func fetchGitAccess(git Env, key string) Access { if v, _ := git.Get(key); len(v) > 0 { access := Access(strings.ToLower(v)) if access == PrivateAccess { @@ -269,7 +269,7 @@ func (e *endpointGitFinder) ReplaceUrlAlias(rawurl string) string { return rawurl } -func initAliases(e *endpointGitFinder, git env) { +func initAliases(e *endpointGitFinder, git Env) { prefix := "url." suffix := ".insteadof" for gitkey, gitval := range git.All() { diff --git a/lfsapi/lfsapi.go b/lfsapi/lfsapi.go index fc3c64c3..96468200 100644 --- a/lfsapi/lfsapi.go +++ b/lfsapi/lfsapi.go @@ -46,11 +46,11 @@ type Client struct { transferMu sync.Mutex // only used for per-host ssl certs - gitEnv env - osEnv env + gitEnv Env + osEnv Env } -func NewClient(osEnv env, gitEnv env) (*Client, error) { +func NewClient(osEnv Env, gitEnv Env) (*Client, error) { if osEnv == nil { osEnv = make(TestEnv) } @@ -90,11 +90,11 @@ func NewClient(osEnv env, gitEnv env) (*Client, error) { return c, nil } -func (c *Client) GitEnv() env { +func (c *Client) GitEnv() Env { return c.gitEnv } -func (c *Client) OSEnv() env { +func (c *Client) OSEnv() Env { return c.osEnv } @@ -129,7 +129,9 @@ func DecodeJSON(res *http.Response, obj interface{}) error { return nil } -type env interface { +// Env is an interface for the config.Environment methods that this package +// relies on. +type Env interface { Get(string) (string, bool) Int(string, int) int Bool(string, bool) bool diff --git a/lfsapi/netrc.go b/lfsapi/netrc.go index cb7700d4..d02bc257 100644 --- a/lfsapi/netrc.go +++ b/lfsapi/netrc.go @@ -11,7 +11,7 @@ type NetrcFinder interface { FindMachine(string) *netrc.Machine } -func ParseNetrc(osEnv env) (NetrcFinder, error) { +func ParseNetrc(osEnv Env) (NetrcFinder, error) { home, _ := osEnv.Get("HOME") if len(home) == 0 { return &noFinder{}, nil diff --git a/lfsapi/proxy.go b/lfsapi/proxy.go index 50187321..80355ab4 100644 --- a/lfsapi/proxy.go +++ b/lfsapi/proxy.go @@ -48,7 +48,7 @@ func ProxyFromClient(c *Client) func(req *http.Request) (*url.URL, error) { } } -func getProxyServers(osEnv env, gitEnv env) (string, string, string) { +func getProxyServers(osEnv Env, gitEnv Env) (string, string, string) { var httpsProxy string httpProxy, _ := gitEnv.Get("http.proxy") if strings.HasPrefix(httpProxy, "https://") {