Why a merge commit for pull requests?

[HebaruSan]

See KSP-CKAN/xKAN-meta_testing#73 for details, we've been using this Action and found the automatic creation of a new merge commit extremely surprising and confusing. Our Action wasn't behaving as intended and it was difficult to figure out why. Newer changes from master that had nothing to do with a pull request were being pulled in to that pull request's validation checks and wasting resources. Finally we took a close look at this repo's README and found the recipe to turn this off, implicitly indicating what was happening.

So now I'm curious, what is the perceived benefit of this being the default, rather than checking out the pull request branch head? Could it be changed to do that?

[polarathene]

I was a bit surprised myself by that behaviour as well, but it doesn't seem like that should change.

So now I'm curious, what is the perceived benefit of this being the default, rather than checking out the pull request branch head? Could it be changed to do that?

I believe one of them is to get a diff of changed files easily. Otherwise you'd need to fetch X history of commits, and git fetch the base branch with enough history to derive a "merge-base" (requires fetch-depth: 2, not the default depth of 1).

It's also apparently what other CI services do, by changing it, you'd get a bunch of other users asking why it's not the default 🤷‍♂️

UPDATE: I came across this scenario explaining what it calls "merge-skew"/"semantic conflicts" (on the main bors page). It refers to separate PRs that have some overlap in functionality, but individually would pass tests, yet when merging to master would break 😬

By using the generated test merge commit, your PR changes can be tested with the context of being merged into the base branch (master/main), and catch that problem in advance before an actual merge (assuming no more updates to base branch in-between).

Github has a related feature for PRs to detect when the base branch sha is outdated, and offers a button in the UI to update via merge commit of the base branch into the PR branch (or a rebase instead), which I guess helps ensure the same thing? (since workflows is run with a fresh context)

---

Gotcha with two actions/checkout steps, where one is with the default merge commit and fetch-depth 2

This was with fetch-depth: 2 and the default ref (test merge commit), with a later action using a ref of head.sha/head.ref and trying to fetch PR commit history followed by a 2nd fetch manually for the base branch, which would fetch enough commit history for a merge-base... unless local history contains existing commits (from the first actions/checkout step) which broke git merge-base.

---

If you don't perform a diff from that merge commit (eg: the associated PR commit instead) to the associated base commit, then you'll have trouble with a deriving a merge-base as well. Without the merge-base (if not diffing from base branch commit to merge commit), then you risk incorrect diffs.

Unfortunately you cannot git fetch additional commit history for the base branch, even if you have enough commits for the other checked out branch, as fetch will notice the commit associated to the base branch and not see a need to fetch any more history than that.

This occurs even if you use the action again in a later step and explicitly set the ref as PR head SHA. The cleanup by the action does not remove the prior merge commit, associated history, or it's ref from the local history.

This gotcha confused me for a while troubleshooting a workflow failure, since both worked in isolation and I was aware of that the action performed some cleanup but misunderstood what that entailed.

To workaround that you'd have to fetch enough commit history from the PR branch to know the earliest commit date to fetch commits from the base branch with --shallow-since (or remove the previous actions history?). Probably not a situation most would run into with the action though 😅

[jcw-]

found the automatic creation of a new merge commit extremely surprising and confusing

I'm solidly in the "astonished" category. The word checkout has a very specific meaning. This is behavior I would expect from an action called: actions/checkout_and_preview_master_merge

It's also apparently what other CI services do

Not CircleCI.

Finally we took a close look at this repo's README and found the recipe to turn this off

The override recipe doesn't work with the merge queue (which uses a branch without a pull request). I believe this behavior is both surprising AND incompatible with the GitHub merge queue?

Example:

on:
  pull_request:
    types: [synchronize, opened, reopened, edited]
  merge_group:
    types: [checks_requested]

[klyonrad]

Hey by the way, if you find this surprising and configure it differently, please don't call your test runs "CI run". CI means Continous Integration. By that philosophy you want your tests to run against the integration of the topic branch into the main branch.

Writing this just to counter. I prefer this behavior and would be quite annoyed if the default changes.

[daredevil82]

Got bit by this recently @klyonrad

Scenario: golang project, feature branches. Coworker merged in a branch with some changes that affected recent work I had done, and I am currently working on fast follows of that original work. Difference in merge times was ~3 hours

What happened was the coworker changes had changed how a db object is instanciated by generating a default uuid vs using the one passed in the parameter (misunderstood requirements). As a result, with integration tests I was getting a FK constraint error. However, the error line number for that exception message was entirely different from the line in my code. It wasn't off by one or anything like that, it was 12 lines apart. The job run code was referencing an error location when defining a test stub object in memory, without any db inserts.

It wasn't until another coworker saw the difference in hashes between the head sha of my branch and what checkout was logging that the aha method came on.

Yes, I could have merged changes from master before then. But I wanted to ensure that my code was passing CI before adding any additional changes. Why not? They're working locally, no merge conflicts found, I would expect CI to run on my branch and then I would explicitly trigger an update afterwards to verify. So my code works, but test break with the merge... makes it alot more obvious where and what is breaking rather than default obfusication.

So this is indeed surprising behavior because the step is merging the branch into master/main, not the other way round.

[3barroso]

Check this out if you need the branch ref: https://github.com/marketplace/actions/checkout#checkout-pull-request-head-commit-instead-of-merge-commit

[kopach]

Hi there.
I can see this merge behavior useful, but it should not be default. It's misleading more than it helps.
@polarathene, I see your point, but I can also see opposite. We can have change on base branch which would silently/ambiguously/unexplicitly/etc break CI on PR but not locally, which could be very frustrating and time-consuming. So what are we optimizing here? With GitHub's merge queue, this merge behavior is accomplished automatically in a much more explicit and clear way.
Also, @3barroso, solution from README you refer to is only related to Pull Request events, not to other events like push, schedule, etc. So, basically this suggests having separate actions per event type, which is not the best.

[polarathene]

@polarathene, I see your point, but I can also see opposite.
We can have change on base branch which would silently/ambiguously/unexplicitly/etc break CI on PR but not locally, which could be very frustrating and time-consuming.

Uhh, isn't that a good thing though? You'd rather the opposite of a PR that didn't break CI yet when merged then introduced the breakage afterwards because it wasn't detected?

So what are we optimizing here

Not really optimizing anything? Preventing problems and being consistent with what you'd find on other CI platforms AFAIK? As stated you can't really make all users happy with a default.

As for the merge queue, I don't think that existed at the time. If it's effectively equivalent, then the default merge commit behaviour probably isn't changing since it'd impact many projects that rely on that implicitly.

---

solution from README you refer to is only related to Pull Request events, not to other events like push, schedule, etc. So, basically this suggests having separate actions per event type, which is not the best.

Do those other events have the same issue? I don't think so?

You can use conditionals to change behaviour of a step based on if it's a pull request or not too. Note that pull_request event already runs in a restricted context for security reasons. There's pull_request_target as an alternative, but you need to be careful not to run untrusted code from the PR, so sometimes you do want separate workflows.

IIRC pull_request_target shouldn't use any workflow change from a PR, but pull_request could have a workflow adjusted which would be used instead.

---

I can see this merge behavior useful, but it should not be default. It's misleading more than it helps.

It's too late to change that AFAIK, especially if doing such could suddenly break CI for PRs across many repos on Github where this sort of change would be rather silent / missed.