plumbing/packfile: PACK encoder #131
Conversation
ajnavarro
commented
Nov 22, 2016
ajnavarro
commented
- Added simple PACK encoder, deltas not supported by now
| } | ||
|
|
||
| // Creates a new packfile Encoder using a specific Writer | ||
| func NewEncoder(w io.Writer, storage storer.ObjectStorer) *Encoder { |
There was a problem hiding this comment.
use short variable names to very local variables, in this case s
| func NewEncoder(w io.Writer, storage storer.ObjectStorer) *Encoder { | ||
| h := sha1.New() | ||
| mw := io.MultiWriter(w, h) | ||
| return &Encoder{storage, mw, h} |
There was a problem hiding this comment.
don't use unlabeled fields initializations
| package packfile | ||
|
|
||
| const ( | ||
| // VersionSupported is the packfile version supported by this parser. |
There was a problem hiding this comment.
Now that the package includes a decoder (parser) and a encoder (composer), we should no longer talk about the version supported by the parser, but by the version supported by the package.
| var signature = []byte{'P', 'A', 'C', 'K'} | ||
|
|
||
| const ( | ||
| lengthBits = uint8(7) // subsequent bytes has 7 bits to store the length |
There was a problem hiding this comment.
this comment makes no sense now that it is the first in the const block. Move it or change the comment.
| ) | ||
|
|
||
| const ( | ||
| maskType = uint8(112) // 0111 0000 |
There was a problem hiding this comment.
Why is this const separated from the others (line 14)?
|
|
||
| // Encode encodes objects specified using a list of hashes. This objects must | ||
| // exists into the storer | ||
| func (e *Encoder) Encode(hashes []plumbing.Hash) (plumbing.Hash, error) { |
There was a problem hiding this comment.
From the signature of the method and the comments it is not clear if you can create a single packfile by calling several times in a row to Encode or not.
I would change the comment here to clarify that every time you call Encode it creates a new packfile and write it to the writer. Something like this:
// Encode creates a packfile containing all the objects referenced in hashes and writes it to the writer in the Encoder.
| return err | ||
| } | ||
|
|
||
| zw := zlib.NewWriter(e.w) |
There was a problem hiding this comment.
A nice optimization would be for the encoder to have a permanent zlib.Writer, instead of creating a new one every time an entry is compressed, so we can reuse its dictionary (which is one of the slow parts in zlib and plays badly with the garbage collector). Would it be possible to do this by using zlib.Reset()?
- Added simple PACK encoder, deltas not supported by now
| storage storer.ObjectStorer | ||
| w io.Writer | ||
| zw *zlib.Writer | ||
| hash hash.Hash |
There was a problem hiding this comment.
use our Hasher, https://godoc.org/gopkg.in/src-d/go-git.v4/plumbing#Hasher
| "gopkg.in/src-d/go-git.v4/plumbing/storer" | ||
| "gopkg.in/src-d/go-git.v4/utils/binary" | ||
|
|
||
| "github.com/klauspost/compress/zlib" |
There was a problem hiding this comment.
why? we are using "compress/zlib" in the Scanner
* plumbing/packfile: PACK encoder - Added simple PACK encoder, deltas not supported by now * Requested changes * Requested changes * Requested changes
