nodejs / release-keys

Node.js release signing keys.
MIT License
12 stars 11 forks source link

Linear history for this repository? #13

Closed richardlau closed 2 years ago

richardlau commented 3 years ago

I noticed that we only have "Merge pull request" enabled for this repository, which is the opposite of what we tend to have elsewhere where we don't allow "Merge pull request" in an attempt to have linear history (i.e. no merge commits). Maybe it doesn't matter, but let's make a conscious decision either way.

image

FWIW this is what our current history graph looks like:

$ git log --graph --oneline
*   d7dd522 (HEAD -> main, upstream/main, upstream/HEAD) Merge pull request #10 from BethGriggs/bethkey
|\
| * 40794ab πŸ”‘ Update key for Beth Griggs
|/
*   f2503f4 Merge pull request #9 from nodejs/move-to-main
|\
| * 1c39ab1 Replace "master" with "HEAD" in URLs
|/
*   5ca9f74 Merge pull request #7 from canterberry/migrate-urls
|\
| * 32f8762 πŸ“ Update email addresses for Beth Griggs and Richard Lau
| * 89ea607 πŸ“ Migrate URLs in README and CLI to nodejs/release-keys repo
* | 61d3143 Merge pull request #5 from canterberry/danielle-adams
|\|
| * 5457e01 πŸ”‘ Add old key for Danielle Adams (used to sign v15.2.0 release)
| * 669afc9 πŸ”‘ Add key for Danielle Adams
* |   085a84e Merge pull request #8 from canterberry/cli-add
|\ \
| |/
|/|
| * 62049ac 🎨 Add gitignore entry for redundant GPG backup files and improve CLI output when adding a signing key
| * d4acc64 🐞 Add missing CLI_DIR prefix when exporting key + missing usage entry
| * dbba43d ✨ Add command for release signers to easily import keys
|/
*   c980295 Merge pull request #2 from canterberry/team-key-updates
|\
| * 7a9f254 Rebuild GPG keychain and keys list from latest documented Release Keys
|/
* b67a2b9 πŸ“ Hot-link from key IDs in the README to the raw keys
BethGriggs commented 2 years ago

+1 to swapping to squash/rebase merges only.

BethGriggs commented 2 years ago

@canterberry, @nodejs/release any objections? If we're going to change the merge setting I'd like to do it before landing the open PRs.

richardlau commented 2 years ago

+1 for changing to enabling "Squash and merge" and "Rebase and merge" and disabling "Create a merge commit".

canterberry commented 2 years ago

A lot of folks prefer linear history because it produces a tidy list of changes over time, but squashing and rebasing can be problematic -- both practically and philosophically -- for a few reasons:

  1. History isn't actually linear -- the current graph accurately reflects how each commit entered the history. It's not as tidy as what we imagine history to be like (a straight line), but it does reflect the reality.
  2. A flattened linear history can be derived from a non-linear graph, but you cannot go the other way around.
  3. Squashing commits causes data loss, by destroying the individual commits, including any cryptographic signatures on those commits made by the author. This undermines the authenticity provided by signed commits.
  4. Rebasing causes data loss, by destroying information about how commits in the PR are related to one another. This information is what would be held in the merge commit. With rebasing, commits from multiple PRs can become intermingled in the linearized history, actually diminishing clarity and readability of the history.
  5. Merge commits encourage PR authors to adopt good commit hygiene (concise, informative commit messages, atomic commits that Do One Thing, etc). (credit where due: rebasing does encourage this as well)
  6. Ultimately, the consumer of this historical data is likely a developer doing a git blame or similar on a line in a file to learn the why for how it came into its current state. Merge commits create a more readable history for this scenario than squashing.

The current configuration was indeed a conscious decision, but that decision was made unilaterally by myself. Ultimately, I defer to @nodejs/release, of course, as the project owner and authority on the matter.

mhdawson commented 2 years ago

I'm in favor of being consistent with what we do across the project unless there is a specific version. Sounds like @canterberry it willing to go along with whatever @nodejs/releasers think is best.

I also think we should probably not block landing the PR that started this discussion as we want @RafaelGSS to be able to help with releases/security releases.

richardlau commented 2 years ago

We discussed this in today's release wg meeting and decided to keep the merge commits for now and see how it goes.