Look at the current reviewRef when submitting #113
Conversation
When a review has been rebased and then force pushed to update a review, for example to fix review comments. It can become impossible to submit with the error message: Refusing to submit a non-fast-forward review. First merge the target ref. this even if the review ref is a ancestor of the target ref and the merge should been a fast forward merge. This is because the check for appraise submit is done against the first commit sha1 that was used and not for the actually reviewRef that will be merged. This fixes that.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
For some reason this mess up the git-appraise list view where submitted code reviews will still be shown as open. IT tells me that there's more to this than just this merge request. |
|
Hi @iveqy, thanks for taking the time and putting in the effort to contribute to the tool. As you noted, there is more to what is going on than just the When you rebase the commits in a review outside of using the That means that even if we disabled the This can be fixed by running I'll add an inline comment giving an example of what I am thinking. |
| } | ||
| source, err := r.GetHeadCommit() | ||
| if err != nil { | ||
| source := r.Request.ReviewRef |
There was a problem hiding this comment.
We still want to use the result of GetHeadCommit in the below check. It will actually use the review ref head if the metadata linking the review commit to the review ref head is properly set up.
However, we also want to give the user instructions on how to fix it if that metadata is not correct.
I think a better fix might be to add logic right after this (right before we check the ancestry of the target ref) to double check that the metadata is correct and give the user instructions on how to fix it if it is not.
Something like:
isReviewAssociatedWithRef, err := repo.IsAncestor(source, r.Request.ReviewRef)
if err != nil {
return err
}
if !isReviewAssociatedWithRef {
return fmt.Errorf("The review commit is not associated with the review ref %q. If the change has been rebased, then reassociate the review commit with the review ref using the command `git appraise rebase %s`", r.Request.ReviewRef, r.Revision)
}
When a review has been rebased and then force pushed to update a review,
for example to fix review comments. It can become impossible to submit
with the error message:
Refusing to submit a non-fast-forward review. First merge the target
ref.
this even if the review ref is a ancestor of the target ref and the
merge should been a fast forward merge. This is because the check for
appraise submit is done against the first commit sha1 that was used and
not for the actually reviewRef that will be merged. This fixes that.