Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:130970
Content-Type: multipart/alternative;
 boundary="------------0yqVHI2N6eX7mQe21jotp20a"
Message-ID: <3ed03ee8-2d3b-4d3c-9f30-93f394e9e047@gmail.com>
Date: Tue, 19 May 2026 16:48:18 +0200
Precedence: list
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PHP-DEV] Merging pull requests instead of closing them
To: internals@lists.php.net
References: <CAPyeZ4NcO_sZYe71Ef_nT0WtXfYhXpBBUFgzBgrrw_A+Oc3ZAg@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CAPyeZ4NcO_sZYe71Ef_nT0WtXfYhXpBBUFgzBgrrw_A+Oc3ZAg@mail.gmail.com>
From: tovilo.ilija@gmail.com (Ilija Tovilo)

This is a multi-part message in MIME format.
--------------0yqVHI2N6eX7mQe21jotp20a
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi Valentin

GitHub marks PRs as "Merged" under either of these two conditions:

  * The PR is merged via GitHub UI
  * The latest commit of the source branch is pushed to the target branch

As explained by Gina, most of us already merge most PRs that target 
master via GitHub UI, unless they require additional changes like a NEWS 
entry. When merging commits manually, the source branch usually requires 
a rebase (as feature/bugfix branches are merged via fast-forward merge). 
Additionally, we frequently amend some information to the commit 
message, like adding a reference to the PR. Either of those change the 
commit SHA and will break GitHub's ability to track the merge status of 
the PR and leave the PR open. To fix this, we usually add "Closes 
GH-xyz" to the commit message to auto-close the PR. Some also just close 
the PR manually with a link to the merged commit. The catch is that the 
status of the PR will be "Closed" rather than "Merged".

To make GitHub pick up on the merge, the source branch would need to be 
force-pushed after the rebase so that the fast-forwarded commit once 
again matches that of the latest commit of the source branch. However, 
this also triggers CI and would incur a non-trivial amount of fully 
redundant resource usage. That's the main reason this has not been 
implemented (apart from the additional step).

The benefit OTOH is that the PR changes would be fully in sync with what 
has actually been merged, which is something Tim has argued for in the 
past. If we can find a way to avoid the additional CI resource usage, 
I'd be happy to implement these changes.

Ilija

On 19.05.26 14:00, Valentin Udaltsov wrote:
> Hi internals,
>
> I want to bring up something that's been bothering me since my 
> contribution <https://github.com/php/php-src/pull/13029>: many PRs in 
> php-src are being closed instead of merged, with the changes pushed 
> separately as standalone commits.
>
> The problem is that GitHub marks these PRs as "Closed", not "Merged". 
> So from the outside — and from the contributor's perspective — it 
> looks like the work was rejected. Their GitHub profile shows no merged 
> contribution, even though the code was accepted and is now in the repo.
>
> Beyond statistics, it also disconnects the commit from the review 
> discussion, and it's just confusing, especially for newer contributors 
> who don't know the convention.
>
> I get that there might be reasons for this — keeping a linear history, 
> concerns about merge commits, etc. But rebasing or squashing before 
> merging solves all of that. Asking a contributor to rebase or squash 
> is totally normal, and most people are happy to do it.
>
> Merging PRs is the standard across open source. It's how you signal 
> that a contribution was accepted.
>
> So my question is: is there a way to make merging the default practice 
> in php-src? Has this been discussed before? If so, what were the blockers?
>
> Thanks,
> Valentin Udaltsov
--------------0yqVHI2N6eX7mQe21jotp20a
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p>Hi Valentin</p>
    <p>GitHub marks PRs as "Merged" under either of these two
      conditions:</p>
    <ul>
      <li>The PR is merged via GitHub UI</li>
      <li>The latest commit of the source branch is pushed to the target
        branch</li>
    </ul>
    <p>As explained by Gina, most of us already merge most PRs that
      target master via GitHub UI, unless they require additional
      changes like a NEWS entry. When merging commits manually, the
      source branch usually requires a rebase (as feature/bugfix
      branches are merged via fast-forward merge). Additionally, we
      frequently amend some information to the commit message, like
      adding a reference to the PR. Either of those change the commit
      SHA and will break GitHub's ability to track the merge status of
      the PR and leave the PR open. To fix this, we usually add "Closes
      GH-xyz" to the commit message to auto-close the PR. Some also just
      close the PR manually with a link to the merged commit. The catch
      is that the status of the PR will be "Closed" rather than
      "Merged".</p>
    <p>To make GitHub pick up on the merge, the source branch would need
      to be force-pushed after the rebase so that the fast-forwarded
      commit once again matches that of the latest commit of the source
      branch. However, this also triggers CI and would incur a
      non-trivial amount of fully redundant resource usage. That's the
      main reason this has not been implemented (apart from the
      additional step).</p>
    <p>The benefit OTOH is that the PR changes would be fully in sync
      with what has actually been merged, which is something Tim has
      argued for in the past. If we can find a way to avoid the
      additional CI resource usage, I'd be happy to implement these
      changes.</p>
    <p>Ilija</p>
    <div class="moz-cite-prefix">On 19.05.26 14:00, Valentin Udaltsov
      wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:CAPyeZ4NcO_sZYe71Ef_nT0WtXfYhXpBBUFgzBgrrw_A+Oc3ZAg@mail.gmail.com">
      <meta http-equiv="content-type" content="text/html; charset=UTF-8">
      <div dir="ltr">
        <div>Hi internals,<br>
          <br>
          I want to bring up something that's been bothering me since <a
            href="https://github.com/php/php-src/pull/13029"
            moz-do-not-send="true">my contribution</a>: many PRs in
          php-src are being closed instead of merged, with the changes
          pushed separately as standalone commits.<br>
          <br>
          The problem is that GitHub marks these PRs as "Closed", not
          "Merged". So from the outside — and from the contributor's
          perspective — it looks like the work was rejected. Their
          GitHub profile shows no merged contribution, even though the
          code was accepted and is now in the repo.<br>
          <br>
          Beyond statistics, it also disconnects the commit from the
          review discussion, and it's just confusing, especially for
          newer contributors who don't know the convention.<br>
          <br>
          I get that there might be reasons for this — keeping a linear
          history, concerns about merge commits, etc. But rebasing or
          squashing before merging solves all of that. Asking a
          contributor to rebase or squash is totally normal, and most
          people are happy to do it.<br>
          <br>
          Merging PRs is the standard across open source. It's how you
          signal that a contribution was accepted.<br>
          <br>
          So my question is: is there a way to make merging the default
          practice in php-src? Has this been discussed before? If so,
          what were the blockers?<br>
          <br>
          Thanks,<br>
          Valentin Udaltsov<br>
        </div>
      </div>
    </blockquote>
  </body>
</html>

--------------0yqVHI2N6eX7mQe21jotp20a--