Require clean history or squash on merge

[phadej]

I'm looking at some git log and it says

f6c6c400b whoops
...
c241917a7 Formatting.
c02ed9424 Formatting, whitespace, 80-col violations.
5a07388f2 Formatting, whitespace, 80-col violations.

That's impossible to cherry-pick. It would be acceptable if we hadn't release branches, where we are interested in backporting stuff, but we do.

cc @hvr @23Skidoo

[23Skidoo]

If these were on a branch and there's a merge commit you can cherry-pick the merge commit and it will apply that PR as a whole.

[hvr]

I don't really have a perfect answer to this dilemma...

I generally support a clean history with logical commits that don't look like somebody commited whenver they'd press the "save" button; but I also prefer to split unrelated whitespace changes and actual code-changes into separate commits.

But then again, there might be phases when there are multiple live git branches shortly before a release one should maybe refrain from conflict-inducing whitespace cleanups for the sake of not making cherry-picking more tedious than it needs to be... :-)

[chshersh]

In @kowainik we're using our own git/GitHub workflow. But in simple words, we do Squash and merge for each PR. So I can recommend this approach, it works exceptionally well for us. You can even configure GitHub settings to disable Merge and Rebase and merge buttons to only do Squash and merge.

The motivation behind such a workflow is that you can have any history in a branch. A branch is like a temporary place for all your work. It can be messy, or it can be clean. For us, it doesn't matter. Anyone can follow the most convenient workflow for them in their branch, and we can review anything, not a problem for us at all. But the history in the master branch is always clean. Each PR converts to a single clean commit after pressing Squash and merge button. This button also automatically adds PR number to the commit, so it's very convenient to browse commit history and jump to relevant PRs quickly if you're interested in the discussion. This workflow also suggests creating small PRs which solve small issues or significant issues incrementally. It's always easier to review smaller PRs rather than git ones.

[hvr]

I see we can all agree that a clean history in the main-branch is highly desirable for various reasons. But the problem with a squash-and-merge workflow is that it tends to promote the abuse-commits-as-save-button symptom I lament which are tedious to review IMO; and having lots of small inter-dependent PRs is also kinda useless with GitHub's UI in its current form (I'm still disappointed that GHC gave up on Phabricator (for CI-related reasons btw) which I consider still ahead of GitLab and GitHub when it comes to the concept of inter-dependent PRs as well as dealing with an iterative review and versioning of PRs). In some way you could see a squash-merge workflow as a very poor substitute for Phabricator's code-review idiom which doesn't cut it because GitHub's code-review UI still doesn't offer crucial features to make it work satisfyingly.

More importantly, I value expressive commit msgs which tell a story and a rationale for each commit (which become quite invaluable when you need to do Git archeology to understand why a line of code was written in this very way) - and if you just collapse such a sub-plot it's essentially lost if it isn't represented in the git-commit-graph (git bisect won't be able to see it either, nor will git log or git blame); and with squash merges it's also easily forgotten to write up a proper commit msg as the author of the PR most likely didn't write one to begin with. And ideally this information is self-contained in the Git repo without needing to constantly switch between Git repo and webbrowser and GitHub's comparatively high-latency UI -- the more I can quickly do with git's CLI and the less I need to rely on http://GitHub.com and get distracted by switching back and forth, the more productive I am.

in summary, I consider squash merges somewhat of a sledgehammer to deal with a poorly structured PR, or alternatively the suggestion to prefer merge-requests a symptom of systematic bad PR discipline rather than a virtue to aspire to... ;-)

[phadej]

But the problem with a squash-and-merge workflow is that it tends to promote the abuse-commits-as-save-button symptom I lament which are tedious to review IMO

I agree 100%. Let's not promote practices which encourage being sloppy to begin with.

and having lots of small inter-dependent PRs is also kinda useless with GitHub's UI in its current form

I was writing a comment about that. I experienced that problem with parser rework, and actually now I had three PRs open (one is already merged), which are all a single sequence.

If there's any auxiliary tool to help with the pain (of the author), I'd like to know about it.

More importantly, I value expressive commit msgs which tell a story and a rationale for each commit

And I was to mention that exactly. I learned a lot from GHCs commit messages. Commit messages explain why the change is made, where comments in the code should only try to explain the current state. It's not helpful to see a single line commit Change Foo, without bigger explanation. One should write a commit message, not a PR description.

Thus I proposed the requirement of clear history or squash on merge (not exclusive or; in that preference order; These, not Either).

We have Conventions sections in CONTRIBUTING.md, adding a bullet point about clean history would be a start. Now we do not even have a preferred way, not speaking about a policy.

[ulysses4ever]

I wonder if this discussion has completed and the ticket should be closed. Although maybe we should promote the squash+merge (along with merge-me) in CONTRIBUTING.md — it currently lacks any mention of mergify.

[andreasabel]

I think whitespace checks should be part of the CI, see e.g. https://github.com/agda/agda/blob/3044510c8966a1876951e807f1393bbc3b5e6169/src/github/workflows/whitespace.yml
(configuration at https://github.com/agda/agda/blob/3044510c8966a1876951e807f1393bbc3b5e6169/fix-whitespace.yaml).

[Mikolaj]

Yes, I think our current process/culture works great, but it should be documented and, if possible, automated further. Another point worth remembering/documenting is changelog and, if feasible, tests. Perhaps the PR template can contain at the end, in addition to those, a mention of mergify's merge_me and squash+merge_me in the context of how to split stuff into commits?

[ffaf1]

Guidelines on how/when to squash stuff — as we have for commit messages — would be useful indeed. If there is an external, authoritative source we could leech from that.

[Mikolaj]

I don't think there is any simple rule about that. If your commits reflect the structure of your PR well, you don't squash. Otherwise, you squash and either split into commits again (lots of work) or just commit a big blob. This involves all kinds of trade-off between the writer, readers, cherry-pickers, etc. I think, among Haskellers, readers are the most important of those.

[ulysses4ever]

CONTRIBUTING.md recently got a subsection discussing GitHub pull request process (2353627). Among other, the section includes a soft suggestion to consider squash+merge me (and discusses the default, merge me label as well). Maybe it's the time to close this one and wait for more specific requests for improvement on this front?

[Mikolaj]

What about the whitespace check in CI? What about referring to CONTRIBUTING.md in our PR template (and a rudimentary overhaul of our templates, while we are at it, e.g., we only have Bug template for an issue IIRC)?

[andreasabel]

What about the whitespace check in CI?

Lifted to #8316

[ulysses4ever]

What about the whitespace check in CI? What about referring to CONTRIBUTING.md in our PR template (and a rudimentary overhaul of our templates, while we are at it, e.g., we only have Bug template for an issue IIRC)?

A whitespace check has been done (thanks, Andreas!). For PR templates, I opened #8511. I think nothing else left here.