Having both merge_requests.merged_commit_sha and merge_requests.merge_commit_sha might be confusing

Problem to solve

We recently added a column merged_commit_sha to the merge_requests table to track the "actually merged SHA" for any merge method. The problem is that there is just a one-letter difference with the existing column merge_commit_sha, the SHA for the "actually merged merge commit" (i.e. for non-fast-forward merges).

Additional context

The following discussion from !130763 (merged) should be addressed:

• @kerrizor started a discussion: (+3 comments)

  These names feel dangerously alike, and I wish I'd noticed sooner so we didn't do this. I'm really quite concerned this is going to cause a lot of confusion.

Possible solutions

• Do we actually need both columns? Maybe we can just let merge_commit_sha cover both use cases? The downside of this is that we might no longer know whether the merge was a fast-forward or had a merge commit.
• Is the name actually going to cause problems? We already have a MergeRequests#merged_commit_sha method that could have been named differently for the same reason. Has it already caused confusion?
• Rename the column. The best name I can come up with is actually_merged_commit_sha, but I am very open to improvements.

changed milestone to %16.5

added grouppipeline execution maintenancerefactor severity2 typemaintenance labels

assigned to @hfyngvason

added devopsverify sectionci labels

changed title from Follow-up from "Track merged_commit_sha on merge" to Having both merge_requests.merged_commit_sha and merge_requests.merge_commit_sha is confusing

changed the description

changed title from Having both merge_requests.merged_commit_sha and merge_requests.merge_commit_sha is confusing to Having both merge_requests.merged_commit_sha and merge_requests.merge_commit_sha might be confusing

marked this issue as blocking #423324

@garyh @patrickbajao @marc_shaw @kerrizor what do you think? I've marked this as a blocker for the backfill issue #423324, but it could also be done the other way around. The important thing is that the issues probably shouldn't be done concurrently.

Ooh, yeah that could get quite confusing

Could we rename it to actual_merged_commit_sha?

EDIT: Oops, you already suggested that! I agree

Do we actually need both columns? Maybe we can just let merge_commit_sha cover both use cases? The downside of this is that we might no longer know whether the merge was a fast-forward or had a merge commit.

What if we added a fast_forward boolean column?

Is the name actually going to cause problems?

I don't honestly know. Even in your explanation, however, I certainly don't know what the distinction is between "actually merged SHA" and "actually merged merge commit" I'm not steeped in the details of the project, so that might be it, but 99% of people encountering it won't be, either.

(I'm not trying to be obtuse here -- I respect that naming things is hard!)

Even in your explanation, however, I certainly don't know what the distinction is between "actually merged SHA" and "actually merged merge commit" I'm not steeped in the details of the project, so that might be it, but 99% of people encountering it won't be, either.

I should just have said "merge commit" instead of the "actually merged merge commit". But otherwise, I agree. It's going to be hard to name this in a way that is concise, intuitive and discoverable. To recap:

1. merge_commit_sha: The SHA of the merge commit created as part of a "true merge".
2. merged_commit_sha: The SHA of the HEAD of whatever actually got merged into the branch. This can be
   • the merge commit (standard and semi-linear merges),
   • the squash commit (fast-forward with squash, possibly including a rebase),
   • the HEAD of a rebased copy of the non-squashed source branch created explicitly for the merge

In isolation, I think merged_commit_sha would be the best name for this functionality. I think actual_merged_commit_sha is a decent alternative, but I dislike the actual_ prefix. And it still invites confusion, because the d is still easy to miss for brains that are used to seeing "merge commit" as opposed to "merged commit". But at least it minimizes the typo risk of using the wrong one.

Can anyone think of a name that describes the purpose of merged_commit_sha without including the string "merge" at all? I'm drawing a blank.

The d difference between the names is not a big deal for me especially that MergeRequest#merged_commit_sha already exists before as a method. I'm probably biased because I can see the difference between merged_commit_sha and merge_commit_sha when I look at them.

Also, I don't think we should re-use merge_commit_sha since it's meant to be set when a merge commit is created. In a FF merge, we don't create one.

In my opinion, it can also be confusing in the future if we rename it to actually_merged_commit_sha because whatever the value of merge_commit_sha is, it is actually merged too. But if we're avoiding a mistake from happening due to a typo, it can certainly help so if we really want to rename it, I'm good with that name. But we should also rename the MergeRequest#merged_commit_sha method.

Since the distinction exists already, I think it is ok to leave as is, as long as we make clear what the differences is with a comment in the code

mentioned in issue gitlab-org/quality/triage-reports#14149 (closed)

mentioned in issue gitlab-org/quality/triage-reports#14243 (closed)

Adding category

added Category:Continuous Integration label

changed milestone to %16.6

added missed:16.5 label

changed milestone to %16.7

added missed:16.6 label

changed milestone to %16.8

added missed:16.7 label

@hfyngvason can you add a weight so I can pull on to the board? I noticed this carried over a bit? cc @rutshah

changed milestone to %Backlog

closed