Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transfer manifest #1430

Merged
merged 5 commits into from Aug 10, 2016
Merged

Transfer manifest #1430

merged 5 commits into from Aug 10, 2016

Conversation

technoweenie
Copy link
Contributor

This extracts the configured transfer adapters to an explicit Manifest type, instead of storing them on the go package itself. We get a few benefits here:

  • Tests operate on immutable *config.Configuration and *transfer.Manifest objects. No resetting global state.
  • The transfer configs are only loaded as needed.

I made sure that a manifest is loaded at most once per git-lfs command.

@@ -24,12 +17,6 @@ const (
// name and dir are to provide context if one func implements many instances
type NewTransferAdapterFunc func(name string, dir Direction) TransferAdapter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to update the comments in this area to point people at the new Manifest type to find registration functions etc

@sinbad
Copy link
Contributor

sinbad commented Aug 10, 2016

👍 Great work. A couple of places where the manifest is re-created when I think it should be passed in if that's the principle we're going with; otherwise the manifest that is created in the command may be different to the one being used in for example the transfer queue. Doesn't affect anything now because manifests are only created as default from config so they match but could cause issues in future.

@technoweenie
Copy link
Contributor Author

Thanks for pointing out my sloppy test and doc typos. I was having a hell of a time getting those custom tests to pass, and just forgot to re-enable them.

I'm going to leave the lfs.Environ() and TransferQueue changes to future refactoring PRs that will remove a reliance on config.Config.

@ttaylorr
Copy link
Contributor

👍 indeed!

@technoweenie technoweenie mentioned this pull request Aug 10, 2016
@ttaylorr ttaylorr merged commit e508fb6 into master Aug 10, 2016
@ttaylorr ttaylorr deleted the transfer-manifest branch August 10, 2016 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants