Hacker News Reader

Comment by lamontcg | Hacker News Reader

I'm a heretic who prefers information in the PR.

That's where the long-form back and forth arguments happen over the edge conditions.

Periodically a few days after a PR has been merged I'll have a discussion with someone and realize some context was never captured, so I'll just add it at the bottom of the closed+merged PR for posterity.

Yeah that means all the important information is in GH but if you migrate to GitLab or bitbucket or whatever they've got tools to pull all that info along with you when you migrate.

I don't really care about individuals commits other than to find the gitsha to find the PR that they were merged into in order to find the full context.

View on HN

I'm an agnostic who keeps my distance from self-proclaimed heretics inhabiting the modern-day Church of Github (which formed as an offshoot of the Cult of Git, which itself emerged after launching a savage religious war with the iconoclastic Sect of Subversion), who prefers to have both a logical, reviewable commit history AND informational pull requests.

Smashing commit history via squash makes debugging harder. Provided you have the interactive rebase chops to pull off crafting a decent granular history, you'll reap significant dividends down the road if you keep that history intact. Because it's right there for you to use with `git blame`, etc.

And then why not have proper discussions in the pull request with reviewers? Sometimes important stuff comes up in review. Sometimes you might close the PR and start anew. Sometimes the PR gets incredibly lengthy. Maybe the result is a commit history which condenses some abominably lengthy PR.

There's no need to choose between them. You can have both!

It may be heretical but I tend to agree with this. Also at my workplace we tend to be standardized around PRs doing squash merges so there’s not really much point in having detailed interim commit messages anyway. As long as the PR has good information (and of course is linked to useful work items with good information) that’s usually good enough for me.

(Of course more often than not people have empty/useless linked work items, and PRs with useless titles and descriptions but that’s a whole different kind of problem.)

I'm with you!

The PR commit can have sooo much good information, that can be used by so many teams. Loom videos for a quick demo of the feature. A concise description of the impact on customers, support teams. Deployment and feature rollout considerations.

I do try to make the individual commits somewhat presentable within the PR... a few extra minutes polishing the subjects lines. But nowhere near what this article, which seems to be suited for non-PR workflows such as mailing lists+patches, or workflows where every PR is squashed?

There's even a pitfall in putting too much "why" in the commit messages -- some programmers will be tempted to keep it out of the code, ie never comment their code.

I used to do that as well. The problem here is that PR belongs to a particular service provider (GitHub, GitLab and whatnot), and thus can be easily lost if you move providers. Or even if you simply move commits to a new repository (for instance you need to clean out some sensitive information from git history). In that sense having as much context in commit messages themselves saves the day.

I subscribe to this and:

> "The Not Rocket Science Rule Of Software Engineering: automatically maintain a repository of code that always passes all the tests"

https://graydon2.dreamwidth.org/1597.html

Intermediate commits to produce a pretty history while not passing the tests reduce their usefulness to about zero.

I'm fully on board with the idea that the main branch should always pass.

I also like the approach where tests are performed in a temporary integration branch, then the main branch gets fast-forwarded to the tip of that integration branch only after the tests pass.

But the idea that intermediate commits must always pass tests forces you into some nasty contortions.

Say that you have to move files around, after which the tests fail, and then make a few minor changes to accommodate the new locations so the tests pass once agian. If you combine those two operations into a single step, troubleshooting that commit is a pain because the diff is gigantic.

For this reason, I prefer a less strict rule: all PRs get a merge commit and merge commits must always pass tests.

I totally agree with this, I'm not a fan of the written-in-stone nature of commit messages. Just like code, it's nice if log history is editable too in case your forgot something, or if you want the message to mention future changes that back reference it.

Write a commit script that pulls in the PR text into the merge commit.

> Periodically a few days after a PR has been merged I'll have a discussion with someone and realize some context was never captured, so I'll just add it at the bottom of the closed+merged PR for posterity.

Reply on news.ycombinator.com