gitbase: update go-borges and support old siva files #911
Conversation
Supports reading siva files created with borges when using
--format siva --non-rooted
Signed-off-by: Javi Fontan <jfontan@gmail.com>
| @@ -227,7 +228,7 @@ func (c *Server) buildDatabase() error { | |||
|
|
|||
| c.sharedCache = cache.NewObjectLRU(c.CacheSize * cache.MiByte) | |||
|
|
|||
| c.rootLibrary = libraries.New(libraries.Options{}) | |||
| c.rootLibrary = libraries.New(nil) | |||
There was a problem hiding this comment.
This enigmatic nil looks so, so for me (and under the hood you check if it's a nil to create an empty option. Always when I see this nil in params I wonder, shall I pass sth. Or what this nil really means (defaults, nothing, etc.)
I know it's not really a part of this PR, but can we make this interface sth. like:
libraries.New().WithOptions(...)
// and maybe handy helper:
libraries.WithOption(...)it will feel more like context.WithCancel(context.Background())
WDYT?
There was a problem hiding this comment.
The original plain storage used this system so we changed the rest to be more homogeneous. We can open an issue in go-borges to propose changing it. Now is the right moment to do so as we are changing the interface.
There was a problem hiding this comment.
We have to think how to unify options in some way and we could address this also in that process.
We could add a WithOptions or SetOptions to the Library interface but I'm not sure if this is something that should belong to there.
Personally, I don't like chain functions/methods calls. I'd rather add specific constructors NewWithOptions to add custom options if you need to work with no default options.
@jfontan shall we open an issue to discuss it there?
| lib, err := siva.NewLibrary(d.Path, osfs.New(d.Path), sivaOpts) | ||
| if err != nil { | ||
| return err | ||
| lib, err = siva.NewLibrary(d.Path, osfs.New(d.Path), sivaOpts) |
There was a problem hiding this comment.
so maybe we can combine these 2 libs, e.g.:
lib := library.New()
//....
lib = library.WithSiva(lib, sivaOpts)
// or lib = lib.WithSiva(sivaOpts)
// or lib = lib.WithSiva(...).WithOldSiva(...)There was a problem hiding this comment.
Right now there are 4 types of libraries:
plain: holds normal repositoressiva: repositories in siva files, the new formatoldsiva: sivas generated by borges, old formatlibraries: a meta library that can hold any other library.
Here we are generating libraries of different types depending on the options we get and adding it to the "meta library" rootLibrary. We could have added an AddSiva or AddOldsiva to the libraries.Library structure but it's meant to hold any structure that implements the interface borges.Library. I don't think that coupling libraries.Library with the already implemented types of library adds anything.
There was a problem hiding this comment.
This is just my taste, so no worries. In this case I thought we may have WithOptions(...)
and in siva, plain, ... you have have certain type's options, so library would be top level, and all types would define specific options.
| @@ -227,7 +228,7 @@ func (c *Server) buildDatabase() error { | |||
|
|
|||
| c.sharedCache = cache.NewObjectLRU(c.CacheSize * cache.MiByte) | |||
|
|
|||
| c.rootLibrary = libraries.New(libraries.Options{}) | |||
| c.rootLibrary = libraries.New(nil) | |||
There was a problem hiding this comment.
I would try to get rid of this mysterious nil
how about sth. like context interface:
lib := library.New().WithOption(...).WithSiva(...)
//or
lib = library.WithOption(library.New())
lib = library.WithSiva(lib, sivaOpts)WDYT?
Supports reading siva files created with borges when using
make upgradecommand if applicable.