Implement Packbuilder-Support #21

2013-05-16T07:17:41-05:00

Merovius commented

2013-05-16 07:17:41 -05:00

(Migrated from github.com)

This basically just wraps the libgit2-functions. There are two exceptions:

I modified the semantics of git_packbuilder_foreach to return a chan []byte. The actual git_packbuilder_foreach call is run in a seperate go-routine. This feels more „go-like“.
I wrapped git_packbuilder_write to (*Packbuilder).WriteToFile, to make the next change possible.

Even more go-like than the channel-approach from above is the Write(w io.Writer)-semantic I added in the second commit. This makes it easy to write the pack out to any writer by wrapping ForEach.
In my opinion this could replace the whole ForEach-/callback mess with a more minimal/clean interface which is as powerful (instead of specifying a callback, the user creates a Writer, which is called with the same semantics).

Also, (_Packbuilder).WriteToFile could be removed (requiring the user to open a file herself and pass the *os.File to (_Packbuilder).Write. This would introduce some C-Call-overhead, I think.

If we do all of the above, we can replace (*Packbuilder).Written by our own implementation, reducing C-Call-overhead (to compensate a little bit for the above introduction of overhead).

Bevor reducing the interface, I wanted to hear your thoughts on all that, if we want to do it (or some of it) we should do it before merging in my opinion.

This basically just wraps the libgit2-functions. There are two exceptions: - I modified the semantics of git_packbuilder_foreach to return a chan []byte. The actual git_packbuilder_foreach call is run in a seperate go-routine. This feels more „go-like“. - I wrapped git_packbuilder_write to (*Packbuilder).WriteToFile, to make the next change possible. Even more go-like than the channel-approach from above is the Write(w io.Writer)-semantic I added in the second commit. This makes it easy to write the pack out to any writer by wrapping ForEach. In my opinion this could replace the whole ForEach-/callback mess with a more minimal/clean interface which is as powerful (instead of specifying a callback, the user creates a Writer, which is called with the same semantics). Also, (_Packbuilder).WriteToFile could be removed (requiring the user to open a file herself and pass the *os.File to (_Packbuilder).Write. This would introduce some C-Call-overhead, I think. If we do all of the above, we can replace (*Packbuilder).Written by our own implementation, reducing C-Call-overhead (to compensate a little bit for the above introduction of overhead). Bevor reducing the interface, I wanted to hear your thoughts on all that, if we want to do it (or some of it) we should do it before merging in my opinion.

technoweenie commented

2013-05-16 11:50:30 -05:00

(Migrated from github.com)

Is there something with Go that I'm missing? I'm getting a compilation error:

./test.go:34: repo.NewPackbuilder undefined (type *git.Repository has no field or method NewPackbuilder)

I'm pretty sure I have the right thing checked out. My GOPATH is /Users/rick/p/go-git-experiment/vendor. I can go to vendor/src/github.com/Merovius/git2go and see that it is on the packbuilder branch. I can even break the import to see that compilation message:

vendor/src/github.com/Merovius/git2go/packbuilder.go:15:2: import "unsafes": cannot find package

I just can't seem to reference anything defined in packbuilder.go.

I threw my test script in https://gist.github.com/technoweenie/5593168

Is there something with Go that I'm missing? I'm getting a compilation error: ``` ./test.go:34: repo.NewPackbuilder undefined (type *git.Repository has no field or method NewPackbuilder) ``` I'm pretty sure I have the right thing checked out. My GOPATH is `/Users/rick/p/go-git-experiment/vendor`. I can go to `vendor/src/github.com/Merovius/git2go` and see that it is on the `packbuilder` branch. I can even break the import to see that compilation message: ``` vendor/src/github.com/Merovius/git2go/packbuilder.go:15:2: import "unsafes": cannot find package ``` I just can't seem to reference anything defined in `packbuilder.go`. I threw my test script in https://gist.github.com/technoweenie/5593168

Merovius commented

2013-05-16 12:32:25 -05:00

(Migrated from github.com)

I don't know how, but it looks like you have introduced a typo in your working copy. In the repo there clearly is no "unsafes" imported ;)

That typo leads to the observed inability to build the packbuilder-branch which will lead to go using the previous build, which will not include it. Try fixing the source (cd /Users/rick/p/go-git-experiment/vendor/src/github.com/Merovius/git2go && git reset --hard should do), then rebuild.

I don't know how, but it looks like you have introduced a typo in your working copy. [In the repo](https://github.com/Merovius/git2go/blob/packbuilder/packbuilder.go#L15) there clearly is no "unsafes" imported ;) That typo leads to the observed inability to build the packbuilder-branch which will lead to go using the previous build, which will not include it. Try fixing the source (`cd /Users/rick/p/go-git-experiment/vendor/src/github.com/Merovius/git2go && git reset --hard` should do), then rebuild.

technoweenie commented

2013-05-16 12:37:08 -05:00

(Migrated from github.com)

I added that on purpose to prove it's running the code from the proper directory :) If I take it out, I get:

./test.go:34: repo.NewPackbuilder undefined (type *git.Repository has no field or method NewPackbuilder)

I added that on purpose to prove it's running the code from the proper directory :) If I take it out, I get: ``` ./test.go:34: repo.NewPackbuilder undefined (type *git.Repository has no field or method NewPackbuilder) ```

Merovius commented

2013-05-16 12:45:03 -05:00

(Migrated from github.com)

Oh sorry, I misread you there.

What if you rm vendor/pkg/github.com/Merovius/git2go.a? I have to admit I have no idea right now, but it works for me…

Oh sorry, I misread you there. What if you `rm vendor/pkg/github.com/Merovius/git2go.a`? I have to admit I have no idea right now, but it works for me…

Merovius commented

2013-05-16 12:53:05 -05:00

(Migrated from github.com)

You can try go build -v -x and look if the paths are right.

You can try `go build -v -x` and look if the paths are right.

technoweenie commented

2013-05-16 13:51:47 -05:00

(Migrated from github.com)

Installing go 1.1 fixed it :feelsgood:

vmg commented

2013-05-16 16:11:08 -05:00

(Migrated from github.com)

I really like the PR as it stands. I don't want to remove WriteToFile because the C implementation is very optimized for atomic writes, and speed is good when it comes to Packfiles.

Also, the ForEach as a chan is pure joy. There are a couple other ForEach in the library which I really want to implement like that too.

I really like the PR as it stands. I don't want to remove `WriteToFile` because the C implementation is very optimized for atomic writes, and speed is good when it comes to Packfiles. Also, the ForEach as a `chan` is pure joy. There are a couple other `ForEach` in the library which I really want to implement like that too.

vmg commented

2013-05-16 16:11:23 -05:00

(Migrated from github.com)

Pure win. Thank you so much!

Sign in to join this conversation.

No reviewers