Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable the Merge button in PRs where the PR branch contains a merging-rebase #119

Open
dscho opened this issue Feb 7, 2025 · 0 comments

Comments

@dscho
Copy link
Member

dscho commented Feb 7, 2025

In Git for Windows, we have the well-established paradigm of infrequently rebasing with the "merging-rebase" strategy: starting from the revision onto which we want to rebase with a merge commit that makes the previous commit history reachable but does not take any code changes (i.e. using -s ours, to make sure that the merge commit is tree-same to its first parent commit, the onto commit of the rebase), and then we replay the branch thicket of topic branches on top. To mark this fake merge more obviously, its commit message is of a specific form, always starting with the tell-tale "Start the merging-rebase".

This served us well over the years, even if it is non-obvious that often you want to look at the commit history excluding that fake merge.

In other words, the command git log HEAD^{/^Start.the.merging-rebase}.. will show the current versions of Git for Windows' patches on top of the latest upstream Git version to which it was rebased.

And git range-diff origin/main^{/^Start.the.merging-rebase}..origin/main HEAD^{/^Start.the.merging-rebase}.. will show the latest evolution of Git for Windows' patches.

However, one thing that shouldn't happen is that Pull Requests that include such a merging-rebase are merged using the Merge button (and neither should they use the Rebase button) because then there would be more than one merging-rebase that are independently reachable. And that would break the ^{/^Start.the.merging-rebase}.. way to limit the commit history to the latest patches, there would now be two copies of them.

Unfortunately, this is exactly what happened recently, though, twice. It is just too easy to miss the fact that this shiny, green Merge button should not be pushed even if the PR is ready to go.

To avoid that, I proposed the following strategy (drawing heavily on the GitForWindowsHelper GitHub App):

  • create a check run called enable-merge-button via the GitHub CLI, i.e.

    gh api -X POST repos/dscho/git/check-runs \
      -f name=enable-merge-button -f head_sha=<tip-OID> -f status=completed -f conclusion=success

    This will need GH_TOKEN to hold a currently-valid installation access token for the GitForWindowsHelper App.

  • add a branch protection rule for main that requires that check before merging.

  • upon receiving the event that a Pull Request was opened or synchronized:

    • iterate over the commits to see whether there is a merging-rebase one

      Sadly, neither the pull_request.opened nor the pull_request.synchronized payload actually contain the list of commits, instead, it would appear that one has to extract the pull_request.base.sha and the pull_request.head.sha attributes and then use the repos/OWNER/REPO/compare/A...B REST API to get to that information (looking at .commits[].commit.message).

    • use the REST API to add the enable-merge-button check, with conclusion=cancelled if the PR branch contains a merging-rebase, and conclusion=success otherwise.

Unfortunately such a canceled check will cause an ugly red x to be shown in the title bar when looking at the PR in the web browser. I would be delighted if we could find a better way to disable the merge button, one that would not cause the PR to look as if it failed.

Hmm. Maybe the best idea would be to make GitForWindowsHelper a required reviewer? It would need to use this REST API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant