only allow rebase merging for PRs

[kylerisse]

Description

I think we should only use rebase merges for our PRs. reasons:

• I believe this is what the nixpkgs repo uses so we'd be in line with them.
• It produces a much nicer git history because there are no merge commits.
• I've started using this in a repo with a flake and my local nix store cache is much faster post merge.

One potential downside is it could be argued that it's slightly harder to determine which PR a commit goes with. That's easily worked around by using a search using the commit ID.

I'd love some feedback from @djacu @sarcasticadmin @MatthewCroughan .

Acceptance Criteria

We choose to do this or not to do this. Either way I believe we should at minimum disable squash merging and at least one of either merge commits or rebase merging in https://github.com/socallinuxexpo/scale-network/settings

What is the expected outcome that resolves this issue?

• only 1 of the 3 options in Merge Request section of Settings is checked, preferably "allow rebase merging", but if not then "allow merge commits" which is the default we use today.

[djacu]

I think of the merge strategies in terms of what kind of information is in the PR and needs to be maintained for future developers.

• squash commit: It's probably easiest to start with a known example. The nixos/rfcs repo only (or primarily?) uses squash commits. The history, changes from discussion/bike shedding, does not matter for the final commit history. If you want to see how the document evolved during the course of the PR, you can look in the PR yourself. But the master branch history only cares about final verions.
• merge commit: Here each commit is meaningful to the history but lacks context without the merge commit. I see this especially in project repositories that are developing some application where each PR is a feature. Or in a library where PRs can be features or API changes. (Someone check me but I think the commits of a merge commit PR get new hashes when merged? I think this is part of the 3rd point @kylerisse mentioned.)
• rebase merge: Here each commit is meaningful and atomic. It needs no context. There isn't a feature or API change that the collection is aiming for. Each commit should usually be able to stand on it's own and have a meaningful commit title and message. It usually should not introduce breaking changes to the repository. (This was really hard to do during [READY] nix.treewide: remove flake-parts #800; I ended up splitting some commits into smaller ones so it was easier to parse.)

Based on the above, which totally call me out if I'm out of pocket, I would lean towards rebase merges. I like linear git histories. It makes using git blame locally easy. This does mean that each commit should be of the highest quality aka good commit message. And possibly pushing back on PR authors to clean up their commits before merging (or doing it yourself :P).

@kylerisse help me understand this scenario because I'm not seeing it. When would we want to search a commit ID in order to find the associated PR? In case there was some discussion in the PR and it had context for why a commit did what it did?

Examples of why I started using rebase merges more.

Here is this repos history

% git log --graph --decorate --pretty=oneline --abbrev-commit --all
*   2435a08 (HEAD -> master, origin/master, origin/HEAD) Merge pull request #800 from socallinuxexpo/djacu/remove-flake-parts
|\  
| * 0951e6f nix.nixos-configurations.{devServer,hypervisor1}: fix configuration
| * 960ce8a nix.flake.inputs.nixpkgs-2405: pin for parity
| * aea5965 nix.packages: init
| * 1939939 nix.treewide: cleanup chore
| * abfa105 nix.nixos-configurations.loghost: fix
| * 125263f nix.treewide: cleanup chore
| * 6b5e294 nix.flake.inputs: remove flake-parts
| * 347301e nix.checks: fix checks
| * 0a8bcb8 nix.devShells: rebind package set
| * 0a4f15c nix.legacyPackages: change package set
| * d549219 nix.flake.inputs: cleanup
| * b0d8f57 nix.devShells: fix devShells
| * cb96464 nix.treewide: step 999 of removing flake-parts
| * 741357a nix.nixosConfigurations: fix configurations
| * ed0870c nix.nixosModules: fix modules
* |   ed2f256 Merge pull request #799 from socallinuxexpo/rh/1732488385
|\ \  
| * | c40be17 openwrt/golden: update tests for new keys
| * | 3f078c4 Revert "Merge pull request #792 from socallinuxexpo/MrHamel/791"
|/ /  
* |   9e612d7 Merge pull request #797 from dann232002/dannykeys
|\ \  
| * | 3cdbce2 Adding danny keys
* | |   87bb631 Merge pull request #798 from carlynlee/carlyn_key
|\ \ \  
| * | | 0dd8a3e miniconfig authentication for carlyn
* | | | c26274b Merge pull request #796 from carlynlee/carlyn_key
|\| | | 
| |/ /  
|/| |   
| * | e5c5104 adding Carlyn's key
|/ /  
* |   718fe19 Merge pull request #792 from socallinuxexpo/MrHamel/791
|\ \  

Here is the history for my personal config (can you tell that I like the same naming convention as they use upstream?)

% git log --graph --decorate --pretty=oneline --abbrev-commit --all
* 9ed8121 (HEAD -> nixos-configurations-add-scheelite, origin/nixos-configurations-add-scheelite) nixos-configurations.scheelite.hardware: change swap
* c0cda18 git-blame-ignore-revs: add scheelite/hardware formatting
* 51bd260 git-blame-ignore-revs: init
* 95a9d4d nixos-configurations.scheelite: formatting
* 2dd6935 nixos-configurations.scheelite: update filesystems
* e2e5e8a nixos-configurations.scheelite: fix tank0 import
* 4f8b57c nixos-configurations.scheelite.impermanence: fix ssh
* be07bff nixos-configurations.scheelite.impermanence: remove NM directory
* 7085a7f home-configurations.scheelite: init djacu
* 561b644 nixos-configurations.scheelite: init configuration
* 44c3c87 nixos-configurations.scheelite: fix tank0 mounting
* b9f9f83 nixos-configurations.scheelite: fix tank partition
* ff230db nixos-configurations.scheelite: fix tank0 zpool
* 479c39d nixos-configurations.scheelite: fix boostrap script mode
* 0dd81d7 nixos-configurations.scheelite: fix root pool mount
* 286db8c nixos-configurations.scheelite: fix root mount
* 4b816d4 nixos-configurations.scheelite: set shell options
* 0adb5ce nixos-configurations.scheelite: fix shebang
* 3f34818 nixos-configurations.scheelite: fix root pool
* 81c3682 nixos-configurations.scheelite: fix tank0
* fa81ef8 nixos-configurations.scheelite: init
* 5617c5e (origin/main, main) flake-inputs.nixvimcfg: bump
* 3b5996c nixos-configurations: cassiterite: update hardware and disk
* 1ad0102 flake-inputs: bump all
* 4e4a2ba nixos-configurations: test-vm: fix
* b1758e5 packages: init
* 6e8e21c legacy-packages: init
* 16bfeda overlays: add nom-check
* 2a90b01 overlays: improve to consume local overlays

[kylerisse]

@kylerisse help me understand this scenario because I'm not seeing it. When would we want to search a commit ID in order to find the associated PR? In case there was some discussion in the PR and it had context for why a commit did what it did?

I don't know. Basically what you showed, the git graph doesn't work with a linear history. I was just trying to think of a downside and show a work around for it. It also came up when chatting with @jshcmpbll . I wasn't initially sure what the answer was. There could likely be some value to seeing a PR based on a git commit.

(Someone check me but I think the commits of a merge commit PR get new hashes when merged? I think this is part of the 3rd point @kylerisse mentioned.)

From what I have seen, a merge commit is a new git sha and new narHash. Technically the rebase merge does produce a different git sha also (and changes commit timestamps), but it doesn't change the narHash.

This does mean that each commit should be of the highest quality aka good commit message. And possibly pushing back on PR authors to clean up their commits before merging (or doing it yourself :P).

I believe that should be the idea with either strategy.

[sarcasticadmin]

Im all for the simplified commit history but Ive got some concerns about the rebase merge. Let me touch on whats been discussed thus far before getting into that at the end.

Either way I believe we should at minimum disable squash merging

Agreed, I dont like these anyway we should just disable them outright.

I believe this is what the nixpkgs repo uses so we'd be in line with them.

I dont have merge access to nixpkgs but Ive seen a mix of PRs merged using various techniques (seems to be up to the merger). Heres a recent merge commit into nixpkgs: NixOS/nixpkgs#354474

From what I have seen, a merge commit is a new git sha and new narHash. Technically the rebase merge does produce a different git sha also (and changes commit timestamps), but it doesn't change the narHash.

Theres an additional merge commit which points to the parents branches: master and feature branches respectively. The commits that were merged into master have the same commits (hashes remaining intacted) + the additional merge commit.

Im not sure why the narHash would be different if only the thing different were merge types since repo content would be the same in either case.

Here is the history for my personal config (can you tell that I like the same naming convention as they use upstream?)

Using @djacu repo as an example: The following PR was rebase merged into the repo: djacu/theonecfg#9 if we take a look at one of those commits in the PR: djacu/theonecfg@46d8e9e it has the following commit hash on master: djacu/theonecfg@6429578 . All of those commits that were rebase merged has a new commit hash on master.

I do like the naming conventions used upstream in nixpkgs 🙂

---

I dont know if post merge builds is worth the tradeoffs (I'll mention some below). I think the build time between changes that are triggered by different commit hashes is negligible since that doesnt effect everything thats been built and cached. Some concerns that come to mind here are:

1.Linear history is great but wouldnt this require that each PR rebase once another is merged regardless of a merge conflict? That seems like a fair bit of overhead for all contributors.

2. Additionally, what happens on reverts? Seems like we'd have to revert each commit instead of just merge commit if we want to revert everything from a problematic PR.

[djacu]

Was this meant to be closed? I see additional comments on slack that look like the conversation was still going but I don't see those comments here.

nvm all good

[davidelang]

Robert James Hernandez wrote:

1.Linear history is great but wouldnt this require that each PR rebase once another is merged regardless of a merge conflict? That seems like a fair bit of overhead for all contributors.

What I value more than a linear history is to have a PR be cleaned up before submission, where the commits in the PR are clean at each step (not having commit 10 fix a bug introduced in commit 2). This usually requires something like an interactive rebase to reorder/squash commits, sometimes re-writing them. basically, make the PR look as if you were perfect and knew exactly what you were doing each step of the way :-) Now, when bugs are only found after it's merged, then you do need to just add the fix and accept that the history has a range where it's broken. But pre-merge, things should be cleaned up if possible. David Lang

[sarcasticadmin]

basically, make the PR look as if you were perfect and knew exactly what you were doing each step of the way :-)

Yes, agreed. I think that's unanimous opinion in the expectation of contributions to this repo and related scale repos.

Now, when bugs are only found after it's merged, then you do need to just add the fix and accept that the history has a range where it's broken.

We won't ever rewrite the history on master

[sarcasticadmin]

Thanks @kylerisse for already making these changes against the repo settings in github: