google / git-appraise

Distributed code review system for Git repos
Apache License 2.0
5.12k stars 145 forks source link

Look at the current reviewRef when submitting #113

Open iveqy opened 2 years ago

iveqy commented 2 years ago

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.

google-cla[bot] commented 2 years ago

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.

iveqy commented 2 years ago

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.

ojarjur commented 1 year ago

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 IsAncestor check.

When you rebase the commits in a review outside of using the git appraise rebase command, then we lose any link between the original commit and the commit you are trying to submit.

That means that even if we disabled the IsAncestor check, the original review would not show up as being submitted, because git-appraise does not know the commits are related.

This can be fixed by running git appraise rebase prior to running git appraise submit, so maybe instead all we need to do is catch this scenario and give the user appropriate instructions.

I'll add an inline comment giving an example of what I am thinking.