Ensure the MR the user approves is pointing to the right HEAD

[bmeneg]

Hi all,

I received a tip from a colleague that the GitLab Merge Request API allow the user to pass the HEAD commit SHA for which that approval should refer to, avoiding the following race condition:

1. user checkout MR v1
2. review it locally 2.1. new MR v2 is pushed upstream
3. approve (MR v1 in mind) 3.1. MR v2 ends up being approved instead

Usually, and by policy, we hope the users fetch/pull/check upstream before approving anything, but it's impossible to guarantee it completely and the race window can be pretty short (even if the user fetches/pulls/checks upstream).

@zaquestion, @prarit, others, is it worth enabling this API feature in lab?

When local and upstream HEAD matches the usual MR object is returned. OTOH, when it doesn't match:

{
    "message": "SHA does not match HEAD of source branch: e1104fd4d67f311ef8c471ad0f3254f986491c91"
}

And go-gitlab also supports it:

 // ApproveMergeRequestOptions represents the available ApproveMergeRequest() options.
 //
 // GitLab API docs:
 // https://docs.gitlab.com/ee/api/merge_request_approvals.html#approve-merge-request
 type ApproveMergeRequestOptions struct {
     SHA *string `url:"sha,omitempty" json:"sha,omitempty"`
 }

I'm personally in favor of enabling it, but not as default behavior, but through an --match-head option or something like that.
But at the same time, I know that some, @prarit, has a different view of that :).

So please, let me know what you think.

[prarit]

My view is that this can happen on any project at anytime. You need to be sure what you reviewed is what you are approving. Any tool, including lab, has a small delta in time which this issue can occur.

[zampierilucas]

My view is that we should always check for the MR head, and if the head changed, notify the user and exit out with an error message. That way we will avoid users approving PRs that they didn't intend to. We could also add a --force option so the user can approve even if the head changed.

[zaquestion]

I'm in favor of enabling this by default, if we're to support fully reviewing MRs from the command line we'll certainly need to give the confidence that what you're approving is what you're reviewed. This is basically optimistic locking for code review :D

I'm not sure what the rationale for not enabling this by default would be? In most cases the users likely won't notice, until it stops them from approving something they didn't review, which is only a good thing.

[bmeneg]

The only reason for not enabling it by default, from my perspective, is to not break the current behavior. But as you well said, I also don't think most users will notice, until they face the real mismatch situation. So, yep, I'm also in favor of enabling it by default and, as mentioned by @zampierilucas, possibly adding a "--force"-like option for the fearless user (rm -rf exists for a reason, right? 😄).