nixpkgs: disable rebase and squash merges

[wolfgangwalther]

While working through the contribution guidelines, we came across this:

Pull requests should not be squash merged in order to keep complete commit messages and GPG signatures intact and must not be when the change doesn't make sense as a single commit.

We wondered whether we should just outright forbid squash merges.

Are there any scenarios where squash merge is required?

Most cases could probably be worked around by pushing to the contributors branch directly. Maybe a small how to could help committers to set things up for that.

Assuming we don't require squash merges, I'd thus like to propose to disable them (and rebase merges, too)

[mweinelt]

I use squash merges for r-ryantm PRs to embed the changelog URL, or, more generally, when I want to correct a commit message on an unsigned single commit PR without further review cycles.

I use rebase, when I collect a series of unsigned commits into temporary branches, where they might receive further work. They will then be part of another merge commit into the target branch.

[MattSturgeon]

I use squash merges for r-ryantm PRs to embed the changelog URL

This sounds like something that could be fixed in the bot. It already includes changelog URLs in the PR description, so it should be simple to add them to commit messages too.

, or, more generally, when I want to correct a commit message on an unsigned single commit PR without further review cycles.

This is a valid scenario. But how common is it? Would doing it via a manual push to the PR branch be a significantly poor workflow?

I use rebase, when I collect a series of unsigned commits into temporary branches, where they might receive further work. They will then be part of another merge commit into the target branch.

I don't see why that can't be done locally, without using GitHub's UI.

Still, I think it's valid that if we go ahead with this we'd want to do it in a way that only affects the "primary" dev branches (master, release-YY.MM). Temporary branches should be able to do more or less what they want.

That's possible with rulesets, IIRC.

Are there any scenarios where squash merge is required?

The only scenario I can think of where it is a requirement, is when the PR has disabled "allow maintainers to push to this branch" and the git history needs squashing and the author is unable or unresponsive.

This is a very niche scenario, AFAIK. The only workaround here would be to cherry-pick the changes into a new PR.

NixOS/nixpkgs#421104 (comment)
But yes... I could probably also do this by pushing to the PR's branch... if I could finally figure out how to do that easily without adding every contributor's fork as a remote (not sure whether that is possible, but it's the annoying part for me).

It if possible to fetch a remote without adding it as a named remote. It is also possible to set such a remote as a branch's upstream.

The easiest way to do this with GitHub is the gh CLI, which offers gh pr checkout. You can supply a PR number, URL, branch name, owner:branch string, etc. By default, it will create a new branch matching the PR's branch name, fetch that branch from the fork, and set it as the branch's upstream.

One gotcha here is that (AFAICT) an anonymous remote doesn't support --force-with-lease, so you must use --force.

[mweinelt]

I remember years of doing manual backports. Of course I can do everything on the CLI, but it complicates the workflow needlessly.

Wrt embedding changelog URLs: nix-community/nixpkgs-update#329

[wolfgangwalther]

That's possible with rulesets, IIRC.

Correct, we can apply different rules to development branches and temporary wip branches.

This indeed leaves us mostly with:

when I want to correct a commit message on an unsigned single commit PR without further review cycles.

I do the same, but I can see myself doing it via CLI, if easy enough. (which it seems to be, according to what Matt wrote)

[wolfgangwalther]

Essentially the question is, whether it's worth it to make this specific case a bit more effort via CLI and on the other side avoid the risk of destroying valid git histories on merge by accident. For example one thing that I always need to be careful with: When I merged one PR with squash merge, the next PR will be preset to squash, too. I can very well see me accidentally merging something with squash that I didn't intend to...

[MattSturgeon]

For example one thing that I always need to be careful with: When I merged one PR with squash merge, the next PR will be preset to squash, too. I can very well see me accidentally merging something with squash that I didn't intend to...

There have also been cases where committers have been unaware of the "usually don't squash merge" policy, either because they are new committers still finding their feet, or they (understandably) haven't read and memorized every single policy.

We often learn these rules more by observing others than by reading guidance cover-to-cover... Without the context of knowing why it was appropriate to squash a specific PR, I can imagine it being easy to misinterpret someone else squash-merging as "we often want PRs to be squashed".

Anecdotally, I recall seeing some confusion over:

Create a commit for each logical unit.

with some interpreting this as meaning each PR is a "logical unit" that should be squashed or squash-merged 😱

---

I'm not saying this is the norm, nor am I saying we can't solve this by informing such committers of the policy.

But I think the ideal is to avoid any doubt, and avoid any accidental squashing, by removing the button from the UI.

---

If committers squashing for the author is rare enough, needing the CLI shouldn't be too big of a deal.

The added friction of needing the CLI may also discourage unnecessary squashing more generally, promoting the idea of "ask the author to squash before you do it yourself" and "think about whether these changes actually make more sense squashed or not"?

Which begs the question: is that is what we want to strive towards? The counter argument is that we're adding friction to otherwise easy to merge PRs; potentially both for the author (being asked to squash) and the committer (first asking, and then using the CLI if no response).

---

I still think avoiding accidental (and/or incorrect but well-intentioned) squash-merging probably outweighs any potential downsides. But that's just my opinion; which doesn't count for much here, as I am not a nixpkgs committer and haven't experienced how this workflow goes in reality!

[pbsds]

I'm strongly against disabling squash merge, seeing as it is the tool to merge otherwise correct PRs with non compliant git commit titles. I make an effort to browse old ignored PRs, where the authors more often than not have lost all motivation to update their PR should I ask so of them. This is a part of the reason why I'm so vocally against unnecessary review cycles in general.

(yes i can force-push changes to their branch, but gh pr checkout breaks git push --force-with-lease, and I fear some contributors may view me doing so as invasive of their fork)

[wolfgangwalther]

yes i can force-push changes to their branch, but gh pr checkout breaks git push --force-with-lease, and I fear some contributors may view me doing so as invasive of their fork

That's up to the contributor, they can disable the "maintainers can edit this PR" checkbox, if they don't want you to do that. I think by adopting such a workflow instead, it would enable you do handle even more of these mostly correct, but abandoned, PRs. Think rebases because of conflicts, etc.

[pbsds]

But why should we disable useful tooling, when a simple PSA would solve the issue?

[wolfgangwalther]

Not sure what "PSA" means here.

The higher level reason for disabling such things is, that too much freedom to do things differently leads to mistakes easily. It is about creating a "lower bound" of acceptable committer behavior (see #122) via codified rules.

Maybe the basic guideline should be:

• Things that are commonly accepted should be easy to do.
• Things that are risky / potentially controversial should either be forbidden and disabled or at least harder to do.

Along these lines, with the explicit mention of avoiding squash merges as much as possible in the contribution guidelines, and the risks (default merge behavior changes after using it, but also the risk of over-using it intentionally) involved, it makes a lot of sense to disable it. To me.

This is all in the direction of "enabling required status checks" and "prevent accidental creation of upstream branches" and so on, and I think it fits right in. It requires adjustment for some individual workflows, but I'd hope that it creates less friction overall.

[pbsds]

Not sure what "PSA" means here.

I had "Public Service Announcement" in mind.

The higher level reason for disabling such things is, that too much freedom to do things differently leads to mistakes easily. It is about creating a "lower bound" of acceptable committer behavior (see #122) via codified rules.

I'm not quite sold on squashing/rebasing somehow being unacceptable or even a mistake in the majority of cases. We currently have a preference for merges, since it preserves the original commit in the commit history. The main case against rewriting commits is when doing so would invalidate a signature. If not clobbering signed commits is a value we want to enforce in nixpkgs, then I'd first investigate less invasive options like whether it is possible to flip some github setting/add a CI action that disables rebasing and squashing on a per-PR-basis, depending on whether it contains any signed commits or not.

[emilazy]

Using merge commits makes git log --first-parent and git bisect --first-parent useful, which are invaluable for Nixpkgs. Rebase merges inherently break that and the results would be catastrophic given our current workflows IMO. (We would at the very least need CI on individual commits to not need --first-parent and the review flow would have to be turned inside out. Since we’re probably not switching to Gerrit any time soon I don’t think rebase merges should ever be used on “main” branches.)

Squashes are okay to fix up PRs when required though. But people misuse squash merges enough that I think it would be okay to use the push‐and‐merge workflow for that case too.

[pbsds]

Thanks for your perspective! I'm convinced and will no longer block this 👍

[khaneliman]

I'm strongly against disabling squash merge, seeing as it is the tool to merge otherwise correct PRs with non compliant git commit titles. I make an effort to browse old ignored PRs, where the authors more often than not have lost all motivation to update their PR should I ask so of them. This is a part of the reason why I'm so vocally against unnecessary review cycles in general.

This is my opinion, as well (without being so strong). I've had back and forth with people and seen them pile up git commits to get it to a good state that just doesn't fit guideli nes. I usually just squash it in and fix up the git message to conform.

(yes i can force-push changes to their branch, but gh pr checkout breaks git push --force-with-lease, and I fear some contributors may view me doing so as invasive of their fork)

I have also come across this, where people have got very upset with force pushing changes to their PR...

That's up to the contributor, they can disable the "maintainers can edit this PR" checkbox, if they don't want you to do that. I think by adopting such a workflow instead, it would enable you do handle even more of these mostly correct, but abandoned, PRs. Think rebases because of conflicts, etc.

.. and reminding them of that option makes them get even more upset, which isn't a good experience either.

Personally, I prefer merges and use squashes when necessary. Rebases are fine to get rid of, imo. But, I don't have a very strong opinion on it, since I'm comfortable pulling changes down to CLI squash if a need arises.. but I'd find myself willing to help in that way less, if I couldn't assist from the UI quickly.