storage/dotgit: add KeepDescriptors option #942
Conversation
jfontan
changed the title
options.go
Outdated
| // performed. | ||
| type PlainOpenOptions struct { | ||
| // Storage layer options. |
There was a problem hiding this comment.
Maybe we should keep this out of Plain* methods and avoid adding Close to general interface. Just adding it to the filesystem implementation would be enough, and this option would be available only by creating the filesystem storage directly.
This way we avoid redundancy in other implementatons or end up exposing a lot of storage-specific options here.
jfontan
force-pushed
the
improvement/maintain-packfiles-open
branch
2 times, most recently
from
fb41588 to
b43e2a0
Compare
jfontan
changed the title
jfontan
force-pushed
the
improvement/maintain-packfiles-open
branch
from
b43e2a0 to
2dfcef2
Compare
plumbing/storer/storer.go
Outdated
|
|
||
| // Options holds configuration for the storage. | ||
| type Options struct { | ||
| // ExclusiveAccess means that the filesystem is not modified externally |
There was a problem hiding this comment.
Since this is options strictly related to a one implementation I don't believe this is the right place to put this options, this options should be pass and used only to the file system implementation
storage/filesystem/dotgit/dotgit.go
Outdated
| @@ -60,18 +61,35 @@ var ( | |||
| // The DotGit type represents a local git repository on disk. This | |||
| // type is not zero-value-safe, use the New function to initialize it. | |||
| type DotGit struct { | |||
| fs billy.Filesystem | |||
| Options storer.Options | |||
There was a problem hiding this comment.
This should be private, if since can't be changed
storage/filesystem/object.go
Outdated
| @@ -268,7 +269,9 @@ func (s *ObjectStorage) getFromPackfile(h plumbing.Hash, canBeDelta bool) ( | |||
| return nil, err | |||
| } | |||
|
|
|||
| defer ioutil.CheckClose(f, &err) | |||
| if !s.dir.Options.KeepDescriptors { | |||
There was a problem hiding this comment.
Instead of requesting the options to dogGit, should be accessed from a local pointer to the storer.
| @@ -411,6 +414,11 @@ func (s *ObjectStorage) buildPackfileIters(t plumbing.ObjectType, seen map[plumb | |||
| }, nil | |||
| } | |||
|
|
|||
| // Close closes all opened files. | |||
| func (s *ObjectStorage) Close() error { | |||
There was a problem hiding this comment.
This is not covered by a test
storage/filesystem/dotgit/dotgit.go
Outdated
| d.objectList = nil | ||
| } | ||
|
|
||
| func (d *DotGit) genObjectList() error { |
There was a problem hiding this comment.
This function is not covered by a test
There was a problem hiding this comment.
By default go coverage only annotates files from the same package tested. The test that exercise this part belongs to storage/filesystem. That's why it does not appear in coverage. The same happens with other functions. I'll add new tests in storage/filesystem/dotgit that forces the usage of those functions.
storage/filesystem/dotgit/dotgit.go
Outdated
| return d.forEachObjectHash(fun) | ||
| } | ||
|
|
||
| err := d.genObjectList() |
There was a problem hiding this comment.
This code isn't covered either.
jfontan
changed the title
This option maintains packfile file descriptors opened after reading objects from them. It improves performance as it does not have to be opening packfiles each time an object is needed. Also adds Close to EncodedObjectStorer to close all the files manualy. Signed-off-by: Javi Fontan <[email protected]>
jfontan
force-pushed
the
improvement/maintain-packfiles-open
branch
from
2dfcef2 to
6384ab9
Compare
jfontan
changed the title
plumbing/storer/object.go
Outdated
| @@ -40,6 +40,8 @@ type EncodedObjectStorer interface { | |||
| // HasEncodedObject returns ErrObjNotFound if the object doesn't | |||
| // exist. If the object does exist, it returns nil. | |||
| HasEncodedObject(plumbing.Hash) error | |||
| // Close any opened files. | |||
| Close() error | |||
There was a problem hiding this comment.
What about having Close() only on the filesystem implementation, not on the general interface?
There was a problem hiding this comment.
Somehow I understood that you didn't want to export Close in git.Repository. It's now deleted from EncodedObjectStorer.
Signed-off-by: Javi Fontan <[email protected]>
plumbing/storer/object_test.go
Outdated
| @@ -157,3 +157,7 @@ func (o *MockObjectStorage) IterEncodedObjects(t plumbing.ObjectType) (EncodedOb | |||
| func (o *MockObjectStorage) Begin() Transaction { | |||
| return nil | |||
| } | |||
|
|
|||
| func (o *MockObjectStorage) Close() error { | |||
There was a problem hiding this comment.
You're right. Deleted that and Close in memory storer. Also added a new test for filesystem storer and KeepDescriptors option.
Also delete Close from MockObjectStorage and memory storer. Signed-off-by: Javi Fontan <[email protected]>
|
@jfontan AppVeyor is failing with OOM when executing |
|
It is indeed the test. The comparison is generating diffs of the packfiles instead of checking if they are the same. Working on a fix. |
jfontan
force-pushed
the
improvement/maintain-packfiles-open
branch
2 times, most recently
from
1865ade to
c0cacb2
Compare
Using equals to compare files it uses diff to do so. This can potentially consume lots of ram. Changed the comparison to use file offsets. If the descriptor is reused the offset is maintained. Signed-off-by: Javi Fontan <[email protected]>
jfontan
force-pushed
the
improvement/maintain-packfiles-open
branch
from
c0cacb2 to
8176f08
Compare
|
@smola it seems the bug in windows tests is fixed. |
Needs #941
This option maintains packfile file descriptors opened after reading objects from them. It improves performance as it does not have to be opening packfiles each time an object is needed.
Also adds
ClosetoEncodedObjectStorerto close all the files manually.