Contrib.social

[policy] Define standard for merging in branches from core sandboxes

Open on Drupal.org →
Created on 1 March 2012, over 13 years ago
Updated 12 March 2025, 5 months ago

Spin-off from #1321540-83: Convert DBTNG to namespaces; separate Drupal bits →

Problem

  • Ugly Drupal core commit log/history after merging in a larger complex branch from a sandbox.

Goal

  • Actively support and endorse merging of larger/complex changes from core sandboxes.
  • Retain a clean commit log/history for Drupal core.
  • Define a standard for merging in branches from core sandboxes.

Details

  • The commit log/history looks very awful after merging in #1321540: Convert DBTNG to namespaces; separate Drupal bits → . Drupal core's commit history makes little sense due to this merge now. It's close to impossible to determine and review the individual commits that belong to this change(set).
  • The common practice is to prepare a to-be-merged branch in a separate merge branch upfront.
    Read a high-level description of this in Rebasing and merging: some git best practices and the often referenced Linus Torvalds' direction on clean history.
  • That dedicated merge branch is rebased onto the latest core branch. Hence,
    1. all change commits are known to be based on the latest code (which may be the case here now, but we factually don't know)
    2. the git commit log/history is clean; i.e.:
      • all commits from the merged branch cleanly appear after each other
      • no upstream merge commits from the sandbox
  • Additionally, pull request branch maintainers are asked to clean up the merge branch during the rebase; e.g.,
    • combine multiple related commits into one (avoiding plenty of "Add more/fix use statements" commits)
    • squash other needlessly verbose commits

    That is, because such commits are completely stupid and pointless with regard to the long-term history. Simply have a quick look at node.module's history now — next to a plethora of senseless upstream merge commits of the sandbox, you also see commit messages like "Add more use statements and class renaming.", which are completely missing context and thus don't make sense in terms of Node module's history.

  • If there's any need for retaining the original branch for downstream repos/branches, then that branch remains to be unaffected.

We should really follow that practice of prepared merge branches, because, in fact, it made more sense to review the patch file in the issue, which ultimately contradicts the merge of the branch (and thus, it would have made more sense to commit the patch instead). A properly prepared merge branch avoids all of these issues.

Docs on the web

📌 Task
Status

Postponed: needs info

Version

11.0 🔥

Component

documentation

Created by

🇩🇪Germany sun Karlsruhe

Live updates comments and jobs are added and updated live.
  • Coding standards

    It involves compliance with, or the content of coding standards. Requires broad community agreement.

  • stale-issue-cleanup

    To track issues in the developing policy for closing stale issues, [Policy, no patch] closing older issues

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 5 months ago →
    🇺🇸United States smustgrave

    Thank you for creating this issue to improve Drupal.

    We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

    Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

    Thanks!

  • Comment 5 months ago →
    🇺🇸United States neclimdul Houston, TX

    This came out of the original Drupal 8 initiatives. We where using sandboxes as a place to experiment and prep and collaborate around merge requests. There was no "merge" process at the time only patch acceptance so the idea here was to come up with a way someone making a complex change with multiple commits could get those merged instead of just committing a patch.

    In a lot of ways this is resolved with issue forks, I wonder if there's a similar problem with squash commits vs preserving the history but I haven't run into the problem yet.

  • Comment about 2 months ago →
    🇺🇸United States smustgrave

    Think this should be re-open?

  • Comment about 1 month ago →
    🇵🇪Peru marvil07

    Yes, probably replacing _core sandboxes_ with _issue forks_ leads us to the same place.

    Currently for core, it seems like almost all issue fork merges are squashed into one commit.
    That is definitely an improvement in git history readability comparing it to sometimes full merge and sometimes a squashed commit.

    To be fair that mainly happened at the start of the git transition.

    As #8 highlights, the main thing is that as a community we are not that good creating patchsets.
    That is maybe a consequence of the original restriction of one patch we use to have, that was dragged on into the git era.

    Even if we do not move to the patchset model, always squashing is the next best thing, since drupal community is good in issue granularity.

    It would be amazing if we move into creating relevant patchsets instead.
    Again, by patchset it is not about patches, it is a bout a set of commits that explain the changes, instead of random incremental changes. because they can help to understand better the changes, and even inspect the git history after the fact. E.g. when changing a given file, one may ask what has happened on this set of lines before, and git blame can point to the right commits, and from there more related commits can be found, which can help in the actual new development of the change being worked on.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024