google / git-appraise

Distributed code review system for Git repos
Apache License 2.0
5.13k stars 146 forks source link

Add more sophisticated support for rebasing a review. #45

Closed ojarjur closed 7 years ago

ojarjur commented 8 years ago

Right now the git appraise submit command supports a --rebase flag that will cause the review to be rebased onto the target branch as part of submitting it.

That has the side effect of the code review data being disconnected from the final, submitted commit. As such, it is only sufficient if you want to use the code review data prior to the submit and not afterwards (for, e.g. browsing review history).

It would be better if the review data was connected to the rebased commit. We still want users to be able to have their intermediate commits garbage collected (if they so desire), so this linkage needs to be done in a way that does not force the intermediate commits to be kept (such as making them a parent of a notes commit).

Since there might be comments on intermediate commits that later get garbage collected, this also requires that the tool handle that scenario gracefully.

Additionally, we may want to add a git appraise rebase command that does this rebasing without submitting the review.

ojarjur commented 8 years ago

My current thinking of how to handle this is to add two optional fields to the request schema:

  1. A field that points to a previous review of which this one is the rewritten continuation.
  2. A field that points to a subsequent review that is the rewritten continuation of this one.

This would make the linkage bi-directional, so the rebase command would have to do the following under the hood:

  1. Kick off the interactive rebase
  2. Add a review request to the new commit pointing at the old review
  3. Append a new request for the old review that points at the new commit as its continuation
  4. Copy the comment notes from the old review to the new one. Since this will reuse the file blob, it should not involve much space overhead.

The tool will also have to know to coalesce these requests into a single review. That probably just requires changes to the GetSummary and ListAll methods in the review package. All operations should be performed on the latest continuation of a review.

Finally, since we want users to have their old commits garbage collected (if they so desire), we'll have to go with the approach of saying that the user is responsible for keeping those old commits around if they want them. However, we can make that easier to do by adding a flag to create a ref pointing at those commits. There are two options I can think of for doing this:

  1. Add --tag/--no-tag flags that control whether or not the rebase command adds a new tag pointing at the old review commits.
  2. Maintain some sort of internal ref that points at all such commits by adding them via a merge commit.

Right now I'm leaning towards the first option, as it feels more "git idiomatic" to me, but it does have the consequence of polluting the tags namespace. A compromise might be to create tag-like refs outside of the "refs/tags/" namespace.

ojarjur commented 8 years ago

After thinking about this quite a bit more, I think the linkage from original commit to rebased-commit should be in one direction only; from the original commit to the rebased one(s).

In essence, what I am now leaning towards is keeping a single commit that is the anchor for all review metadata. This is consistent with how comments are always anchored at the first commit, even if they are about a subsequent commit.

This approach has the benefits of:

  1. Being a much smaller change to the current format. All we need to do is add an optional 'alias' field to the request object.
  2. Having a single source of truth for the request(s) for a review. This, in turn, means we don't have to worry about things like coalescing review requests stored in multiple notes or reading comments from multiple notes.

This has the downside of not failing as gracefully in the case that a user forgets to keep a pointer to their original commits (and they get garbage collected). This means that creating a ref pointing to the original history is more important.

As to that archival ref, I've started thinking that instead of this being a tag or a series of tags, that it can be a single ref (e.g. 'refs/devtools/archive') that can also be pushed/pulled by the git appraise push and git appraise pull commands. This would avoid us from polluting the tag namespace, and would also prevent the scenario that a user forgot to fetch the relevant archived commits from the remote repo.

ojarjur commented 8 years ago

This will be fixed by #63