-
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
Support pulling the git-notes for reviews from an untrusted repository. #71
Comments
Some things that would be important here:
I think one way of achieving this would be to have a flag specifying the author whose comments should be pulled. If no such author is given, then the user is prompted for each new comment to decide if it should be included or excluded. |
It might be worthwhile to make the work flow be something like "Pull new comments from the user for all reviews", but that may be more tricky than pulling the comments for only a single review, since we have to first figure out which reviews have new comments in the fork. |
My thinking on this issue has evolved recently. My previous suggestion has always felt too cumbersome and limited to me, and now I think I may know a better way to handle this. The core idea is that there should be a graph defining how separate remote repositories are linked together. The This graph should be stored in the repository itself so that third-parties can pull review metadata from multiple forks at once. That allows the network of developers to continue collaborating on changes even if the "authoritative" remote repository stops being updated. A repository owner could add a fork by running a command like To make this work we would need to make sure the following properties hold:
I do not think we need a way to audit incoming requests/comments before adding them, since we now have a way to edit comments without losing the original comment. However, we do need to gracefully handle multiple, unrelated review requests for the same commit. Right now we track all of the requests, but treat the last one as the "current" request with all previous ones being obsolete. In this new model we need to be able to distinguish between multiple, semantically different reviews on the same commit. That is needed so that one forker cannot hide another forker's review request by shadowing it with their own. We might do this by adding a We should probably also add something like a There are a number of things about this approach that I like:
|
Regarding authenticating metadata; I think we should try to imitate the model git uses for GPG signing commits. This would mean adding a When displaying reviews/comments/etc, we would include information about whether or not the metadata was signed and whether or not the signature could be verified. We would also want to add a To sign a metadata entry, we could do something like the following:
Verifying the signature would then involve running that process in reverse. This would mean that signing changes and checking signatures would be an optional feature that each individual community could decide for itself whether or not to adopt. It would also mean that this feature would be orthogonal to the rest of the work described in this issue. I will probably split off a separate issue to track it. |
For storing the graph of forks, I am now leaning towards a design based on storing the The list of forks would be stored as a git commit. The files in that commit's trees would be named after the names of the forks. The contents of each file would list the fork's URLs and fetch specs. By using a git commit, we can track the history of the list of forks, which seems like a valuable property to have. When someone runs If the fork fetch spec ends in something starting with Thus, if your remote is named We'll then need to make the tool append git notes from all refs of the form |
I really like where it's going. Thanks! I have a question regarding importing comments from a fork. I'm not sure I agree with the "only import comments from the owner of the fork". I think that external contributors might have interesting point of view. |
@dolanor Thanks for chiming in. I appreciate the feedback. I do want external contributors to be able to comment, and I want them to be able to comment on any review (not just the ones they request). My thinking about how to do this was that each external contributor should have their own, personal fork for which they are listed as an owner. The tool would then fetch and merge comments from each external contributor, but only from their fork. For example, consider the scenario where there are two forks, one named "alice" with an owner email of "[email protected]" and another named "bob" with an owner email of "[email protected]". In this scenario, anyone pulling from the forks would fetch all comments from both the fork "alice" and the fork "bob". However, they would only merge comments from the "alice" fork if the author was "[email protected]", and they would only merge comments from the "bob" fork if the author was "[email protected]". This way, we validate that comments are coming from the fork that they are supposed to come from, and we can do that validation automatically (without manual user intervention). Would that address your concern about letting external contributors comment, or is there another scenario that is missing? |
Yes it would. |
@dolanor The design (for which I have an initial implementation in this branch) does not rely on the user knowing all the forks. Instead, the forks are stored in a special ref in the repository and can be pushed/pulled with automatic merging. So, for example, if I have a repository at https://github.com/ojarjur/example, and Alice creates a fork of that repository at https://git.example.com/alice/example, then I would first have to run: git appraise fork add -o [email protected] alice https://git.example.com/alice/example
git appraise push Then any third party could clone my repo at https://github.com/ojarjur/example, and run If Alice gives Bob permission to push to her repository, then any reviews or comments he pushes as "[email protected]" will be ignored because he was not listed as one of the owners of that fork. Instead, Bob would need to set up his own repository (let's say he uses https://git.example.com/bob/example), and then convince me to add his fork to the list stored in the main repo. However, once I add his fork, anyone will then start picking up his reviews and comments without needing to do any manual steps aside from running |
Ok, I see. So the right to comment would not be transitive from one repo to another. Each owner of the repo needs to explicitely add a fork. In the case of
If ojarjur adds alice and bob's repos as forks, then anybody fetching reviews from upstream would get both alice's and bob's comments, right? What if alice adds bob's repo to her own. Somebody fetching reviews from alice's repo would also get bob's comments, right? But now, if ojarjur adds alice's fork, and alice adds bob's fork, does ojarjur gets bob's comment since alice added bob's fork? |
Yes.
Yes
Yes, but to clarify, in order for this to happen that person would have to:
Assuming they did both steps, they will see bob's comments that he pushed to https://git.example.com/bob/example
No. The fetching from forks is also not transitive. So, if I pull from https://github.com/ojarjur/example, then I fetch from all of the forks listed there, but I do not pull forks of those forks.
No. The tree of forks is basically flat. |
I wonder if the extra step required (because of no transitivity) would affect the amount of users to the repository. Apart from that, I think the rest is good. |
I agree, there are similarities.
Yes. I actually hope there will eventually be some sort of a web UI that offers self-service support for adding forks.
I don't think this would go all the way back to being centralized. I think, at the most extreme, we would just wind up with a bunch of federated services. A repository may have just one host, but its forks could be on different hosts as long as the original host does not try to lock people in.
Thanks for chiming in and offering your thoughts and feedback; I really appreciate that. |
👋 Hi all! Is this
Still a thing which y'all are interested in adding to |
@pittma Yes, it is, and contributions for it would definitely be welcome. It is orthogonal to the rest of this work, so I should really split out a separate issue to track it. I will do that right now. |
This is meant for situations where an outside contributor requests a pull from their repository to an upstream repository.
In that scenario, the outside contributor can pull reviews from the upstream repository, and can push their review metadata to their repository. However, the maintainers of the upstream repository probably do not want to pull review metadata for all reviews from that outside contributor's repository, but do want to pull review metadata for that one request.
I imagine a scenario like the following:
upstream
) is hosted somewhere.fork
).git request-pull
or something like a GitHub pull request) from their fork into the upstream repository.git appraise pull <upstream>
to fetch the review request and comments from the upstream repository. They then respond to the comments and rungit appraise push <fork>
to push their comments to their fork.git appraise pull --only-comments <review> <fork>
to pull just the comments for that review from the fork, and then they rungit appraise push <upstream>
to push the combined review metadata to the upstream repository.Since the upstream only really cares about the comments from the contributor (the review requests will be different because the review refs will be different, and the maintainers may want to abandon the review), I'm thinking this can all be supported by adding a flag to the
git appraise pull
subcommand that tells it to only pull the notes from therefs/notes/devtools/discuss
ref, and to only merge in the notes for a specified review.The text was updated successfully, but these errors were encountered: