Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linear history for this repository? #13

Closed
richardlau opened this issue Jul 5, 2021 · 6 comments
Closed

Linear history for this repository? #13

richardlau opened this issue Jul 5, 2021 · 6 comments

Comments

@richardlau
Copy link
Member

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
Copy link
Member

+1 to swapping to squash/rebase merges only.

@BethGriggs
Copy link
Member

@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
Copy link
Member Author

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

@canterberry
Copy link
Collaborator

canterberry commented May 23, 2022

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
Copy link
Member

mhdawson commented Jun 14, 2022

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants