Hacker News Reader

Comment by alexrtan | Hacker News Reader

I don’t understand why people waste time manually rebasing pull requests when you can squash merge with one button and get essentially the same result. Not quite as bad as merging a bunch of commits that aren’t bisectable because they didn’t pass CI though.

View on HN

First of all, because it makes reviewing the code for reviewers much easier. Reviewing a messy set of random uncurated commits is really suboptimal.

Second, you might have more than one commit you want to merge to master, not just a single commit.

Also rebasing is trivial and very quick once you have done some basic reading about it.

Rebasing is not that complicated, usually. (if people sending PRs make sure their code is working for each commit)

Yeah but the other problem in pull requests is that if someone makes changes since the last time I reviewed and then they force push I can’t see the difference from the last time I reviewed. Also if they rebase and push multiple commits there’s no guarantee they would all pass CI which means you can’t guarantee it will work with bisecting.

Force push doesn't delete the commit from your computer so you should still be able to diff between the (disjoint) commits using the hash.

If someone else has done the pull request, I review it on GitHub, then they rebase and force push the original commit wouldn’t be on my machine unless I had pulled it earlier correct? Which is something I never do.

If you don't have the ref locally but you have it open in a browser tab, you should† still be able to manually hack the URL to get a diff. Eg https://github.com/git/git/compare/95d1613a9fce4bcfc82d8ba48...

† My company's not on Github Enterprise and I don't have a repo and a second user handy to double check on public Github.

> if someone makes changes since the last time I reviewed and then they force push I can’t see the difference from the last time I reviewed

You need better tools. Even in GitHub you can click on "force pushed" and see the diff.

> there’s no guarantee they would all pass CI

There wasn't any guarantee before, either. If you want that guarantee, make the CI build every commit.

> There wasn't any guarantee before, either. If you want that guarantee, make the CI build every commit.

Guarantee is the wrong word. The point is that if you always squash merge after passing the entire test suite you don’t have a bunch of potential garbage commits in history that you have to wade through when bisecting.

As for building every commit that’s probably a tough sell and a poor use of money for what benefit?

But if it's only gonna find monstrous feature-at-a-time commits, what good is a bisect anyway? I want the culprit commit to be 5 minutes of bad ideas, not 5 minutes of bad ideas mixed with three days of good ones.

Make smaller PRs then. It's a lot slower to bisect if only some of the commits even compile.

If the PR is small enough to naturally fit into a single commit the rebasing vs squashing multiple commits discussion disappears (just please don't merge it!)... there is a single commit.

Many developers make commits that are “wip” with messages to match. Maybe you commit before you go to lunch, or when you go home for the day. These are useless commits and should be rebased or squashed, but many of these same devs won’t do that unless you force them or the tooling does it for them automatically. A minority of devs build a clean history of commits that tells a story, carefully crafted over time (usually manufactured after the fact with liberal use of rebase). For these devs squash is awful. Unfortunately they are truly a minority and the majority ruins things for them.

That's fine. You're checking the difference between the current state of master and the current state of the feature branch, not the difference between the old state of the feature branch and the current state of the feature branch.

Reply on news.ycombinator.com