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.