-
Notifications
You must be signed in to change notification settings - Fork 146
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
Adds GPG signing and verification #90
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
CLA signed. |
CLAs look good, thanks! |
@pittma Thanks for taking this on! It will take me some time to go through the full change, but I plan to give you feedback by the end of the week. |
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 for the delay getting back to you.
I added some minor comments, but overall I think this looks very good.
Thanks so much for putting in the time and effort to bring this together. I think that this will be a great addition to the tool.
Just let me know when you are happy with the state of the PR and want me to take another look.
review/gpg/signable.go
Outdated
|
||
// Signature is `Sig`'s implementation of `Signable`. Through this function, an | ||
// object which needs to implement `Signable` need only embed `Sig` | ||
// anonymously. See, e.g., review/equest.go. |
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.
Minor typo here: This should say "review/request.go" instead of "review/equest.go"
// Copy its contents. | ||
sig := *sigPtr | ||
// Overwrite the value with the placeholder. | ||
*sigPtr = placeholder |
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 think maybe we should add a defer statement here to set the value of *sigPtr
back to sig
prior to the method returning.
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.
Whew. Seriously good point 😄.
repository/repo.go
Outdated
|
||
// MergeNotesAndArchive merges the notes and archives from `remote` into | ||
// the local branch. | ||
MergeNotesAndArchive(remote, notesRefPattern, |
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 wonder if we should split this in to two separate methods; MergeNotes
and MergeArchives
. The reason is that we will soon want to add something like MergeForks
, and maybe even MergeKeys
, and we don't want to have to combine all of those together into a single method name.
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.
Overall this looks really good. Thanks for putting this together, I'm really excited about this feature.
After going through the code more deeply, I have found a few more issues that I want to address before we merge this. Sorry that I didn't catch these earlier.
To reiterate, though; overall this looks very good. Thanks!
repository/git.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
rvws, err := repo.runGitCommand("ls-tree", "-r", "--name-only", hash) |
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 tried this command and it shows the object names with slashes (git-notes can be divided into subdirectories with the path parts taken from the SHA1 hash of the noted object).
I think we need to remove the slashes from all of the strings in the slice before returning.
repository/git.go
Outdated
return []string{}, nil | ||
} | ||
|
||
newReviews, err := repo.runGitCommand("diff", "--name-only", beforeHash, |
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 really like this approach for finding what notes have changed, but I think we also need to drop any slashes from the result.
// This is accomplished by determining which files in the notes tree have | ||
// changed because the _names_ of these files correspond to the revisions they | ||
// point to. | ||
func (repo *GitRepo) FetchAndReturnNewReviewHashes(remote, notesRefPattern, |
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 that I didn't catch this before, but I think we are missing one scenario here.
Specifically, I think this will not return reviews which previously existed, but for which there are new comments.
I.E. the new review requests are accounted for, but not the new review comments.
I think the only change that is needed to account for that is to duplicate the ls-tree
or diff
command for the 'refs/notes//devtools/discuss' ref, and then merge that list with the list for the review requests.
repository/repo.go
Outdated
@@ -117,10 +121,10 @@ type Repo interface { | |||
// current ref should only move forward, as opposed to creating a bubble merge. | |||
// The messages argument(s) provide text that should be included in the default | |||
// merge commit message (separated by blank lines). | |||
MergeRef(ref string, fastForward bool, messages ...string) error | |||
MergeRef(ref string, fastForward, sign bool, messages ...string) error |
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 that I didn't notice this earlier, but this (and the RebaseRef
change below) is a change to the public API.
I can't find any external references to this on public GitHub, but not all potential client code lives there.
We should define new methods that either take the sign argument, or unconditionally sign the commits.
review/review.go
Outdated
func (thread *CommentThread) Verify(key string) error { | ||
err := gpg.Verify(key, &thread.Comment) | ||
if err != nil { | ||
return err |
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.
Let's wrap this error with a message explaining which comment could not be verified.
Otherwise, the user will not know how to act on the error message they see.
return err | ||
} | ||
} | ||
return nil |
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 think we need to verify the review request too.
review/gpg/signable.go
Outdated
|
||
// Verify verifies the signatures on the request and its comments with the | ||
// given key. | ||
func Verify(key string, s Signable) error { |
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.
Is "key" required here? I haven't used gpg yet so my knowledge of it is very superficial, but it looks like the --verify
command just takes the document and the signature.
This is important as the key we have is the user's key, but the requests/comments will be signed by other users.
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.
Thanks so much for putting this together and for your rapid iterations on feedback.
Overall this looks great.
I do have one more small thing that I did not catch before (sorry about that), but after that this should be good to go.
Thanks again for all of your help improving the tool and building such a valuable feature!
review/review.go
Outdated
@@ -646,7 +691,7 @@ func (r *Review) AddComment(c comment.Comment) error { | |||
// review will be added to the 'refs/devtools/archives/reviews' ref prior | |||
// to being rewritten. That ensures the review history is kept from being | |||
// garbage collected. | |||
func (r *Review) Rebase(archivePrevious bool) error { | |||
func (r *Review) Rebase(archivePrevious, sign bool) error { |
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.
Can you please add a RebaseAndSign method here similarly to how you added for the Repo interface?
That will both prevent backwards-incompatible changes, and also keep the Review API consistent with the Repo API.
The current method body could just be a helper that both Rebase
and RebaseAndSign
invoke.
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.
On it!
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.
Thanks for putting this together; this new feature is a huge improvement for the tool!
Addresses #89
Hey there!
Here's my work so far w.r.t. implementing signing and verification. As it stands now, signing is available for both requests and acceptances, and we can verify those at
pull
time. I wanted to share now to see if there were any major contentions with the path I've gone down before I went through the rote repetition of adding signing functionality to the other comment-based commands.Cheers!