Use rebases instead of merge commits

[geekosaur]

Per #10048 (comment) ff.

In studying the Mergify documentation, I discovered the actual rules are slightly different than the ones we thought it was using, so I used the ones the documentation cited (cf.
https://docs.mergify.com/workflow/actions/merge/#parameters).

Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

• Patches conform to the coding conventions.
• Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

[geekosaur]

While we probably want to backport this in the name of cleaning up git history for the branch, there's no particular reason to require it for the 3.12.1.0 release.

[fgaz]

I also like rebasing and getting rid of merge commits, but in this case it will interfere with the --prlog feature of changelog-d that needs the pr number in the commit message

[Mikolaj]

These are the --no-fast-forward merge commits, right? What's the problem with them? The normal merge commits are bad, that's obvious, but I think merge prevents them (except when we use the merge+no-rebase flag).

[ulysses4ever]

@fgaz

I also like rebasing and getting rid of merge commits, but in this case it will interfere with the --prlog feature of changelog-d that needs the pr number in the commit message

Before you wrote:

squash+merge will result in one commit with (#10048) appended to the first line of the commit message, while merge will result in two commits: the one being merged and the merge commit Merge #10048 into master

So, we currently have two styles of merging ("merge" and "rebase") and this PR makes the style uniform ("rebase"). Are you saying that currently PRs using the "rebase" style are not handled by changelog-d properly? This is concerning...

Furthermore, the "rebase" style does preserve the PR number, as you say: the number is appended to the commit message. Maybe, changelog-d can be taught to recognize this format of commit messages?

[fgaz]

@ulysses4ever

So, we currently have two styles of merging ("merge" and "rebase") and this PR makes the style uniform ("rebase"). Are you saying that currently PRs using the "rebase" style are not handled by changelog-d properly? This is concerning...

Furthermore, the "rebase" style does preserve the PR number, as you say: the number is appended to the commit message. Maybe, changelog-d can be taught to recognize this format of commit messages?

In fact it already can:

• https://codeberg.org/fgaz/changelog-d/src/commit/50aea04e681549c9ca464fce5d4a12c8a6c05b9d/ChangeLog.md?display=source#L20
• https://codeberg.org/fgaz/changelog-d/commit/e798dfcff83dc40d3075299d9902a49b9e0051b3

The only type of merge it cannot recognize is what github calls "Rebase and merge", since it doesn't contain the pr number.

Unfortunately I noticed this isn't documented anywhere, I opened an issue about that.

[fgaz]

Why are we merging this? Didn't we agree that this breaks changelog-d?

[ffaf1]

I removed the merge me just to get this straight. @fgaz is there no way around that? (i.e. something we can do to changelog-d or something we can ask GitHub)

[ulysses4ever]

I understood "in fact it already can" as if there's no problem at all. Perhaps, I'm deeply misunderstanding what's going on.

[fgaz]

@ulysses4ever I think I misunderstood your comment. I thought you were asking if changelog-d had problem handling the current ways we already rebase, not the one introduced by this pr. Maybe let's clarify in the matrix room?

[geekosaur]

I understood it the same way Artem did. Could you repeat your clarification here for the record?

[geekosaur]

Copied from Matrix:

I think a big source of confusion is that "rebase" is a bit overloaded in this context. There is update_method = rebase, method = rebase, and method = squash, and all three perform some kind of rebase.

• update_method = rebase updates the branch before any kind of merging, and it doesn't have anything to do with how the pr is actually merged, it just makes the history less confusing. we trigger it with the merge label in conjunction with method = merge (equivalent to clicking "merge pr"→"create a merge commit" in github) to create a no-fast-forward merge commit while still maintaining a linear history. method = merge is detected by changelog-d because the merge commit contains the pr number
• method = squash is a merge method. it squashes the commits into a single one named after the pr, including the pr number, and merges by rebasing. it is equivalent to clicking "merge pr"→"squash and merge" in github. we trigger this with the squash+merge me label. this is also detected by changelog-d because the new commit message contains the pr number
• method = rebase is also a merge method, and it just merges by rebasing, fast-forwarding without any merge commit. it is equivalent to clicking "merge pr"→"rebase and merge" in github. we currently do not use this anywhere. it is not detected by changelog-d because it results in no commit containing the pr number. geekosaur's pr enables this method
  Are we on the same page? Anything that I missed or that should be clarified?

[geekosaur]

Hold on, I need to edit documentation to match.

Before I do so: currently, if for some reason you want to preserve the commits instead of squashing, you need to use merge+no rebase. Should I add a separate (and ideally disrecommended) merge+no squash tag/method?

[geekosaur]

Clarification: it seems to me that if a given PR has one or more commits that need to be separated, it's likely because it's questionable or might be reverted in the future. In this case, it seems to me that in that case, either the PR isn't really ready to merge or it should be split into multiple PRs.

(The primary exception I can think of is when it's a workaround for another problem, that will be reverted when the actual problem is solved; see for example #9923. Even then, I think it would warrant being a separate PR rather than being embedded in a different one.)

[Mikolaj]

Before I do so: currently, if for some reason you want to preserve the commits instead of squashing, you need to use merge+no rebase. Should I add a separate (and ideally disrecommended) merge+no squash tag/method?

I don't think that's accurate. Label merge preserves commits, creating also the extra fast-forward merge commit.

Regarding whether that's a legitimate merge method --- for large PRs, it helps readability if separate tasks are in separate commits, each of which compiles and, ideally, passes tests. Also, regardless of size, if a PR containes minor refactoring, re-indentation, etc., it's a good practice to commit them separately from the actual big change. I don't think it's viable to create separate PRs for minor changes to few files.

OTOH, rarely anybody has time to read commits in separation, but OTOOH, after the first read of the whole diff, it's still useful to be able to focus on a specific task encapsulated in a separate commit. E.g., verify that a purported minor refactoring really did not change the behaviour.

[geekosaur]

I was under the impression that we preferred refactors to be separate PRs, but okay.

How about this? It's possible to conditionalize on number of commits, so the merge me rule could be split into two on number of commits and a single-commit PR could be automatically rebased. The rest would be a documentation change.

[geekosaur]

I don't think that's accurate. Label merge preserves commits, creating also the extra fast-forward merge commit.

I meant the current state of this PR, not what merge me does now.

[geekosaur]

I'm closing this for now. Future work: Mergify supports commit templates, which should allow addition of the PR number to rebase commits. In the meantime, the queue change to our Mergify config broke this in ways that need to be rewired anyway, so the only point of this PR is the discussion. (Ideally this could be converted to a discussion issue, but for some reason GitHub's API only allows the other direction.)