plumbing: format, packfile fix issue #129, related #124, and documentation improvements #130
Conversation
| // returned. | ||
| // | ||
| // If the ObjectStorer implements storer.Transactioner, a transation is created | ||
| // during the Decode execution, if something fails the Rollback is called |
There was a problem hiding this comment.
This comments need improvements.
NewDecoder returns a new Decoder that reads from s and store the objects in o.
This line is OK, but does not clearly explain the purpose and usage of the type, as we will see in the next sentence.
ObjectStorer can be nil, in this case the objects are not stored but Offsets can be call to retrieve the objets offset in the packfile being scan.
This line is confusing for several reasons:
- English has to be improved.
- The documentation of the
Offsetsmethod has to be improved, and it should probably call Decode internally. - The purpose of a
Decoderwithout anObjectStoreris not clear. According to the first line, the main purpose of aDecoderis to store objects from a packfile into anObjectStorer, so what happens when noObjectStoreris available? how is this useful?
If ObjectStorer is nil and the Scanner is not Seekable, ErrNonSeekable is returned.
This is OK, once the previous line is clarified.
If the ObjectStorer implements storer.Transactioner, a transation is created during the Decode execution, if something fails the Rollback is called.
s/transation/transaction/
This is not true, a Rollback is only called when the setObject operation fails on the transaction, but not when other operations fails. See my comments below.
Overall, the purpose of this type is not clear, we should redesign it and clarify its purpose and usage.
We should start by explaining the problem, how the type is expected to be used. Then explain the possible decoders we support (seekable-transactioner, seekable-non-transactioner, ...). As this is very relevant for it use, the cause of a lot of code repetitions and has been the cause of several bugs in the past.
There was a problem hiding this comment.
It will be nice to explain all the possible combinations of ObjectStorers and seekable scanners, from the very beginning, just to make it easier later to document the internal parts.
| } | ||
|
|
||
| // recallByHashNonSeekable read from a object from a Tx, if we are in a | ||
| // transaction, we should read from there, if not we read from the ObjectStorer |
There was a problem hiding this comment.
This sentence needs to be rewritten.
| // returned. | ||
| // | ||
| // If the ObjectStorer implements storer.Transactioner, a transation is created | ||
| // during the Decode execution, if something fails the Rollback is called | ||
| func NewDecoder(s *Scanner, o storer.ObjectStorer) (*Decoder, error) { | ||
| if !s.IsSeekable && o == nil { | ||
| return nil, ErrNonSeekable |
There was a problem hiding this comment.
Improve the name of the error and use a function to clarify the reason behind the if condition.
| if err != plumbing.ErrObjectNotFound { | ||
| return obj, err | ||
| } | ||
|
|
||
| return nil, plumbing.ErrObjectNotFound | ||
|
|
| @@ -1,4 +1,4 @@ | |||
| package packfile | |||
| package packfile_test | |||
There was a problem hiding this comment.
I like public testing, but why?
There was a problem hiding this comment.
It may be nice to begin the test files with constructor of the 4 (or more) kinds of Decoder where are going to need:
- seekable-transactioner
- seekable-non-transactioner
- no-seekable-non-transactioner
- non-seekable-non-transactioner
Just for general clarity and sanity. It will help a lot with the testing, simplicity and coverage.
There was a problem hiding this comment.
Then you will have the same complexity in the other end. Then before use it (in several lines) you need to put the switch (in the better of the cases)
The package is public due to a circular dependency
| }) | ||
| } | ||
|
|
||
| func (s *ReaderSuite) TestDecodeNoSeekableWithNoTxStorer(c *C) { |
| } | ||
|
|
||
| scanner := packfile.NewScanner(reader) | ||
| storage, _ := filesystem.NewStorage(fs.New()) |
There was a problem hiding this comment.
How do you know this storage is not a TxStorer? It should be explicitly asserted in the test. Also the opposite should be asserted in the previous test.
mcuadros
changed the title
|
When do you plan to merge these changes into v4 branch? |
This issue fix issue #129 and is related to #124.
I made a test that covers exactly the problem happening, we have four types of decodings: