-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft: Support GPG signing #691
base: master
Are you sure you want to change the base?
Conversation
8f40793
to
3d5292e
Compare
Thanks for contributing @OdNairy! This looks like a lot of works so thanks for pursuing it. |
} | ||
|
||
NSString* plainToSign = [[NSString alloc] initWithCString:body encoding:NSUTF8StringEncoding]; | ||
NSString* signature = [key signSignature:plainToSign]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasderraugh I would like to trigger a separate discussion regarding UI blocking calls.
I'm focusing on signing commits functionality in current PR. However, even with the partial support I already observe significant UI regression – the main queue gets blocked for a 0.2-0.3 second when password for the gpg key already stored the keychain. It even can be an infinite period of time when GPG Keychain presents screen for GPG Key Password input.
Do you have any UI/UX related ideas on how we can play around this issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasderraugh Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't realize there was an outstanding question. Let me think about what we could do here and I'll get back to you soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasderraugh any updates? 🙂
GitUpKit/Core/GCRepository+Bare.m
Outdated
} | ||
|
||
if (gpgSignature != NULL) { | ||
CALL_LIBGIT2_FUNCTION_GOTO(cleanupBuffer, git_commit_create_with_signature, &oid, self.private, commitBuffer.ptr, gpgSignature, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasderraugh Separate thread regarding git-up/libgit2 fork.
It looks like libgit2 doesn't implement 100% gpg support yet so I have to implement some additional methods in it.
Is there any process for fork updates? Should it be just PR with functionality or I have to implement some tests in libgit2 as well?
Generally, what was the motivation to link against fork instead of upstream? Would it be possible to rebase on top of upstream? Is there a chance that we may migrate to the upstream instead of the fork?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a fork of libgit2 because a few patches were needed with some new APIs, bug fixes and change of behavior. Not all would be accepted in upstream and at the time maintainers could take weeks to respond. You can see the 15 or so custom commits here:
https://github.com/git-up/libgit2/commits/gitup
The README in the fork repo explains how to update to upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swisspol – looks like we standstill with libgit2, maybe suggested override below can help? (i think libgit2 are bit resistant to add features, even signing – they usually say it's more to the clients)
but gosh, I'm missing that in GitUp so much :) hope resolution will be found. Also, please, lmk if we can support somehow?
So great! Can't wait to this being implemented. Thanks 🥇 |
Just started using GitUp this week and wondered where the slew of unsigned commits I had were coming from. The existence of this PR explains a lot 😅 Looks like you've put some serious work into this, @OdNairy, hugely appreciated that you decided too. Are you just waiting to hear back from @lucasderraugh on the UI issue? I'd like to offer my two cents on the topic, if it helps. I use TortoiseGit as my non-terminal interface to Git when I'm on Windows. Combined with Gpg4win, I'm able to sign my commits there. TortoiseGit doesn't actually do the git operations itself, just invokes the standard binary with specific options. That means that when it comes time to commit, I tell this ridiculous story to illustrate that the issue you've run into isn't abnormal, and at least in my case is something I'd absolutely expect. A sub half-second hit while it grabs the key from the agent isn't something I'd even blink at. Further, allowing a reasonable wait (20 secs +/-?) for Cheers |
@nitz I don't have tons of time to review the details of PRs these days. But I'm still willing to help move the project forward. I honestly don't care what the initial UI looks like (UI can easily be iterated on in future releases), but here is the 1 thing I do care about: Adding in support for GPG signing CANNOT regress the performance or workflows for people not using this feature. I'm not sure if @OdNairy is willing to keep working on these changes, I personally don't have a good understanding of gpg signing so I'm deferring to those that do to understand a good approach here. I will absolutely test out the final result before merging in anything, but that's where I stand here. |
Definitely makes sense. That implementation doesn't impact performance for non-gpg users. Only GitUp binary size would be impacted for everybody. |
@OdNairy this PR looks great! As you probably know Git shells out to Have you considered simplifying this, by just supporting what's configured in |
0eb80f0
to
a152b20
Compare
Hi all There are few updates:
I also can confirm that all operations still works as expected. Except the single-commit repository 🤡 . There is something broken with that thing so I probably need help from @lucasderraugh with it. Stay tuned! |
10aa199
to
af7370c
Compare
The System Region leads to false positive test failures in some regions
Supported operations: * Commit * Merge * Cherry-pick
96d073e
to
ac1ce50
Compare
Update: I was able to eliminate almost all extra code from the libgit2 to support GPG sign. It is a single new function PR now 🎉 git-up/libgit2#3. |
I've found a serious issue in my PR. When GitUp overwrite commit of some other author, my implementation still signed it with local key. The git doesn't do it. So I have to implement some filtering logic first. |
@OdNairy hi! Any chances to move this rock forward? 👋 |
Closes: #42
GnuPG Made Easy (gpgme) documentation, source code
Completeness: 85%
Preview binary (use right click - Open to open the app):
GitUp.zip
Checklist (update: 23 Sep, 2022):
@nitz May I ask for your help on it?
Technical debt:
GitUp/GitUpKit/Core/GCCommitDatabase.m
Line 988 in 515ffe8
git_commit
object for tests and GPG status presentationgit_commit_extract_signature
Raw commit data sample
gpg
with pipelines in favor of gpgme dependency (git example)gpgme_op_sign
withgpgme_op_sign_start
+gpgme_wait
for async signature calculation. (doc1, doc2)Useful links:
create_commit_cb
, deprecatesigning_cb
libgit2/libgit2#6016Technical references:
How does
git
sign commits?Callstack:
How does
libgit2
sign commits?Unfortunately, there are no overall sign commit support in libgit2. Also, GitUp uses it's own fork which is quite outdated. The good thing is that it gives us enough flexibility to implement custom signature adding functionality to libgit2 and reuse it in GitUpKit.
Dependencies