libgit2 / pygit2

Python bindings for libgit2
https://www.pygit2.org/
Other
1.58k stars 382 forks source link

feat: add commit signing #1142

Closed kuwv closed 2 years ago

kuwv commented 2 years ago

Overview

This pull request will add commit signing capability to the repository object. It is modeled off both the libgit2 and rugged implementations. It will allow integration with a 3rd party GPG signing tool.

References

https://github.com/libgit2/pygit2/issues/647 https://github.com/libgit2/rugged/blob/master/ext/rugged/rugged_commit.c#L791 https://github.com/libgit2/rugged/blob/master/ext/rugged/rugged_commit.c#L842 https://github.com/libgit2/pygit2/blob/master/test/test_commit_gpg.py https://github.com/libgit2/libgit2/issues/6293

kuwv commented 2 years ago

@jdavid I am currently writing the tests for this but I wanted to get this in front of you first.

jdavid commented 2 years ago

Thanks @kuwv, looks good. There is some code duplication, but it doesn't look easy to avoid, so it's ok. Only waiting for the tests then.

kuwv commented 2 years ago

Thanks @kuwv, looks good. There is some code duplication, but it doesn't look easy to avoid, so it's ok. Only waiting for the tests then.

Yeah, I guess two functions could potentially be combined later but I didn't want to introduce any breaking changes to the API. So, I choose to model Rugged and git2-rs.

One question, together neither function updates the HEAD reference. The Rugged implementation also does not. Should I add the reference update to commit_with_signature or should the API consumer do it?

Edit: Actually, I just remembered. I think I'd have to import an additional lib to perform the head update. I'll update the documentation for now instead.

jdavid commented 2 years ago

Please use gpgsigned.zip and remove gpgsigned.git

In my latest commit b472bdaff I've moved to use .zip files only, because with libgit2 1.4.3 there were ownership issues with .tar and raw .git data. I'm surprised that the tests pass for aarch64, but even then I think it's better to only use zip files.

kuwv commented 2 years ago

Please use gpgsigned.zip and remove gpgsigned.git

sure

I've moved to use .zip files only...

Yup, I saw that. Pardon my cruft, just needed to check.

Aside from that should I be worried about the appveyor tests?

kuwv commented 2 years ago

Alright, should be good now. Any additional changes you would like to see here?

Maybe shorten create_commit_with_signature to create_commit_signature or add_commit_signature?

jdavid commented 2 years ago

Aside from that should I be worried about the appveyor tests?

No, the appveyor tests are failing because of the upgrade to libgit2 v1.4.3, so it's unrelated.

Alright, should be good now. Any additional changes you would like to see here?

Only one last thing: can you rebase and squash? At least commits e0880ce8739cb2f18fd96c36b2d387be009f5590 and c8e55097456c0c59cfd8eb619e4e6db8a6657422 should be squashed so we don't to keep .gpgsigned.git in history, which would add weight to the repo.

Maybe shorten create_commit_with_signature to create_commit_signature or add_commit_signature?

No, I think the current name is fine, we often follow libgit2's naming.

kuwv commented 2 years ago

@jdavid Should be good now.

jdavid commented 2 years ago

Thanks!