Add sign capability to git_rebase_commit#4913
Conversation
|
I understand the need here and this makes sense. But it's disappointing to break the API. If we move this callback into the |
|
I think that seems reasonable. I'll see what I can do about your request. Thank you for taking a look at this! |
implausible
force-pushed
the
feature/signing-rebase-commits
branch
from
b122c74 to
996578b
Compare
|
Hopefully that looks a little better, @ethomson. |
implausible
force-pushed
the
feature/signing-rebase-commits
branch
2 times, most recently
from
0deafa7 to
b652973
Compare
|
Failure in setup phase due to connection issues: |
implausible
force-pushed
the
feature/signing-rebase-commits
branch
from
b652973 to
b4ead50
Compare
|
I give up! Mingw doesn't want to be downloaded right now. |
implausible
force-pushed
the
feature/signing-rebase-commits
branch
from
b4ead50 to
e665b00
Compare
tiennou
left a comment
tiennou
left a comment
There was a problem hiding this comment.
A quick batch of comments. Nothing required per se, just API-design prodding 😉.
2 callbacks have been added to git_rebase_options, git_rebase_commit_signature_cb and git_rebase_commit_signature_field_cb. When git_rebase_commit_signature_cb is present in git_rebase_options, it will be called whenever git_rebase_commit is performed, giving an opportunity to sign the commit. The signing procedure can be skipped if the callback specifies passthrough as the error. The git_rebase_commit_signature_field_cb will only be called if the other callback is present or did not passthrough, and it provides means to specify which field a signature is for. Git_rebase_options was chosen as the home for these callbacks as it keeps backwards compatibility with the current rebase api.
Reduces the number of callbacks for signing a commit during a rebase operation to just one callback. That callback has 2 out git_buf parameters for signature and signature field. We use git_buf here, because we cannot make any assumptions about the heap allocator a user of the library might be using.
implausible
force-pushed
the
feature/signing-rebase-commits
branch
from
e665b00 to
3a8ef82
Compare
In the case that we want to build merge + commit, cherrypick + commit, or even just build a commit with signing callback, `git_rebase_commit_signature_cb` particular callback should be made more generic. We also renamed `signature_cb` to `signing_cb` to improve clarity on the purpose of the callback (build a difference between a git_signature and the act of signing). So we've ended up with `git_commit_signing_cb`.
|
@tiennou The tests failed due to a connection error. Is there any chance you can restart the offending suite? |
|
I requeued the build. Note that you should be able to do so on your own PRs by commenting |
|
Aha, I will use that next time :). |
We should clear the error before calling the signing_cb to allow the signing_cb to set its own errors. If the CB did not provide an error, we should set our own generic error before exiting rebase_commit__create
tiennou
left a comment
tiennou
left a comment
There was a problem hiding this comment.
Sorry for falling off the face of the earth @implausible. I think this is mostly ready, and my point about commit creation should be postponed to its own PR.
pks-t
left a comment
pks-t
left a comment
There was a problem hiding this comment.
Sorry, I didn't really have time to dig into this earlier :(
|
Just to clarify: you didn't yet address the comments from both @tiennou and my reviews, did you? That would be totally fine, I just want to make sure that we do not have mismatching expectations on the current state of this PR. |
|
No, I actually just discovered the comments yesterday while bumping LibGit2 in NodeGit. Now that I'm aware of them I will take a look at them. Thanks! |
If provided with a null signature, skip adding the signature header and create the commit anyway.
This simplifies the flow of rebase_commit__create because it doesn't have to juggle 2 different commit flows (one with signature and one without).
Use ci_git_fail_with where appropriate. Use correct initializer for callback.
|
This looks fine to me, but I'm really not fond of the layering, so I'm not "in love" API-wise… You can see the vision I had on #5116, but I haven't had the time to finish it (and I think it'd work better with some kind of Thus, opinions on the low-level part of #5116 would be ❤️, and then those typedefs can move out to somewhere more generally useful ? |
|
I'm +1 on merging this as-is, the changes to the public API seem reasonable to me. It feels like this is not mutually exclusive to further refactoring the commit creation process and merging this wouldn't really change our progress there. @tiennou you suggested pulling out some typedefs - which ones were you thinking? Would the public API here change based on that? |
|
Gah, sorry, tapped the close button accidentally. 🙄 |
tiennou
left a comment
tiennou
left a comment
There was a problem hiding this comment.
It feels like this is not mutually exclusive to further refactoring the commit creation process and merging this wouldn't really change our progress there. @tiennou you suggested pulling out some typedefs - which ones were you thinking? Would the public API here change based on that?
I've re-reviewed, and I think it's true, the API would just hide the signing-callback dance as the last part of the commit-building process. So apart from having to (maybe) deprecate the other systems that create commits, this one would already be covered… mostly: it all depends on how customizable you want to be — this would still miss support for arbitrary commit headers that nobody asked for.
pks-t
left a comment
pks-t
left a comment
There was a problem hiding this comment.
Let's merge it, then! Thanks to all of you, and especially to @implausible!
|
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 |
|
Thanks!! |
4 participants
This PR aims to add the ability to sign a rebase commit using the git_rebase_commit API. It seemed most convenient to extend the method to take a signature callback and signature_field callback, which help provide the signature data for
git_commit_create_with_signatureAPI.The general flow is that if you have options and you have a
signature_cb, thengit_rebase_commitwill call thatsignature_cbwith the commit content buffer retrieved fromgit_commit_create_buffer, providing an opportunity to sign the commit content. The owner of thesignature_cbis expected to create a string, though if GIT_PASSTHROUGH is specified, the signing process will fallback to creating an unsigned commit. If a signature is provided from thesignature_cb, thesignature_field_cbwill be called if it is provided. We will then create the commit usinggit_commit_create_from_signature.If
NULLoptions are passed, the commit is created using the previously implemented flow.I have added additional tests to confirm that signing works in git_rebase_commit and fixed any tests that would've broken from a change in API.