Merge PRs using --no-ff instead of --squash

[mkleczek]

@steve-chavez

I noticed you squash PRs into a single commit.
On the other hand @wolfgangwalther in his reviews suggests to isolate unrelated refactors into separate commits in the same PR.

Squashing PRs looses those commits (that happened with #4396 - see https://github.com/PostgREST/postgrest/pull/4396/files#r2425469503).

OTOH creating separate PRs for small refactors done during work on larger PRs is problematic since it would require stacked PRs etc.

In my experience the best of both worlds is to git merge --no-ff PRs. It retains linear history in main (ie. main contains only merge commits) but also smaller commits "inside" PRs to make fine granular change history visible.

WDYT?

[wolfgangwalther]

Squashing PRs looses those commits (that happened with #4396 - see https://github.com/PostgREST/postgrest/pull/4396/files#r2425469503).

I think what happened here is:

• The refactor should have been kept a separate commit.
• The changelog should have been in the same commit as the change it was about.

We want a linear history on main, so no merge commits. Our options are rebase and squash. We even disabled the "merge commit" button from the UI.

This means that the history in the PR needs to be written with that in mind. The best way would have been for you, @mkleczek, to amend the changelog to the right commit - and always rewrite the history of your PR when you make changes, to keep it in a state that can be merged onto main in a linear fashion, where each commit is atomic on its own.

Edit: But to be clear, I agree that squash was wrong here, because the refactor should have been kept separate!

[mkleczek]

I don't understand - do you suggest to open separate PRs for these commits? (That's what I'm questioning here).

[mkleczek]

Also see your suggestion here https://github.com/PostgREST/postgrest/pull/4396/files#r2425814007

[mkleczek]

Handling changelog changes is not something I'm discussing here. What I am asking about is this commit that was lost: 5c844c8

[wolfgangwalther]

I don't understand - do you suggest to open separate PRs for these commits?

No, certainly not.

Also see your suggestion here https://github.com/PostgREST/postgrest/pull/4396/files#r2425814007

Yes, I am aware - I still think that was the right call, and I agree with you that this separate commit should not have been squashed.

Handling changelog changes is not something I'm discussing here. What I am asking about is this commit that was lost: 5c844c8

But the changelog entry is what very likely caused all of this! Steve doesn't just squash willy-nilly. He didn't look carefully enough about the refactor commit that is now lost, but he did look at the changelog commit and realize "this should not be a separate commit" and that's why he squashed.

The best case of how things should have been is this:

• The PR has two commits, the performance fix and the refactor. The changelog is in the performance fix commit, because it belongs to that - it forms one atomic unit.
• The merge is either done with fast-forward (which is not available via GitHub's UI...) or via "rebase merge", which the UI provides. The latter is the canonical way on GitHub to keep linear histories.

[mkleczek]

Ok, got it. Makes sense.

[steve-chavez]

but he did look at the changelog commit and realize "this should not be a separate commit" and that's why he squashed.

The above is exactly what happened. 9765f0e looked off, so I chose to squash, and I thought 5c844c8 was related to the same perf commit. @mkleczek Sorry about that.

As an improvement, I think we can allow auto-merge on Github. Then we can be more picky on the commits before giving approval (I approved thinking about doing a squash later).

[steve-chavez]

Enabled auto-merge. Let's see how it goes.

[mkleczek]

Yeah... it is all clear to me now. I assumed you are going to git merge --no-ff and left the changelog update commit to document work history.
Sorry for the additional work it caused.

I am not sure if automation is going to help, maybe some information in contribution guide about how we expect commits in PRs to look like. Even one sentence stating:

Each commit in a PR must be a single coherent unit of work possible to submit as a separate standalone PR on its own.

[wolfgangwalther]

Enabled auto-merge. Let's see how it goes.

And I disabled it again. For auto-merge to work, we first need to set up required status checks, otherwise this won't have a (positive) effect.

[wolfgangwalther]

I am not sure if automation is going to help, maybe some information in contribution guide about how we expect commits in PRs to look like. Even one sentence stating:

Each commit in a PR must be a single coherent unit of work possible to submit as a separate standalone PR on its own.

I agree, this is mostly a communication thing - I'd welcome such an addition in the right place. Can you open a PR?

[mkleczek]

@wolfgangwalther #4421

[taimoorzaeem]

Another thing we can do is to enforce changelog entries based on PR or commit pattern via some github action maybe like https://github.com/dangoslen/changelog-enforcer based on a certain prefix like fix: ... or add: ... etc. Not sure if that would be worth it 😿.

[steve-chavez]

That sounds like a good idea 👍 . So we don't forget to update the changelog.

[taimoorzaeem]

Noted this in my backlog for now. 👍