storage/filesystem: call initialization explicitly, fixes #408 #409
Conversation
|
Fixes #408 |
storage/filesystem/storage_test.go
Outdated
| ) | ||
|
|
||
| func Test(t *testing.T) { TestingT(t) } | ||
|
|
||
| type StorageSuite struct { | ||
| test.BaseStorageSuite | ||
| Dir string |
There was a problem hiding this comment.
Let's make this attribute private rather than public.
storage/filesystem/storage_test.go
Outdated
| @@ -42,3 +45,9 @@ func (s *StorageSuite) TestFilesystem(c *C) { | |||
|
|
|||
| c.Assert(storage.Filesystem(), Equals, fs) | |||
| } | |||
|
|
|||
| func (s *StorageSuite) TestStructureShouldBeCreatedLazily(c *C) { | |||
There was a problem hiding this comment.
The test name is misleading, what Structure?, and we are also not testing that contents are created lazily, just that the dir is created empty.
I suggest a new name for the test: TestNewStorageShouldNotAddAnyContentsToDir.
|
Of course storage_test.go:31: StorageSuite.TestNewStorage is failing now, that was the whole point of the patch :). By the way, the name of that test was not very informative :). |
|
@alcortesm What I meant is, if the behavior has changed, this test should not test if the directories are created, so this test should pass too (removing the Stats line, the entire test because now does not make sense, or whatever) |
|
@ajnavarro yes, I understood you and I think you are right. My comment was just to point out that the name of the test and the things it tested were not aligned. I also agree with you that that whole test makes no sense now. |
|
@alcortesm @ajnavarro Right. I removed |
|
What do you mean by "the structure" and why it should be there? Can you write a test to illustrate exactly what has to be there and what is not really needed until we use it for something? |
Codecov Report
@@ Coverage Diff @@
## master #409 +/- ##
==========================================
- Coverage 77.68% 77.03% -0.65%
==========================================
Files 124 124
Lines 8998 9010 +12
==========================================
- Hits 6990 6941 -49
- Misses 1233 1307 +74
+ Partials 775 762 -13
Continue to review full report at Codecov.
|
filesystem.Storage was initializing the gitdir (creating objects and refs) on NewStorage. But this should be done only on init and clone operations, not on open. Now there is a new interface storer.Initializer that storers can implement if they need any initialization step before init or clone. filesystem.Storage is one of such implementations. git.Init and git.Clone now call to the storer Init() method if it does implement it. Otherwise, it just ignores initialization.
smola
changed the title
|
@mcuadros and me talked about this offline and agreed that this should be done explicitly. That is: a storer needs a way to get called when initialization is being done. The end result is essentially the same: as you see, the test has not changed. |
|
I see, thanks! |
|
|
||
| _, err = fs.Stat("refs/tags") |
There was a problem hiding this comment.
Its only purpose was checking that refs/tags was created before any action. This is no longer the case, so it's enough if we check the other cases.
smola
dismissed
ajnavarro’s stale review
issues addressed; @ajnavarro is on holidays now
filesystem.Storage was initializing the gitdir (creating objects
and refs) on NewStorage. But this should be done only on init and
clone operations, not on open.
Now there is a new interface storer.Initializer that storers can
implement if they need any initialization step before init or clone.
filesystem.Storage is one of such implementations.
git.Init and git.Clone now call to the storer Init() method if it
does implement it. Otherwise, it just ignores initialization.