PR merge strategy for long-lived branches

[smoya]

Reason/Context

It is a common operation for contributors to update branches bringing changes from that main (master) branch. More common for long-lived branches. For example, updating the next-major branch with the latest master changes and creating then a PR with such an update. Also, to merge those long-lived branches back to master once the development is considered done.

All repositories have only enabled the "Squash and merge" merge strategy, so no other can be chosen. See:

This works perfectly most of the time except for syncing branches, like the example above and for merging long-lived branches to master. When the PR is merged with the Squash and merge strategy, all those commits will be lost forever. That, applied to the previous example, means all the latest commits from master will be lost in the next-major branch.

That translates into losing history of commits:

• In the case of merging to master, means all commits pushed to the long-lived branch will be lost, and the code will be re-authored by the creator of the PR that merges it into master.
  • For example, I did not create this file but Lokesh did
• In the opposite case, merging master into the long-lived branch, since commits are also lost, diff between those branches will persist (in terms of commits. The code is the same). That also creates a problem in our CI since in practice, your branch is not up to date, and GH will keep showing you your branch is out of date.

Description

To make it work, the Squash and merge strategy should not be used for long-lived branches. Instead, the Rebase and merge strategy is preferred. As an alternative, Create a merge commit (but this adds an extra commit).

Currently, there is no simple mechanism to do that, as only the Squash and merge strategy is allowed. Code owners need to temporarily enable the alternative strategy via repository settings, manually merge the PR, and then revert the config change. See:

@asyncapi-bot has a feature to let a PR to keep up with changes in the target branch. However, this will stop working whenever conflicts appear. If those get fixed in a new PR, the same issue will happen with the merge strategy.

Solution

I guess there can be several solutions but the ones I see now are:

1. To keep it like it is now. Write some guide somewhere about how both code owners and contributors should proceed with these kind of PRs.
2. Enable Rebase and merge strategy for all repos.
3. To code a new command in @asyncapi-bot that does the same as /rtm but with the alternative merge strategy. Maybe we can even use the same and add an argument to it. I.e. /rtm rebase or similar. Not sure if this is possible (how to detect the intention of the PR in a generic way?), but maybe the bot could also write a comment saying "Remember to merge with Rebase and merge by /rtm rebase".

Relates to

• https://github.com/asyncapi/.github/issues/181

cc @jonaslagoni @derberg @fmvilas @dalelane @M3lkior

[fmvilas]

I’d go for the “Create a merge commit” strategy. I’ve had the worst experiences of my life with Git and rebasing 😅 An extra commit is not a problem.

[derberg]

/rtm rebase

Bot has the same limits as other users. So if certain strategy is not enabled, bot also cannot use it. So I don't think it is an option

enable Rebase and merge

This is risky (although probably checkable) as someone can rebase to master by mistake

To keep it like it is now

I suggest we do this. And do not restrict if this is rebase or merge, more important to say !squash. It could be even automated with bot. So whenever there is a PR from master to feature branch, bot drops a comment with instruction

[smoya]

Whats the think you don't like with enabling Create a merge commit @derberg ? All Pr's will still need to be merged by /rtm, and no one except Code Owners will see the button for merging.

[derberg]

Whats the think you don't like with enabling Create a merge commit @derberg ?

sorry? 😅 you mean enabling permanently or temporarily? as permanently my opinion is the same, no matter if rebase or merge commit

[smoya]

Whats the think you don't like with enabling Create a merge commit @derberg ?

sorry? 😅 you mean enabling permanently or temporarily? as permanently my opinion is the same, no matter if rebase or merge commit

Permanently. If we allow Create a merge commit strategy, your concern will be dropped:

This is risky (although probably checkable) as someone can rebase to master by mistake

[derberg]

Create a merge commit is also a problem when PR is merged this way, as it is a merge commit without title and therefor conventional commit. How rebase works in GitHub, can you set a custom commit message?

[M3lkior]

Please no; don't activate the merge commit strategy ; this strategy adds extra commit on repos and the git log history become horrible.

The best way is to do a Fast-forward merge to keep a clean and linear history if the two branches are up-to-date; you can see some comparison in this good article: https://blog.mergify.com/whats-the-best-git-merge-strategy/

On my side, i apply the Fast-forward merge since few years thanks to code automation (when i'm using the Git flow way i.e using main and develop).

Here is my Github Action pipeline that automatically merge main into develop after my release process (the release add few release/tags commits to main and develop needs to be rebased to be up-to-date). It can be transposed to merge main into next-major on AsyncAPI repos, i guess

rebase-develop-onto-main:
    name: Rebase develop onto main
    runs-on: [self-hosted, Linux, standard]
    needs:
      - release
    steps:
      - name: Checkout repository
        uses: actions/[email protected]
        with:
          ref: main
          fetch-depth: 0
          token: ${{ secrets.SHARED_GITHUB_CHECKOUT_ACTION_TOKEN }}

      - name: Merge into develop
        run: |
          git fetch origin
          git checkout develop && git pull origin develop
          git rebase main
          git push origin develop

I’d go for the “Create a merge commit” strategy. I’ve had the worst experiences of my life with Git and rebasing 😅 An extra commit is not a problem.

Yep @fmvilas a lot of people suffering with the rebase command but this is an amazing command when used correctly :)

[smoya]

a lot of people suffering with the rebase command but this is an amazing command when used correctly :)

I agree. Also, the point is that only code owners will be able to merge with such alternative. In fact, squash is already available, and it does the same except that it unifies all commits of the PR into one.

It can be transposed to merge main into next-major on AsyncAPI repos, i guess

The point is that conflicts may appear when keeping up that branch with master. We do have already an action that keeps a branch up-to-date if a PR exists with the label autoupdate (which you can add it thanks to @asyncapi-bot). But still, same issue with conflicts.

[smoya]

I edited the description of this issue to clarify the case of merging long-lived branches into master. Something discussed here as well https://github.com/asyncapi/avro-schema-parser/pull/191#issuecomment-1503672514

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity :sleeping:

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart:

[smoya]

Lately, we have been following the strategy of enabling Merge in repositories and, so far so good, it worked. Always recommended to add a note in the PR description such as:

Warning This PR should be merged manually by using Merge with commit strategy. More info on https://github.com/asyncapi/community/issues/641

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity :sleeping:

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart:

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity :sleeping:

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart:

[smoya]

This issue can be a source of knowledge for the work Maintainers Growth Working Group plans to do; documenting the onboarding of new maintainers. cc @AceTheCreator @alequetzalli @thulieblack @Mayaleeeee @smoya