Add support for creating signed commits and signing commits during a rebase #626
Loading…
Reference in New Issue
No description provided.
Delete Branch "master"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This all works but I have a few questions about submitting it:
git_commit_create_with_signatureis in the 'repository' block of functions in the documentation, but practically it's very difficult to use like that (you would need to look up the commit you were using to get the commit content anyway) so I added it to theCommitstruct instead. The other sensible way would possibly be to add it as a method on theRepositorystruct, make it take a commit ID, and make it looks up the commit to read the content, but then it's just a very roundabout way of doing itRebaseOptions.fromCdoes not load the function pointer forgit_commit_signing_cb(there is a note about it), but I don't know if that use case would ever come up. If it was, there would need to be another bridge function in C that called that function pointer.git_commit_create_with_signatureand I can't find any examples, so what I did in the tests (and what I would probably do if I was using this in the real world) is to create the signed commit, then just re-point head at that signed commit.WithSignatureUsingto theCommitto allow passing a callback to create the signature for the commit, this isn't a 1:1 mapping with the libgit2 api obviously but I figure it's what 99% of people using this library will write themselves anywayIt looks like the tests are failing because of the module dependencies, which I suppose is why there were no dependencies there already. 1.9 hasn't had any releases for over 2 years and 1.10 hasn't had a release for a year and a half, is there any reason they're still in the CI? If they do have to be supported then I'm open to suggestions on how to write tests for this functionality without adding any dependencies
I'll try to review this later today, but here are the answers to most questions:
Once this lands, we can cherry-pick it to the older branches so that you don't need to maintain a fork.
That's totally fine.
huh, https://libgit2.org/libgit2/#HEAD/group/commit/git_commit_create_with_signature seems to indicate that it is in the 'commit' block. and it's defined in https://github.com/libgit2/libgit2/blob/HEAD/include/git2/commit.h#L489-494
who know what happened, but in any case, based on those two links AND the intended usage, declaring it in
Commitis totally fine.We can ignore that case for the time being. As you mention, it's likely that the case won't ever come up. Even thinking how to exercise this functionality in those circumstances requires some super contrived setup.
TODO: answer this.
yeah, that makes sense. Given that the callback would need to be implemented for the
RebaseOptions, that same callback can be reused for theCommit.Is that fixed with
? I think that it's only the case of telling older versions of Go to also fetch the modules needed for testing.
sorry for the delay
@ -37,3 +37,3 @@make build-libgit2-staticgo get -tags static github.com/${{ github.repository }}/...go get -tags static -t github.com/${{ github.repository }}/...go build -tags static github.com/${{ github.repository }}/...yay! it worked :D
these two also need to be
defer C.free(...)d.@ -43,0 +73,4 @@var csf *C.char = nilif signatureField != "" {csf = C.CString(signatureField)since
C.CString()makes a copy, this needs adefer C.free(unsafe.Pointer(csf))to avoid a memory leak: https://blog.golang.org/cgo#TOC_2.@ -43,0 +85,4 @@defer C.free(unsafe.Pointer(cTotalCommit))defer C.free(unsafe.Pointer(cSignature))ret := C.git_commit_create_with_signature(for consistency with
Amend()would it be possible to call this variablecerr?what happens if the
RebaseOptionsis stored in thepointerHandlesby pointer?For consistency with all the other things stored in the
HandleList, which are stored as pointer.git_buf_grow()internally callsgit__realloc, which callsrealloc(3), which says that "If the new size is larger than the old size, the added memory will not be initialized.". This means that it's not guaranteed that the very last byte in the buffer is going to be a null byte after L107.Would it make sense to call
git_buf_sanitizeto ensure the null byte is there?huh, [
C.CString()] says it does return a NULL-terminated C-compatible string: https://blog.golang.org/cgo#TOC_2.@ -72,0 +98,4 @@defer C.free(cstr)// libgit2 requires the contents of the buffer to be NULL-terminated.// C.CString() guarantees that the returned buffer will bein order to support callback authors being able to set a specific libgit2 error, we could do something like:
@ -72,0 +103,4 @@if int(C.git_buf_set(buf, cstr, clen+1)) != 0 {return errors.New("could not set buffer")}this needs a
C.free(...).this can be removed!
it's probably okay to always ask for the
commitOptsto avoid having two flavors of the utility functions. Tests don't need to have backwards-compatibility guarantees.IIUC this can be simplified to:
might as well use the new function! Same in L250.
given that this is only going to be used once, might as well inline this function
@ -177,3 +298,3 @@// Init rebaserebase, err := repo.InitRebase(nil, nil, onto, nil)rebase, err := repo.InitRebase(nil, nil, onto, opts)if err != nil {maybe add a
as the first line of this function: https://golang.org/pkg/testing/#T.Helper
can this block be kept?
@ -43,0 +85,4 @@defer C.free(unsafe.Pointer(cTotalCommit))defer C.free(unsafe.Pointer(cSignature))ret := C.git_commit_create_with_signature(There isn't really a lot of consistency with other functions, sometimes it's called
code, sometimesret, sometimes,ecode, sometimescerr, etc.@ -43,0 +73,4 @@var csf *C.char = nilif signatureField != "" {csf = C.CString(signatureField)Added
Added
@ -72,0 +98,4 @@defer C.free(cstr)// libgit2 requires the contents of the buffer to be NULL-terminated.// C.CString() guarantees that the returned buffer will beGood idea, added
@ -72,0 +103,4 @@if int(C.git_buf_set(buf, cstr, clen+1)) != 0 {return errors.New("could not set buffer")}Fixed
I did have this doing memset() to null bytes but figured that it was probably done by libgit internally, apparently not...I think I'll just make it do that again to make sure that it has a null byte and to avoid any issues with sizes being incorrect
I suppose the easier thing to do would be to just do clen+1 in the git_buf_set as well then, fixed
Removed
Oops, will accept the suggestion
It's already checked directly above, I don't think it needs to do it again?
Done
@ -177,3 +298,3 @@// Init rebaserebase, err := repo.InitRebase(nil, nil, onto, nil)rebase, err := repo.InitRebase(nil, nil, onto, opts)if err != nil {Added, should make logs a bit nicer
It's used in 2 places, unless you mean you want to to refactor it so both of those invocations are inside the for loop?
Ah yes, it misses the case in which the head commit is not signed, fixed
Fixed this and the other similar ones
just relatively minor things left.
can
gofmtbe run on the change? (i need to see if there's a way to do that automatically)welp, i missed this. it should make the godoc a bit better.
@ -69,14 +71,66 @@ func newRebaseOperationFromC(c *C.git_rebase_operation) *RebaseOperation {return operationOK, second try:
git_buf_clear()will remove whatever is in the buffer, andgit_buf_put()grows the buffer AND adds the NULL byte at the end.ah! did not expand the diff ^^;; it's fine to remove.
this should be unneeded now.
Fixed
@ -69,14 +71,66 @@ func newRebaseOperationFromC(c *C.git_rebase_operation) *RebaseOperation {return operationIt seems like those functions aren't defined in the 'buffer.h' header and are also not specified in the documentation, I tried manually adding the
extern ...but got an undefined reference error. Are they intended only for internal use?Fixed this and also changed the name of the type/field to be more appropriate (based on feedback from other PR)
@ -41,2 +41,4 @@}// RawHeader gets the full raw text of the commit header.func (c *Commit) RawHeader() string {one more thing i forgot, can the public functions have a docstring? most of the contents can be copied from the C docs.
@ -69,14 +71,66 @@ func newRebaseOperationFromC(c *C.git_rebase_operation) *RebaseOperation {return operationargh, yes u_u they are not marked
GIT_EXTERN.but why is it not possible to use
?
git_buf_setresizes the buffer (plus the space for the NULL byte), and adds the trailing NULL byte. That's even independent of whether the original c-string had a NULL pointer or not. (This time around I did test this suggestion locally, so I'm sure this one does definitely work).for consistency with the other entries in this file.
@ -69,14 +71,66 @@ func newRebaseOperationFromC(c *C.git_rebase_operation) *RebaseOperation {return operationI tried this before and although it works in CI (see: github actions on that commit), when running locally I get this error
Fixed
@ -41,2 +41,4 @@}// RawHeader gets the full raw text of the commit header.func (c *Commit) RawHeader() string {Fixed
@ -69,14 +71,66 @@ func newRebaseOperationFromC(c *C.git_rebase_operation) *RebaseOperation {return operationinteresting, that means that we need to have at least one CI configuration where assertions are enabled.
okay, what about
proof of guarantee:
4149493443/src/cmd/cgo/out.go (L1709-L1716)@ -69,14 +71,66 @@ func newRebaseOperationFromC(c *C.git_rebase_operation) *RebaseOperation {return operationFixed (and removed the old wrapper)
Thanks again!