3 ms·

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 quit

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.

pca006132 4y ago
Rebasing is not that complicated, usually. (if people sending PRs make sure their code is working for each commit)
- alexrtan 4y ago
  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.
  - fragmede 4y ago
    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.
    - alexrtan 4y ago
      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.
      - fragmede 4y ago
        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/95d1613a9fce4bcfc82d8ba48eb3224388f0b7e1...master 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.
  - LaLaLand122 4y ago
    > 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.
    - alexrtan 4y ago
      > 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?
      - __MatrixMan__ 4y ago
        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.
        snovv_crash 4y ago
        Make smaller PRs then. It's a lot slower to bisect if only some of the commits even compile.
        LaLaLand122 4y ago
        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.
        sulam 4y ago
  - mrkeen 4y ago
    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.
int0x80 4y ago
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.

pca006132 4y ago

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

alexrtan 4y ago
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.
- fragmede 4y ago
  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.
  - alexrtan 4y ago
    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.
    - fragmede 4y ago
      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/95d1613a9fce4bcfc82d8ba48eb3224388f0b7e1...master 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.
- LaLaLand122 4y ago
  > 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.
  - alexrtan 4y ago
    > 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?
    - __MatrixMan__ 4y ago
      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.
      - snovv_crash 4y ago
        Make smaller PRs then. It's a lot slower to bisect if only some of the commits even compile.
        LaLaLand122 4y ago
        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.
        sulam 4y ago
- mrkeen 4y ago
  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.

int0x80 4y ago

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.