Skip to content

Why a merge commit for pull requests? #504

Open

Description

@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?

Activity

  1. changed the title [-]Why a merge commit?[/-] [+]Why a merge commit for pull requests?[/+] on May 14, 2021
  2. polarathene commented on Jun 27, 2022

    @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 😅

  3. jcw- commented on May 2, 2023

    @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]
    
  4. klyonrad commented on Jun 19, 2023

    @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.

  5. daredevil82 commented on Dec 30, 2023

    @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.

  6. 3barroso commented on Sep 5, 2024

    @3barroso
  7. kopach commented on Oct 16, 2024

    @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.

  8. polarathene commented on Oct 16, 2024

    @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.

  9. chris-braidwell commented on Feb 4, 2025

    @chris-braidwell

    The default behavior is misleading and dangerous.

    I expect GHA to operate on my PR's commit, not silently perform a merge to main and operate on that. The default behavior may make sense if all you do in your GHA jobs is run CI tests. It makes zero sense if you're building versioned assets. I just had the experience that a later, unrelated DB migration was built into a docker image and nearly deployed -- a near-disaster.

    I'd wager the vast majority of people using the checkout action don't understand this default behavior. It leads directly to building and releasing software that's NOT the intended version. Think about it: the output of your build changes, not due to changes in your branch, but changes in unrelated upstream commits. It violates the principle of least amazement, and no doubt is causing massive invisible defects in many projects. This is the worst tooling foot-gun I've seen in a long time.

    The problem can be resolved by eliminating the default and requiring an explicit choice of strategy.

  10. zachsaw-lmg commented on Oct 13, 2025

    @zachsaw-lmg

    Well said @chris-braidwell.

    The default behavior of creating a synthetic merge commit for PRs made sense before merge queues existed, but today it’s legacy and misleading.

    Merge queues already provide explicit, reliable integration testing against main. We should stop relying on hidden "pretend merges" as the default --- they cause confusion (I’ve personally lost hours chasing CI behaviors I couldn’t reproduce locally because the CI run's merge was temporary and timing-dependent), waste resources, and risk building artifacts that don’t match the branch head.

    In short: merge queues are the modern solution, and the default should be updated to reflect that.

  11. dezfowler commented on Nov 4, 2025

    @dezfowler

    Just been bitten by this.

    Completely agree with @chris-braidwell ... I've never come across another CI tool that works this way in 20+ years of development so I'm going to guess this is unexpected behaviour for most.

    How is non-deterministic behaviour ever a good choice for a CI tool?

    Very odd.

  12. jsoref commented on Dec 19, 2025

    @jsoref
    Contributor

    If you want deterministic behavior, use on: push instead.

    If you want truly safe CI, you should use on: merge_group which is safer, because otherwise the testing you're doing may be stale by the time you get around to merging.

  13. mrmckeb commented on Dec 21, 2025

    @mrmckeb

    @jsoref Correct me if I'm missing something, but wouldn't that result in many more actions runs - as teams are now running CI checks on every push to every branch (assuming no additional configuration), even if a PR hasn't been created?

    I think the other problem is that again, this is not what people think is happening when they use pull_request. Every engineer I speak to about this behaviour is completely unaware of it, which isn't great for a core part of a build system.

  14. jsoref commented on Dec 21, 2025

    @jsoref
    Contributor

    Each event for which you run things is indeed a thing for which an action runs, yes, that's the way of the world.

    I've been dealing w/ CI like things for a very long time (long before GitHub), and people are used to the idea that builds may have to happen more than once because of races.

    Every engineer I speak to about this behaviour is completely unaware of it

    I'm not sure with whom you're speaking, and it frankly doesn't matter.

    The documentation is reasonably clear that if you don't want the default Checkout [for] pull request ... of merge commit, you have to ask. (There are bugs in the documentation, especially the docs for clean are pretty lousy.)

    Changing defaults for any heavily used action (and checkout is probably the most used action) can't be taken lightly.

  15. mrmckeb commented on Dec 21, 2025

    @mrmckeb

    I'm sure it isn't your intention, but your comments are coming off as a little dismissive @jsoref.

    I've been dealing w/ CI like things for a very long time (long before GitHub), and people are used to the idea that builds may have to happen more than once because of races.

    I appreciate that you have a lot of experience in this space, but we all have different experiences. These are the four platforms I've worked most with in the last 5 years or so:

    I'm not sure with whom you're speaking, and it frankly doesn't matter.

    The documentation is reasonably clear [...]

    I did a quick verbal poll in the office, and spoke to some friends outside of my office. People are used to CIs working in a specific way, and this is the outlier from my research and experience (noted above).

    I'd also argue that it's not actually clear in the documentation. Many people reporting issues around this have found out only when they hit unexpected outcomes in their CIs, and the documentation only notes how to opt out of this behaviour, as a recipe (scenario).

    Changing defaults for any heavily used action (and checkout is probably the most used action) can't be taken lightly.

    I 100% agree. I would bet that a notable majority of those users are actually unaware of the default behaviour, and validating what users are currently expecting/understanding could be the next step here.

    I understand that this is inherited from pull_request where the documentation notes "Last merge commit on the GITHUB_REF branch". Perhaps a change is needed there, we already have pull_request_target, so perhaps the fix is introducing pull_request_head.

  16. pakal commented on Mar 16, 2026

    @pakal

    The default current behaviour for pull_request makes sense, but it's completely hidden in the Readme of this action, I guess this should be a clear warning at the beginning, to tell/remind people that by default the PR checkouts will work on a MERGED commit, not the actual head of the merged branch.

    For now, it only appears to people when they stumble against it in their CI/CD, or notice the unexpected "https://github.com/marketplace/actions/checkout#checkout-pull-request-head-commit-instead-of-merge-commit" ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions