jj-vcs
Commit signing backend implementation
This is an expansion on the work that was started by @necauqua in #2728 with the intent of getting the core implementation ready to land.
This involved some small behavioural tweaks such as adding config to control whether or not commit signatures are verified and display by default. This also involved a lot of massaging to get the test suite green again.
One significant addition to the original work is the addition of an SSH signing backend which works as closely as possible to the git SSH signing workflow.
Checklist
If applicable:
- [x] I have updated
CHANGELOG.md - [x] I have updated the documentation (README.md, docs/, demos/)
- [x] I have updated the config schema (cli/src/config-schema.json)
- [x] I have added tests to cover my changes
Wow it was an absolute nightmare to get the tests passing on Windows. I even have access to a windows machine and this was like pulling teeth.
So glad that's actually working now.
Surely nix can't be as bad. Right?
@martinvonz I think this PR should be ready for an initial review. I've managed to get all the tests passing and it seems to function well in my personal use of it.
Would be great if I can get some eyes on it to see if I am missing anything or any changes are still required.
Let me know!
@martinvonz I think this PR should be ready for an initial review. I've managed to get all the tests passing and it seems to function well in my personal use of it.
Thank you! I'll try to review at least the first part (the gpg support) later tonight.
@martinvonz @yuja Ok I think I have addressed everything (except for https://github.com/martinvonz/jj/pull/3007#discussion_r1489345364 which I'm not really sure is actionable).
Ready for round 2 :)
This PR has gotten really big. It's hard for me to find time to review it. I think it would have been better to get the gpg signing in as one PR and then add the ssh signing in a later PR. That way you can at least get some useful functionality in a shorter time. Maybe it's too late to split this up now that Yuya has already reviewed much of it, so this is probably more of a suggestion for future PRs.
@martinvonz Yea I have been regretting bundling in those changes with this PR. I can split those patches out, they are more or less self contained - but as you say there is so much context in this PR now that it's probably a bad idea.
Sorry about this.
Yeeeah, I don't think I mentioned it loud enough, but I was initially structuring the 4 or so commits in the original PR to be isolated enough to be merged separately, and then I just dumped them all in one PR just for it to be there for someone to pick up (tyvm for your work @julienvincent)
Ok I have split this PR into two parts. Each of these two parts can be further split if necessary.
Part 1
Contains the surrounding changes needed to land commit signing and includes the GPG backend and the SSH backend.
This part just enables signing commits with the existing jj commands. It should be merged first.
This PR now represents part 1.
Part 2
Contains everything for displaying (and verifying) commit signatures. Additionally it includes the sign command which is not necessary to land part 1.
Part 1 can be further spit to move the SSH signing backend into its own PR.
Part 2 can be further split to move the sign command into its own PR.
I have not opened a PR for part 2 yet.
I looked over all the outstanding comments and I think everything that is still unresolved is relating to the commits I have now split out of this PR.
@yuja When you get a moment can you look over the remaining items in this PR. I think what is remaining has mostly already been reviewed.
Thanks for your time!
I'll open up the second PR once this one has landed and will try carry over the outstanding comments.
Vey minor nit is commit message format, would look great to have the "sign: " prefix, I think it's (not too strongly enforced, but still) a convention here
Thanks again for a bunch of extra work you've put in to push this over, great stuff
Ok, everything addressed! Let me know if you need anything more from me.
Anything needed to merge this? I'm not sure what the process is
Once it's approved and you haven't made any non-trivial changes since the last round of reviews, feel free to merge. Thank for all your work on this! And thanks again to @necauqua too!
Ok but I don't have permission to merge. Does someone else need to do it?
Ok but I don't have permission to merge. Does someone else need to do it?
Oops, sorry! Now you do (once you accept the invite).
Oh! Ok awesome. I will merge this now :)
Thank for all your work on this! You are most welcome!
Thanks for all the help getting this ready.
FYI, this adds a dev dependency on gpg. The tests are failing for me locally since I don't have it installed. Might be worth documenting this somewhere if it isn't already (I didn't see it anywhere).
FYI any future readers - this was also noted here #3096 and will be addressed hopefully later today in #3100.