add SignKey gpg entity param to allow easier pgp signing of commits

[anandkumarpatel]

Reopening of #1142

This PR creates a new param for the Commit struct called SignKey.
SignKey is an openpgp.Entity type which is the standard way of reading and validating pgp keys.

When specified the key will be used to sign the commit. This makes it so the user does not have to sign the key externally and duplicate some of the logic the CreateCommit function does.

To maintain backward compatibility, if the Verification.Signature is defined SignKey will have no effect.

This provides similar functionality that go-git has: src-d/go-git@7b6c126

Note from the git docs: https://github.com/octokit/routes/blob/152c0b5d156844e17a1805d0509c9a0b7cfc848f/routes/api.github.com/git.json#L282

To pass a `signature` parameter, you need to first manually create a valid PGP signature, which can be complicated. You may find it easier to [use the command line]

So I wanted to make it less complicated 😄

[gmlewis]

Thank you, @anandkumarpatel.
I've requested some changes, please, when you have time.

[gmlewis]

Please use the getters... commit.GetTree().

[anandkumarpatel]

There are no getters defined for createCommit object

[gmlewis]

Yes, whups, my bad. Thanks, @anandkumarpatel.

[anandkumarpatel]

@gmlewis fixed most of the comments you left. The only issue was with using getters on the createCommit object. they do not exist so I left those as is but used getters on the sub-objects that had them.

I also updated all the short names in the test file and defined them more verbosely.

[gmlewis]

I also updated all the short names in the test file and defined them more verbosely.

Just FYI... the other names were fine. As you'll see in a lot of other tests, we use o for org and r for repo all over the place, and it would be nice to be consistent for these...

But it was just a bit challenging at a quick glance to figure out in the tests that:

...

m

was the commit message. Thanks.

[anandkumarpatel]

@gmlewis ready for round 2 👊 review.

• added nil checks to both functions
• added respective tests for those nil checks
• renamed variable to start lower case
• reverted renaming of test variables to be consistent to the rest of the project (r, o, ...etc)

[gmlewis]

Thank you, @anandkumarpatel!
Just a couple minor tweaks, please, and then I think we are good to go.

[anandkumarpatel]

@gmlewis Just pushed, lmk if there is something else.

[gmlewis]

Thank you for bearing with me, @anandkumarpatel!
I think this is going to turn out nicely. Just a couple more minor tweaks, please.

[anandkumarpatel]

@gmlewis Fixed the issues.

• checked for empty message
• added respective test
• simplified adding message (removed sprintf)

[gmlewis]

Thank you, @anandkumarpatel!
LGTM.

Awaiting second LGTM before merging.

[gmlewis]

friendly ping @gauntface

[gauntface]

Mostly LGTM, only question I had is regarding the expected behavior if you set commit.SigningKey AND commit.Verification.

At the moment the signature will be commit.Verification, is that expected? Would it be better if we returned an error if both are set?

[anandkumarpatel]

@gauntface I would assume if commit.Verification is set then the user wants to use that to sign instead of the commit.SigningKey.
My assumption is that a default SigningKey is always passed and that Verification would only be passed in special cases which should override SigningKey.

Are my assumptions invalid?

[gauntface]

I'll LGTM as is. Thinking on it more, I'm just projecting my general like of enforced input -> behavior regarding the inputs. It may never be an issue.

[gmlewis]

Good discussion. Thanks, @gauntface and @anandkumarpatel !
I like the fact that the comment for SigningKey says Ignored if Verification.Signature is defined. so there shouldn't be much confusion in this regard.
Merging.

[gmlewis]

Thank you, @anandkumarpatel!
Merging.