Skip to content

gitbase/repository_pool: change repository to be an interface #384

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

Merged
merged 3 commits into from
Jul 12, 2018
Merged

gitbase/repository_pool: change repository to be an interface #384

merged 3 commits into from
Jul 12, 2018

Conversation

jfontan
Copy link
Contributor

@jfontan jfontan commented Jul 11, 2018

Also add billyRepo for billy filesystems.

I'm not totally happy with the naming of repository interface as there's already a Repository structure and all the gitRepository/gitRepo for struct and constructor. Suggestions welcome.

@erizocosmico
Copy link
Contributor

Why do we need to add billy repositories to gitbase? IMHO, testing some more filesystem errors might not be worth introducing all this exclusively for testing.

@jfontan
Copy link
Contributor Author

jfontan commented Jul 11, 2018

@erizocosmico moved billyRepository to the test file (fs_error_test.go)

fs_error_test.go Outdated
}

var brokenFS billy.Filesystem
if brokenType == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

what's 0? do we need a const for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means no broken flags, I've just added a constant.

@erizocosmico
Copy link
Contributor

What I mean is: do we really benefit from making Repository an interface and the complexity this comes with just to test some FS errors?

@jfontan
Copy link
Contributor Author

jfontan commented Jul 12, 2018

Even if we don't add more types of repositories and move to other ways to test errors (maybe fixtures with broken packfiles) I still think it makes sense. It puts the different logic for siva and git repos in one place instead of different code paths in other parts:

https://github.com/src-d/gitbase/pull/384/files#diff-b7e4ef15e429a4c21afc4a8822b635beL129

jfontan added 3 commits July 12, 2018 12:14
Also add billyRepo for billy filesystems

Signed-off-by: Javi Fontan <jfontan@gmail.com>
Signed-off-by: Javi Fontan <jfontan@gmail.com>
Also delete leftover from previous implementation

Signed-off-by: Javi Fontan <jfontan@gmail.com>
Copy link
Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

@jfontan rebase and LGTM

@jfontan
Copy link
Contributor Author

jfontan commented Jul 12, 2018

@ajnavarro I've just rebased

@ajnavarro ajnavarro merged commit 824075c into src-d:master Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants